CVE-2022-0088: Cross-Site Request Forgery (CSRF) (CWE-352)
value: Cross-Site Request Forgery (CSRF) in GitHub repository yourls/yourls prior to 1.8.3.
CWE description: The web application does not, or can not, sufficiently verify whether a well-formed, valid, consistent request was intentionally provided by the user who submitted the request.
CVSS v3 base score: 7.4
Published: 2022-04-03T09:15Z
Repository: https://github.com/yourls/yourls
Language: PHP
Files changed: .gitignore, admin/admin-ajax.php, includes/functions-auth.php, includes/functions-html.php, tests/tests/auth/login_cookie.php, tests/tests/auth/logout.php, tests/tests/auth/misc.php, tests/tests/auth/nonces.php
Commit date: 2022-04-02 13:49:37 +0200
Commit message: Add nonce to the logout link (#3264)

* Add nonce to the logout link
* Add tests for cookies being set or reset
* More tests: check nonces are different for different actions & users

Fixes #3170

Diff: diff --git a/.gitignore b/.gitignore
index 4f03131e..3b293ef2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -29,10 +29,11 @@ user/*
 
 includes/**/tests/
 build/
 coverage/
 phpunit.xml
+.phpunit.result.cache
 tests/yourls-tests-config.php
 tests/vendor/
 tests/data/auth/config-test-auth-hashed.php
 tests/data/htaccess/test*
 
diff --git a/admin/admin-ajax.php b/admin/admin-ajax.php
index 77f68ac2..103cf1c8 100644
--- a/admin/admin-ajax.php
+++ b/admin/admin-ajax.php
@@ -38,15 +38,10 @@
 		yourls_verify_nonce( 'delete-link_'.$_REQUEST['id'], $_REQUEST['nonce'], false, 'omg error' );
 		$query = yourls_delete_link_by_keyword( $_REQUEST['keyword'] );
 		echo json_encode(array('success'=>$query));
 		break;
 
-	case 'logout':
-		// unused for the moment
-		yourls_logout();
-		break;
-
 	default:
 		yourls_do_action( 'yourls_ajax_'.$action );
 
 }
 
diff --git a/includes/functions-auth.php b/includes/functions-auth.php
index 09c8f1a4..3a6f0494 100644
--- a/includes/functions-auth.php
+++ b/includes/functions-auth.php
@@ -18,10 +18,11 @@ function yourls_maybe_require_auth() {
 }
 
 /**
  * Check for valid user via login form or stored cookie. Returns true or an error message
  *
+ * @return bool|string|mixed true if valid user, error message otherwise. Can also call yourls_die() or redirect to login page. Oh my.
  */
 function yourls_is_valid_user() {
 	// Allow plugins to short-circuit the whole function
 	$pre = yourls_apply_filter( 'shunt_is_valid_user', null );
 	if ( null !== $pre ) {
@@ -30,11 +31,13 @@ function yourls_is_valid_user() {
 
 	// $unfiltered_valid : are credentials valid? Boolean value. It's "unfiltered" to allow plugins to eventually filter it.
 	$unfiltered_valid = false;
 
 	// Logout request
-	if( isset( $_GET['action'] ) && $_GET['action'] == 'logout' ) {
+	if( isset( $_GET['action'] ) && $_GET['action'] == 'logout' && isset( $_REQUEST['nonce'] ) ) {
+        // The logout nonce is associated to fake user 'logout' since at this point we don't know the real user
+        yourls_verify_nonce('admin_logout', $_REQUEST['nonce'], 'logout');
 		yourls_do_action( 'logout' );
 		yourls_store_cookie( null );
 		return yourls__( 'Logged out successfully' );
 	}
 
@@ -442,10 +445,12 @@ function yourls_store_cookie( $user = null ) {
 
 	// Some browsers refuse to store localhost cookie
 	if ( $domain == 'localhost' )
 		$domain = '';
 
+	yourls_do_action( 'pre_setcookie', $user, $time, $path, $domain, $secure, $httponly );
+
     if ( !headers_sent( $filename, $linenum ) ) {
         yourls_setcookie( yourls_cookie_name(), yourls_cookie_value( $user ), $time, $path, $domain, $secure, $httponly );
 	} else {
 		// For some reason cookies were not stored: action to be able to debug that
 		yourls_do_action( 'setcookie_failed', $user );
@@ -574,11 +579,11 @@ function yourls_salt( $string ) {
 /**
  * Create a time limited, action limited and user limited token
  *
  */
 function yourls_create_nonce( $action, $user = false ) {
-	if( false == $user )
+	if( false === $user )
 		$user = defined( 'YOURLS_USER' ) ? YOURLS_USER : '-1';
 	$tick = yourls_tick();
 	$nonce = substr( yourls_salt($tick . $action . $user), 0, 10 );
 	// Allow plugins to alter the nonce
 	return yourls_apply_filter( 'create_nonce', $nonce, $action, $user );
@@ -610,28 +615,29 @@ function yourls_nonce_url( $action, $url = false, $name = 'nonce', $user = false
  * Returns true if valid, dies otherwise (yourls_die() or die($return) if defined)
  * if $nonce is false or unspecified, it will use $_REQUEST['nonce']
  *
  */
 function yourls_verify_nonce( $action, $nonce = false, $user = false, $return = '' ) {
-	// get user
-	if( false == $user )
-		$user = defined( 'YOURLS_USER' ) ? YOURLS_USER : '-1';
+	// Get user
+	if( false === $user ) {
+        $user = defined('YOURLS_USER') ? YOURLS_USER : '-1';
+    }
 
-	// get current nonce value
-	if( false == $nonce && isset( $_REQUEST['nonce'] ) )
-		$nonce = $_REQUEST['nonce'];
+	// Get nonce value from $_REQUEST if not specified
+	if( false === $nonce && isset( $_REQUEST['nonce'] ) ) {
+        $nonce = $_REQUEST['nonce'];
+    }
 
 	// Allow plugins to short-circuit the rest of the function
-	$valid = yourls_apply_filter( 'verify_nonce', false, $action, $nonce, $user, $return );
-	if ($valid) {
+	if (yourls_apply_filter( 'verify_nonce', false, $action, $nonce, $user, $return ) === true) {
 		return true;
 	}
 
-	// what nonce should be
+	// What nonce should be
 	$valid = yourls_create_nonce( $action, $user );
 
-	if( $nonce == $valid ) {
+	if( $nonce === $valid ) {
 		return true;
 	} else {
 		if( $return )
 			die( $return );
 		yourls_die( yourls__( 'Unauthorized action or expired link' ), yourls__( 'Error' ), 403 );
diff --git a/includes/functions-html.php b/includes/functions-html.php
index 4de8891f..c88880a9 100644
--- a/includes/functions-html.php
+++ b/includes/functions-html.php
@@ -748,11 +748,15 @@ function yourls_login_screen( $error_msg = '' ) {
  */
 function yourls_html_menu() {
 
 	// Build menu links
 	if( defined( 'YOURLS_USER' ) ) {
-		$logout_link = yourls_apply_filter( 'logout_link', sprintf( yourls__('Hello <strong>%s</strong>'), YOURLS_USER ) . ' (<a href="' . yourls_admin_url( 'index.php' ) . '?action=logout" title="' . yourls_esc_attr__( 'Logout' ) . '">' . yourls__( 'Logout' ) . '</a>)' );
+	    // Create a logout link with a nonce associated to fake user 'logout' : the user is not yet defined
+	    // when the logout check is done -- see yourls_is_valid_user()
+	    $logout_url = yourls_nonce_url( 'admin_logout',
+		yourls_add_query_arg(['action' => 'logout'], yourls_admin_url('index.php')), 'nonce', 'logout');
+		$logout_link = yourls_apply_filter('logout_link', sprintf( yourls__('Hello <strong>%s</strong>'), YOURLS_USER ) . ' (<a href="' . $logout_url . '" title="' . yourls_esc_attr__( 'Logout' ) . '">' . yourls__( 'Logout' ) . '</a>)' );
 	} else {
 		$logout_link = yourls_apply_filter( 'logout_link', '' );
 	}
 	$help_link   = yourls_apply_filter( 'help_link',   '<a href="' . yourls_site_url( false ) .'/readme.html">' . yourls__( 'Help' ) . '</a>' );
 
diff --git a/tests/tests/auth/login_cookie.php b/tests/tests/auth/login_cookie.php
index 7cba9e74..050671da 100644
--- a/tests/tests/auth/login_cookie.php
+++ b/tests/tests/auth/login_cookie.php
@@ -9,19 +9,24 @@
  */
 class Auth_Login_Cookie_Tests extends PHPUnit\Framework\TestCase {
 
     protected $cookie;
     protected $request;
+    protected $backup_yourls_actions;
 
     protected function setUp(): void {
         $this->cookie = $_COOKIE;
         $this->request = $_REQUEST;
+        global $yourls_actions;
+        $this->backup_yourls_actions = $yourls_actions;
     }
 
     protected function tearDown(): void {
         $_COOKIE = $this->cookie;
         $_REQUEST = $this->request;
+        global $yourls_actions;
+        $yourls_actions = $this->backup_yourls_actions;
     }
 
     public static function setUpBeforeClass(): void {
         yourls_add_filter( 'is_API', 'yourls_return_false' );
     }
@@ -50,29 +55,33 @@ public function test_cookie_value() {
 	public function test_cookie_life() {
         $this->assertTrue( is_int(yourls_get_cookie_life()) );
     }
 
 	/**
-	 * Test login with valid cookie
+	 * Test login with valid cookie - also check that cookie is set
 	 */
 	public function test_login_valid_cookie() {
         global $yourls_user_passwords;
         $random_user = array_rand($yourls_user_passwords);
         $_COOKIE[yourls_cookie_name()] = yourls_cookie_value( $random_user );
         unset($_REQUEST);
 
+        $this->assertSame( 0, yourls_did_action('pre_setcookie') );
         $this->assertTrue(yourls_check_auth_cookie());
         $this->assertTrue(yourls_is_valid_user());
+        $this->assertSame( 1, yourls_did_action('pre_setcookie') );
     }
 
 	/**
-	 * Test login with invalid cookie
+	 * Test login with invalid cookie - also check that no cookie is set
 	 */
 	public function test_login_invalid_cookie() {
         $_COOKIE[yourls_cookie_name()] = yourls_cookie_value( rand_str() );
         unset($_REQUEST);
 
+        $this->assertSame( 0, yourls_did_action('pre_setcookie') );
         $this->assertFalse(yourls_check_auth_cookie());
         $this->assertNotTrue(yourls_is_valid_user());
+        $this->assertSame( 0, yourls_did_action('pre_setcookie') );
     }
 
 }
diff --git a/tests/tests/auth/logout.php b/tests/tests/auth/logout.php
index 5de8c84a..a7153d9e 100644
--- a/tests/tests/auth/logout.php
+++ b/tests/tests/auth/logout.php
@@ -7,46 +7,62 @@
  */
 class Logout_Func_Tests extends PHPUnit\Framework\TestCase {
 
     protected $backup_get;
     protected $backup_request;
+    private static $user;
 
     protected function setUp(): void {
         $this->backup_get     = $_GET;
         $this->backup_request = $_REQUEST;
+        self::$user = false;
     }
 
     protected function tearDown(): void {
         $_GET = $this->backup_get;
         $_REQUEST = $this->backup_request;
     }
 
+    public static function setUpBeforeClass(): void {
+        yourls_add_action( 'pre_setcookie', function ($in) {
+            self::$user = $in[0]; // $in[0] is the user ID passed to yourls_setcookie()
+        } );
+    }
+
+    public static function tearDownAfterClass(): void {
+        yourls_remove_all_actions('pre_setcookie');
+    }
+
     /**
-     * Check logout procedure - phase 1
+     * Check logout procedure - phase 1 - we're logging in
      */
     public function test_logout_user_is_logged_in() {
         $_REQUEST['nonce'] = yourls_create_nonce('admin_login');
         $valid = yourls_is_valid_user();
         $this->assertTrue($valid);
+        $this->assertSame(self::$user, 'yourls');
     }
 
     /**
-     * Check logout procedure - phase 2
+     * Check logout procedure - phase 2 - we're logging out and checking that cookie was reset
      * @depends test_logout_user_is_logged_in
      */
     public function test_logout_user_logs_out() {
         $_GET['action'] = 'logout';
+        $_REQUEST['nonce'] = yourls_create_nonce('admin_logout', 'logout');
         $invalid = yourls_is_valid_user();
         $this->assertNotTrue( $invalid );
+        $this->assertSame(self::$user, null);
     }
 
     /**
-     * Check logout procedure - phase 3
+     * Check logout procedure - phase 3 - check we can log in again
      * @depends test_logout_user_logs_out
      */
     public function test_logout_user_is_logged_in_back() {
         $_REQUEST['nonce'] = yourls_create_nonce('admin_login');
         $valid = yourls_is_valid_user();
         $this->assertTrue( $valid );
+        $this->assertSame(self::$user, 'yourls');
     }
 
 }
diff --git a/tests/tests/auth/misc.php b/tests/tests/auth/misc.php
index b753fe4f..11a61696 100644
--- a/tests/tests/auth/misc.php
+++ b/tests/tests/auth/misc.php
@@ -9,6 +9,8 @@ class Misc_Auth_Tests extends PHPUnit\Framework\TestCase {
 
     public function test_yourls_skip_password_hashing_is_bool() {
         $this->assertIsBool(yourls_skip_password_hashing());
     }
 
+
+
 }
diff --git a/tests/tests/auth/nonces.php b/tests/tests/auth/nonces.php
index 21dc9f42..4c295806 100644
--- a/tests/tests/auth/nonces.php
+++ b/tests/tests/auth/nonces.php
@@ -45,11 +45,10 @@ public function test_create_nonce_field() {
 	 * Check nonce URL creation
 	 */
 	public function test_create_nonce_url() {
         $url = yourls_nonce_url( rand_str(), rand_str(), rand_str(), rand_str() );
         $this->assertTrue( is_string($url) );
-        // $this->assertIsString($url);
     }
 
 	/**
 	 * Test valid nonce
 	 */
@@ -75,6 +74,25 @@ public function test_invalid_nonce() {
 
         // This should trigger yourls_die()
         $this->assertTrue(yourls_verify_nonce(rand_str(), rand_str(), rand_str()));
     }
 
+    /**
+     * Check nonces are different for different actions & users
+     */
+    public function test_nonce_different_for_different_actions_and_users() {
+        $action1 = rand_str();
+        $action2 = rand_str();
+        $user1 = rand_str();
+        $user2 = rand_str();
+
+        $nonce_a1 = yourls_create_nonce($action1);
+        $nonce_a2 = yourls_create_nonce($action2);
+        $nonce_a1_u1 = yourls_create_nonce($action1, $user1);
+        $nonce_a1_u2 = yourls_create_nonce($action1, $user2);
+
+        $this->assertNotEquals($nonce_a1, $nonce_a2);
+        $this->assertNotEquals($nonce_a1_u1, $nonce_a1_u2);
+        $this->assertNotEquals($nonce_a1, $nonce_a1_u1);
+    }
+
 }

Vulnerable code: 	case 'logout':
		// unused for the moment
		yourls_logout();
		break;

	if( isset( $_GET['action'] ) && $_GET['action'] == 'logout' ) {
	if( false == $user )
	// get user
	if( false == $user )
		$user = defined( 'YOURLS_USER' ) ? YOURLS_USER : '-1';
	// get current nonce value
	if( false == $nonce && isset( $_REQUEST['nonce'] ) )
		$nonce = $_REQUEST['nonce'];
	$valid = yourls_apply_filter( 'verify_nonce', false, $action, $nonce, $user, $return );
	if ($valid) {
	// what nonce should be
	if( $nonce == $valid ) {
		$logout_link = yourls_apply_filter( 'logout_link', sprintf( yourls__('Hello <strong>%s</strong>'), YOURLS_USER ) . ' (<a href="' . yourls_admin_url( 'index.php' ) . '?action=logout" title="' . yourls_esc_attr__( 'Logout' ) . '">' . yourls__( 'Logout' ) . '</a>)' );
	 * Test login with valid cookie
	 * Test login with invalid cookie
     * Check logout procedure - phase 1
     * Check logout procedure - phase 2
     * Check logout procedure - phase 3
        // $this->assertIsString($url);
Fixed code: .phpunit.result.cache
 * @return bool|string|mixed true if valid user, error message otherwise. Can also call yourls_die() or redirect to login page. Oh my.
	if( isset( $_GET['action'] ) && $_GET['action'] == 'logout' && isset( $_REQUEST['nonce'] ) ) {
        // The logout nonce is associated to fake user 'logout' since at this point we don't know the real user
        yourls_verify_nonce('admin_logout', $_REQUEST['nonce'], 'logout');
	yourls_do_action( 'pre_setcookie', $user, $time, $path, $domain, $secure, $httponly );

	if( false === $user )
	// Get user
	if( false === $user ) {
        $user = defined('YOURLS_USER') ? YOURLS_USER : '-1';
    }
	// Get nonce value from $_REQUEST if not specified
	if( false === $nonce && isset( $_REQUEST['nonce'] ) ) {
        $nonce = $_REQUEST['nonce'];
    }
	if (yourls_apply_filter( 'verify_nonce', false, $action, $nonce, $user, $return ) === true) {
	// What nonce should be
	if( $nonce === $valid ) {
	    // Create a logout link with a nonce associated to fake user 'logout' : the user is not yet defined
	    // when the logout check is done -- see yourls_is_valid_user()
	    $logout_url = yourls_nonce_url( 'admin_logout',
		yourls_add_query_arg(['action' => 'logout'], yourls_admin_url('index.php')), 'nonce', 'logout');
		$logout_link = yourls_apply_filter('logout_link', sprintf( yourls__('Hello <strong>%s</strong>'), YOURLS_USER ) . ' (<a href="' . $logout_url . '" title="' . yourls_esc_attr__( 'Logout' ) . '">' . yourls__( 'Logout' ) . '</a>)' );
    protected $backup_yourls_actions;
        global $yourls_actions;
        $this->backup_yourls_actions = $yourls_actions;
        global $yourls_actions;
        $yourls_actions = $this->backup_yourls_actions;
	 * Test login with valid cookie - also check that cookie is set
        $this->assertSame( 0, yourls_did_action('pre_setcookie') );
        $this->assertSame( 1, yourls_did_action('pre_setcookie') );
	 * Test login with invalid cookie - also check that no cookie is set
        $this->assertSame( 0, yourls_did_action('pre_setcookie') );
        $this->assertSame( 0, yourls_did_action('pre_setcookie') );
    private static $user;
        self::$user = false;
    public static function setUpBeforeClass(): void {
        yourls_add_action( 'pre_setcookie', function ($in) {
            self::$user = $in[0]; // $in[0] is the user ID passed to yourls_setcookie()
        } );
    }

    public static function tearDownAfterClass(): void {
        yourls_remove_all_actions('pre_setcookie');
    }

     * Check logout procedure - phase 1 - we're logging in
        $this->assertSame(self::$user, 'yourls');
     * Check logout procedure - phase 2 - we're logging out and checking that cookie was reset
        $_REQUEST['nonce'] = yourls_create_nonce('admin_logout', 'logout');
        $this->assertSame(self::$user, null);
     * Check logout procedure - phase 3 - check we can log in again
        $this->assertSame(self::$user, 'yourls');


    /**
     * Check nonces are different for different actions & users
     */
    public function test_nonce_different_for_different_actions_and_users() {
        $action1 = rand_str();
        $action2 = rand_str();
        $user1 = rand_str();
        $user2 = rand_str();

        $nonce_a1 = yourls_create_nonce($action1);
        $nonce_a2 = yourls_create_nonce($action2);
        $nonce_a1_u1 = yourls_create_nonce($action1, $user1);
        $nonce_a1_u2 = yourls_create_nonce($action1, $user2);

        $this->assertNotEquals($nonce_a1, $nonce_a2);
        $this->assertNotEquals($nonce_a1_u1, $nonce_a1_u2);
        $this->assertNotEquals($nonce_a1, $nonce_a1_u1);
    }