Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http filter: add CSRF filter #6470

Merged
merged 18 commits into from
Apr 23, 2019
Merged

Conversation

dschaller
Copy link
Member

Signed-off-by: Derek Schaller dschaller@lyft.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: Add a Cross-Site Request Forgery filter
Risk Level: Low
Testing: Change includes unit and integration tests and I manually tested the filter as well.
Docs Changes: Included
Release Notes: Included
Fixes #245

Cross-Site Request Forgery (CSRF) HTTP Filter

CSRF (also known as XSRF, Sea Surf, and Session Riding) is an attack that occurs when a malicious third-party exploits a vulnerability (generally through social engineering) that allows them to submit a malicious request on the user’s behalf. The request relies on the victim having a valid session on the destination site and utilizes this to assume the identity and permissions of the user.

Possible Mitigation Patterns

There are a few different popular mitigation patterns that exist and some of those are either entirely stateful or have a stateful option. For purposes of this feature I am going to ignore the stateful patterns and focus on the stateless ones. This is because we don’t want to introduce state management into Envoy if there is another option which is just as secure.

A common pattern to mitigate CSRF attacks is utilizing tokens that help discern requests made by the user versus requests made by a malicious third-party. This approach is the most popular and is the recommended approach by OWASP. There are three approaches here, the synchronizer token pattern, encryption based token pattern, or HMAC based token pattern. All three of these work by generating a cryptographically secure token that is associated with the user’s current session. This token is embedded into the HTML and/or added to request headers/parameters and submitted to the server which is responsible for verifying the existence and validity of this token. The main difference between the three approaches is the data that is used in generating the secure token and how the token is verified. The synchronizer pattern generates a token per session or request and therefore validates the submitted token using the stored value. The encryption pattern generates a token using the user’s ID, timestamp, and nonce and validates the submitted token by validating it and comparing any potentially stored user ID to the submitted one and checking that the timestamp is still valid. The HMAC pattern is similar to the encryption pattern with the exception that it uses a strong HMAC function and includes an additional field for the operation a request is performing.

Another stateless pattern to mitigate CSRF attacks is origin verification. This mechanism works by determining the origin of a request submitted to the server and comparing it against the origin of the request’s destination. If they both match the server accepts the request as valid and if they do not the server throws the request away. The server determines the origin of a request by first using the Origin header, if it is present. The reason for it’s priority is that it will be present in HTTP requests that originate from a HTTPS URL; unlike the Referer header. However, if the Origin header is not present the Referer header is used as a fallback and if neither are present the request will be blocked. The request’s destination can be determined in a number of ways including adding target origin knowledge to your application, using the Host header, or using the X-Forwarded-Host header. The addition of target origin logic into the application directly is the most secure approach as it lives on the server and therefore is a trusted value. Reliance on headers can seem like a potential pitfall but this pattern works because the relied on headers can only be set by browsers as they are on the forbidden headers list. This means that they cannot be altered programmatically by a malicious third-party. This pattern has drawbacks including its reliance on the behavior of external technology and the potential for the Origin header not being included or being set to null.

One more promising mitigation pattern is utilizing the Samesite cookie attribute. This attribute on a cookie can be set to either Strict or Lax. If set to Strict browsers will not attach the cookie to any cross-site requests. If set to Lax the cookie would be attached to any top-level navigation cross-site request that uses a “safe” HTTP method. However, this approach has some known exploits such as malicious third-parties triggering top-level navigations or opening new windows and prerendering. Also, this attribute is relatively new and therefore is not widely supported by all browsers.

Chosen Mitigation Pattern

When assessing both potential mitigation patterns you can see some obvious pros for token based patterns. It is the recommended OWASP approach, it is the most secure, it has the least number of dependencies on assumptions from external technologies. However, it’s main drawback is that it relies on developers remembering to incorporate generated tokens in forms when submitted. This additional overhead in large scale operations is likely to result in engineers forgetting to submit this token. Moving concern from services and into the Envoy layer entirely adds the advantage that CSRF can be dropped into existing architectures that utilize Envoy for their networking layer. Additionally, this eliminates the need to manage handling of key rotation when the secret key is swapped out. While it’s not ideal to rely on behavior of external technology (namely the Origin header) since this header is relied on for other functionality it seems like a safe assumption this value won’t change in the near future.

Implementation

An HTTP filter will be added to Envoy that will parse incoming requests to determine their origin. This will be done by first checking for the presence of an Origin header and grabbing it’s value if present. In the absence of this header the filter will check for the Referer header and, if available, grab it’s value. In the scenario where both are absent the request will be rejected. Upon rejection a 403 will be returned to the client. If one or both headers are present we will determine the destination origin using the

The filter assumes that consumers aren’t using GET requests for state changing operations, which would violate RFC2616, section 9.1.1.

Derek Schaller added 3 commits April 2, 2019 17:17
Signed-off-by: Derek Schaller <dschaller@lyft.com>
Signed-off-by: Derek Schaller <dschaller@lyft.com>
Signed-off-by: Derek Schaller <dschaller@lyft.com>
@mattklein123
Copy link
Member

@jmarantz any interest in doing first pass on this? I can do second pass.

@mattklein123 mattklein123 self-assigned this Apr 3, 2019
Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

starting from some API level comment.

api/envoy/api/v2/route/route.proto Outdated Show resolved Hide resolved
Signed-off-by: Derek Schaller <dschaller@lyft.com>
@dschaller
Copy link
Member Author

@lizan thanks for the review. Pushed an update to use per_route_config instead of adding a csrf field on the route config.

Signed-off-by: Derek Schaller <dschaller@lyft.com>
@dschaller
Copy link
Member Author

@lizan @mattklein123 any chance I can get some eyes on this? :)

@jmarantz
Copy link
Contributor

I'm taking a pass now; sorry for the delay.

Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some really great doc in here. You probably said this in the doc somewhere, but the threat model here is social engineering to trick users into clicking on evil URLs that run unexpected requests on sites they have cookies for, and Envoy can provide protection for this if it's used anywhere in the path between client and origin (or ultimate upstream). Is that right?

api/envoy/config/filter/http/csrf/v2/csrf.proto Outdated Show resolved Hide resolved
docs/root/configuration/http_filters/csrf_filter.rst Outdated Show resolved Hide resolved
docs/root/configuration/http_filters/csrf_filter.rst Outdated Show resolved Hide resolved
docs/root/configuration/http_filters/csrf_filter.rst Outdated Show resolved Hide resolved
docs/root/configuration/http_filters/csrf_filter.rst Outdated Show resolved Hide resolved
test/extensions/filters/http/csrf/csrf_filter_test.cc Outdated Show resolved Hide resolved
test/extensions/filters/http/csrf/csrf_filter_test.cc Outdated Show resolved Hide resolved
test/extensions/filters/http/csrf/csrf_filter_test.cc Outdated Show resolved Hide resolved
@dschaller
Copy link
Member Author

@jmarantz thanks for the review!

the threat model here is social engineering to trick users into clicking on evil URLs that run unexpected requests on sites they have cookies for, and Envoy can provide protection for this if it's used anywhere in the path between client and origin (or ultimate upstream). Is that right?

That's the gist of it. More specifically the unexpected requests generally involve modification requests to user's account through put, post, or delete methods.

Signed-off-by: Derek Schaller <dschaller@lyft.com>
Signed-off-by: Derek Schaller <dschaller@lyft.com>
Derek Schaller added 3 commits April 12, 2019 14:33
Signed-off-by: Derek Schaller <dschaller@lyft.com>
Signed-off-by: Derek Schaller <dschaller@lyft.com>
Signed-off-by: Derek Schaller <dschaller@lyft.com>
@dschaller
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #6470 (comment) was created by @dschaller.

see: more, trace.

Signed-off-by: Derek Schaller <dschaller@lyft.com>
@dschaller
Copy link
Member Author

@mattklein123 any chance you can take a second pass over this?

@dschaller
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: release (failed build)

🐱

Caused by: a #6470 (comment) was created by @dschaller.

see: more, trace.

@dschaller
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: release (failed build)

🐱

Caused by: a #6470 (comment) was created by @dschaller.

see: more, trace.

Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you merge latest master to see if the release test flakiness is fixed?

source/extensions/filters/http/csrf/csrf_filter.h Outdated Show resolved Hide resolved
Derek Schaller added 3 commits April 18, 2019 22:18
Signed-off-by: Derek Schaller <dschaller@lyft.com>
Signed-off-by: Derek Schaller <dschaller@lyft.com>
Signed-off-by: Derek Schaller <dschaller@lyft.com>
@dschaller
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #6470 (comment) was created by @dschaller.

see: more, trace.

Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, per extension policy I think we need an entry in CODEOWNERS, @dschaller did you discuss with @mattklein123 for the extension sponsorship?

Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yanavlasov who is interested in security topics.

I think the PR looks pretty good (modulo trivial nits) but I'm not entirely clear what threat-model this is protecting from.

source/extensions/filters/http/csrf/csrf_filter.cc Outdated Show resolved Hide resolved
source/extensions/filters/http/csrf/csrf_filter.cc Outdated Show resolved Hide resolved
source/extensions/filters/http/csrf/csrf_filter.cc Outdated Show resolved Hide resolved

class CsrfFilterTest : public testing::Test {
public:
CsrfFilterConfigSharedPtr setupConfig() {
Copy link
Contributor

@yanavlasov yanavlasov Apr 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method references member variables, which means they have to be defined before the config_ variable. Could you leave a comment about the correct order of member definitions, please?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on what you're asking for? The proto api documentation should include the fields that are required for using this filter. From a technical standpoint there is no preferred order to these setters.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have implicit order of definition of member variables of the CsrfFilterTest class. For instance the filter_ must be defined after config_ which must be defined after stats_ and runtime_ members. Otherwise filter_ or config_ would be initialized using references to uninitialized objects. If someone accidentally changes that order the test could become flaky or crash.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering how someone could change the order. The filter requires a configuration, the configuration requires a policy, stat prefix string, stat object, and runtime object. I could be wrong but I think the test would fail if I changed the ordering of any of these. Does that sound right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do this all over the place so I think this is fine.

Copy link
Contributor

@yanavlasov yanavlasov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. It would be interesting to see case study for deploying this filter for an already existing app.

@dschaller
Copy link
Member Author

dschaller commented Apr 19, 2019

Thanks for the review @lizan, @jmarantz, and @yanavlasov.

@lizan - I did not discuss extension sponsorship with @mattklein123. Let me chat with him offline about this.

@jmarantz can I help clarify anything around what threat-model this is protecting? I attempted to address this in the description but it's possible that I didn't do so in a clear way. :)

@yanavlasov - I could look into that. My goal after adding this filter is to deploy it to our cluster at Lyft, replacing the existing distributed CSRF logic we have today.

Derek Schaller added 2 commits April 19, 2019 15:41
Signed-off-by: Derek Schaller <dschaller@lyft.com>
Signed-off-by: Derek Schaller <dschaller@lyft.com>
@jmarantz
Copy link
Contributor

RE threat-model -- I read your excellent PR description but I was trying to place myself in the position of an attacker, submitting a forged request on the user's behalf. How would that be prevented?

I think in the past I've relied on an encrypted token generated when displaying a form, which expires after some amount of time. That way anyone intercepting the HTTP POST could modify and play it back, but that attack would last only as long as we chose to make the timeout.

If an attacker somehow sniffs an HTTP POST in this scenario, what stops the attacker from playing back an altered form of it, including all the request headers you are utilizing? I don't understand how browsers disallowing modification of some headers would help -- the HTTP client doing the attack need not be a browser that cares about such restrictions.

@dschaller
Copy link
Member Author

Thanks for the reading through that @jmarantz. :) I'm going to try to address everything but let me know if I miss something.

CSRF is an attack that only occurs within a user's web browser. This is primarily because requests that enforce CSRF protection (PUT, POST, and DELETE) require authentication, which usually stores user data in the user's session or as secure cookies. Since most sites automatically include the sensitive data provided by authenticating in all browser requests there is no way to know if the request containing the user's data is coming from a trusted source or a malicious one. CSRF is not intended to mitigate malicious non-browser based requests. This is because if the malicious party was able to retrieve the required session data and cookies they could make a request on the user's behalf.

Let's take the token-based mitigation pattern that you mentioned above. Generally, the issued tokens are sent with every response as a cookie. If I am able to sniff a user's request and find their session ID I would be able to make a GET request to any endpoint, get a new CSRF token, and make a subsequent POST request on their behalf.

The main purpose of CSRF is to prevent sites from spoofing a request on the user's behalf within a browser. For example, if a user ventures onto my malicious site, which appears harmless to them, I could dispatch requests from the browser to google.com and all the credentials associated with that site would be sent along. The destination, in this case google.com, would consider the request valid without CSRF because there is nothing that says it's not intentional. With the chosen CSRF mitigation pattern applied the request would fail due to the Origin header being set as my site and not google.com.

A real-life example is cited in section 1 of Robust Defenses for Cross-Site Request Forgery.

For example, in late 2007 [42], Gmail had a CSRF vulnerability. When a Gmail user visited a malicious site, the malicious site could generate a request to Gmail that Gmail treated as part of its ongoing session with the victim. In November 2007, a web attacker exploited this CSRF vulnerability to inject an email filter into David Airey’s Gmail account [1].1 This filter forwarded all of David Airey’s email to the attacker’s email address, which allowed the attacker to assume control of davidairey.com because Airey’s domain registrar used email authentication, leading to significant inconvenience and financial loss.

jmarantz
jmarantz previously approved these changes Apr 23, 2019
Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. The threat model you are defending against is when you trick someone to run attacker-controlled JavaScript on a logged-in domain. Separate defenses are needed to avoid being able to sniff requests from outside the browser and forge modifications of them.

Can you include a reference to the quote about gmail in the doc? Otherwise LGTM.

Signed-off-by: Derek Schaller <dschaller@lyft.com>
@dschaller
Copy link
Member Author

Just as an update, I sync'ed with @mattklein123 offline and he is ok with sponsoring this extension.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skimmed through and looks awesome. Great work! Let's ship and iterate if needed. Thank you!

@mattklein123 mattklein123 merged commit eaaa918 into envoyproxy:master Apr 23, 2019
@dschaller dschaller deleted the xsrf-filter branch April 23, 2019 22:39
mpuncel added a commit to mpuncel/envoy that referenced this pull request Apr 24, 2019
* master:
  docs: add extension policy (envoyproxy#6678)
  ext_authz: added ability to detect partial request body data (envoyproxy#6583)
  version_history.rst: jwt_authn change missed 1.10.0 (envoyproxy#6684)
  docs: fix link in pull request template (envoyproxy#6679)
  Explicitly convert absl::string_view to std::string. (envoyproxy#6687)
  docs: improving watermark docs/comments (envoyproxy#6683)
  http filter: add CSRF filter (envoyproxy#6470)
  event: reintroduce dispatcher stats (envoyproxy#6659)
  security: postmortem for CVE-2019-990[01] (envoyproxy#6597)
  Improve build rules for (test only) library quic_port_utils. (envoyproxy#6672)
  spell check: skip unsupported extensions when called with a file (envoyproxy#6648)
  Changed TestHooks to ListenerHooks (envoyproxy#6642)
  proto: move extension-specific linking validation into extensions (envoyproxy#6657)
  stats: add/test heterogenous set of StatNameStorage objects. (envoyproxy#6504)
  docs: move xds protocol to rst (envoyproxy#6670)
  fix version history order (envoyproxy#6671)

Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

XSRF protection filter
5 participants