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
ptaoussanis committed Nov 2, 2018
1 parent 394caa6 commit 442a347
Showing 1 changed file with 70 additions and 32 deletions.
102 changes: 70 additions & 32 deletions src/taoensso/sente.cljc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
* Callback replies: :chsk/closed, :chsk/timeout, :chsk/error
* Client-side events:
[:chsk/handshake [<?uid> <?csrf-token> <?handshake-data> <first-handshake?>]]
[:chsk/handshake [<?uid> nil[4] <?handshake-data> <first-handshake?>]]
[:chsk/state [<old-state-map> <new-state-map>]]
[:chsk/recv <ev-as-pushed-from-server>] ; Server>user push
[:chsk/ws-ping]
Expand All @@ -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 <WebSocket-on-error-event>}
:last-ws-close - ?{:udt _ :ev <WebSocket-on-close-event>
Expand All @@ -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.2 for security reasons.
A `nil` remains for semi-backwards-compatibility with pre-v1.2 clients."

{:author "Peter Taoussanis (@ptaoussanis)"}

Expand Down Expand Up @@ -274,7 +276,8 @@
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.
: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.
Expand All @@ -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}}]]

Expand Down Expand Up @@ -484,6 +490,22 @@
;; 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= 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
Expand All @@ -496,6 +518,11 @@
;; Does not participate in `conns_` (has specific req->resp)
:ajax-post-fn
(fn [ring-req]
(cond
(bad-csrf? ring-req)
(bad-csrf-fn ring-req)

:else
(interfaces/ring-req->server-ch-resp web-server-ch-adapter ring-req
{:on-open
(fn [server-ch websocket?]
Expand Down Expand Up @@ -528,15 +555,14 @@
(go
(<! (async/timeout ms))
(reply-fn :chsk/timeout)))
(reply-fn :chsk/dummy-cb-200))))}))
(reply-fn :chsk/dummy-cb-200))))})))

;; Ajax handshake/poll, or WebSocket handshake
:ajax-get-or-ws-handshake-fn
(fn [ring-req]
(let [sch-uuid (enc/uuid-str 6)
params (get ring-req :params)
client-id (get params :client-id)
csrf-token (csrf-token-fn ring-req)
uid (user-id-fn ring-req client-id)

receive-event-msg! ; Partial
Expand All @@ -557,16 +583,22 @@
(let [?handshake-data (handshake-data-fn ring-req)
handshake-ev
(if (nil? ?handshake-data) ; Micro optimization
[:chsk/handshake [uid csrf-token]]
[:chsk/handshake [uid csrf-token ?handshake-data]])]
[:chsk/handshake [uid nil]]
[:chsk/handshake [uid nil ?handshake-data]])]
(interfaces/sch-send! server-ch websocket?
(pack packer handshake-ev))))]

(if (str/blank? client-id)
(cond

(str/blank? client-id)
(let [err-msg "Client's Ring request doesn't have a client id. Does your server have the necessary keyword Ring middleware (`wrap-params` & `wrap-keyword-params`)?"]
(errorf (str err-msg ": %s") ring-req) ; Careful re: % in req
(throw (ex-info err-msg {:ring-req ring-req})))

(bad-csrf? ring-req)
(bad-csrf-fn ring-req)

:else
(interfaces/ring-req->server-ch-resp web-server-ch-adapter ring-req
{:on-open
(fn [server-ch websocket?]
Expand Down Expand Up @@ -873,26 +905,22 @@
(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
{:type chsk-type ; :auto -> e/o #{:ws :ajax}, etc.
: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)

Expand Down Expand Up @@ -1009,7 +1037,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")
Expand Down Expand Up @@ -1112,10 +1141,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)
Expand Down Expand Up @@ -1166,7 +1195,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
Expand Down Expand Up @@ -1248,7 +1278,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
Expand Down Expand Up @@ -1287,10 +1322,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)}
Expand Down Expand Up @@ -1333,7 +1368,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 []
Expand All @@ -1350,16 +1385,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))))

Expand Down Expand Up @@ -1393,7 +1428,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
Expand All @@ -1414,6 +1449,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]
Expand Down Expand Up @@ -1465,9 +1503,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)
Expand Down

0 comments on commit 442a347

Please sign in to comment.