Bug 66436: Fix invalid handling of padded encrypted bytes

The encrypted data is padded to 16 bytes, but these additional bytes
were not read from the stream

Make the reading of the additional bytes "lenient" to not introduce
breaking change if some existing functionality did produce non-aligned
data for some reason.

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1907444 13f79535-47bb-0310-9956-ffa450edef68

2 files changed, 207 insertions, 39 deletions

diff --git a/poi-ooxml/src/test/java/org/apache/poi/poifs/crypt/tests/TestAgileEncryptionParameters.java b/poi-ooxml/src/test/java/org/apache/poi/poifs/crypt/tests/TestAgileEncryptionParameters.java
index 27930ce1d0..e0ad0febea 100644
--- a/poi-ooxml/src/test/java/org/apache/poi/poifs/crypt/tests/TestAgileEncryptionParameters.java
+++ b/poi-ooxml/src/test/java/org/apache/poi/poifs/crypt/tests/TestAgileEncryptionParameters.java
@@ -18,16 +18,14 @@ package org.apache.poi.poifs.crypt.tests;
 
 import static org.junit.jupiter.api.Assertions.assertArrayEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
-import static org.junit.jupiter.api.Assumptions.assumeTrue;
 
+import java.io.ByteArrayInputStream;
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.stream.Stream;
 
-import javax.crypto.Cipher;
-
 import org.apache.commons.io.output.UnsynchronizedByteArrayOutputStream;
 import org.apache.poi.POIDataSamples;
 import org.apache.poi.poifs.crypt.ChainingMode;
@@ -39,67 +37,127 @@ import org.apache.poi.poifs.crypt.Encryptor;
 import org.apache.poi.poifs.crypt.HashAlgorithm;
 import org.apache.poi.poifs.filesystem.POIFSFileSystem;
 import org.apache.poi.util.IOUtils;
-import org.junit.jupiter.api.BeforeAll;
+import org.apache.poi.xssf.usermodel.XSSFCell;
+import org.apache.poi.xssf.usermodel.XSSFRow;
+import org.apache.poi.xssf.usermodel.XSSFSheet;
+import org.apache.poi.xssf.usermodel.XSSFWorkbook;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
 
 class TestAgileEncryptionParameters {
-
-    static byte[] testData;
-
-    public static Stream<Arguments> data() {
+    public static Stream<Arguments> data() throws Exception {
         CipherAlgorithm[] caList = {CipherAlgorithm.aes128, CipherAlgorithm.aes192, CipherAlgorithm.aes256, CipherAlgorithm.rc2, CipherAlgorithm.des, CipherAlgorithm.des3};
         HashAlgorithm[] haList = {HashAlgorithm.sha1, HashAlgorithm.sha256, HashAlgorithm.sha384, HashAlgorithm.sha512, HashAlgorithm.md5};
         ChainingMode[] cmList = {ChainingMode.cbc, ChainingMode.cfb};
 
+        List<byte[]> byteList = initTestData();
+
         List<Arguments> data = new ArrayList<>();
         for (CipherAlgorithm ca : caList) {
             for (HashAlgorithm ha : haList) {
                 for (ChainingMode cm : cmList) {
-                    data.add(Arguments.of(ca,ha,cm));
+                    // do not iterate all byte-arrays here to keep runtime of test at bay
+                    data.add(Arguments.of(byteList.get(0), EncryptionMode.agile, ca, ha, cm));
+                    data.add(Arguments.of(byteList.get(1), EncryptionMode.agile, ca, ha, cm));
                 }
             }
         }
 
+        // iterate all byte-array for each encryption-mode
+        // they usually only support certain algorithms, so keep them fixed
+        // also to not have a very long test-runtime
+        for (byte[] bytes : byteList) {
+            data.add(Arguments.of(bytes, EncryptionMode.agile, CipherAlgorithm.aes192, HashAlgorithm.sha256, ChainingMode.cbc));
+
+            data.add(Arguments.of(bytes, EncryptionMode.standard, CipherAlgorithm.aes128, HashAlgorithm.sha1, ChainingMode.ecb));
+
+            // CryptoAPI does not support getDataStream()
+            // data.add(Arguments.of(bytes, EncryptionMode.cryptoAPI, CipherAlgorithm.rc4, HashAlgorithm.sha1, ChainingMode.cfb));
+
+            // xor does not support createEncryptionInfoEntry()
+            // data.add(Arguments.of(bytes, EncryptionMode.xor, CipherAlgorithm.des3, HashAlgorithm.md5, ChainingMode.cfb));
+
+            data.add(Arguments.of(bytes, EncryptionMode.binaryRC4, CipherAlgorithm.rc4, HashAlgorithm.sha512, ChainingMode.ecb));
+        }
+
         return data.stream();
     }
 
-    @BeforeAll
-    public static void initTestData() throws Exception {
-        InputStream testFile = POIDataSamples.getDocumentInstance().openResourceAsStream("SampleDoc.docx");
-        testData = IOUtils.toByteArray(testFile);
-        testFile.close();
+    public static List<byte[]> initTestData() throws Exception {
+        List<byte[]> data = new ArrayList<>();
+
+        // read a sample file for encrypting
+        try (InputStream testFile = POIDataSamples.getDocumentInstance().openResourceAsStream("SampleDoc.docx")) {
+            data.add(IOUtils.toByteArray(testFile));
+        }
+
+        // create a small sample workbook for encrypting
+        UnsynchronizedByteArrayOutputStream bosOrig = new UnsynchronizedByteArrayOutputStream();
+        try (XSSFWorkbook workbook = new XSSFWorkbook()) {
+            XSSFSheet sheet = workbook.createSheet();
+            XSSFRow row = sheet.createRow(0);
+            XSSFCell cell = row.createCell(0);
+            cell.setCellValue("Hello Apache POI");
+            workbook.write(bosOrig);
+        }
+        bosOrig.close();
+
+        data.add(IOUtils.toByteArray(new ByteArrayInputStream(bosOrig.toByteArray())));
+
+        // test with a dummy-block of data that is a multiple of 16
+        byte[] testData = new byte[4000];
+        for (int i = 0; i < testData.length; i++) {
+            testData[i] = (byte)i;
+        }
+        data.add(testData);
+
+        // test with a dummy-block of data that is not a multiple of 16
+        testData = new byte[3292];
+        for (int i = 0; i < testData.length; i++) {
+            testData[i] = (byte)i;
+        }
+        data.add(testData);
+
+        return data;
     }
 
     @ParameterizedTest
     @MethodSource("data")
-    void testAgileEncryptionModes(CipherAlgorithm ca, HashAlgorithm ha, ChainingMode cm) throws Exception {
-        int maxKeyLen = Cipher.getMaxAllowedKeyLength(ca.jceId);
-        assumeTrue(maxKeyLen >= ca.defaultKeySize, "Please install JCE Unlimited Strength Jurisdiction Policy files");
-
-        UnsynchronizedByteArrayOutputStream bos = new UnsynchronizedByteArrayOutputStream();
-
-        POIFSFileSystem fsEnc = new POIFSFileSystem();
-        EncryptionInfo infoEnc = new EncryptionInfo(EncryptionMode.agile, ca, ha, -1, -1, cm);
+    void testAgileEncryptionModes(byte[] testData, EncryptionMode mode, CipherAlgorithm ca, HashAlgorithm ha, ChainingMode cm)
+            throws Exception {
+        EncryptionInfo infoEnc = new EncryptionInfo(mode, ca, ha, -1, -1, cm);
         Encryptor enc = infoEnc.getEncryptor();
         enc.confirmPassword("foobaa");
-        OutputStream os = enc.getDataStream(fsEnc);
-        os.write(testData);
-        os.close();
-        bos.reset();
-        fsEnc.writeFilesystem(bos);
-        fsEnc.close();
-
-        POIFSFileSystem fsDec = new POIFSFileSystem(bos.toInputStream());
-        EncryptionInfo infoDec = new EncryptionInfo(fsDec);
-        Decryptor dec = infoDec.getDecryptor();
-        boolean passed = dec.verifyPassword("foobaa");
-        assertTrue(passed);
-        InputStream is = dec.getDataStream(fsDec);
-        byte[] actualData = IOUtils.toByteArray(is);
-        is.close();
-        fsDec.close();
-        assertArrayEquals(testData, actualData, "Failed roundtrip - "+ca+"-"+ha+"-"+cm);
+
+        byte[] inputData;
+        try (POIFSFileSystem fsEnc = new POIFSFileSystem()) {
+            try (OutputStream os = enc.getDataStream(fsEnc)) {
+                os.write(testData);
+            }
+
+            try (UnsynchronizedByteArrayOutputStream bos = new UnsynchronizedByteArrayOutputStream()) {
+                fsEnc.writeFilesystem(bos);
+
+                bos.close();
+                inputData = bos.toByteArray();
+            }
+        }
+
+        byte[] actualData;
+        try (POIFSFileSystem fsDec = new POIFSFileSystem(new ByteArrayInputStream(inputData))) {
+            EncryptionInfo infoDec = new EncryptionInfo(fsDec);
+            Decryptor dec = infoDec.getDecryptor();
+            boolean passed = dec.verifyPassword("foobaa");
+            assertTrue(passed);
+            InputStream is = dec.getDataStream(fsDec);
+
+            actualData = IOUtils.toByteArray(is);
+            is.close();
+        }
+
+        // input-data and resulting decrypted data should be equal
+        assertArrayEquals(testData, actualData,
+                "Having " + testData.length + " bytes and parameters - " + ca + "-" + ha + "-" + cm);
     }
 }
diff --git a/poi-ooxml/src/test/java/org/apache/poi/xssf/usermodel/TestXSSFBugs.java b/poi-ooxml/src/test/java/org/apache/poi/xssf/usermodel/TestXSSFBugs.java
index f700d98a93..4522797792 100644
--- a/poi-ooxml/src/test/java/org/apache/poi/xssf/usermodel/TestXSSFBugs.java
+++ b/poi-ooxml/src/test/java/org/apache/poi/xssf/usermodel/TestXSSFBugs.java
@@ -30,12 +30,17 @@ import static org.apache.poi.xssf.XSSFTestDataSamples.writeOutAndReadBack;
 import static org.junit.jupiter.api.Assertions.*;
 import static org.junit.jupiter.api.Assumptions.assumeFalse;
 
+import java.io.BufferedReader;
+import java.io.ByteArrayInputStream;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.io.OutputStream;
 import java.nio.charset.StandardCharsets;
+import java.security.GeneralSecurityException;
 import java.time.Duration;
 import java.time.Instant;
 import java.time.LocalDateTime;
@@ -65,6 +70,10 @@ import org.apache.poi.openxml4j.opc.PackagePart;
 import org.apache.poi.openxml4j.opc.PackageRelationship;
 import org.apache.poi.openxml4j.opc.PackagingURIHelper;
 import org.apache.poi.openxml4j.util.ZipSecureFile;
+import org.apache.poi.poifs.crypt.Decryptor;
+import org.apache.poi.poifs.crypt.EncryptionInfo;
+import org.apache.poi.poifs.crypt.EncryptionMode;
+import org.apache.poi.poifs.crypt.Encryptor;
 import org.apache.poi.poifs.filesystem.DirectoryNode;
 import org.apache.poi.poifs.filesystem.DocumentEntry;
 import org.apache.poi.poifs.filesystem.DocumentInputStream;
@@ -90,6 +99,7 @@ import org.apache.poi.ss.formula.functions.Function;
 import org.apache.poi.ss.formula.ptg.Ptg;
 import org.apache.poi.ss.usermodel.*;
 import org.apache.poi.ss.util.*;
+import org.apache.poi.util.IOUtils;
 import org.apache.poi.util.LocaleUtil;
 import org.apache.poi.util.TempFile;
 import org.apache.poi.util.XMLHelper;
@@ -3747,4 +3757,104 @@ public final class TestXSSFBugs extends BaseTestBugzillaIssues {
             assertEquals(23, stylesTable.getFonts().size());
         }
     }
+
+    private static final String secretKey = "foobaa";
+
+    @Test
+    void testBug66436() throws IOException, InvalidFormatException, GeneralSecurityException {
+        final File temp_excel_poi = TempFile.createTempFile("temp_excel_poi", ".xlsx");
+        final File temp_excel_poi_encrypt = TempFile.createTempFile("temp_excel_poi_encrypt", ".xlsx");
+        final File temp_excel_poi_decrypt = TempFile.createTempFile("temp_excel_poi_decrypt", ".xlsx");
+
+        /* create new excel by poi */
+        try (XSSFWorkbook workbook = new XSSFWorkbook();
+                FileOutputStream foss = new FileOutputStream(temp_excel_poi)) {
+            XSSFSheet sheet = workbook.createSheet();
+            XSSFRow row = sheet.createRow(0);
+            XSSFCell cell = row.createCell(0);
+            cell.setCellValue("Hello Apache POI");
+            workbook.write(foss);
+        }
+
+        // read bytes of workbook before
+        UnsynchronizedByteArrayOutputStream bosOrig = new UnsynchronizedByteArrayOutputStream();
+        try (FileInputStream fis = new FileInputStream(temp_excel_poi)) {
+            IOUtils.copy(fis, bosOrig);
+        }
+
+        // for the encrypted bytes
+        UnsynchronizedByteArrayOutputStream bosEnc = new UnsynchronizedByteArrayOutputStream();
+
+        /* encrypt excel by poi */
+        try (POIFSFileSystem fs = new POIFSFileSystem()) {
+            EncryptionInfo info = new EncryptionInfo(EncryptionMode.agile);
+            Encryptor enc = info.getEncryptor();
+            enc.confirmPassword(secretKey);
+
+            // Read in an existing OOXML file and write to encrypted output stream
+            // don't forget to close the output stream otherwise the padding bytes aren't added
+            try (OPCPackage opc = OPCPackage.open(new FileInputStream(temp_excel_poi));
+                    OutputStream os = enc.getDataStream(fs)) {
+                opc.save(os);
+            }
+
+            fs.writeFilesystem(bosEnc);
+
+            bosEnc.close();
+
+            // Write out the encrypted version
+            try (FileOutputStream fos = new FileOutputStream(temp_excel_poi_encrypt)) {
+                IOUtils.copy(new ByteArrayInputStream(bosEnc.toByteArray()), fos);
+            }
+        }
+
+        // for the decrytped bytes
+        UnsynchronizedByteArrayOutputStream bosDec = new UnsynchronizedByteArrayOutputStream();
+
+        /* decrypt excel by poi */
+        try (POIFSFileSystem fileSystem = new POIFSFileSystem(temp_excel_poi_encrypt)) {
+            EncryptionInfo info = new EncryptionInfo(fileSystem);
+            Decryptor d = Decryptor.getInstance(info);
+            if (!d.verifyPassword(secretKey)) {
+                throw new RuntimeException("Unable to process: document is encrypted");
+            }
+
+            // parse dataStream
+            try (InputStream dataStream = d.getDataStream(fileSystem)) {
+                IOUtils.copy(dataStream, bosDec);
+            }
+
+            try (FileOutputStream fos = new FileOutputStream(temp_excel_poi_decrypt)) {
+                IOUtils.copy(new ByteArrayInputStream(bosDec.toByteArray()), fos);
+            }
+        }
+
+        // input-data and resulting decrypted data should be equal
+        /* This is a flaky assertion, maybe there is a timestamp in the files which can differ
+        in the two byte-arrays. Other tests verify this for encryption/decryption anyway
+        assertArrayEquals(bosOrig.toByteArray(), bosDec.toByteArray(),
+                "Having " + bosOrig.size() + " bytes");*/
+
+        // also make sure the original and the resulting decrypted
+        // file can be read, i.e. is a valid Zip
+        readByCommonsCompress(temp_excel_poi);
+        readByCommonsCompress(temp_excel_poi_decrypt);
+    }
+
+    private static void readByCommonsCompress(File temp_excel_poi) throws IOException {
+        /* read by commons-compress*/
+        try (ZipFile zipFile = new ZipFile(temp_excel_poi)) {
+            ZipArchiveEntry entry = zipFile.getEntry("xl/workbook.xml");
+            InputStream inputStream = zipFile.getInputStream(entry);
+
+            BufferedReader bufferedReader = new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8));
+            while (true) {
+                String line = bufferedReader.readLine();
+                if (line == null) {
+                    break;
+                }
+                //System.out.println(line);
+            }
+        }
+    }
 }
\ No newline at end of file