Skip to content

Commit

Permalink
[#137] NB SECURITY FIX, BREAKING: fix broken CSRF protection (@daniel…
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
ptaoussanis committed Dec 2, 2018
1 parent 41a74e1 commit ae3afd5
Show file tree
Hide file tree
Showing 5 changed files with 131 additions and 75 deletions.
10 changes: 5 additions & 5 deletions example-project/project.clj
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
(defproject com.taoensso.examples/sente "1.13.0"
(defproject com.taoensso.examples/sente "1.14.0-SNAPSHOT"
:description "Sente, reference web-app example project"
:url "https://github.com/ptaoussanis/sente"
:license {:name "Eclipse Public License"
Expand All @@ -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.13.1"] ; <--- Sente
[com.taoensso/sente "1.14.0-SNAPSHOT"] ; <--- Sente
[com.taoensso/timbre "4.10.0"]

;;; TODO Choose (uncomment) a supported web server -----------------------
Expand All @@ -25,16 +25,16 @@
;; [aleph "0.4.1"]
;; -----------------------------------------------------------------------

[ring "1.6.3"]
[ring "1.7.1"]
[ring/ring-defaults "0.3.2"] ; Includes `ring-anti-forgery`, etc.
;; [ring-anti-forgery "1.0.0"]
;; [ring-anti-forgery "1.3.0"]

[compojure "1.6.1"] ; Or routing lib of your choice
[hiccup "1.0.5"] ; Optional, just for HTML

;;; Transit deps optional; may be used to aid perf. of larger data payloads
;;; (see reference example for details):
[com.cognitect/transit-clj "0.8.309"]
[com.cognitect/transit-clj "0.8.313"]
[com.cognitect/transit-cljs "0.8.256"]]

:plugins
Expand Down
9 changes: 9 additions & 0 deletions example-project/src/example/client.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,14 @@

;;;; Define our Sente channel socket (chsk) client

(def ?csrf-token
(when-let [el (.getElementById js/document "sente-csrf-token")]
(.getAttribute el "data-csrf-token")))

(if ?csrf-token
(->output! "CSRF token detected in HTML, great!")
(->output! "CSRF token NOT detected in HTML, default Sente config will reject requests"))

(let [;; For this example, select a random protocol:
rand-chsk-type (if (>= (rand) 0.5) :ajax :auto)
_ (->output! "Randomly selected chsk type: %s" rand-chsk-type)
Expand All @@ -41,6 +49,7 @@
{:keys [chsk ch-recv send-fn state]}
(sente/make-channel-socket-client!
"/chsk" ; Must match server Ring routing URL
?csrf-token
{:type rand-chsk-type
:packer packer})]

Expand Down
7 changes: 6 additions & 1 deletion example-project/src/example/server.clj
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
(:require
[clojure.string :as str]
[ring.middleware.defaults]
[ring.middleware.anti-forgery :as anti-forgery]
[compojure.core :as comp :refer (defroutes GET POST)]
[compojure.route :as route]
[hiccup.core :as hiccup]
Expand Down Expand Up @@ -65,6 +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*}]
[:p "An Ajax/WebSocket" [:strong " (random choice!)"] " has been configured for this example"]
[:hr]
[:p [:strong "Step 1: "] " try hitting the buttons:"]
Expand Down Expand Up @@ -116,7 +118,10 @@
"**NB**: Sente requires the Ring `wrap-params` + `wrap-keyword-params`
middleware to work. These are included with
`ring.middleware.defaults/wrap-defaults` - but you'll need to ensure
that they're included yourself if you're not using `wrap-defaults`."
that they're included yourself if you're not using `wrap-defaults`.
You're also STRONGLY recommended to use `ring.middleware.anti-forgery`
or something similar."
(ring.middleware.defaults/wrap-defaults
ring-routes ring.middleware.defaults/site-defaults))

Expand Down
4 changes: 2 additions & 2 deletions project.clj
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
(defproject com.taoensso/sente "1.13.1"
(defproject com.taoensso/sente "1.14.0-SNAPSHOT"
:author "Peter Taoussanis <https://www.taoensso.com>"
:description "Realtime web comms for Clojure/Script"
:url "https://github.com/ptaoussanis/sente"
Expand All @@ -13,7 +13,7 @@
:dependencies
[[org.clojure/clojure "1.9.0"]
[org.clojure/core.async "0.4.490"]
[com.taoensso/encore "2.102.0"]
[com.taoensso/encore "2.105.0"]
[org.clojure/tools.reader "1.3.2"]
[com.taoensso/timbre "4.10.0"]]

Expand Down
Loading

0 comments on commit ae3afd5

Please sign in to comment.