Do not use md5() in yourls_salt() (#3278)

* Do not use md5() in yourls_salt()
* More tests while we're here

Fixes #3277

6 files changed, 107 insertions, 17 deletions

diff --git a/admin/plugins.php b/admin/plugins.php
index 4648ac38..2c3ca924 100644
--- a/admin/plugins.php
+++ b/admin/plugins.php
@@ -13,7 +13,7 @@ if( isset( $_GET['page'] ) && !empty( $_GET['page'] ) ) {
 if( isset( $_GET['action'] ) ) {
 
 	// Check nonce
-	yourls_verify_nonce( 'manage_plugins', $_REQUEST['nonce'] );
+	yourls_verify_nonce( 'manage_plugins', $_REQUEST['nonce'] ?? '');
 
 	// Check plugin file is valid
 	if(isset( $_GET['plugin'] ) && yourls_is_a_plugin_file(YOURLS_PLUGINDIR . '/' . $_GET['plugin'] . '/plugin.php') ) {
diff --git a/includes/functions-auth.php b/includes/functions-auth.php
index 3a6f0494..f7216eb5 100644
--- a/includes/functions-auth.php
+++ b/includes/functions-auth.php
@@ -568,12 +568,31 @@ function yourls_tick() {
 }
 
 /**
- * Return salted string
+ * Return hashed string
  *
+ * This function is badly named, it's not a salt or a salted string : it's a cryptographic hash.
+ *
+ * @since 1.4.1
+ * @param string $string   string to salt
+ * @return string          hashed string
  */
 function yourls_salt( $string ) {
 	$salt = defined('YOURLS_COOKIEKEY') ? YOURLS_COOKIEKEY : md5(__FILE__) ;
-	return yourls_apply_filter( 'yourls_salt', md5 ($string . $salt), $string );
+	return yourls_apply_filter( 'yourls_salt', hash_hmac( yourls_hmac_algo(), $string,  $salt), $string );
+}
+
+/**
+ * Return an available hash_hmac() algorithm
+ *
+ * @since 1.8.3
+ * @return string  hash_hmac() algorithm
+ */
+function yourls_hmac_algo() {
+    $algo = yourls_apply_filter( 'hmac_algo', 'sha256' );
+    if( !in_array( $algo, hash_hmac_algos() ) ) {
+        $algo = 'sha256';
+    }
+    return $algo;
 }
 
 /**
@@ -581,8 +600,9 @@ function yourls_salt( $string ) {
  *
  */
 function yourls_create_nonce( $action, $user = false ) {
-	if( false === $user )
-		$user = defined( 'YOURLS_USER' ) ? YOURLS_USER : '-1';
+	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
diff --git a/tests/data/auth/empty.php b/tests/data/auth/empty.php
new file mode 100644
index 00000000..e69de29b
--- /dev/null
+++ b/tests/data/auth/empty.php
diff --git a/tests/tests/auth/auth.php b/tests/tests/auth/auth.php
index b9c88f3c..3cd8b0f2 100644
--- a/tests/tests/auth/auth.php
+++ b/tests/tests/auth/auth.php
@@ -122,7 +122,7 @@ class Auth_Func_Tests extends PHPUnit\Framework\TestCase {
         // Check that md5 hashed passwords match the password
         $this->assertTrue( yourls_check_password_hash( 'random_md5', self::$random_password ) );
 
-        // Unknow user, existing password
+        // Unknown user, existing password
         $this->assertFalse( yourls_check_password_hash( rand_str(), self::$random_password ) );
 
         // Known user, invalid password
@@ -133,11 +133,11 @@ class Auth_Func_Tests extends PHPUnit\Framework\TestCase {
      * Check that valid login / phpass password is deemed valid
      */
     public function test_valid_phpass() {
-        // Check that phppass hashed passwords match the password
+        // Check that phpass hashed passwords match the password
         $this->assertTrue(  yourls_check_password_hash( 'random_phpass1', self::$random_password ) );
         $this->assertTrue(  yourls_check_password_hash( 'random_phpass2', self::$random_password ) );
 
-        // unknow user, existing valid password
+        // unknown user, existing valid password
         $this->assertFalse( yourls_check_password_hash( rand_str(), self::$random_password ) );
 
         // known users, invalid passwords
@@ -177,11 +177,50 @@ class Auth_Func_Tests extends PHPUnit\Framework\TestCase {
     }
 
     /**
+     * Check that encrypting un-writable file returns expected error
+     */
+    public function test_hash_passwords_now_unwritable() {
+        // generate un-writable file
+        $file = YOURLS_TESTDATA_DIR . '/auth/unwritable.php';
+        touch( $file );
+
+        if(yourls_is_windows()) {
+            exec( 'attrib +r ' . escapeshellarg( $file ) );
+        } else {
+            chmod( $file, 0444 );
+        }
+
+        $this->assertSame('cannot write file', yourls_hash_passwords_now( $file ) );
+
+        // cleanup
+        if(yourls_is_windows()) {
+            exec( 'attrib -r ' . escapeshellarg( $file ) );
+        } else {
+            chmod( $file, 0644 );
+        }
+        unlink( $file );
+    }
+
+    /**
+     * Check that encrypting non-existent or unreadable file returns expected error
+     */
+    public function test_hash_passwords_now_non_existent() {
+        $this->assertSame('cannot read file', yourls_hash_passwords_now( rand_str() ) );
+    }
+
+    /**
+     * Check that encrypting empty file returns expected error
+     */
+    public function test_hash_passwords_now_empty() {
+        $this->assertSame('could not read file', yourls_hash_passwords_now( YOURLS_TESTDATA_DIR . '/auth/empty.php' ) );
+    }
+
+    /**
      * Check that in-file password encryption works as expected with different kinds of passwords
      *
      * This test checks that encrypting the config file, with different kinds of pwd, results in a valid
      * PHP file as expected. It doesn't test that the different kinds of password get correctly hashed
-     * and can be correctly decyphered. This task is covered in test_hash_and_check()
+     * and can be correctly deciphered. This task is covered in test_hash_and_check()
      */
     public function test_hash_passwords_special_chars_now() {
 
@@ -237,10 +276,12 @@ class Auth_Func_Tests extends PHPUnit\Framework\TestCase {
     /**
      * Check that we don't hash passwords in config file if user explicitly doesn't want it
      *
-     * Actually we're not testing this :-(
+     * (Note that we're checking with the filter, it can also be enforced with a constant)
      */
-    public function zz_test_maybe_hash_passwords_YOURLS_NO_HASH_PASSWORD() {
-        // we're not testing anything because it currently relies on a defined constant
+    public function test_maybe_hash_passwords_YOURLS_NO_HASH_PASSWORD() {
+        yourls_add_filter('skip_password_hashing', 'yourls_return_true');
+        $this->assertFalse( yourls_maybe_hash_passwords() );
+        yourls_remove_filter('skip_password_hashing', 'yourls_return_true');
     }
 
     /**
@@ -256,6 +297,4 @@ class Auth_Func_Tests extends PHPUnit\Framework\TestCase {
         putenv('YOURLS_PASSWORD');
     }
 
-
-
 }
diff --git a/tests/tests/auth/misc.php b/tests/tests/auth/misc.php
index 11a61696..61156864 100644
--- a/tests/tests/auth/misc.php
+++ b/tests/tests/auth/misc.php
@@ -7,10 +7,36 @@
  */
 class Misc_Auth_Tests extends PHPUnit\Framework\TestCase {
 
+    protected function tearDown(): void {
+        yourls_remove_all_filters( 'hmac_algo' );
+    }
+
     public function test_yourls_skip_password_hashing_is_bool() {
         $this->assertIsBool(yourls_skip_password_hashing());
     }
 
+    public function test_yourls_salt_return_string() {
+        $this->assertIsString(yourls_salt(rand_str()));
+    }
+
+    public function test_yourls_hmac_algo_default() {
+        $this->assertTrue( in_array(yourls_hmac_algo(), hash_hmac_algos()) );
+    }
 
+    public function test_yourls_hmac_algo_custom() {
+        // get random hash_hmac_algo
+        $rnd_algo = hash_hmac_algos()[mt_rand(0, count(hash_hmac_algos()) - 1)];
+        yourls_add_filter('hmac_algo', function() use ($rnd_algo) {
+            return $rnd_algo;
+        } );
+
+        // make sure it's the one we set
+        $algo = yourls_hmac_algo();
+        $this->assertSame($algo, $rnd_algo);
+    }
+
+    public function test_yourls_hmac_algo_non_existent() {
+        $this->assertSame( yourls_hmac_algo('omgozh'), 'sha256' );
+    }
 
 }
diff --git a/tests/tests/auth/nonces.php b/tests/tests/auth/nonces.php
index 4c295806..ba7ffed6 100644
--- a/tests/tests/auth/nonces.php
+++ b/tests/tests/auth/nonces.php
@@ -34,11 +34,16 @@ class Auth_Nonce_Tests extends PHPUnit\Framework\TestCase {
     }
 
 	/**
-	 * Check nonce field creation
+	 * Check nonce field creation and output
 	 */
-	public function test_create_nonce_field() {
-        $field = yourls_nonce_field( rand_str(), rand_str(), rand_str(), false );
+	public function test_create_nonce_field_echo() {
+        $action = rand_str();
+        $name = rand_str();
+        $user = rand_str();
+        $field = yourls_nonce_field( $action, $name, $user, false );
         $this->assertTrue( is_string($field) );
+        $this->expectOutputString( $field . "\n" );
+        $field = yourls_nonce_field( $action, $name, $user, true );
     }
 
 	/**