-
-
Notifications
You must be signed in to change notification settings - Fork 196
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
Securing cross origin requests [CVE-2019-1000022] #137
Comments
From the article:
|
Hi Daniel,
First step would be suggesting a use case where they're not suitable so that we know what we're talking about exactly (why they're unsuitable in this case, etc.).
Is there a reason you wouldn't want to be using Content Security Policy for this? |
UPDATE: ^^This is incorrect, I was mistaken in my understanding of what CSP provided. |
Actually, I think that it might still be important to check |
Okay to close this? |
Sure, for now. I may revisit this in the future. |
So, I'm in the process of a security review on code for a consulting client, and I'm looking at websockets. I've looked at the Sente code now for a couple of hours, and I cannot see how the current implementation is secure. Worse, it looks to me like it is providing a way for an attacker to actually obtain the CSRF token, since sente sends it as part of the handshake...all an attacker needs to do, it seems to me, is know that sente is in use, emulate the handshake logic, and now the attacker not only knows the CSRF, but they can hijack the websocket for a fully async "authorized" communication channel. My understanding is that CSRF only works if the secret is embedded into the HTML (because js cannot request HTML pages cross-origin) and manually transferred into the js network request back to the server. If it can be "queried" by js with nothing but a cookie as backup, then the security is broken. I did read your example, and it is true that requiring a user to auth after the connection is established is the "gold standard" (assuming you require they type a password), but that requires the user to log in again if they reload the page (or temporarily lose the network connection). As far as I can tell the current CSRF logic basically just gets you "around" the anti-forgery middleware denying requests...it doesn't actually provide any real security, and I do think it actually compromises the anti-forgery protections on "other" POSTs. Perhaps you intended for users of the library to implement their own security around this (which might be the case...it requires HTML twiddling that sente cannot do, and origin checks that would require more config). I'm putting this in this closed issue because there is some chance I've misunderstood something. If not, I'd recommend removing the CSRF from the handshake, and documenting that it has to be passed through the HTML as a parameter given to sente on the client. E.g. the HTML serve would embed a js var or something into the head of the page, and cljs would read that var to set the option for sente. |
(Disclaimer: I'm not a security professional.) I've spent the last couple of days looking into WebSocket security and I'm mostly left confused. I don't understand how Sente's anti-CSRF token implementation protects from CSRF attacks. (It might work when using the Ajax long-polling bits of Sente — I haven't looked into that part because I don't use it myself). For example, say I have a web app at https://foo.com and I've logged into it with my browser. Sente is listening in at wss://foo.com/chsk. The web app also has the Ring anti-forgery middleware correctly configured, which means Sente can use the anti-CSRF token. I then open another tab in the same browser and type this into the browser's Developer Console: var ws = new WebSocket("wss://foo.com/chsk?client-id=foo")
ws.onmessage = function (event) {
console.log(event.data);
} Because the WebSocket connection is not subject to the Same-Origin Policy, I can open the connection successfully. The session cookie is passed along with the WebSocket connection request, which means authentication isn't required. The anti-CSRF token Sente uses seems to have no effect. Embedding the anti-CSRF token into the HTML might be better than the current situation, but I don't see how even that would currently prevent CSRF attacks, seeing as Sente doesn't actually seem to require the anti-CSRF token to establish a WebSocket connection. Also, it seems to me that if we did that, we'd be using the anti-CSRF token as a kind of an authorization mechanism, which isn't what the anti-CSRF token is for. I don't really see how the anti-CSRF token is relevant for WebSocket requests, but it's entirely possible that I'm missing something. With regard to CSP: I've seen recommendations to add the That's not true, however. As I understand it, a content-security policy basically says:
In other words, with WebSockets, if your CSP is So, if https://foo.com has an XSS vulnerability that an attacker uses to try to connect to a WebSocket endpoint elsewhere (e.g. to display misleading information to the user or something like that), the `connect-src 'self' CSP would prevent that. As I see it, a CSP does not protect the user from CSRF attacks — at least with regard to WebSockets. The only thing that I'm aware of that protects the user from CSRF attacks targeting WebSocket endpoints is to check that the value of the Origin header matches the current domain in the WebSocket handshake route handler (which is what @danielcompton proposed earlier). For example, a Ring middleware for https://foo.com would look something like this: (defn wrap-ensure-origin
[handler]
(fn [request]
(if (= (get-in request [:headers "origin"]) "https://foo.com")
(handler request)
{:status 403
:body "Forbidden"}))) You'd then wrap that middleware around Sente's If your app runs on multiple different domains, you'll unfortunately have to add the domain names in your app's configuration. That means that the best thing Sente can do is to advise users to add middleware that checks the Origin header into their web apps, I think. That's my understanding of the whole picture. I'm hoping someone will correct me if I'm wrong on something — I'm learning as I go here. |
…compton, @awkay, @eerohele) A pair of CRITICAL security issues were identified by contributors: 1. Sente was leaking its CSRF token from its WebSocket handshake route. And since in the common case, this is a shared token also used by the rest of the application, this means that Sente was often in practice leaking the application's CSRF token. 2. No CSRF protection was being provided for WebSocket handshakes. This commit makes the following changes- 1. [BREAKING] The client-side :chsk/handshake event now always has `nil` where it once provided the csrf-token provided by the server. I.e. before: `[:chsk/handshake [<?uid> <csrf-token> <?handshake-data> <first-handshake?>]] after: `[:chsk/handshake [<?uid> nil <?handshake-data> <first-handshake?>]] 2. [BREAKING] `make-channel-socket-client!` now takes an extra argment: an explicit csrf-token. The value for the token should be extracted from the page HTML (see example project). 3. CSRF *checks* are now performed by Sente directly, and don't depend on an external route wrapper like `ring-anti-forgery`, etc. 4. CSRF checks now cover all Sente's internal endpoints, including Ajax POSTs, long-polling requests, and WebSocket handshakes. 5. Sente will now by default fail to work without CSRF tokens properly configured.
Just pushed an attempted minimal fix, including an updated example project. It's on Clojars as This doesn't do any origin checking (yet), but should prevent the CSRF leakage - and extends the CSRF check over all endpoints, including the WebSocket handshake. The commit is small if anyone wants to help check. I'd also be open to a PR that adds a convenient way to do origin checking. Perhaps a predicate of the Origin header? Any input welcome. Again, much thanks for bringing attention to this. |
I started evaluating the change, but I'm having trouble setting up the requisite middleware correctly (not Sente's fault). I'll keep at it, but in the meantime: I think v1.2.0 already exists, right? So I think the version number should be something different. |
Just some preliminary results: the new CSRF scheme seems to work OK in that I can't connect to a WebSocket endpoint from a different origin without the anti-CSRF token any more. However, for some reason, with 1.2.0-SNAPSHOT, I can't connect to a WebSocket endpoint e.g. with wscat even if I supply the correct anti-CSRF token in the query string: $ wscat --connect "ws://localhost:8000/chsk?client-id=46563081-259b-4ddb-a21f-af8b8eb87256&csrf-token=<redacted>"
error: Unexpected server response: 403 With 1.13.1, that obviously works because Sente didn't check the anti-CSRF token. I didn't have the time to check why I get that 403 yet. It's not a problem for my use case, but might be for someone else. For example, if you're testing your WebSocket endpoints with something like Gniazdo. |
So, this like is for promotion and reconnect, right? 442a347?w=1#diff-793a4aa02e2757c465c85e63ff89f346R1040 Headers are used on the initial part now it seems. If I'm reading that right, then I agree. My workaround on the current version was forced to pass the token as a param on setup, which isn't ideal. I don't have time to actually run any live tests on it right now, but thanks for looking into it! |
…compton, @awkay, @eerohele) A pair of CRITICAL security issues were identified by contributors: 1. Sente was leaking its CSRF token from its WebSocket handshake route. And since in the common case, this is a shared token also used by the rest of the application, this means that Sente was often in practice leaking the application's CSRF token. 2. No CSRF protection was being provided for WebSocket handshakes. This commit makes the following changes- 1. [BREAKING] The client-side :chsk/handshake event now always has `nil` where it once provided the csrf-token provided by the server. I.e. before: `[:chsk/handshake [<?uid> <csrf-token> <?handshake-data> <first-handshake?>]] after: `[:chsk/handshake [<?uid> nil <?handshake-data> <first-handshake?>]] 2. [BREAKING] `make-channel-socket-client!` now takes an extra argment: an explicit csrf-token. The value for the token should be extracted from the page HTML (see example project). 3. CSRF *checks* are now performed by Sente directly, and don't depend on an external route wrapper like `ring-anti-forgery`, etc. 4. CSRF checks now cover all Sente's internal endpoints, including Ajax POSTs, long-polling requests, and WebSocket handshakes. 5. Sente will now by default fail to work without CSRF tokens properly configured.
Have pushed If there's no issues with this release, will document further and release as |
…compton, @awkay, @eerohele) A pair of CRITICAL security issues were identified by contributors: 1. Sente was leaking its CSRF token from its WebSocket handshake route. And since in the common case, this is a shared token also used by the rest of the application, this means that Sente was often in practice leaking the application's CSRF token. 2. No CSRF protection was being provided for WebSocket handshakes. This commit makes the following changes- 1. [BREAKING] The client-side :chsk/handshake event now always has `nil` where it once provided the csrf-token provided by the server. I.e. before: `[:chsk/handshake [<?uid> <csrf-token> <?handshake-data> <first-handshake?>]] after: `[:chsk/handshake [<?uid> nil <?handshake-data> <first-handshake?>]] 2. [BREAKING] `make-channel-socket-client!` now takes an extra argment: an explicit csrf-token. The value for the token should be extracted from the page HTML (see example project). 3. CSRF *checks* are now performed by Sente directly, and don't depend on an external route wrapper like `ring-anti-forgery`, etc. 4. CSRF checks now cover all Sente's internal endpoints, including Ajax POSTs, long-polling requests, and WebSocket handshakes. 5. Sente will now by default fail to work without CSRF tokens properly configured.
It would be good to get a CVE-number (Common Vulnerabilities and Exposures) for this, so developers using lein-nvd or similar tool would be notified about this. |
@korkeala Nice idea, thank you for the suggestion. I've just requested a CVE through the Distributed Weakness Filing Project. I'll update here (and test |
Great, that project looks good (though I'm not familiar with it). One request should be sufficient, there is only one central vulnerability database (https://nvd.nist.gov/) which the dependency check tools use to download CVE-data. So as long as the request ends up there, it will be enough. |
The docstring for
I think the Sente example project should be updated to reflect that. That is: --- a/example-project/src/example/server.clj
+++ b/example-project/src/example/server.clj
@@ -66,7 +66,7 @@
(defn landing-pg-handler [ring-req]
(hiccup/html
[:h1 "Sente reference example"]
- [:div#sente-csrf-token {:data-csrf-token anti-forgery/*anti-forgery-token*}]
+ [:div#sente-csrf-token {:data-csrf-token (force anti-forgery/*anti-forgery-token*)}]
[:p "An Ajax/WebSocket" [:strong " (random choice!)"] " has been configured for this example"]
[:hr]
[:p [:strong "Step 1: "] " try hitting the buttons:"] Alternatively, the Ring anti-forgery middleware also stores the anti-CSRF token in the request, so this should also work: --- a/example-project/src/example/server.clj
+++ b/example-project/src/example/server.clj
@@ -66,7 +66,7 @@
(defn landing-pg-handler [ring-req]
(hiccup/html
[:h1 "Sente reference example"]
- [:div#sente-csrf-token {:data-csrf-token anti-forgery/*anti-forgery-token*}]
+ [:div#sente-csrf-token {:data-csrf-token (:anti-forgery-token ring-req)}]
[:p "An Ajax/WebSocket" [:strong " (random choice!)"] " has been configured for this example"]
[:hr]
[:p [:strong "Step 1: "] " try hitting the buttons:"] The benefit of retrieving the token from the request is that you don't need to think about whether Other than that, the current implementation seems to work insofar as the handshake no longer leaks the anti-CSRF token. You also can't connect to the WebSocket endpoint without the token. However, after this change, Sente can no longer work with (many) non-browser WebSocket clients. Unless I'm mistaken (and it's entirely possible that I am), the current CSRF protection scheme relies on the browser being able to hold on to a session that contains the anti-CSRF token, but many non-browser clients don't have that ability. I'm not sure how big of a deal that is. It does mean that since the anti-CSRF token is the only means of CSRF protection and it's mandatory, you can't test Sente WebSocket endpoints with a tool like wscat or Gniazdo any more. Using the $ wscat --help
Usage: wscat [options] (--listen <port> | --connect <url>)
Options:
...
-o, --origin <origin> optional origin
... |
Thanks for the response @eerohele! Example updated. PR welcome to add Origin header verification 👍 |
FYI, `system' at "0.4.3-SNAPSHOT" is incorporating the latest recommendation for the anti forgery token. Thank you. |
Just a note that to further improve security, Sente should pass the anti-CSRF token as part of Sec-WebSocket-Protocol header (see https://stackoverflow.com/a/35108078/232644) and not as a request parameter as it currently does. That would likely require some custom changes to ring handlers, but perhaps it'd be better for Sente to handle (more of) the verification directly. Might warrant a different issue. |
To use origin/referrer checking instead of an anti-CSRF token, disable CSRF token checking and set the list of origins that are allowed to connect to your WebSocket endpoint: (sente/make-channel-socket-server! (get-sch-adapter) {:csrf-token-fn nil :allowed-origins #{"http://site1.com" "http://site2.com"}) The current implementation checks both the Origin and the Referer header as per the OWASP CSRF Prevention Cheat Sheet.
@kaosko Hi Kalle, open to a PR and/or new-issue if you have an idea in mind- thanks! |
Have created #418 for the |
From https://www.christian-schneider.net/CrossSiteWebSocketHijacking.html, if somebody wasn't using CSRF tokens, it seems like it would be possible for any malicious website to open up a web socket to do Bad Things. I know that CSRF tokens are highly recommended, but they're not suitable for all cases (I think). It could be good to also add CORS style protection to Sente to allow only whitelisted origins. What are your thoughts?
N.B. CORS itself has no influence on websocket connections.
The text was updated successfully, but these errors were encountered: