Merge pull request #2197 from oleibman/sheetpasswd

Fix Worksheet Passwords

phpstan-baseline.neon

@@ -4020,11 +4020,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
@@ -6635,11 +6630,6 @@ parameters:
 			count: 1
 			path: tests/PhpSpreadsheetTests/Worksheet/RowCellIterator2Test.php
 
-		-
-			message: "#^Parameter \\#1 \\$options of static method PhpOffice\\\\PhpSpreadsheet\\\\Settings\\:\\:setLibXmlLoaderOptions\\(\\) expects int, null given\\.$#"
-			count: 1
-			path: tests/PhpSpreadsheetTests/Worksheet/WorksheetNamedRangesTest.php
-
 		-
 			message: "#^Property PhpOffice\\\\PhpSpreadsheetTests\\\\Writer\\\\Csv\\\\CsvEnclosureTest\\:\\:\\$cellValues has no typehint specified\\.$#"
 			count: 1
@@ -6680,11 +6670,6 @@ parameters:
 			count: 3
 			path: tests/PhpSpreadsheetTests/Writer/Html/HtmlCommentsTest.php
 
-		-
-			message: "#^Parameter \\#1 \\$options of static method PhpOffice\\\\PhpSpreadsheet\\\\Settings\\:\\:setLibXmlLoaderOptions\\(\\) expects int, null given\\.$#"
-			count: 1
-			path: tests/PhpSpreadsheetTests/Writer/Xlsx/FloatsRetainedTest.php
-
 		-
 			message: "#^Parameter \\#2 \\$locale of function setlocale expects string\\|null, string\\|false given\\.$#"
 			count: 1
@@ -6695,16 +6680,6 @@ parameters:
 			count: 1
 			path: tests/PhpSpreadsheetTests/Writer/Xlsx/LocaleFloatsTest.php
 
-		-
-			message: "#^Parameter \\#1 \\$options of static method PhpOffice\\\\PhpSpreadsheet\\\\Settings\\:\\:setLibXmlLoaderOptions\\(\\) expects int, null given\\.$#"
-			count: 2
-			path: tests/PhpSpreadsheetTests/Writer/Xlsx/StartsWithHashTest.php
-
-		-
-			message: "#^Parameter \\#1 \\$options of static method PhpOffice\\\\PhpSpreadsheet\\\\Settings\\:\\:setLibXmlLoaderOptions\\(\\) expects int, null given\\.$#"
-			count: 2
-			path: tests/PhpSpreadsheetTests/Writer/Xlsx/UnparsedDataCloneTest.php
-
 		-
 			message: "#^Cannot call method getDrawingCollection\\(\\) on PhpOffice\\\\PhpSpreadsheet\\\\Worksheet\\\\Worksheet\\|null\\.$#"
 			count: 4
@@ -6715,11 +6690,6 @@ parameters:
 			count: 4
 			path: tests/PhpSpreadsheetTests/Writer/Xlsx/UnparsedDataCloneTest.php
 
-		-
-			message: "#^Parameter \\#1 \\$options of static method PhpOffice\\\\PhpSpreadsheet\\\\Settings\\:\\:setLibXmlLoaderOptions\\(\\) expects int, null given\\.$#"
-			count: 1
-			path: tests/PhpSpreadsheetTests/Writer/Xlsx/UnparsedDataTest.php
-
 		-
 			message: "#^Parameter \\#1 \\$data of function simplexml_load_string expects string, string\\|false given\\.$#"
 			count: 2

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/SettingsTest.php

@@ -41,6 +41,7 @@ class SettingsTest extends TestCase
 
     public function testSetXMLSettings(): void
     {
+        $original = Settings::getLibXmlLoaderOptions();
         Settings::setLibXmlLoaderOptions(LIBXML_DTDLOAD | LIBXML_DTDATTR | LIBXML_DTDVALID);
         $result = Settings::getLibXmlLoaderOptions();
         self::assertTrue((bool) ((LIBXML_DTDLOAD | LIBXML_DTDATTR | LIBXML_DTDVALID) & $result));
@@ -48,5 +49,6 @@ class SettingsTest extends TestCase
         if (\PHP_VERSION_ID < 80000) {
             self::assertFalse(libxml_disable_entity_loader());
         }
+        Settings::setLibXmlLoaderOptions($original);
     }
 }

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,51 @@
+<?php
+
+namespace PhpOffice\PhpSpreadsheetTests\Shared;
+
+use PhpOffice\PhpSpreadsheet\Shared\PasswordHasher;
+use PhpOffice\PhpSpreadsheet\Spreadsheet;
+use PhpOffice\PhpSpreadsheetTests\Functional\AbstractFunctional;
+
+class PasswordReloadTest extends AbstractFunctional
+{
+    /**
+     * @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/PhpSpreadsheetTests/Worksheet/WorksheetNamedRangesTest.php

@@ -4,7 +4,6 @@ namespace PhpOffice\PhpSpreadsheetTests\Worksheet;
 
 use PhpOffice\PhpSpreadsheet\Exception;
 use PhpOffice\PhpSpreadsheet\Reader\Xlsx;
-use PhpOffice\PhpSpreadsheet\Settings;
 use PhpOffice\PhpSpreadsheet\Spreadsheet;
 use PHPUnit\Framework\TestCase;
 
@@ -17,8 +16,6 @@ class WorksheetNamedRangesTest extends TestCase
 
     protected function setUp(): void
     {
-        Settings::setLibXmlLoaderOptions(null); // reset to default options
-
         $reader = new Xlsx();
         $this->spreadsheet = $reader->load('tests/data/Worksheet/namedRangeTest.xlsx');
     }

tests/PhpSpreadsheetTests/Writer/Xlsx/ConditionalTest.php

@@ -2,7 +2,6 @@
 
 namespace PhpOffice\PhpSpreadsheetTests\Writer\Xlsx;
 
-use PhpOffice\PhpSpreadsheet\Settings;
 use PhpOffice\PhpSpreadsheet\Spreadsheet;
 use PhpOffice\PhpSpreadsheet\Style\Conditional;
 use PhpOffice\PhpSpreadsheet\Style\Fill;
@@ -11,24 +10,6 @@ use PhpOffice\PhpSpreadsheetTests\Functional\AbstractFunctional;
 
 class ConditionalTest extends AbstractFunctional
 {
-    /**
-     * @var int
-     */
-    private $prevValue;
-
-    protected function setUp(): void
-    {
-        $this->prevValue = Settings::getLibXmlLoaderOptions();
-
-        // Disable validating XML with the DTD
-        Settings::setLibXmlLoaderOptions($this->prevValue & ~LIBXML_DTDVALID & ~LIBXML_DTDATTR & ~LIBXML_DTDLOAD);
-    }
-
-    protected function tearDown(): void
-    {
-        Settings::setLibXmlLoaderOptions($this->prevValue);
-    }
-
     /**
      * Test check if conditional style with type 'notContainsText' works on xlsx.
      */

tests/PhpSpreadsheetTests/Writer/Xlsx/DrawingsTest.php

@@ -4,30 +4,11 @@ namespace PhpOffice\PhpSpreadsheetTests\Writer\Xlsx;
 
 use PhpOffice\PhpSpreadsheet\IOFactory;
 use PhpOffice\PhpSpreadsheet\Reader\Xlsx;
-use PhpOffice\PhpSpreadsheet\Settings;
 use PhpOffice\PhpSpreadsheet\Shared\File;
 use PhpOffice\PhpSpreadsheetTests\Functional\AbstractFunctional;
 
 class DrawingsTest extends AbstractFunctional
 {
-    /**
-     * @var int
-     */
-    private $prevValue;
-
-    protected function setUp(): void
-    {
-        $this->prevValue = Settings::getLibXmlLoaderOptions();
-
-        // Disable validating XML with the DTD
-        Settings::setLibXmlLoaderOptions($this->prevValue & ~LIBXML_DTDVALID & ~LIBXML_DTDATTR & ~LIBXML_DTDLOAD);
-    }
-
-    protected function tearDown(): void
-    {
-        Settings::setLibXmlLoaderOptions($this->prevValue);
-    }
-
     /**
      * Test save and load XLSX file with drawing on 2nd worksheet.
      */

tests/PhpSpreadsheetTests/Writer/Xlsx/FloatsRetainedTest.php

@@ -3,7 +3,6 @@
 namespace PhpOffice\PhpSpreadsheetTests\Writer\Xlsx;
 
 use PhpOffice\PhpSpreadsheet\Reader\Xlsx as Reader;
-use PhpOffice\PhpSpreadsheet\Settings;
 use PhpOffice\PhpSpreadsheet\Shared\File;
 use PhpOffice\PhpSpreadsheet\Spreadsheet;
 use PhpOffice\PhpSpreadsheet\Writer\Xlsx as Writer;
@@ -19,7 +18,6 @@ class FloatsRetainedTest extends TestCase
     public function testIntyFloatsRetainedByWriter($value): void
     {
         $outputFilename = File::temporaryFilename();
-        Settings::setLibXmlLoaderOptions(null);
         $sheet = new Spreadsheet();
         $sheet->getActiveSheet()->getCell('A1')->setValue($value);
 

tests/PhpSpreadsheetTests/Writer/Xlsx/StartsWithHashTest.php

@@ -4,7 +4,6 @@ namespace PhpOffice\PhpSpreadsheetTests\Writer\Xlsx;
 
 use PhpOffice\PhpSpreadsheet\Cell\DataType;
 use PhpOffice\PhpSpreadsheet\Reader\Xlsx as Reader;
-use PhpOffice\PhpSpreadsheet\Settings;
 use PhpOffice\PhpSpreadsheet\Shared\File;
 use PhpOffice\PhpSpreadsheet\Spreadsheet;
 use PhpOffice\PhpSpreadsheet\Writer\Xlsx as Writer;
@@ -16,7 +15,6 @@ class StartsWithHashTest extends TestCase
     public function testStartWithHash(): void
     {
         $outputFilename = File::temporaryFilename();
-        Settings::setLibXmlLoaderOptions(null);
         $spreadsheet = new Spreadsheet();
         $sheet = $spreadsheet->getActiveSheet();
         $sheet->setCellValueExplicit('A1', '#define M', DataType::TYPE_STRING);
@@ -41,7 +39,6 @@ class StartsWithHashTest extends TestCase
     {
         // Make sure raw data indicates A3 is an error, but A2 isn't.
         $outputFilename = File::temporaryFilename();
-        Settings::setLibXmlLoaderOptions(null);
         $spreadsheet = new Spreadsheet();
         $sheet = $spreadsheet->getActiveSheet();
         $sheet->setCellValueExplicit('A1', '#define M', DataType::TYPE_STRING);

tests/PhpSpreadsheetTests/Writer/Xlsx/UnparsedDataCloneTest.php

@@ -2,7 +2,6 @@
 
 namespace PhpOffice\PhpSpreadsheetTests\Writer\Xlsx;
 
-use PhpOffice\PhpSpreadsheet\Settings;
 use PhpOffice\PhpSpreadsheet\Shared\File;
 use PHPUnit\Framework\TestCase;
 use ZipArchive;
@@ -16,7 +15,6 @@ class UnparsedDataCloneTest extends TestCase
     {
         $sampleFilename = 'tests/data/Writer/XLSX/drawing_on_2nd_page.xlsx';
         $resultFilename = File::temporaryFilename();
-        Settings::setLibXmlLoaderOptions(null); // reset to default options
         $reader = new \PhpOffice\PhpSpreadsheet\Reader\Xlsx();
         $spreadsheet = $reader->load($sampleFilename);
         $spreadsheet->setActiveSheetIndex(1);
@@ -63,7 +61,6 @@ class UnparsedDataCloneTest extends TestCase
         $resultFilename1 = File::temporaryFilename();
         $resultFilename2 = File::temporaryFilename();
         self::assertNotEquals($resultFilename1, $resultFilename2);
-        Settings::setLibXmlLoaderOptions(null); // reset to default options
         $reader = new \PhpOffice\PhpSpreadsheet\Reader\Xlsx();
         $spreadsheet = $reader->load($sampleFilename);
         $sheet = $spreadsheet->setActiveSheetIndex(1);

tests/PhpSpreadsheetTests/Writer/Xlsx/UnparsedDataTest.php

@@ -17,7 +17,6 @@ class UnparsedDataTest extends TestCase
     {
         $sampleFilename = 'tests/data/Writer/XLSX/form_pass_print.xlsm';
         $resultFilename = File::temporaryFilename();
-        Settings::setLibXmlLoaderOptions(null); // reset to default options
         $reader = new \PhpOffice\PhpSpreadsheet\Reader\Xlsx();
         $excel = $reader->load($sampleFilename);
 

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)],
 ];