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

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 {
-
+    public static Stream<Arguments> data() throws Exception {
-    static byte[] testData;
-
-    public static Stream<Arguments> data() {
         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 List<byte[]> initTestData() throws Exception {
-    public static void initTestData() throws Exception {
+        List<byte[]> data = new ArrayList<>();
-        InputStream testFile = POIDataSamples.getDocumentInstance().openResourceAsStream("SampleDoc.docx");
+
-        testData = IOUtils.toByteArray(testFile);
+        // read a sample file for encrypting
-        testFile.close();
+        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 {
+    void testAgileEncryptionModes(byte[] testData, EncryptionMode mode, CipherAlgorithm ca, HashAlgorithm ha, ChainingMode cm)
-        int maxKeyLen = Cipher.getMaxAllowedKeyLength(ca.jceId);
+            throws Exception {
-        assumeTrue(maxKeyLen >= ca.defaultKeySize, "Please install JCE Unlimited Strength Jurisdiction Policy files");
+        EncryptionInfo infoEnc = new EncryptionInfo(mode, ca, ha, -1, -1, cm);
-
-        UnsynchronizedByteArrayOutputStream bos = new UnsynchronizedByteArrayOutputStream();
-
-        POIFSFileSystem fsEnc = new POIFSFileSystem();
-        EncryptionInfo infoEnc = new EncryptionInfo(EncryptionMode.agile, 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());
+        byte[] inputData;
-        EncryptionInfo infoDec = new EncryptionInfo(fsDec);
+        try (POIFSFileSystem fsEnc = new POIFSFileSystem()) {
-        Decryptor dec = infoDec.getDecryptor();
+            try (OutputStream os = enc.getDataStream(fsEnc)) {
-        boolean passed = dec.verifyPassword("foobaa");
+                os.write(testData);
-        assertTrue(passed);
+            }
-        InputStream is = dec.getDataStream(fsDec);
+
-        byte[] actualData = IOUtils.toByteArray(is);
+            try (UnsynchronizedByteArrayOutputStream bos = new UnsynchronizedByteArrayOutputStream()) {
-        is.close();
+                fsEnc.writeFilesystem(bos);
-        fsDec.close();
+
-        assertArrayEquals(testData, actualData, "Failed roundtrip - "+ca+"-"+ha+"-"+cm);
+                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);
     }
 }

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);
+            }
+        }
+    }
 }

poi/src/main/java/org/apache/poi/poifs/crypt/ChunkedCipherInputStream.java

@@ -196,6 +196,17 @@ public abstract class ChunkedCipherInputStream extends LittleEndianInputStream {
             throw new EOFException("buffer underrun");
         }
 
+        // encrypted data is processed in chunks of 16 bytes,
+        // so try to read some more data if the current data is not a
+        // multiple of 16 bytes
+        if (totalBytes % 16 != 0) {
+            int toRead = 16 - totalBytes % 16;
+            int read = super.read(plain, totalBytes, toRead);
+            if (read > 0) {
+                totalBytes += read;
+            }
+        }
+
         System.arraycopy(plain, 0, chunk, 0, totalBytes);
 
         invokeCipher(totalBytes, totalBytes == chunkSize);

poi/src/test/java/org/apache/poi/poifs/crypt/agile/TestAgileDecryptor.java

@@ -0,0 +1,102 @@
+/* ====================================================================
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+==================================================================== */
+
+package org.apache.poi.poifs.crypt.agile;
+
+import static org.apache.poi.poifs.crypt.Decryptor.DEFAULT_POIFS_ENTRY;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+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 org.apache.commons.io.output.UnsynchronizedByteArrayOutputStream;
+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.DocumentInputStream;
+import org.apache.poi.poifs.filesystem.POIFSFileSystem;
+import org.apache.poi.util.HexDump;
+import org.apache.poi.util.IOUtils;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+class TestAgileDecryptor {
+    @SuppressWarnings("PrimitiveArrayArgumentToVarargsMethod")
+    public static Stream<Arguments> data() {
+        List<Arguments> data = new ArrayList<>();
+        data.add(Arguments.of(new byte[15]));
+        data.add(Arguments.of(new byte[16]));
+        data.add(Arguments.of(new byte[17]));
+        data.add(Arguments.of(new byte[3292]));
+        data.add(Arguments.of(new byte[3293]));
+        data.add(Arguments.of(new byte[4000]));
+
+        return data.stream();
+    }
+
+    @ParameterizedTest
+    @MethodSource("data")
+    void testAgileDecryptor(byte[] testData) throws Exception {
+        EncryptionInfo infoEnc = new EncryptionInfo(EncryptionMode.agile);
+        Encryptor enc = infoEnc.getEncryptor();
+        enc.confirmPassword("f");
+
+        byte[] encData;
+        byte[] encDocument;
+        try (POIFSFileSystem fsEnc = new POIFSFileSystem()) {
+            try (OutputStream os = enc.getDataStream(fsEnc)) {
+                os.write(testData);
+            }
+
+            UnsynchronizedByteArrayOutputStream bos = new UnsynchronizedByteArrayOutputStream();
+            fsEnc.writeFilesystem(bos);
+
+            bos.close();
+            encData = bos.toByteArray();
+
+            DocumentInputStream dis = fsEnc.getRoot().createDocumentInputStream(DEFAULT_POIFS_ENTRY);
+            /*long _length =*/ dis.readLong();
+            encDocument = IOUtils.toByteArray(dis);
+        }
+
+        byte[] actualData;
+        try (POIFSFileSystem fsDec = new POIFSFileSystem(new ByteArrayInputStream(encData))) {
+            EncryptionInfo infoDec = new EncryptionInfo(fsDec);
+            Decryptor dec = infoDec.getDecryptor();
+            assertTrue(dec.verifyPassword("f"));
+            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, had expected \n" +
+                HexDump.dump(testData, 0, 0) + " and actual \n" +
+                HexDump.dump(actualData, 0, 0) + " encrypted \n" +
+                HexDump.dump(encDocument, 0, 0) + " full encrypted \n" +
+                HexDump.dump(encData, 0, 0));
+    }
+}