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

Add support for origin/referrer checking #338

Closed
wants to merge 5 commits into from
Closed

Add support for origin/referrer checking #338

wants to merge 5 commits into from

Conversation

eerohele
Copy link

I've tried to follow the instructions in the OWASP CSRF Prevention Cheat Sheet to the best of my ability, but I'd really appreciate another set of eyes on this.

Unit tests for bad-origin? might be nice, but since the project currently has no unit tests, I didn't want to add any. I added some test cases inside a (comment) form, though. The implementation could also be more concise, but I attempted to make the conditions as clear as possible.

You can test the implementation with the example project by running lein install in the Sente root directory and making changes like this like this:

diff --git a/example-project/project.clj b/example-project/project.clj
index 4b01a0e..82d0bd8 100644
--- a/example-project/project.clj
+++ b/example-project/project.clj
@@ -15,7 +15,7 @@
    [org.clojure/core.async    "0.4.490"]
    [org.clojure/tools.nrepl   "0.2.13"] ; Optional, for Cider

-   [com.taoensso/sente        "1.14.0-RC1"] ; <--- Sente
+   [com.taoensso/sente        "1.14.0-RC2"] ; <--- Sente
    [com.taoensso/timbre       "4.10.0"]

    ;;; TODO Choose (uncomment) a supported web server -----------------------
@@ -41,7 +41,6 @@
   :plugins
   [[lein-pprint         "1.2.0"]
    [lein-ancient        "0.6.15"]
+   ;; If the Austin dependency is enabled, starting a Leiningen REPL fails with an NPE
+   ;; on Leiningen 2.9.1.
-   [com.cemerick/austin "0.1.6"]
    [lein-cljsbuild      "1.1.7"]
    [cider/cider-nrepl   "0.21.0"]] ; Optional, for use with Emacs

diff --git a/example-project/src/example/server.clj b/example-project/src/example/server.clj
index 8fbb0d7..6048cc3 100644
--- a/example-project/src/example/server.clj
+++ b/example-project/src/example/server.clj
@@ -42,7 +42,9 @@

       chsk-server
       (sente/make-channel-socket-server!
-       (get-sch-adapter) {:packer packer})
+       (get-sch-adapter) {:packer packer
+                          :csrf-token-fn nil
+                          :allowed-origins #{"http://localhost:50000"}})

       {:keys [ch-recv send-fn connected-uids
               ajax-post-fn ajax-get-or-ws-handshake-fn]}
@@ -245,7 +247,7 @@
     (reset! web-server_ stop-fn)))

 (defn stop!  []  (stop-router!)  (stop-web-server!))
-(defn start! [] (start-router!) (start-web-server!) (start-example-broadcaster!))
+(defn start! [] (start-router!) (start-web-server! 50000) (start-example-broadcaster!))

 (defn -main "For `lein run`, etc." [] (start!))

After the changes, the example project should work as before.

You can then open another browser tab on http://neverssl.com or some other non-HTTPS site, open the developer console, and do something like this:

var ws = new WebSocket("ws://localhost:50000/chsk?client-id=19f0a64d-e753-4894-b421-745547189d57");

That should throw an error because http://neverssl.com isn't an allowed origin. You can then try adding http://neverssl.com (no trailing slash) into :allowed-origins and try again. It should then allow the connection.

ptaoussanis and others added 5 commits January 23, 2019 19:59
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.
Prior to updating, running Leiningen 2.9.1 fails with this error:

  Syntax error compiling var at (cider/nrepl/middleware/pprint.clj:74:3)
@ptaoussanis
Copy link
Member

Merging manually, thanks for this!

ptaoussanis pushed a commit that referenced this pull request Oct 19, 2019
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.
@eerohele eerohele deleted the check-origin branch October 19, 2019 14:54
@drewverlee
Copy link

drewverlee commented Aug 17, 2022

Given a malicious non-browser client can supply any origin it wants would mean the server checking that origin would send the malicious client the sensitive cookies?

The OWASP cheatsheet says "browser" clients can't forge the origin, but not that other clients can't.

However... sense the client isn't the browser, which has the sensitive cookies, this malicious client wouldn't get said cookies.

Which is what this author concludes as well

A non-browser client, however, cannot access the session cookies stored in your browser. Even if the attacker spoofs Origin, their request will be denied because they’re not authenticated.

Though now i'm worried what they mean by "not authenticated" as i thought the vulnerability was during the unauthenticated phase as per this quote in the change log:

Bug: Previous versions of Sente were leaking the server-side CSRF token to the client during the (unauthenticated) WebSocket handshake process.

@ptaoussanis
Copy link
Member

@drewverlee Hi Drew! I'm not 100% sure that I understand what you're asking/suggesting - could you please rephrase? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants