Fix sanitization issues (#28)

* Fix sanitization issues
duosecurity · Nov 1, 2023 · 8351859 · 8351859
1 parent 4a48b0a
commit 8351859
Show file tree

Hide file tree

Showing 5 changed files with 107 additions and 59 deletions.
diff --git a/class-duouniversal-settings.php b/class-duouniversal-settings.php
@@ -122,7 +122,7 @@ function duo_settings_failmode() {
 
 	function duoup_failmode_validate( $failmode ) {
 		$failmode = sanitize_text_field( $failmode );
-		if ( ! in_array( $failmode, array( 'open', 'closed' ) ) ) {
+		if ( ! in_array( $failmode, array( 'open', 'closed' ), true ) ) {
 			add_settings_error( 'duoup_failmode', '', 'Failmode value is not valid' );
 			$current_failmode = $this->duo_utils->duo_get_option( 'duoup_failmode', 'open' );
 			return $current_failmode;
@@ -147,7 +147,7 @@ function duo_settings_roles() {
 				"name='duoup_roles[" . \esc_attr( $key ) . "]' " .
 				"type='checkbox' " .
 				"value='" . \esc_attr( $role ) . "' " .
-				( in_array( $role, $selected ) ? 'checked' : '' ) .
+				( in_array( $role, $selected, true ) ? 'checked' : '' ) .
 			'/>' .
 			\esc_html( $role ) .
 			'<br />' );
@@ -197,7 +197,7 @@ function duoup_xmlrpc_validate( $option ) {
 	}
 
 	function duo_add_link( $links ) {
-		$settings_link = '<a href="options-general.php?page=duo_universal_wordpress">' . \translate( 'Settings', 'duo_universal_wordpress' ) . '</a>';
+		$settings_link = '<a href="options-general.php?page=duo_universal_wordpress">' . \__( 'Settings', 'duo_universal_wordpress' ) . '</a>';
 		array_unshift( $links, $settings_link );
 		return $links;
 	}
@@ -271,36 +271,37 @@ function duo_mu_options() {
 
 	function duo_update_mu_options() {
 		if ( isset( $_POST['duoup_client_id'] ) ) {
-			$client_id = $this->duoup_client_id_validate( $_POST['duoup_client_id'] );
+			$client_id = $this->duoup_client_id_validate( sanitize_text_field( \wp_unslash( $_POST['duoup_client_id'] ) ) );
 			$result    = \update_site_option( 'duoup_client_id', $client_id );
 		}
 
 		if ( isset( $_POST['duoup_client_secret'] ) ) {
-			$client_secret = $this->duoup_client_secret_validate( $_POST['duoup_client_secret'] );
+			$client_secret = $this->duoup_client_secret_validate( sanitize_text_field( \wp_unslash( $_POST['duoup_client_secret'] ) ) );
 			$result        = \update_site_option( 'duoup_client_secret', $client_secret );
 		}
 
 		if ( isset( $_POST['duoup_api_host'] ) ) {
-			$host   = $this->duoup_api_host_validate( $_POST['duoup_api_host'] );
+			$host   = $this->duoup_api_host_validate( sanitize_text_field( \wp_unslash( $_POST['duoup_api_host'] ) ) );
 			$result = \update_site_option( 'duoup_api_host', $host );
 		}
 
 		if ( isset( $_POST['duoup_failmode'] ) ) {
-			$failmode = $this->duoup_failmode_validate( $_POST['duoup_failmode'] );
+			$failmode = $this->duoup_failmode_validate( sanitize_text_field( \wp_unslash( $_POST['duoup_failmode'] ) ) );
 			$result   = \update_site_option( 'duoup_failmode', $failmode );
 		} else {
 			$result = \update_site_option( 'duoup_failmode', 'open' );
 		}
 
 		if ( isset( $_POST['duoup_roles'] ) ) {
-			$roles  = $this->duoup_roles_validate( $_POST['duoup_roles'] );
+            // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized
+			$roles  = $this->duoup_roles_validate( \wp_unslash( $_POST['duoup_roles'] ) );
 			$result = \update_site_option( 'duoup_roles', $roles );
 		} else {
 			$result = \update_site_option( 'duoup_roles', array() );
 		}
 
 		if ( isset( $_POST['duoup_xmlrpc'] ) ) {
-			$xmlrpc = $this->duoup_xmlrpc_validate( $_POST['duoup_xmlrpc'] );
+			$xmlrpc = $this->duoup_xmlrpc_validate( sanitize_text_field( \wp_unslash( $_POST['duoup_xmlrpc'] ) ) );
 			$result = \update_site_option( 'duoup_xmlrpc', $xmlrpc );
 		} else {
 			$result = \update_site_option( 'duoup_xmlrpc', 'on' );

diff --git a/class-duouniversal-utilities.php b/class-duouniversal-utilities.php
@@ -83,15 +83,22 @@ function duo_get_uri() {
 		// paths (for which protocols are not required/enforced), and REQUEST_URI
 		// always includes the leading slash in the URI path.
 		if ( ! isset( $_SERVER['REQUEST_URI'] )
+            // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.MissingUnslash
 			|| ( ! empty( $_SERVER['QUERY_STRING'] ) && ! strpos( \sanitize_url( $_SERVER['REQUEST_URI'] ), '?', 0 ) )
 		) {
-			$current_uri = \sanitize_url( substr( $_SERVER['PHP_SELF'], 1 ) );
+			if ( ! isset( $_SERVER['PHP_SELF'] ) ) {
+				throw new Exception( 'Could not determine request URI' );
+			}
+            // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.MissingUnslash
+			$current_uri = isset( $_SERVER['PHP_SELF'] ) ? substr( \sanitize_url( $_SERVER['PHP_SELF'] ), 1 ) : null;
 			if ( isset( $_SERVER['QUERY_STRING'] ) ) {
+                // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.MissingUnslash
 				$current_uri = \sanitize_url( $current_uri . '?' . $_SERVER['QUERY_STRING'] );
 			}
 
 			return $current_uri;
 		} else {
+            // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.MissingUnslash
 			return \sanitize_url( $_SERVER['REQUEST_URI'] );
 		}
 	}
@@ -114,4 +121,8 @@ function duo_debug_log( $message ) {
 	function new_WP_User( $id, $name = '', $site_id = '' ) {
 		return new \WP_User( $id, $name, $site_id );
 	}
+
+	function new_WP_Error( $code, $message = '', $data = '' ) {
+		return new \WP_Error( $code, $message, $data );
+	}
 }
diff --git a/class-duouniversal-wordpressplugin.php b/class-duouniversal-wordpressplugin.php
@@ -93,10 +93,20 @@ function get_page_url() {
 		// the script was queried through HTTPS. However, IIS will set the
 		// value to 'off' HTTPS was not used, so we have to check that special
 		// case.
-		$https_used = ( ! empty( $_SERVER['HTTPS'] ) && strtolower( \sanitize_text_field( $_SERVER['HTTPS'] ) ) !== 'off' );
-		$port       = absint( $_SERVER['SERVER_PORT'] );
-		$protocol   = ( $https_used || 443 === $port ) ? 'https://' : 'http://';
-		return \sanitize_url( $protocol . $_SERVER['HTTP_HOST'] . $this->duo_utils->duo_get_uri(), array( 'http', 'https' ) );
+		$https_used = ( ! empty( $_SERVER['HTTPS'] ) && strtolower( \sanitize_text_field( wp_unslash( $_SERVER['HTTPS'] ) ) ) !== 'off' );
+
+		if ( ! isset( $_SERVER['SERVER_PORT'] ) ) {
+			throw new Exception( 'Could not determine server port' );
+		}
+
+		$port     = isset( $_SERVER['SERVER_PORT'] ) ? absint( $_SERVER['SERVER_PORT'] ) : null;
+		$protocol = ( $https_used || 443 === $port ) ? 'https://' : 'http://';
+
+		if ( ! isset( $_SERVER['HTTP_HOST'] ) ) {
+			throw new Exception( 'Could not determine host' );
+		}
+		$host = ! empty( $_SERVER['HTTP_HOST'] ) ? \sanitize_text_field( wp_unslash( $_SERVER['HTTP_HOST'] ) ) : null;
+		return \sanitize_url( $protocol . $host . $this->duo_utils->duo_get_uri(), array( 'http', 'https' ) );
 	}
 
 	function exit() {
@@ -135,42 +145,44 @@ function duo_authenticate_user( $user = '', $username = '', $password = '' ) {
 		if ( isset( $_GET['duo_code'] ) ) {
 			// doing secondary auth.
 			if ( isset( $_GET['error'] ) ) {
-				$error_msg = \sanitize_text_field( $_GET['error'] ) . ':' . \sanitize_text_field( $_GET['error_description'] );
-				$error     = \WP_Error(
+				$error_msg = \sanitize_text_field( wp_unslash( $_GET['error'] ) );
+				if ( isset( $_GET['error_description'] ) ) {
+					$error_msg .= ': ' . \sanitize_text_field( wp_unslash( $_GET['error_description'] ) );
+				}
+				$error = $this->duo_utils->new_WP_Error(
 					'Duo authentication failed',
-					\translate( "<strong>ERROR</strong>: $error_msg" )
+                    // phpcs:ignore WordPress.WP.I18n.NonSingularStringLiteralText
+					\__( 'ERROR: ' . $error_msg )
 				);
-				$this->duo_debug_log( $error_msg );
+				$this->duo_debug_log( $error->get_error_message() );
 				return $error;
 			}
 
 			if ( ! isset( $_GET['state'] ) ) {
-				$error_msg = 'Missing state';
-				$error     = \WP_Error(
+				$error = $this->duo_utils->new_WP_Error(
 					'Duo authentication failed',
-					\translate( "<strong>ERROR</strong>: $error_msg" )
+					\__( 'ERROR: Missing state' )
 				);
-				$this->duo_debug_log( $error_msg );
+				$this->duo_debug_log( $error->get_error_message() );
 				return $error;
 			}
 			$this->duo_debug_log( 'Doing secondary auth' );
 
 			// Get authorization token to trade for 2FA.
-			$code = \sanitize_text_field( $_GET['duo_code'] );
+			$code = \sanitize_text_field( wp_unslash( $_GET['duo_code'] ) );
 
 			// Get state to verify consistency and originality.
-			$state = \sanitize_text_field( $_GET['state'] );
+			$state = \sanitize_text_field( wp_unslash( $_GET['state'] ) );
 
 			// Retrieve the previously stored state and username from the session.
 			$associated_user = $this->get_username_from_oidc_state( $state );
 
 			if ( empty( $associated_user ) ) {
-				$error_msg = 'No saved state please login again';
-				$error     = \WP_Error(
+				$error = $this->duo_utils->new_WP_Error(
 					'Duo authentication failed',
-					\translate( "<strong>ERROR</strong>: $error_msg" )
+					\__( 'ERROR: No saved state please login again' )
 				);
-				$this->duo_debug_log( $error_msg );
+				$this->duo_debug_log( $error->get_error_message() );
 				return $error;
 			}
 			try {
@@ -179,12 +191,11 @@ function duo_authenticate_user( $user = '', $username = '', $password = '' ) {
 				$decoded_token                  = $this->duo_client->exchangeAuthorizationCodeFor2FAResult( $code, $associated_user );
 			} catch ( Duo\DuoUniversal\DuoException $e ) {
 				$this->duo_debug_log( $e->getMessage() );
-				$error_msg = 'Error decoding Duo result. Confirm device clock is correct.';
-				$error     = \WP_Error(
+				$error = $this->duo_utils->new_WP_Error(
 					'Duo authentication failed',
-					\translate( "<strong>ERROR</strong>: $error_msg" )
+					\__( 'ERROR: Error decoding Duo result. Confirm device clock is correct.' )
 				);
-				$this->duo_debug_log( $error_msg );
+				$this->duo_debug_log( $error->get_error_message() );
 				return $error;
 			}
 			$this->duo_debug_log( "Completed secondary auth for $associated_user" );
@@ -232,12 +243,11 @@ function duo_authenticate_user( $user = '', $username = '', $password = '' ) {
 						$this->update_user_auth_status( $user->user_login, 'authenticated' );
 						return $user;
 					} else {
-						$error_msg = '2FA Unavailable. Confirm Duo client/secret/host values are correct';
-						$error     = \WP_Error(
-							'Duo authentication_failed',
-							\translate( "<strong>Error</strong>: $error_msg" )
+						$error = $this->duo_utils->new_WP_Error(
+							'Duo authentication failed',
+							\__( 'Error: 2FA Unavailable. Confirm Duo client/secret/host values are correct' )
 						);
-						$this->duo_debug_log( $error_msg );
+						$this->duo_debug_log( $error->get_error_message() );
 						$this->clear_user_auth( $user );
 						return $error;
 					}

diff --git a/tests/duoUniversalAuthenticationTest.php b/tests/duoUniversalAuthenticationTest.php
@@ -120,7 +120,7 @@ function testPromptRedirect(): void
         WP_Mock::userFunction('wp_logout')->once();
 
         $authentication->duo_start_second_factor($user);
-        $this->assertConditionsMet(); 
+        $this->assertConditionsMet();
     }
 
     /**
@@ -168,7 +168,7 @@ function testStartSecondFactorLogout(): void
         WP_Mock::passthruFunction('wp_redirect');
 
         $authentication->duo_start_second_factor($user);
-        $this->assertConditionsMet(); 
+        $this->assertConditionsMet();
     }
 
     /**
@@ -252,15 +252,20 @@ function testAuthUserAPIErrorSet(): void
                 ]
             )
             ->getMock();
+        $error = $this->getMockBuilder(stdClass::class)
+            ->setMockClassName('WP_Error')
+            ->addMethods(["get_error_message"])
+            ->getMock();
         $this->duo_utils->method('duo_auth_enabled')->willReturn(true);
-        WP_Mock::passthruFunction('translate');
-        WP_Mock::userFunction('WP_Error', [ 'return_arg' => 1 ]);
+        $this->duo_utils->method('new_WP_Error')->willReturn($error)->with("Duo authentication failed", "ERROR: test error: test description");
+        WP_Mock::passthruFunction('__');
+        WP_Mock::passthruFunction('wp_unslash');
 
         $_GET['duo_code'] = "testcode";
         $_GET['error'] = "test error";
         $_GET['error_description'] = "test description";
         $result = $authentication->duo_authenticate_user();
-        $this->assertRegExp("/test description/", $result);
+        $this->assertConditionsMet();
     }
 
     /**
@@ -276,13 +281,18 @@ function testAuthUserStateMissing(): void
                 ]
             )
             ->getMock();
+        $error = $this->getMockBuilder(stdClass::class)
+            ->setMockClassName('WP_Error')
+            ->addMethods(["get_error_message"])
+            ->getMock();
         $this->duo_utils->method('duo_auth_enabled')->willReturn(true);
-        WP_Mock::passthruFunction('translate');
-        WP_Mock::userFunction('WP_Error', ['return_arg' => 1]);
+        $this->duo_utils->method('new_WP_Error')->willReturn($error)->with("Duo authentication failed", "ERROR: Missing state");
+        WP_Mock::passthruFunction('__');
+        WP_Mock::passthruFunction('wp_unslash');
 
         $_GET['duo_code'] = "testcode";
-        $result = $authentication->duo_authenticate_user();
-        $this->assertRegExp("/Missing state/", $result);
+        $authentication->duo_authenticate_user();
+        $this->assertConditionsMet();
     }
 
     /**
@@ -299,16 +309,21 @@ function testAuthUserUserMissing(): void
                 ]
             )
             ->getMock();
+        $error = $this->getMockBuilder(stdClass::class)
+            ->setMockClassName('WP_Error')
+            ->addMethods(["get_error_message"])
+            ->getMock();
         $authentication->method('get_username_from_oidc_state')->willReturn(null);
         $this->duo_utils->method('duo_auth_enabled')->willReturn(true);
-        WP_Mock::passthruFunction('translate');
-        WP_Mock::userFunction('WP_Error', [ 'return_arg' => 1 ]);
+        $this->duo_utils->method('new_WP_Error')->willReturn($error)->with("Duo authentication failed", "ERROR: No saved state please login again");
+        WP_Mock::passthruFunction('__');
+        WP_Mock::passthruFunction('wp_unslash');
         $_GET['duo_code'] = "testcode";
         $_GET['state'] = "teststate";
 
-        $result = $authentication->duo_authenticate_user();
+        $authentication->duo_authenticate_user();
 
-        $this->assertRegExp("/No saved state/", $result);
+        $this->assertConditionsMet();
     }
 
     /**
@@ -326,17 +341,22 @@ function testAuthUserExceptionHandling(): void
                 ]
             )
             ->getMock();
-        WP_Mock::passthruFunction('translate');
-        WP_Mock::userFunction('WP_Error', [ 'return_arg' => 1 ]);
+        $error = $this->getMockBuilder(stdClass::class)
+            ->setMockClassName('WP_Error')
+            ->addMethods(["get_error_message"])
+            ->getMock();
+        WP_Mock::passthruFunction('__');
+        WP_Mock::passthruFunction('wp_unslash');
         $this->duo_utils->method('duo_auth_enabled')->willReturn(true);
+        $this->duo_utils->method('new_WP_Error')->willReturn($error)->with("Duo authentication failed", "ERROR: Error decoding Duo result. Confirm device clock is correct.");
         $authentication->method('get_username_from_oidc_state')->willReturn("test user");
         $this->duo_client->method('exchangeAuthorizationCodeFor2FAResult')->willThrowException(new Duo\DuoUniversal\DuoException("there was a problem"));
         $_GET['duo_code'] = "testcode";
         $_GET['state'] = "teststate";
 
-        $result = $authentication->duo_authenticate_user();
+        $authentication->duo_authenticate_user();
 
-        $this->assertRegExp("/Error decoding Duo result/", $result);
+        $this->assertConditionsMet();
     }
 
     /**
@@ -349,6 +369,7 @@ function testAuthUserSuccess(): void
             $map[$key] = $value;
         };
         WP_Mock::userFunction('set_transient', ['return' => $callback]);
+        WP_Mock::passthruFunction('wp_unslash');
         $authentication = $this->getMockBuilder(DuoUniversal_WordpressPlugin::class)
             ->setConstructorArgs(array($this->duo_utils, $this->duo_client))
             ->onlyMethods(
@@ -608,19 +629,23 @@ function testAuthUserSecondaryExceptionFailmodeClose(): void
         $user = $this->getMockBuilder(stdClass::class)
             ->setMockClassName('WP_User')
             ->getMock();
+        $error = $this->getMockBuilder(stdClass::class)
+            ->setMockClassName('WP_Error')
+            ->addMethods(["get_error_message"])
+            ->getMock();
         $user->user_login = "test user";
         $user->roles = [];
         $this->duo_utils->method('new_WP_user')->willReturn($user);
-        WP_Mock::userFunction('WP_Error', ['return_arg' => 1]);
-        WP_Mock::passthruFunction('translate');
+        $this->duo_utils->method('new_WP_Error')->willReturn($error)->with("Duo authentication failed", "Error: 2FA Unavailable. Confirm Duo client/secret/host values are correct");
+        WP_Mock::passthruFunction('__');
         WP_Mock::passthruFunction('remove_action');
         WP_Mock::userFunction('wp_authenticate_username_password', [ 'return' => $user ]);
         $this->duo_utils->method('duo_get_option')->willReturn('closed');
 
         $result = $authentication->duo_authenticate_user(null, "test user");
 
         $this->assertFalse(array_key_exists("duo_auth_test user_status", $map));
-        $this->assertRegExp("/2FA Unavailable/", $result);
+        $this->assertConditionsMet();
     }
 
     /**
@@ -645,7 +670,7 @@ function testVerifyAuthDisabled(): void
         $result = $authentication->duo_verify_auth();
 
         $this->assertEquals($result, null);
-        $this->assertConditionsMet(); 
+        $this->assertConditionsMet();
     }
 
     /**

diff --git a/tests/duoUniversalSettingsTest.php b/tests/duoUniversalSettingsTest.php
@@ -294,7 +294,7 @@ public function testSettingsRoles(): void
             ->onlyMethods(['duo_get_option', 'duo_get_roles'])
             ->getMock();
 
-        $duo_utils->method('duo_get_option')->willReturn(["uses_2fa"]);
+        $duo_utils->method('duo_get_option')->willReturn(["uses_2fa" => true]);
         $duo_utils->method('duo_get_roles')->willReturn($roles);
         $settings = new Duo\DuoUniversalWordpress\DuoUniversal_Settings($duo_utils);
 
@@ -578,6 +578,7 @@ public function testDuoMultisiteUpdateWithPostValues(): void
         WP_Mock::userFunction('update_site_option')->once()->with('duoup_failmode', 'closed');
         WP_Mock::userFunction('update_site_option')->once()->with('duoup_roles', $duoup_roles);
         WP_Mock::userFunction('update_site_option')->once()->with('duoup_xmlrpc', 'off');
+        WP_Mock::passthruFunction('wp_unslash');
 
         $settings = new Duo\DuoUniversalWordpress\DuoUniversal_Settings($this->duo_utils);
         $settings->duo_update_mu_options();