Fix Worksheet Passwords

Fix for issue #1897.

The existing hashing code seems to work correctly almost all the time, but there are exceptions. It is replaced by an exact implementation of the spec, including a link to the spec in the comments. Cases known to fail are added to the unit test suite.

The spec expects the string to be at most 255 bytes (yes, bytes not characters). The program had permitted any length; it will now throw an exception when the maximum length is exceeded.

Xls does not support any hashing algorithm except basic. The Xls writer had, nevertheless, accepted the results of any of the other possible algorithms. This leads to (a) a worksheet that can't be unprotected, and (b) deprecation notices during the write (because it is using hexdec, which expects only hex characters, and the other algorithms generate non-hex characters). I have changed Xls writer to ignore passwords generated by other algorithms. An alternative would be to have the password hasher generate both an algorithmic password (for use by Xlsx) and a basic password (for use by Xls); I think that is too complex a solution, but can look into it if you think it worthwhile.

I do not see any current support for Worksheet passwords in ODS Reader or Writer. I did not add support in this PR.

I added a new test to confirm the password for reading a spreadsheet is consistent with the one used for writing it. As you can see from the comments for the new test, it had an unusual problem with a somewhat unusual solution.

phpstan-baseline.neon

@@ -4310,11 +4310,6 @@ parameters:
 			count: 1
 			path: src/PhpSpreadsheet/Shared/OLERead.php
 
-		-
-			message: "#^Argument of an invalid type array\\<int, string\\>\\|false supplied for foreach, only iterables are supported\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Shared/PasswordHasher.php
-
 		-
 			message: "#^Method PhpOffice\\\\PhpSpreadsheet\\\\Shared\\\\StringHelper\\:\\:sanitizeUTF8\\(\\) should return string but returns string\\|false\\.$#"
 			count: 1

src/PhpSpreadsheet/Shared/PasswordHasher.php

@@ -2,11 +2,13 @@
 
 namespace PhpOffice\PhpSpreadsheet\Shared;
 
-use PhpOffice\PhpSpreadsheet\Exception;
+use PhpOffice\PhpSpreadsheet\Exception as SpException;
 use PhpOffice\PhpSpreadsheet\Worksheet\Protection;
 
 class PasswordHasher
 {
+    const MAX_PASSWORD_LENGTH = 255;
+
     /**
      * Get algorithm name for PHP.
      */
@@ -34,36 +36,40 @@ class PasswordHasher
             return $mapping[$algorithmName];
         }
 
-        throw new Exception('Unsupported password algorithm: ' . $algorithmName);
+        throw new SpException('Unsupported password algorithm: ' . $algorithmName);
     }
 
     /**
      * Create a password hash from a given string.
      *
-     * This method is based on the algorithm provided by
+     * This method is based on the spec at:
+     * https://interoperability.blob.core.windows.net/files/MS-OFFCRYPTO/[MS-OFFCRYPTO].pdf
+     * 2.3.7.1 Binary Document Password Verifier Derivation Method 1
+     *
+     * It replaces a method based on the algorithm provided by
      * Daniel Rentz of OpenOffice and the PEAR package
      * Spreadsheet_Excel_Writer by Xavier Noguer <xnoguer@rezebra.com>.
      *
+     * Scrutinizer will squawk at the use of bitwise operations here,
+     * but it should ultimately pass.
+     *
      * @param string $pPassword Password to hash
      */
     private static function defaultHashPassword(string $pPassword): string
     {
-        $password = 0x0000;
+        $verifier = 0;
-        $charPos = 1; // char position
+        $pwlen = strlen($pPassword);
-
+        $passwordArray = pack('c', $pwlen) . $pPassword;
-        // split the plain text password in its component characters
+        for ($i = $pwlen; $i >= 0; --$i) {
-        $chars = preg_split('//', $pPassword, -1, PREG_SPLIT_NO_EMPTY);
+            $intermediate1 = (($verifier & 0x4000) === 0) ? 0 : 1;
-        foreach ($chars as $char) {
+            $intermediate2 = 2 * $verifier;
-            $value = ord($char) << $charPos++; // shifted ASCII value
+            $intermediate2 = $intermediate2 & 0x7fff;
-            $rotated_bits = $value >> 15; // rotated bits beyond bit 15
+            $intermediate3 = $intermediate1 | $intermediate2;
-            $value &= 0x7fff; // first 15 bits
+            $verifier = $intermediate3 ^ ord($passwordArray[$i]);
-            $password ^= ($value | $rotated_bits);
         }
+        $verifier ^= 0xCE4B;
 
-        $password ^= strlen($pPassword);
+        return strtoupper(dechex($verifier));
-        $password ^= 0xCE4B;
-
-        return strtoupper(dechex($password));
     }
 
     /**
@@ -82,6 +88,9 @@ class PasswordHasher
      */
     public static function hashPassword(string $password, string $algorithm = '', string $salt = '', int $spinCount = 10000): string
     {
+        if (strlen($password) > self::MAX_PASSWORD_LENGTH) {
+            throw new SpException('Password exceeds ' . self::MAX_PASSWORD_LENGTH . ' characters');
+        }
         $phpAlgorithm = self::getAlgorithm($algorithm);
         if (!$phpAlgorithm) {
             return self::defaultHashPassword($password);

src/PhpSpreadsheet/Writer/Xls/Worksheet.php

@@ -2138,7 +2138,7 @@ class Worksheet extends BIFFwriter
     private function writePassword(): void
     {
         // Exit unless sheet protection and password have been specified
-        if (!$this->phpSheet->getProtection()->getSheet() || !$this->phpSheet->getProtection()->getPassword()) {
+        if (!$this->phpSheet->getProtection()->getSheet() || !$this->phpSheet->getProtection()->getPassword() || $this->phpSheet->getProtection()->getAlgorithm() !== '') {
             return;
         }
 

tests/PhpSpreadsheetTests/Shared/PasswordHasherTest.php

@@ -2,6 +2,7 @@
 
 namespace PhpOffice\PhpSpreadsheetTests\Shared;
 
+use PhpOffice\PhpSpreadsheet\Exception as SpException;
 use PhpOffice\PhpSpreadsheet\Shared\PasswordHasher;
 use PHPUnit\Framework\TestCase;
 
@@ -14,6 +15,9 @@ class PasswordHasherTest extends TestCase
      */
     public function testHashPassword($expectedResult, ...$args): void
     {
+        if ($expectedResult === 'exception') {
+            $this->expectException(SpException::class);
+        }
         $result = PasswordHasher::hashPassword(...$args);
         self::assertEquals($expectedResult, $result);
     }

tests/PhpSpreadsheetTests/Shared/PasswordReloadTest.php

@@ -0,0 +1,63 @@
+<?php
+
+namespace PhpOffice\PhpSpreadsheetTests\Shared;
+
+use PhpOffice\PhpSpreadsheet\Shared\PasswordHasher;
+use PhpOffice\PhpSpreadsheet\Spreadsheet;
+use PhpOffice\PhpSpreadsheetTests\Functional\AbstractFunctional;
+
+class PasswordReloadTest extends AbstractFunctional
+{
+    /**
+     * I don't know why separate process is needed for this test.
+     * I get a weird error without it, and I would rather just scrap the test
+     *     than spend any more time debugging it.
+     * The test works fine without separate process (on Windows) with:
+     *     php vendor\phpunit\phpunit\phpunit tests\PhpSpreadsheetTests\Shared\
+     * But it fails with:
+     *     php vendor\phpunit\phpunit\phpunit tests\PhpSpreadsheetTests\
+     * The error is a mysterious:
+     *     simplexml_load_string(): validity error : Validation failed: no DTD found !
+     *
+     * @runInSeparateProcess
+     * @preserveGlobalState disabled
+     * @dataProvider providerPasswords
+     */
+    public function testPasswordReload(string $format, string $algorithm, bool $supported = true): void
+    {
+        $password = 'hello';
+        $spreadsheet = new Spreadsheet();
+        $sheet = $spreadsheet->getActiveSheet();
+        $sheet->getCell('A1')->setValue(1);
+        $protection = $sheet->getProtection();
+        $protection->setAlgorithm($algorithm);
+        $protection->setPassword($password);
+        $protection->setSheet(true);
+
+        $reloadedSpreadsheet = $this->writeAndReload($spreadsheet, $format);
+        $resheet = $reloadedSpreadsheet->getActiveSheet();
+        $reprot = $resheet->getProtection();
+        $repassword = $reprot->getPassword();
+        $hash = '';
+        if ($supported) {
+            $readAlgorithm = $reprot->getAlgorithm();
+            self::assertSame($algorithm, $readAlgorithm);
+            $salt = $reprot->getSalt();
+            $spin = $reprot->getSpinCount();
+            $hash = PasswordHasher::hashPassword($password, $readAlgorithm, $salt, $spin);
+        }
+        self::assertSame($repassword, $hash);
+        $spreadsheet->disconnectWorksheets();
+        $reloadedSpreadsheet->disconnectWorksheets();
+    }
+
+    public function providerPasswords(): array
+    {
+        return [
+            'Xls basic algorithm' => ['Xls', ''],
+            'Xls cannot use SHA512' => ['Xls', 'SHA-512', false],
+            'Xlsx basic algorithm' => ['Xlsx', ''],
+            'Xlsx can use SHA512' => ['Xlsx', 'SHA-512'],
+        ];
+    }
+}

tests/data/Shared/PasswordHashes.php

@@ -51,4 +51,9 @@ return [
         'Symbols_salt',
         100000,
     ],
+    // Additional tests suggested by Issue #1897
+    ['DCDF', 'ABCDEFGHIJKLMNOPQRSTUVW'],
+    ['ECD1', 'ABCDEFGHIJKLMNOPQRSTUVWX'],
+    ['88D2', 'ABCDEFGHIJKLMNOPQRSTUVWXY'],
+    'password too long' => ['exception', str_repeat('x', 256)],
 ];