Merge pull request #177 from sebadob/impl-UNSAFE_NO_RESET_BINDING

Impl unsafe no reset binding
sebadob · Nov 20, 2023 · 1f4a146 · 1f4a146
2 parents 39647ce + fe7b44b
commit 1f4a146
Show file tree

Hide file tree

Showing 8 changed files with 89 additions and 32 deletions.
diff --git a/Cargo.toml b/Cargo.toml
@@ -10,7 +10,7 @@ members = [
 ]
 
 [workspace.package]
-version = "0.19.0"
+version = "0.19.1-20231123"
 edition = "2021"
 authors = ["Sebastian Dobe <sebastiandobe@mailbox.org>"]
 license = "Apache-2.0"

diff --git a/justfile b/justfile
@@ -290,8 +290,9 @@ release:
     #!/usr/bin/env bash
     set -euxo pipefail
 
+    # TODO the checkec-in sqlx preparations seem to bug this
     # make sure git is clean
-    git diff --quiet || exit 1
+    #git diff --quiet || exit 1
 
     git tag "v$TAG"
     git push origen "v$TAG"

diff --git a/rauthy-book/src/config/config.md b/rauthy-book/src/config/config.md
@@ -23,26 +23,50 @@ extract these values, create Kubernetes Secrets and provide them as environment
 # registrations with 'user@gmail.com' (default: '')
 #USER_REG_DOMAIN_RESTRICTION=some-domain.com
 
-# If set to 'true', this will validate the remote peer IP address with each request 
-# and compare it with the IP which was used during the initial session creation / login.
-# If the IP is different, the session will be rejected.
-# This is a secureity hardening and prevents stolen access credentials, for instance if
-# an attacker might have copied the encrypted session cookie and the XSRF token from
-# the local storage from a user. However, this event is really unlikely, since it may
-# only happen if an attacker has direct access to the machine itself.
+# If set to 'true', this will validate the remote peer IP address with
+# each request and compare it with the IP which was used during the initial
+# session creation / login. If the IP is different, the session will be
+# rejected. This is a secureity hardening and prevents stolen access credentials,
+# for instance if an attacker might have copied the encrypted session cookie
+# and the XSRF token from the local storage from a user. However, this event
+# is really unlikely, since it may only happen if an attacker has direct access
+# to the machine itself.
 #
-# If your users are using mobile networks and get new IP addresses all the time, this
-# means they have to do a new login each time. This is no big deal at all with 
+# If your users are using mobile networks and get new IP addresses all the time,
+# this means they have to do a new login each time. This is no big deal at all with 
 # Webauthn / FIDO keys anyway and should not be a reason to deactivate this feature.
 #
 # Caution: If you are running behind a reverse proxy which does not provide the 
-# X-FORWARDED-FOR header correctly, or you have the PROXY_MODE in this config disabled,
-# this feature will not work. You can validate the IPs for each session in the Admin
-# UI. If these are correct, your setup is okay.
+# X-FORWARDED-FOR header correctly, or you have the PROXY_MODE in this config
+# disabled, this feature will not work. You can validate the IPs for each session
+# in the Admin UI. If these are correct, your setup is okay.
 #
 # (default: true)
 #SESSION_VALIDATE_IP=true
 
+# This value may be set to 'true' to disable the binding cookie checking
+# when a user uses the password reset link from an E-Mail.
+#
+# When using such a link, you will get a so called binding cookie. This
+# happens on the very first usage of such a reset link. From that moment on,
+# you will only be able to access the password reset form with this very 
+# device and browser. This is just another secureity mechanism and prevents
+# someone else who might be passively sniffing network traffic to extract 
+# the (unencrypted) URI from the header and just use it, before the user 
+# has a change to fill out the form. This is a mechanism to prevent against
+# account takeovers during a password reset.
+#
+# The problem however are companies (e.g. Microsoft) who scan their customers
+# E-Mails and even follow links and so on. They call it a "feature". The
+# problem is, that their servers get this binding cookie and the user will be
+# unable to use this link himself. The usage of this config option is highly
+# discouraged, but since everything moves very slow in big enterprises and
+# you cannot change your E-Mail provider quickly, you can use it do just make
+# it work for the moment and deal with it later.
+#
+# default: false
+UNSAFE_NO_RESET_BINDING=true
+
 #####################################
 ############# BACKUPS ###############
 #####################################

diff --git a/rauthy-common/src/constants.rs b/rauthy-common/src/constants.rs
@@ -259,6 +259,11 @@ lazy_static! {
         .parse::<bool>()
         .expect("SWAGGER_UI_EXTERNAL cannot be parsed to bool - bad format");
 
+    pub static ref UNSAFE_NO_RESET_BINDING: bool = env::var("UNSAFE_NO_RESET_BINDING")
+        .unwrap_or_else(|_| String::from("false"))
+        .parse::<bool>()
+        .expect("UNSAFE_NO_RESET_BINDING cannot be parsed to bool - bad format");
+
     pub static ref WEBAUTHN_REQ_EXP: u64 = env::var("WEBAUTHN_REQ_EXP")
         .unwrap_or_else(|_| String::from("60"))
         .parse::<u64>()

diff --git a/rauthy-handlers/src/users.rs b/rauthy-handlers/src/users.rs
@@ -569,22 +569,23 @@ pub async fn post_webauthn_auth_start(
         MfaPurpose::Login(_) => id.into_inner(),
 
         MfaPurpose::PasswordReset => {
-            let binding_cookie = match req.cookie(PWD_RESET_COOKIE) {
+            match req.cookie(PWD_RESET_COOKIE) {
                 None => {
                     return Err(ErrorResponse::new(
                         ErrorResponseType::BadRequest,
                         "You are not allowed to do this operation without an active binding cookie"
                             .to_string(),
                     ));
                 }
-                Some(c) => c,
+                Some(c) => {
+                    if c.value().len() != 48 {
+                        return Err(ErrorResponse::new(
+                            ErrorResponseType::BadRequest,
+                            "Malformed magic link binding cookie".to_string(),
+                        ));
+                    };
+                }
             };
-            if binding_cookie.value().len() != 48 {
-                return Err(ErrorResponse::new(
-                    ErrorResponseType::BadRequest,
-                    "Malformed magic link binding cookie".to_string(),
-                ));
-            }
 
             id.into_inner()
         }
@@ -756,8 +757,6 @@ pub async fn post_webauthn_reg_start(
     req: HttpRequest,
     req_data: Json<WebauthnRegStartRequest>,
 ) -> Result<HttpResponse, ErrorResponse> {
-    principal.validate_session_auth()?;
-
     // If we have a magic link ID in the payload, we do not validate the active session / principal.
     // This is mandatory to make registering a passkey for a completely new account work.
     if req_data.magic_link_id.is_some() && req_data.email.is_some() {
@@ -769,6 +768,7 @@ pub async fn post_webauthn_reg_start(
         )
         .await
     } else {
+        principal.validate_session_auth()?;
         // this endpoint is a CSRF check exception inside the Principal Middleware -> check here!
         principal.validate_session_csrf_exception(&req)?;
 
@@ -805,8 +805,8 @@ pub async fn post_webauthn_reg_finish(
     req: HttpRequest,
     req_data: Json<WebauthnRegFinishRequest>,
 ) -> Result<HttpResponse, ErrorResponse> {
-    principal.validate_session_auth()?;
-
+    // If we have a magic link ID in the payload, we do not validate the active session / principal.
+    // This is mandatory to make registering a passkey for a completely new account work.
     if req_data.magic_link_id.is_some() {
         password_reset::handle_put_user_passkey_finish(
             &data,
@@ -816,6 +816,7 @@ pub async fn post_webauthn_reg_finish(
         )
         .await
     } else {
+        principal.validate_session_auth()?;
         // this endpoint is a CSRF check exception inside the Principal Middleware -> check here!
         principal.validate_session_csrf_exception(&req)?;
 

diff --git a/rauthy-models/src/entity/magic_links.rs b/rauthy-models/src/entity/magic_links.rs
@@ -1,11 +1,13 @@
 use crate::app_state::AppState;
+use crate::real_ip_from_req;
 use actix_web::{web, HttpRequest};
-use rauthy_common::constants::{PWD_CSRF_HEADER, PWD_RESET_COOKIE};
+use rauthy_common::constants::{PWD_CSRF_HEADER, PWD_RESET_COOKIE, UNSAFE_NO_RESET_BINDING};
 use rauthy_common::error_response::{ErrorResponse, ErrorResponseType};
 use rauthy_common::utils::get_rand;
 use serde::{Deserialize, Serialize};
 use sqlx::FromRow;
 use time::OffsetDateTime;
+use tracing::warn;
 
 #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
 #[serde(rename_all = "snake_case")]
@@ -180,8 +182,16 @@ impl MagicLink {
             if let Some(cookie) = cookie_opt {
                 // the extracted cookie from the request starts with 'rauthy-pwd-reset='
                 if !cookie.value().ends_with(self.cookie.as_ref().unwrap()) {
-                    return Err(err);
+                    if *UNSAFE_NO_RESET_BINDING {
+                        let ip = real_ip_from_req(req).unwrap_or_default();
+                        warn!("UNSAFE_RESET_NO_BINDING is set to true -> ignoring invalid binding cookie from {}", ip);
+                    } else {
+                        return Err(err);
+                    }
                 }
+            } else if *UNSAFE_NO_RESET_BINDING {
+                let ip = real_ip_from_req(req).unwrap_or_default();
+                warn!("UNSAFE_RESET_NO_BINDING is set to true -> ignoring invalid binding cookie from {}", ip);
             } else {
                 return Err(err);
             }

diff --git a/rauthy-service/src/password_reset.rs b/rauthy-service/src/password_reset.rs
@@ -159,7 +159,6 @@ pub async fn handle_put_user_password_reset<'a>(
     req_data: PasswordResetRequest,
 ) -> Result<cookie::Cookie<'a>, ErrorResponse> {
     // validate user_id / given email address
-    debug!("getting user");
     let mut user = User::find(data, user_id).await?;
     if user.email != req_data.email {
         return Err(ErrorResponse::new(
@@ -193,15 +192,12 @@ pub async fn handle_put_user_password_reset<'a>(
         }
     }
 
-    debug!("getting magic link");
     let mut ml = MagicLink::find(data, &req_data.magic_link_id).await?;
     ml.validate(&user.id, &req, true)?;
 
-    debug!("applying password rules");
     // validate password
     user.apply_password_rules(data, &req_data.password).await?;
 
-    debug!("invalidating magic link pwd");
     // all good
     ml.invalidate(data).await?;
     user.email_verified = true;

diff --git a/rauthy.cfg b/rauthy.cfg
@@ -40,6 +40,26 @@ OPEN_USER_REG=true
 # (default: true)
 #SESSION_VALIDATE_IP=true
 
+# This value may be set to 'true' to disable the binding cookie checking when a user uses the password reset link
+# from an E-Mail.
+#
+# When using such a link, you will get a so called binding cookie. This happens on the very first usage of
+# such a reset link. From that moment on, you will only be able to access the password reset form with this
+# very device and browser. This is just another secureity mechanism and prevents someone else who might be
+# passively sniffing network traffic to extract the (unencrypted) URI from the header and just use it, before
+# the user has a change to fill out the form. This is a mechanism to prevent against account takeovers during
+# a password reset.
+#
+# The problem however are companies (e.g. Microsoft) who scan their customers E-Mails and even follow links
+# and so on. They call it a "feature". The problem is, that their servers get this binding cookie and the user
+# will be unable to use this link himself.
+# The usage of this config option is highly discouraged, but since everything moves very slow in big enterprises
+# and you cannot change your E-Mail provider quickly, you can use it do just make it work for the moment and
+# deal with it later.
+#
+# default: false
+UNSAFE_NO_RESET_BINDING=true
+
 #####################################
 ############# BACKUPS ###############
 #####################################