From 560fe804d6e117243aa0d63527135d770cf9ea11 Mon Sep 17 00:00:00 2001 From: Scott Lamb Date: Wed, 31 Mar 2021 10:44:08 -0700 Subject: [PATCH] use SameSite=Lax instead of SameSite=Strict To improve reliability of live streams (#59) on Safari. Safari was dropping the cookie from websocket update requests. (But it worked sometimes. I don't get why.) I saw folks on the Internet thinking this related to HttpOnly: * https://developer.apple.com/forums/thread/104488 * https://stackoverflow.com/q/47742807/23584 but I still see this behavior without HttpOnly. SameSite=Strict vs SameSite=Lax appears to make a difference. Try that instead. SameSite=Strict is pointless for us anyway as noted in a new comment. Turning off HttpOnly would be more unfortunate security-wise. --- design/api.md | 2 +- server/db/auth.rs | 1 + server/src/web.rs | 79 ++++++++++++++++++++++++++++++++++---------- ui/src/setupProxy.js | 13 +------- 4 files changed, 64 insertions(+), 31 deletions(-) diff --git a/design/api.md b/design/api.md index 922bb9d..e46213c 100644 --- a/design/api.md +++ b/design/api.md @@ -26,7 +26,7 @@ The request should have an `application/json` body containing a dict with `username` and `password` keys. On successful authentication, the server will return an HTTP 204 (no content) -with a `Set-Cookie` header for the `s` cookie, which is an opaque, HttpOnly +with a `Set-Cookie` header for the `s` cookie, which is an opaque, `HttpOnly` (unavailable to Javascript) session identifier. If authentication or authorization fails, the server will return a HTTP 403 diff --git a/server/db/auth.rs b/server/db/auth.rs index 1928d46..315a72b 100644 --- a/server/db/auth.rs +++ b/server/db/auth.rs @@ -233,6 +233,7 @@ impl Session { } /// A raw session id (not base64-encoded). Sensitive. Never stored in the database. +#[derive(Copy, Clone)] pub struct RawSessionId([u8; 48]); impl RawSessionId { diff --git a/server/src/web.rs b/server/src/web.rs index f6e6785..deb73ce 100644 --- a/server/src/web.rs +++ b/server/src/web.rs @@ -7,7 +7,6 @@ use crate::json; use crate::mp4; use base::{bail_t, ErrorKind}; use base::{clock::Clocks, format_err_t}; -use bytes::{BufMut, BytesMut}; use core::borrow::Borrow; use core::str::FromStr; use db::dir::SampleFileDir; @@ -1100,34 +1099,32 @@ impl Service { } .to_owned(); let mut l = self.db.lock(); + + // If the request came in over https, tell the browser to only send the cookie on https + // requests also. let is_secure = self.is_secure(&req); - let flags = (auth::SessionFlag::HttpOnly as i32) - | (auth::SessionFlag::SameSite as i32) - | (auth::SessionFlag::SameSiteStrict as i32) + + // Use SameSite=Lax rather than SameSite=Strict. Safari apparently doesn't send + // SameSite=Strict cookies on WebSocket upgrade requests. There's no real security + // difference for Moonfire NVR anyway. SameSite=Strict exists as CSRF protection for + // sites that (unlike Moonfire NVR) don't follow best practices by (a) + // mutating based on GET requests and (b) not using CSRF tokens. + use auth::SessionFlag; + let flags = (SessionFlag::HttpOnly as i32) + | (SessionFlag::SameSite as i32) | if is_secure { - auth::SessionFlag::Secure as i32 + SessionFlag::Secure as i32 } else { 0 }; let (sid, _) = l .login_by_password(authreq, &r.username, r.password, Some(domain), flags) .map_err(|e| plain_response(StatusCode::UNAUTHORIZED, e.to_string()))?; - let s_suffix = if is_secure { - &b"; HttpOnly; Secure; SameSite=Strict; Max-Age=2147483648; Path=/"[..] - } else { - &b"; HttpOnly; SameSite=Strict; Max-Age=2147483648; Path=/"[..] - }; - let mut encoded = [0u8; 64]; - base64::encode_config_slice(&sid, base64::STANDARD_NO_PAD, &mut encoded); - let mut cookie = BytesMut::with_capacity("s=".len() + encoded.len() + s_suffix.len()); - cookie.put(&b"s="[..]); - cookie.put(&encoded[..]); - cookie.put(s_suffix); + let cookie = encode_sid(sid, flags); Ok(Response::builder() .header( header::SET_COOKIE, - HeaderValue::from_maybe_shared(cookie.freeze()) - .expect("cookie can't have invalid bytes"), + HeaderValue::try_from(cookie).expect("cookie can't have invalid bytes"), ) .status(StatusCode::NO_CONTENT) .body(b""[..].into()) @@ -1295,6 +1292,27 @@ impl Service { } } +/// Encodes a session into `Set-Cookie` header value form. +fn encode_sid(sid: db::RawSessionId, flags: i32) -> String { + let mut cookie = String::with_capacity(128); + cookie.push_str("s="); + base64::encode_config_buf(&sid, base64::STANDARD_NO_PAD, &mut cookie); + use auth::SessionFlag; + if (flags & SessionFlag::HttpOnly as i32) != 0 { + cookie.push_str("; HttpOnly"); + } + if (flags & SessionFlag::Secure as i32) != 0 { + cookie.push_str("; Secure"); + } + if (flags & SessionFlag::SameSiteStrict as i32) != 0 { + cookie.push_str("; SameSite=Strict"); + } else if (flags & SessionFlag::SameSite as i32) != 0 { + cookie.push_str("; SameSite=Lax"); + } + cookie.push_str("; Max-Age=2147483648; Path=/"); + cookie +} + #[derive(Debug, Eq, PartialEq)] struct StaticFileRequest<'a> { path: &'a str, @@ -1730,6 +1748,31 @@ mod tests { .unwrap(); assert_eq!(resp.status(), reqwest::StatusCode::BAD_REQUEST); } + + #[test] + fn encode_sid() { + use super::encode_sid; + use db::auth::{RawSessionId, SessionFlag}; + let s64 = "3LbrruP5vj/hpE8kvYTz/rNDg4BleRiTCHGA3Ocm91z/YrtxHDxexmrz46biZJxJ"; + let s = RawSessionId::decode_base64(s64.as_bytes()).unwrap(); + assert_eq!( + encode_sid( + s, + (SessionFlag::Secure as i32) + | (SessionFlag::HttpOnly as i32) + | (SessionFlag::SameSite as i32) + | (SessionFlag::SameSiteStrict as i32) + ), + format!( + "s={}; HttpOnly; Secure; SameSite=Strict; Max-Age=2147483648; Path=/", + s64 + ) + ); + assert_eq!( + encode_sid(s, SessionFlag::SameSite as i32), + format!("s={}; SameSite=Lax; Max-Age=2147483648; Path=/", s64) + ); + } } #[cfg(all(test, feature = "nightly"))] diff --git a/ui/src/setupProxy.js b/ui/src/setupProxy.js index 5805024..1d92d16 100644 --- a/ui/src/setupProxy.js +++ b/ui/src/setupProxy.js @@ -21,24 +21,13 @@ module.exports = (app) => { // this attribute in the proxy with code from here: // https://github.com/chimurai/http-proxy-middleware/issues/169#issuecomment-575027907 // See also discussion in guide/developing-ui.md. - // - // Additionally, Safari appears to (sometimes?) prevent http-only cookies - // (meaning cookies that Javascript shouldn't be able to access) from - // being passed to WebSocket requests (possibly only when not using - // https/wss). Also strip HttpOnly when using Safari. - // https://developer.apple.com/forums/thread/104488 onProxyRes: (proxyRes, req, res) => { const sc = proxyRes.headers["set-cookie"]; if (Array.isArray(sc)) { proxyRes.headers["set-cookie"] = sc.map((sc) => { return sc .split(";") - .filter( - (v) => - v.trim().toLowerCase() !== "secure" && - (v.trim().toLowerCase() !== "httponly" || - !req.headers["user-agent"].includes("Safari")) - ) + .filter((v) => v.trim().toLowerCase() !== "secure") .join("; "); }); }