From bcb9eac1b51eb37161f0fdd8d1ab6be61c839b47 Mon Sep 17 00:00:00 2001 From: Josh Cunningham Date: Tue, 27 Mar 2018 14:33:39 -0700 Subject: [PATCH] Fixed improper state handling Originally was generating and setting a nonce value in a state object, then checking that nonce on return from Auth0 instead of checking the entire state parameter. --- WP_Auth0.php | 4 +- lib/WP_Auth0_Lock10_Options.php | 6 +-- lib/WP_Auth0_Lock_Options.php | 2 +- lib/WP_Auth0_LoginManager.php | 46 ++++++++++++++----- ...Handler.php => WP_Auth0_Nonce_Handler.php} | 17 +++---- templates/auth0-login-form-lock10.php | 11 ++++- 6 files changed, 57 insertions(+), 29 deletions(-) rename lib/{WP_Auth0_State_Handler.php => WP_Auth0_Nonce_Handler.php} (82%) diff --git a/WP_Auth0.php b/WP_Auth0.php index a29f0a44..111f9897 100644 --- a/WP_Auth0.php +++ b/WP_Auth0.php @@ -13,6 +13,7 @@ define( 'AUTH0_DB_VERSION', 17 ); define( 'WPA0_VERSION', '3.5.2' ); define( 'WPA0_CACHE_GROUP', 'wp_auth0' ); +define( 'WPA0_STATE_COOKIE_NAME', 'auth0_state' ); /** * Main plugin class @@ -120,9 +121,6 @@ public function init() { $this->check_signup_status(); - // Store nonce value before headers are sent - WP_Auth0_State_Handler::getInstance()->setCookie(); - WP_Auth0_Email_Verification::init(); } diff --git a/lib/WP_Auth0_Lock10_Options.php b/lib/WP_Auth0_Lock10_Options.php index 65417c86..c51ba2b3 100644 --- a/lib/WP_Auth0_Lock10_Options.php +++ b/lib/WP_Auth0_Lock10_Options.php @@ -101,7 +101,7 @@ public function get_state_obj( $redirect_to = null ) { $stateObj = array( 'interim' => ( isset( $_GET['interim-login'] ) && $_GET['interim-login'] == 1 ), - 'nonce' => WP_Auth0_State_Handler::getInstance()->get() + 'nonce' => WP_Auth0_Nonce_Handler::getInstance()->get() ); if ( !empty( $redirect_to ) ) { @@ -111,8 +111,6 @@ public function get_state_obj( $redirect_to = null ) { $stateObj["redirect_to"] = addslashes( $_GET['redirect_to'] ); } - $stateObj["state"] = 'nonce'; - return base64_encode( json_encode( $stateObj ) ); } @@ -214,7 +212,7 @@ public function get_sso_options() { unset( $options["authParams"] ); $options["state"] = $this->get_state_obj( $redirect_to ); - $options["nonce"] = 'nonce'; + $options["nonce"] = WP_Auth0_Nonce_Handler::getInstance()->get(); return $options; } diff --git a/lib/WP_Auth0_Lock_Options.php b/lib/WP_Auth0_Lock_Options.php index c44a136c..e2974c63 100644 --- a/lib/WP_Auth0_Lock_Options.php +++ b/lib/WP_Auth0_Lock_Options.php @@ -100,7 +100,7 @@ public function modal_button_name() { public function get_state_obj( $redirect_to = null ) { $stateObj = array( 'interim' => ( isset( $_GET['interim-login'] ) && $_GET['interim-login'] == 1 ), - 'nonce' => WP_Auth0_State_Handler::getInstance()->get() + 'nonce' => WP_Auth0_Nonce_Handler::getInstance()->get() ); if ( !empty( $redirect_to ) ) { $stateObj["redirect_to"] = addslashes( $redirect_to ); diff --git a/lib/WP_Auth0_LoginManager.php b/lib/WP_Auth0_LoginManager.php index c61b52af..0dfd3ec1 100755 --- a/lib/WP_Auth0_LoginManager.php +++ b/lib/WP_Auth0_LoginManager.php @@ -7,6 +7,7 @@ class WP_Auth0_LoginManager { protected $ignore_unverified_email; protected $users_repo; protected $state; + protected $state_decoded; public function __construct( WP_Auth0_UsersRepo $users_repo, $a0_options = null, $default_role = null, $ignore_unverified_email = false ) { @@ -157,11 +158,9 @@ public function init_auth0() { $this->die_on_login( $error_msg ? $error_msg : sanitize_text_field( $this->query_vars( 'error' ) ) ); } - // Check for valid state nonce + // Check for valid state nonce, set in WP_Auth0_Lock10_Options::get_state_obj() // See https://auth0.com/docs/protocols/oauth2/oauth-state - $state_decoded = $this->get_state(); - $state_nonce = isset( $state_decoded->nonce ) ? $state_decoded->nonce : ''; - if ( ! WP_Auth0_State_Handler::getInstance()->validate( $state_nonce ) ) { + if ( ! $this->validate_state() ) { $this->die_on_login( __( 'Invalid state', 'wp-auth0' ) ); } @@ -275,7 +274,7 @@ public function redirect_login() { $userinfo = json_decode( $userinfo_resp_body ); if ( $this->login_user( $userinfo, $data->id_token, $data->access_token ) ) { - $state_decoded = $this->get_state(); + $state_decoded = $this->get_state( TRUE ); if ( ! empty( $state_decoded->interim ) ) { include WPA0_PLUGIN_DIR . 'templates/login-interim.php'; } else { @@ -302,7 +301,7 @@ public function redirect_login() { public function implicit_login() { $token = $_POST['token']; - $state_decoded = $this->get_state(); + $state_decoded = $this->get_state( TRUE ); $secret = $this->a0_options->get_client_secret_as_key(); @@ -544,16 +543,41 @@ protected function query_vars( $key ) { /** * Get and store state returned from Auth0 * - * @return object|null + * @param bool $decoded - `true` to return decoded state + * + * @return string|object|null */ - protected function get_state() { + protected function get_state( $decoded = FALSE ) { if ( empty( $this->state ) ) { - $state_val = urldecode( $this->query_vars( 'state' ) ); + // Get and store base64 encoded state + $state_val = $this->query_vars( 'state' ); + $state_val = urldecode( $state_val ); + $this->state = $state_val; + + // Decode and store $state_val = base64_decode( $state_val ); - $this->state = json_decode( $state_val ); + $this->state_decoded = json_decode( $state_val ); + } + + if ( $decoded ) { + return is_object( $this->state_decoded ) ? $this->state_decoded : null; + } else { + return $this->state; } - return is_object( $this->state ) ? $this->state : null; + } + + /** + * Check the state send back from Auth0 with the one stored in the user's browser + * + * @return bool + */ + protected function validate_state() { + $valid = isset( $_COOKIE[ WPA0_STATE_COOKIE_NAME ] ) + ? $_COOKIE[ WPA0_STATE_COOKIE_NAME ] === $this->get_state() + : FALSE; + setcookie( WPA0_STATE_COOKIE_NAME, '', 0, '/' ); + return $valid; } /** diff --git a/lib/WP_Auth0_State_Handler.php b/lib/WP_Auth0_Nonce_Handler.php similarity index 82% rename from lib/WP_Auth0_State_Handler.php rename to lib/WP_Auth0_Nonce_Handler.php index 9c67349d..ccd281bc 100644 --- a/lib/WP_Auth0_State_Handler.php +++ b/lib/WP_Auth0_Nonce_Handler.php @@ -1,6 +1,6 @@ _uniqid; - return setcookie( self::COOKIE_NAME, $this->_uniqid, time() + self::COOKIE_EXPIRES ); + return setcookie( self::COOKIE_NAME, $this->_uniqid, time() + self::COOKIE_EXPIRES, '/' ); } /** @@ -118,13 +118,14 @@ public function reset() { } /** - * Generate a pseudo-random ID (not cryptographically secure) + * Generate a random ID + * If using on PHP 7, it will be cryptographically secure * - * @see https://stackoverflow.com/a/1846229/728480 + * @see https://secure.php.net/manual/en/function.random-bytes.php * * @return string */ public function generateNonce() { - return md5( uniqid( rand(), true ) ); + return function_exists( 'random_bytes' ) ? bin2hex( random_bytes( 32 ) ) : md5( uniqid( rand(), true ) ); } } \ No newline at end of file diff --git a/templates/auth0-login-form-lock10.php b/templates/auth0-login-form-lock10.php index 4065ce6b..726f284b 100644 --- a/templates/auth0-login-form-lock10.php +++ b/templates/auth0-login-form-lock10.php @@ -28,7 +28,7 @@ $title = "Sign In"; } -$options = json_encode( $lock_options->get_lock_options() ); +$options = $lock_options->get_lock_options(); ?> @@ -59,11 +59,18 @@