From c7d3bd087c49bdd0b33ed23ff583bf58ba705a1c Mon Sep 17 00:00:00 2001 From: Martin Fischer Date: Fri, 29 Jan 2021 20:29:42 +0100 Subject: remove CSRF tokens (SameSite support is good enough) --- Cargo.toml | 2 +- README.md | 27 +++++----- examples/csrf/Cargo.toml | 15 ------ examples/csrf/src/main.rs | 87 --------------------------------- examples/form/Cargo.toml | 15 ++++++ examples/form/src/main.rs | 84 +++++++++++++++++++++++++++++++ src/request.rs | 122 +--------------------------------------------- 7 files changed, 116 insertions(+), 236 deletions(-) delete mode 100644 examples/csrf/Cargo.toml delete mode 100644 examples/csrf/src/main.rs create mode 100644 examples/form/Cargo.toml create mode 100644 examples/form/src/main.rs diff --git a/Cargo.toml b/Cargo.toml index 278de4e..fc4ba99 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -7,7 +7,7 @@ description = "A lightweight layer on top of hyper to facilitate building web ap repository = "https://git.push-f.com/sputnik" edition = "2018" categories = ["web-programming::http-server"] -keywords = ["hyper", "web", "cookie", "csrf", "hmac"] +keywords = ["hyper", "web", "cookie", "hmac"] # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html diff --git a/README.md b/README.md index a3c0a71..9d61453 100644 --- a/README.md +++ b/README.md @@ -3,7 +3,7 @@ A microframework based on [Hyper](https://hyper.rs/) providing traits to: * extend `http::request::Parts` with query parameter deserialization & cookie parsing -* extend `hyper::Body` with form deserialization (and optional [CSRF](https://en.wikipedia.org/wiki/Cross-site_request_forgery) protection) +* extend `hyper::Body` with form deserialization (and JSON deserialization with the `json` feature) * extend `http::response::Builder` with methods to set & delete cookies and set the Content-Type Furthermore Sputnik provides what's necessary to implement [signed & expiring @@ -20,6 +20,12 @@ conversions for every error type, which you want to short-circuit with the `?` operator. This can be easily done with [thiserror](https://crates.io/crates/thiserror) because Sputnik restricts its error types to the `'static` lifetime. +## Security Considerations + +Protect your application against [CSRF](https://en.wikipedia.org/wiki/Cross-site_request_forgery) +by setting `SameSite` to `Lax` or `Strict` for your cookies and checking that the `Origin` +header matches your domain name (especially if you have unauthenticated POST endpoints). + ## Example ```rust @@ -29,8 +35,8 @@ use hyper::{Method, Server, StatusCode, Body}; use hyper::http::request::Parts; use hyper::http::response::Builder; use serde::Deserialize; -use sputnik::{mime, request::{SputnikParts, SputnikBody, CsrfToken}, response::SputnikBuilder}; -use sputnik::request::CsrfProtectedFormError; +use sputnik::{mime, request::{SputnikParts, SputnikBody}, response::SputnikBuilder}; +use sputnik::request::FormError; type Response = hyper::Response; @@ -39,13 +45,13 @@ enum Error { #[error("page not found")] NotFound(String), #[error("{0}")] - CsrfError(#[from] CsrfProtectedFormError) + FormError(#[from] FormError) } fn render_error(err: Error) -> (StatusCode, String) { match err { Error::NotFound(msg) => (StatusCode::NOT_FOUND, msg), - Error::CsrfError(err) => (StatusCode::BAD_REQUEST, err.to_string()), + Error::FormError(err) => (StatusCode::BAD_REQUEST, err.to_string()), } } @@ -57,22 +63,19 @@ async fn route(req: &mut Parts, body: Body) -> Result { } } -fn get_form(req: &mut Parts) -> Response { +fn get_form(_req: &mut Parts) -> Response { Builder::new() .content_type(mime::TEXT_HTML) .body( - format!( - "
{}
", - CsrfToken::from_request(req).html_input() - ).into() + "
".into() ).unwrap() } #[derive(Deserialize)] struct FormData {text: String} -async fn post_form(req: &mut Parts, body: Body) -> Result { - let msg: FormData = body.into_form_csrf(req).await?; +async fn post_form(_req: &mut Parts, body: Body) -> Result { + let msg: FormData = body.into_form().await?; Ok(Builder::new().body( format!("hello {}", msg.text).into() ).unwrap()) diff --git a/examples/csrf/Cargo.toml b/examples/csrf/Cargo.toml deleted file mode 100644 index b6768ed..0000000 --- a/examples/csrf/Cargo.toml +++ /dev/null @@ -1,15 +0,0 @@ -[package] -name = "csrf" -version = "0.1.0" -authors = ["Martin Fischer "] -edition = "2018" -publish = false - -# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html - -[dependencies] -hyper = "0.13" -sputnik = {path = "../../"} -serde = { version = "1.0", features = ["derive"] } -tokio = { version = "0.2", features = ["full"] } -thiserror = "1.0" \ No newline at end of file diff --git a/examples/csrf/src/main.rs b/examples/csrf/src/main.rs deleted file mode 100644 index 53ea87f..0000000 --- a/examples/csrf/src/main.rs +++ /dev/null @@ -1,87 +0,0 @@ -use std::convert::Infallible; -use hyper::service::{service_fn, make_service_fn}; -use hyper::{Method, Server, StatusCode, Body}; -use hyper::http::request::Parts; -use hyper::http::response::Builder; -use serde::Deserialize; -use sputnik::{mime, request::{SputnikParts, SputnikBody, CsrfToken}, response::SputnikBuilder}; -use sputnik::request::CsrfProtectedFormError; - -type Response = hyper::Response; - -#[derive(thiserror::Error, Debug)] -enum Error { - #[error("page not found")] - NotFound(String), - #[error("{0}")] - CsrfError(#[from] CsrfProtectedFormError) -} - -fn render_error(err: Error) -> (StatusCode, String) { - match err { - Error::NotFound(msg) => (StatusCode::NOT_FOUND, msg), - Error::CsrfError(err) => (StatusCode::BAD_REQUEST, err.to_string()), - } -} - -async fn route(req: &mut Parts, body: Body) -> Result { - match (&req.method, req.uri.path()) { - (&Method::GET, "/form") => Ok(get_form(req)), - (&Method::POST, "/form") => post_form(req, body).await, - _ => return Err(Error::NotFound("page not found".to_owned())) - } -} - -fn get_form(req: &mut Parts) -> Response { - Builder::new() - .content_type(mime::TEXT_HTML) - .body( - format!( - "
{}
", - CsrfToken::from_request(req).html_input() - ).into() - ).unwrap() -} - -#[derive(Deserialize)] -struct FormData {text: String} - -async fn post_form(req: &mut Parts, body: Body) -> Result { - let msg: FormData = body.into_form_csrf(req).await?; - Ok(Builder::new().body( - format!("hello {}", msg.text).into() - ).unwrap()) -} - -async fn service(req: hyper::Request) -> Result, Infallible> { - let (mut parts, body) = req.into_parts(); - match route(&mut parts, body).await { - Ok(mut res) => { - for (k,v) in parts.response_headers().iter() { - res.headers_mut().append(k, v.clone()); - } - Ok(res) - } - Err(err) => { - let (code, message) = render_error(err); - // you can easily wrap or log errors here - Ok(hyper::Response::builder().status(code).body(message.into()).unwrap()) - } - } -} - -#[tokio::main] -async fn main() { - let service = make_service_fn(move |_| { - async move { - Ok::<_, hyper::Error>(service_fn(move |req| { - service(req) - })) - } - }); - - let addr = ([127, 0, 0, 1], 8000).into(); - let server = Server::bind(&addr).serve(service); - println!("Listening on http://{}", addr); - server.await; -} \ No newline at end of file diff --git a/examples/form/Cargo.toml b/examples/form/Cargo.toml new file mode 100644 index 0000000..6f899df --- /dev/null +++ b/examples/form/Cargo.toml @@ -0,0 +1,15 @@ +[package] +name = "sputnik-demo" +version = "0.1.0" +authors = ["Martin Fischer "] +edition = "2018" +publish = false + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] +hyper = "0.13" +sputnik = {path = "../../"} +serde = { version = "1.0", features = ["derive"] } +tokio = { version = "0.2", features = ["full"] } +thiserror = "1.0" \ No newline at end of file diff --git a/examples/form/src/main.rs b/examples/form/src/main.rs new file mode 100644 index 0000000..e354f9f --- /dev/null +++ b/examples/form/src/main.rs @@ -0,0 +1,84 @@ +use std::convert::Infallible; +use hyper::service::{service_fn, make_service_fn}; +use hyper::{Method, Server, StatusCode, Body}; +use hyper::http::request::Parts; +use hyper::http::response::Builder; +use serde::Deserialize; +use sputnik::{mime, request::{SputnikParts, SputnikBody}, response::SputnikBuilder}; +use sputnik::request::FormError; + +type Response = hyper::Response; + +#[derive(thiserror::Error, Debug)] +enum Error { + #[error("page not found")] + NotFound(String), + #[error("{0}")] + FormError(#[from] FormError) +} + +fn render_error(err: Error) -> (StatusCode, String) { + match err { + Error::NotFound(msg) => (StatusCode::NOT_FOUND, msg), + Error::FormError(err) => (StatusCode::BAD_REQUEST, err.to_string()), + } +} + +async fn route(req: &mut Parts, body: Body) -> Result { + match (&req.method, req.uri.path()) { + (&Method::GET, "/form") => Ok(get_form(req)), + (&Method::POST, "/form") => post_form(req, body).await, + _ => return Err(Error::NotFound("page not found".to_owned())) + } +} + +fn get_form(_req: &mut Parts) -> Response { + Builder::new() + .content_type(mime::TEXT_HTML) + .body( + "
".into() + ).unwrap() +} + +#[derive(Deserialize)] +struct FormData {text: String} + +async fn post_form(_req: &mut Parts, body: Body) -> Result { + let msg: FormData = body.into_form().await?; + Ok(Builder::new().body( + format!("hello {}", msg.text).into() + ).unwrap()) +} + +async fn service(req: hyper::Request) -> Result, Infallible> { + let (mut parts, body) = req.into_parts(); + match route(&mut parts, body).await { + Ok(mut res) => { + for (k,v) in parts.response_headers().iter() { + res.headers_mut().append(k, v.clone()); + } + Ok(res) + } + Err(err) => { + let (code, message) = render_error(err); + // you can easily wrap or log errors here + Ok(hyper::Response::builder().status(code).body(message.into()).unwrap()) + } + } +} + +#[tokio::main] +async fn main() { + let service = make_service_fn(move |_| { + async move { + Ok::<_, hyper::Error>(service_fn(move |req| { + service(req) + })) + } + }); + + let addr = ([127, 0, 0, 1], 8000).into(); + let server = Server::bind(&addr).serve(service); + println!("Listening on http://{}", addr); + server.await; +} \ No newline at end of file diff --git a/src/request.rs b/src/request.rs index d89248c..8059e07 100644 --- a/src/request.rs +++ b/src/request.rs @@ -2,17 +2,14 @@ use cookie::Cookie; use mime::Mime; -use serde::{Deserialize, de::DeserializeOwned}; +use serde::de::DeserializeOwned; use hyper::{HeaderMap, body::Bytes, header, http::request::Parts}; use time::Duration; use std::{collections::HashMap, sync::Arc}; -use rand::{Rng, distributions::Alphanumeric}; use async_trait::async_trait; use crate::response::{SputnikHeaders, delete_cookie}; -const CSRF_COOKIE_NAME : &str = "csrf"; - /// Adds convenience methods to [`http::request::Parts`](Parts). pub trait SputnikParts { /// Parses the query string of the request into a given struct. @@ -79,16 +76,6 @@ impl SputnikParts for Parts { } } -/// A cookie-based CSRF token to be used with [`SputnikBody::into_form_csrf`]. -#[derive(Clone)] -pub struct CsrfToken(String); - -impl std::fmt::Display for CsrfToken { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", self.0) - } -} - const FLASH_COOKIE_NAME: &str = "flash"; /// Show the user a message after redirecting them. @@ -150,75 +137,20 @@ impl Flash { } } -impl CsrfToken { - /// Returns a CSRF token, either extracted from the `csrf` cookie or newly - /// generated if the cookie wasn't sent (in which case a set-cookie header is - /// appended to [`SputnikParts::response_headers`]). - /// - /// If there is no cookie, calling this method multiple times only generates - /// a new token on the first call, further calls return the previously - /// generated token. - pub fn from_request(req: &mut Parts) -> Self { - if let Some(token) = req.extensions.get::() { - return token.clone() - } - csrf_token_from_cookies(req) - .unwrap_or_else(|| { - let token: String = rand::thread_rng().sample_iter( - Alphanumeric - // must be HTML-safe because we embed it in CsrfToken::html_input - ).take(16).collect(); - let mut c = Cookie::new(CSRF_COOKIE_NAME, token.clone()); - c.set_secure(Some(true)); - c.set_max_age(Some(Duration::hours(1))); - - req.response_headers().set_cookie(c); - let token = CsrfToken(token); - req.extensions.insert(token.clone()); - token - }) - } - - /// Returns a hidden HTML input to be embedded in forms that are received - /// with [`SputnikBody::into_form_csrf`]. - pub fn html_input(&self) -> String { - format!("", self) - } -} - /// Adds deserialization methods to [`hyper::Body`]. #[async_trait] pub trait SputnikBody { async fn into_bytes(self) -> Result; /// Parses a `application/x-www-form-urlencoded` request body into a given struct. - /// - /// This does make you vulnerable to CSRF, so you normally want to use - /// [`SputnikBody::into_form_csrf()`] instead. async fn into_form(self) -> Result; - /// Parses a `application/x-www-form-urlencoded` request body into a given struct. - /// Protects from CSRF by checking that the request body contains the same token retrieved from the cookies. - /// - /// The HTML form must embed a hidden input generated with [`CsrfToken::html_input`]. - async fn into_form_csrf(self, req: &mut Parts) -> Result; - /// Attempts to deserialize the request body as JSON. #[cfg(feature = "json")] #[cfg_attr(docsrs, doc(cfg(feature = "json")))] async fn into_json(self) -> Result; } -fn csrf_token_from_cookies(req: &mut Parts) -> Option { - req.cookies() - .get(CSRF_COOKIE_NAME) - .map(|cookie| { - let token = CsrfToken(cookie.value().to_string()); - req.extensions.insert(token.clone()); - token - }) -} - #[async_trait] impl SputnikBody for hyper::Body { async fn into_bytes(self) -> Result { @@ -230,19 +162,6 @@ impl SputnikBody for hyper::Body { Ok(serde_urlencoded::from_bytes::(&full_body)?) } - async fn into_form_csrf(self, req: &mut Parts) -> Result { - let full_body = self.into_bytes().await?; - let csrf_data = serde_urlencoded::from_bytes::(&full_body).map_err(|_| CsrfProtectedFormError::NoCsrf)?; - match csrf_token_from_cookies(req) { - Some(token) => if token.to_string() == csrf_data.csrf { - Ok(serde_urlencoded::from_bytes::(&full_body)?) - } else { - Err(CsrfProtectedFormError::Mismatch) - } - None => Err(CsrfProtectedFormError::NoCookie) - } - } - #[cfg(feature = "json")] #[cfg_attr(docsrs, doc(cfg(feature = "json")))] async fn into_json(self) -> Result { @@ -251,11 +170,6 @@ impl SputnikBody for hyper::Body { } } -#[derive(Deserialize)] -struct CsrfData { - csrf: String, -} - #[derive(thiserror::Error, Debug)] #[error("query deserialize error: {0}")] pub struct QueryError(pub serde_urlencoded::de::Error); @@ -289,38 +203,4 @@ pub enum JsonError { #[error("json deserialize error: {0}")] Deserialize(#[from] serde_json::Error), -} - -#[derive(thiserror::Error, Debug)] -pub enum CsrfProtectedFormError { - #[error("{0}")] - Body(#[from] BodyError), - - #[error("form deserialize error: {0}")] - Deserialize(#[from] serde_urlencoded::de::Error), - - #[error("no csrf token in form data")] - NoCsrf, - - #[error("no csrf cookie")] - NoCookie, - - #[error("csrf parameter doesn't match csrf cookie")] - Mismatch, -} - -#[cfg(test)] -mod tests { - use hyper::Request; - - use super::*; - - #[test] - fn test_csrf_token() { - let mut parts = Request::new(hyper::Body::empty()).into_parts().0; - let tok1 = CsrfToken::from_request(&mut parts); - let tok2 = CsrfToken::from_request(&mut parts); - assert_eq!(tok1.to_string(), tok2.to_string()); - assert_eq!(parts.response_headers().len(), 1); - } } \ No newline at end of file -- cgit v1.2.3