From dd6982f0ce0cb30167d5bcf152e8971a20526915 Mon Sep 17 00:00:00 2001 From: Peter Taoussanis Date: Fri, 2 Nov 2018 10:45:21 +0100 Subject: [PATCH] [#137] NB SECURITY FIX, BREAKING: fix broken CSRF protection (@danielcompton, @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 [ ]] after: `[:chsk/handshake [ nil ]] 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. --- example-project/project.clj | 10 +- example-project/src/example/client.cljs | 9 ++ example-project/src/example/server.clj | 7 +- project.clj | 4 +- src/taoensso/sente.cljc | 176 +++++++++++++++--------- 5 files changed, 131 insertions(+), 75 deletions(-) diff --git a/example-project/project.clj b/example-project/project.clj index be33b72..d3ac7f6 100644 --- a/example-project/project.clj +++ b/example-project/project.clj @@ -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" @@ -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 ----------------------- @@ -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 diff --git a/example-project/src/example/client.cljs b/example-project/src/example/client.cljs index 7cfa629..8f6f96f 100644 --- a/example-project/src/example/client.cljs +++ b/example-project/src/example/client.cljs @@ -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) @@ -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})] diff --git a/example-project/src/example/server.clj b/example-project/src/example/server.clj index b8f945b..f72d4df 100644 --- a/example-project/src/example/server.clj +++ b/example-project/src/example/server.clj @@ -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] @@ -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:"] @@ -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)) diff --git a/project.clj b/project.clj index bbdef31..6f504f9 100644 --- a/project.clj +++ b/project.clj @@ -1,4 +1,4 @@ -(defproject com.taoensso/sente "1.13.1" +(defproject com.taoensso/sente "1.14.0-SNAPSHOT" :author "Peter Taoussanis " :description "Realtime web comms for Clojure/Script" :url "https://github.com/ptaoussanis/sente" @@ -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"]] diff --git a/src/taoensso/sente.cljc b/src/taoensso/sente.cljc index 81765a8..8f0bc78 100644 --- a/src/taoensso/sente.cljc +++ b/src/taoensso/sente.cljc @@ -29,7 +29,7 @@ * Callback replies: :chsk/closed, :chsk/timeout, :chsk/error * Client-side events: - [:chsk/handshake [ ]] + [:chsk/handshake [ nil[4] ]] [:chsk/state [ ]] [:chsk/recv ] ; Server>user push [:chsk/ws-ping] @@ -47,7 +47,6 @@ :ever-opened? - Truthy iff chsk handshake has ever completed successfully :first-open? - Truthy iff chsk just completed first successful handshake :uid - User id provided by server on handshake, or nil - :csrf-token - CSRF token provided by server on handshake, or nil :handshake-data - Arb user data provided by server on handshake :last-ws-error - ?{:udt _ :ev } :last-ws-close - ?{:udt _ :ev @@ -74,7 +73,10 @@ * Single HTTP req+session persists over entire chsk session but cannot modify sessions! Use standard a/sync HTTP Ring req/resp for logins, etc. * Easy to wrap standard HTTP Ring resps for transport over chsks. Prefer - this approach to modifying handlers (better portability)." + this approach to modifying handlers (better portability). + + [4] Used to be a csrf-token. Was removed in v1.14 for security reasons. + A `nil` remains for semi-backwards-compatibility with pre-v1.14 clients." {:author "Peter Taoussanis (@ptaoussanis)"} @@ -101,8 +103,8 @@ [taoensso.sente :as sente-macros :refer (elide-require)]))) (if (vector? taoensso.encore/encore-version) - (enc/assert-min-encore-version [2 79 1]) - (enc/assert-min-encore-version 2.79)) + (enc/assert-min-encore-version [2 105 0]) + (enc/assert-min-encore-version 2.105)) (def sente-version "Useful for identifying client/server mismatch" [1 11 0]) @@ -273,8 +275,9 @@ :connected-uids ; Watchable, read-only (atom {:ws #{_} :ajax #{_} :any #{_}}). Common options: - :user-id-fn ; (fn [ring-req]) -> unique user-id for server>user push. - :csrf-token-fn ; (fn [ring-req]) -> CSRF token for Ajax POSTs. + :user-id-fn ; (fn [ring-req]) -> unique user-id for server>user push. + :csrf-token-fn ; ?(fn [ring-req]) -> CSRF-token for Ajax POSTs and WS handshake. + ; CSRF check will be skipped iff nil (NOT RECOMMENDED!). :handshake-data-fn ; (fn [ring-req]) -> arb user data to append to handshake evs. :ws-kalive-ms ; Ping to keep a WebSocket conn alive if no activity ; w/in given msecs. Should be different to client's :ws-kalive-ms. @@ -296,18 +299,21 @@ [web-server-ch-adapter & [{:keys [recv-buf-or-n ws-kalive-ms lp-timeout-ms send-buf-ms-ajax send-buf-ms-ws - user-id-fn csrf-token-fn handshake-data-fn packer] + user-id-fn bad-csrf-fn csrf-token-fn handshake-data-fn packer] :or {recv-buf-or-n (async/sliding-buffer 1000) ws-kalive-ms (enc/ms :secs 25) ; < Heroku 55s timeout lp-timeout-ms (enc/ms :secs 20) ; < Heroku 30s timeout send-buf-ms-ajax 100 send-buf-ms-ws 30 user-id-fn (fn [ring-req] (get-in ring-req [:session :uid])) + bad-csrf-fn (fn [ring-req] {:status 403 :body "Bad CSRF token"}) csrf-token-fn (fn [ring-req] (or (:anti-forgery-token ring-req) (get-in ring-req [:session :csrf-token]) (get-in ring-req [:session :ring.middleware.anti-forgery/anti-forgery-token]) - (get-in ring-req [:session "__anti-forgery-token"]))) + (get-in ring-req [:session "__anti-forgery-token"]) + #_:sente/no-reference-csrf-token + )) handshake-data-fn (fn [ring-req] nil) packer :edn}}]] @@ -484,6 +490,26 @@ ;; undefined): nil) + bad-csrf? + (fn [ring-req] + (if (nil? csrf-token-fn) ; Provides a way to disable CSRF check + false + (if-let [reference-csrf-token (csrf-token-fn ring-req)] + (let [csrf-token-from-client + (or + (get-in ring-req [:params :csrf-token]) + (get-in ring-req [:headers "x-csrf-token"]) + (get-in ring-req [:headers "x-xsrf-token"]))] + + (not + (enc/const-str= + reference-csrf-token + csrf-token-from-client))) + + true ; By default fail if no CSRF token + ))) + + ev-msg-const {:ch-recv ch-recv :send-fn send-fn @@ -496,39 +522,44 @@ ;; Does not participate in `conns_` (has specific req->resp) :ajax-post-fn (fn [ring-req] - (interfaces/ring-req->server-ch-resp web-server-ch-adapter ring-req - {:on-open - (fn [server-ch websocket?] - (assert (not websocket?)) - (let [params (get ring-req :params) - ppstr (get params :ppstr) - client-id (get params :client-id) - [clj has-cb?] (unpack packer ppstr) - reply-fn - (let [replied?_ (atom false)] - (fn [resp-clj] ; Any clj form - (when (compare-and-set! replied?_ false true) - (tracef "Chsk send (ajax post reply): %s" resp-clj) - (interfaces/sch-send! server-ch websocket? - (pack packer resp-clj)))))] - - (put-server-event-msg>ch-recv! ch-recv - (merge ev-msg-const - {;; Note that the client-id is provided here just for the - ;; user's convenience. non-lp-POSTs don't actually need a - ;; client-id for Sente's own implementation: - :client-id client-id #_"unnecessary-for-non-lp-POSTs" - :ring-req ring-req - :event clj - :uid (user-id-fn ring-req client-id) - :?reply-fn (when has-cb? reply-fn)})) - - (if has-cb? - (when-let [ms lp-timeout-ms] - (go - (server-ch-resp web-server-ch-adapter ring-req + {:on-open + (fn [server-ch websocket?] + (assert (not websocket?)) + (let [params (get ring-req :params) + ppstr (get params :ppstr) + client-id (get params :client-id) + [clj has-cb?] (unpack packer ppstr) + reply-fn + (let [replied?_ (atom false)] + (fn [resp-clj] ; Any clj form + (when (compare-and-set! replied?_ false true) + (tracef "Chsk send (ajax post reply): %s" resp-clj) + (interfaces/sch-send! server-ch websocket? + (pack packer resp-clj)))))] + + (put-server-event-msg>ch-recv! ch-recv + (merge ev-msg-const + {;; Note that the client-id is provided here just for the + ;; user's convenience. non-lp-POSTs don't actually need a + ;; client-id for Sente's own implementation: + :client-id client-id #_"unnecessary-for-non-lp-POSTs" + :ring-req ring-req + :event clj + :uid (user-id-fn ring-req client-id) + :?reply-fn (when has-cb? reply-fn)})) + + (if has-cb? + (when-let [ms lp-timeout-ms] + (go + (server-ch-resp web-server-ch-adapter ring-req {:on-open (fn [server-ch websocket?] @@ -873,7 +909,7 @@ (have? [:el #{:ws :ajax}] chsk-type) (have? handshake? clj) (tracef "receive-handshake! (%s): %s" chsk-type clj) - (let [[_ [?uid ?csrf-token ?handshake-data]] clj + (let [[_ [?uid _ ?handshake-data]] clj {:keys [chs ever-opened?_]} chsk first-handshake? (compare-and-set! ever-opened?_ false true) new-state @@ -881,18 +917,14 @@ :open? true :ever-opened? true :uid ?uid - :csrf-token ?csrf-token :handshake-data ?handshake-data :first-open? first-handshake?} handshake-ev [:chsk/handshake - [?uid ?csrf-token ?handshake-data first-handshake?]]] + [?uid nil ?handshake-data first-handshake?]]] (assert-event handshake-ev) - (when (str/blank? ?csrf-token) - (warnf "SECURITY WARNING: no CSRF token available for use by Sente")) - (swap-chsk-state! chsk #(merge % new-state)) (put! (:internal chs) handshake-ev) @@ -1009,7 +1041,8 @@ (WebSocket. (enc/merge-url-with-query-string url (merge params ; 1st (don't clobber impl.): - {:client-id client-id}))) + {:client-id client-id + :csrf-token (:csrf-token @state_)}))) (catch :default e (errorf e "WebSocket error") @@ -1112,10 +1145,10 @@ chsk))))) #?(:cljs - (defn- new-ChWebSocket [opts] + (defn- new-ChWebSocket [opts csrf-token] (map->ChWebSocket (merge - {:state_ (atom {:type :ws :open? false :ever-opened? false}) + {:state_ (atom {:type :ws :open? false :ever-opened? false :csrf-token csrf-token}) :instance-handle_ (atom nil) :retry-count_ (atom 0) :ever-opened?_ (atom false) @@ -1166,7 +1199,8 @@ default-client-side-ajax-timeout-ms) :resp-type :text ; We'll do our own pstr decoding :headers - (merge (:headers ajax-opts) ; 1st (don't clobber impl.): + (merge + (:headers ajax-opts) ; 1st (don't clobber impl.) {:X-CSRF-Token csrf-token}) :params @@ -1248,7 +1282,12 @@ ;; reply immediately with a handshake response, ;; letting us confirm that our client<->server comms ;; are working: - (when-not (:open? @state_) {:handshake? true}))}) + (when-not (:open? @state_) {:handshake? true})) + + :headers + (merge + (:headers ajax-opts) ; 1st (don't clobber impl.) + {:X-CSRF-Token (:csrf-token @state_)})}) (fn ajax-cb [{:keys [?error ?content]}] (if ?error @@ -1287,10 +1326,10 @@ chsk)))) #?(:cljs - (defn- new-ChAjaxSocket [opts] + (defn- new-ChAjaxSocket [opts csrf-token] (map->ChAjaxSocket (merge - {:state_ (atom {:type :ajax :open? false :ever-opened? false}) + {:state_ (atom {:type :ajax :open? false :ever-opened? false :csrf-token csrf-token}) :instance-handle_ (atom nil) :ever-opened?_ (atom false) :curr-xhr_ (atom nil)} @@ -1333,7 +1372,7 @@ (fn [] ;; Remove :auto->:ajax downgrade watch (remove-watch state_ :chsk/auto-ajax-downgrade) - (-chsk-connect! (new-ChAjaxSocket ajax-chsk-opts))) + (-chsk-connect! (new-ChAjaxSocket ajax-chsk-opts (:csrf-token @state_)))) ws-conn! (fn [] @@ -1350,16 +1389,16 @@ (-chsk-disconnect! impl :downgrading-ws-to-ajax) (reset! impl_ (ajax-conn!)))))))))) - (-chsk-connect! (new-ChWebSocket ws-chsk-opts)))] + (-chsk-connect! (new-ChWebSocket ws-chsk-opts (:csrf-token @state_))))] (reset! impl_ (or (ws-conn!) (ajax-conn!))) chsk)))) #?(:cljs - (defn- new-ChAutoSocket [opts] + (defn- new-ChAutoSocket [opts csrf-token] (map->ChAutoSocket (merge - {:state_ (atom {:type :auto :open? false :ever-opened? false}) + {:state_ (atom {:type :auto :open? false :ever-opened? false :csrf-token csrf-token}) :impl_ (atom nil)} opts)))) @@ -1393,7 +1432,7 @@ :ws-kalive-ms ; Ping to keep a WebSocket conn alive if no activity ; w/in given msecs. Should be different to server's :ws-kalive-ms." - [path & + [path ?csrf-token & [{:keys [type protocol host params recv-buf-or-n packer ws-kalive-ms client-id ajax-opts wrap-recv-evs? backoff-ms-fn] :as opts @@ -1414,6 +1453,9 @@ (when (not (nil? _deprecated-more-opts)) (warnf "`make-channel-socket-client!` fn signature CHANGED with Sente v0.10.0.")) (when (contains? opts :lp-timeout) (warnf ":lp-timeout opt has CHANGED; please use :lp-timout-ms.")) + (when (str/blank? ?csrf-token) + (warnf "WARNING: no CSRF token provided. Connections will FAIL if server-side CSRF check is enabled (as it is by default).")) + (let [packer (coerce-packer packer) [ws-url ajax-url] @@ -1465,9 +1507,9 @@ ?chsk (-chsk-connect! (case type - :ws (new-ChWebSocket ws-chsk-opts) - :ajax (new-ChAjaxSocket ajax-chsk-opts) - :auto (new-ChAutoSocket auto-chsk-opts)))] + :ws (new-ChWebSocket ws-chsk-opts ?csrf-token) + :ajax (new-ChAjaxSocket ajax-chsk-opts ?csrf-token) + :auto (new-ChAutoSocket auto-chsk-opts ?csrf-token)))] (if-let [chsk ?chsk] (let [chsk-state_ (:state_ chsk)