fix: validate entire ciphertext has been processed before returning (#191)

WesleyRosenblum · web-flow · commit a6d6309e6592 · 2020-05-26T13:24:52.000-07:00
* fix: validate entire ciphertext has been processed before returning

* Updating changelog
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,9 +1,10 @@
 # Changelog
 
-## 1.6.2 -- unreleased
+## 1.6.2 -- 2020-05-26
 
 ### Patches
 * Validate final frame length does not exceed the frame size in the message header [PR #166](https://github.com/aws/aws-encryption-sdk-java/pull/166)
+* Validate entire ciphertext has been processed before returning [PR #191](https://github.com/aws/aws-encryption-sdk-java/pull/191)
 
 ### Maintenance
 * Update AWS Java SDK version from 1.11.561 to 1.11.704. [PR #186](https://github.com/aws/aws-encryption-sdk-java/pull/186)
diff --git a/src/main/java/com/amazonaws/encryptionsdk/internal/BlockDecryptionHandler.java b/src/main/java/com/amazonaws/encryptionsdk/internal/BlockDecryptionHandler.java
@@ -13,16 +13,15 @@
 
 package com.amazonaws.encryptionsdk.internal;
 
-import java.util.Arrays;
-
-import javax.crypto.Cipher;
-import javax.crypto.SecretKey;
-
 import com.amazonaws.encryptionsdk.CryptoAlgorithm;
 import com.amazonaws.encryptionsdk.exception.AwsCryptoException;
 import com.amazonaws.encryptionsdk.exception.BadCiphertextException;
 import com.amazonaws.encryptionsdk.model.CipherBlockHeaders;
 
+import javax.crypto.Cipher;
+import javax.crypto.SecretKey;
+import java.util.Arrays;
+
 /**
  * The block decryption handler is an implementation of CryptoHandler that
  * provides methods to decrypt content encrypted and stored in a single block.
@@ -97,6 +96,11 @@ public BlockDecryptionHandler(final SecretKey decryptionKey, final short nonceLe
     synchronized public ProcessingSummary processBytes(final byte[] in, final int off, final int len,
             final byte[] out,
             final int outOff) throws AwsCryptoException {
+
+        if (complete_) {
+            throw new AwsCryptoException("Ciphertext has already been processed.");
+        }
+
         final byte[] bytesToParse = new byte[unparsedBytes_.length + len];
         // If there were previously unparsed bytes, add them as the first
         // set of bytes to be parsed in this call.
@@ -166,6 +170,9 @@ synchronized public ProcessingSummary processBytes(final byte[] in, final int of
      */
     @Override
     synchronized public int doFinal(final byte[] out, final int outOff) throws BadCiphertextException {
+        if (!complete_) {
+            throw new BadCiphertextException("Unable to process entire ciphertext.");
+        }
         return 0;
     }
 
diff --git a/src/main/java/com/amazonaws/encryptionsdk/internal/DecryptionHandler.java b/src/main/java/com/amazonaws/encryptionsdk/internal/DecryptionHandler.java
@@ -326,6 +326,10 @@ public int doFinal(final byte[] out, final int outOff) throws BadCiphertextExcep
         } else {
             int result = contentCryptoHandler_.doFinal(out, outOff);
 
+            if (!ciphertextHeaders_.isComplete() || !contentCryptoHandler_.isComplete()) {
+                throw new BadCiphertextException("Unable to process entire ciphertext.");
+            }
+
             return result;
         }
     }
diff --git a/src/main/java/com/amazonaws/encryptionsdk/internal/FrameDecryptionHandler.java b/src/main/java/com/amazonaws/encryptionsdk/internal/FrameDecryptionHandler.java
@@ -78,16 +78,16 @@ public FrameDecryptionHandler(final SecretKey decryptionKey, final short nonceLe
      * 
      * @param in
      *            the input byte array.
-     * @param inOff
+     * @param off
      *            the offset into the in array where the data to be decrypted starts.
-     * @param inLen
+     * @param len
      *            the number of bytes to be decrypted.
      * @param out
      *            the output buffer the decrypted plaintext bytes go into.
      * @param outOff
      *            the offset into the output byte array the decrypted data starts at.
      * @return the number of bytes written to out and processed
-     * @throws InvalidCiphertextException
+     * @throws BadCiphertextException
      *             if frame number is invalid/out-of-order or if the bytes do not decrypt correctly.
      * @throws AwsCryptoException
      *             if the content type found in the headers is not of frame type.
@@ -96,6 +96,11 @@ public FrameDecryptionHandler(final SecretKey decryptionKey, final short nonceLe
     public ProcessingSummary processBytes(final byte[] in, final int off, final int len, final byte[] out,
             final int outOff)
             throws BadCiphertextException, AwsCryptoException {
+
+        if (complete_) {
+            throw new AwsCryptoException("Ciphertext has already been processed.");
+        }
+
         final long totalBytesToParse = unparsedBytes_.length + (long) len;
         if (totalBytesToParse > Integer.MAX_VALUE) {
             throw new AwsCryptoException(
@@ -200,6 +205,10 @@ public ProcessingSummary processBytes(final byte[] in, final int off, final int
      */
     @Override
     public int doFinal(final byte[] out, final int outOff) {
+        if (!complete_) {
+            throw new BadCiphertextException("Unable to process entire ciphertext.");
+        }
+
         return 0;
     }
 
diff --git a/src/test/java/com/amazonaws/encryptionsdk/AwsCryptoTest.java b/src/test/java/com/amazonaws/encryptionsdk/AwsCryptoTest.java
@@ -32,6 +32,7 @@
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.nio.charset.StandardCharsets;
+import java.util.Arrays;
 import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.Map;
@@ -112,6 +113,31 @@ private void doTamperedEncryptDecrypt(final CryptoAlgorithm cryptoAlg, final int
         }
     }
 
+    private void doTruncatedEncryptDecrypt(final CryptoAlgorithm cryptoAlg, final int byteSize, final int frameSize) {
+        final byte[] plaintextBytes = new byte[byteSize];
+
+        final Map<String, String> encryptionContext = new HashMap<>(1);
+        encryptionContext.put("ENC1", "Encrypt-decrypt test with %d" + byteSize);
+
+        encryptionClient_.setEncryptionAlgorithm(cryptoAlg);
+        encryptionClient_.setEncryptionFrameSize(frameSize);
+
+        final byte[] cipherText = encryptionClient_.encryptData(
+                masterKeyProvider,
+                plaintextBytes,
+                encryptionContext).getResult();
+        final byte[] truncatedCipherText = Arrays.copyOf(cipherText, cipherText.length - 1);
+        try {
+            encryptionClient_.decryptData(
+                    masterKeyProvider,
+                    truncatedCipherText
+            ).getResult();
+            Assert.fail("Expected BadCiphertextException");
+        } catch (final BadCiphertextException ex) {
+            // Expected exception
+        }
+    }
+
     private void doEncryptDecryptWithParsedCiphertext(final int byteSize, final int frameSize) {
         final byte[] plaintextBytes = new byte[byteSize];
 
@@ -195,6 +221,31 @@ public void encryptDecryptWithBadSignature() {
         }
     }
 
+    @Test
+    public void encryptDecryptWithTruncatedCiphertext() {
+        for (final CryptoAlgorithm cryptoAlg : EnumSet.allOf(CryptoAlgorithm.class)) {
+            final int[] frameSizeToTest = TestUtils.getFrameSizesToTest(cryptoAlg);
+
+            for (int i = 0; i < frameSizeToTest.length; i++) {
+                final int frameSize = frameSizeToTest[i];
+                int[] bytesToTest = { 0, 1, frameSize - 1, frameSize, frameSize + 1, (int) (frameSize * 1.5),
+                        frameSize * 2, 1000000 };
+
+                for (int j = 0; j < bytesToTest.length; j++) {
+                    final int byteSize = bytesToTest[j];
+
+                    if (byteSize > 500_000) {
+                        continue;
+                    }
+
+                    if (byteSize >= 0) {
+                        doTruncatedEncryptDecrypt(cryptoAlg, byteSize, frameSize);
+                    }
+                }
+            }
+        }
+    }
+
     @Test
     public void encryptDecryptWithParsedCiphertext() {
         for (final CryptoAlgorithm cryptoAlg : EnumSet.allOf(CryptoAlgorithm.class)) {
diff --git a/src/test/java/com/amazonaws/encryptionsdk/internal/BlockDecryptionHandlerTest.java b/src/test/java/com/amazonaws/encryptionsdk/internal/BlockDecryptionHandlerTest.java
@@ -13,7 +13,6 @@
 
 package com.amazonaws.encryptionsdk.internal;
 
-import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 
 import java.nio.ByteBuffer;
@@ -22,6 +21,7 @@
 import javax.crypto.SecretKey;
 import javax.crypto.spec.SecretKeySpec;
 
+import com.amazonaws.encryptionsdk.exception.BadCiphertextException;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -58,11 +58,9 @@ public void estimateOutputSize() {
         assertTrue(outSize >= inLen);
     }
 
-    @Test
-    public void decryptWithoutHeaders() {
-        final byte[] out = new byte[1];
-        final int returnedLen = blockDecryptionHandler_.doFinal(out, 0);
-        assertEquals(0, returnedLen);
+    @Test(expected= BadCiphertextException.class)
+    public void doFinalCalledWhileNotComplete() {
+        blockDecryptionHandler_.doFinal(new byte[1], 0);
     }
 
     @Test(expected = AwsCryptoException.class)
@@ -90,4 +88,23 @@ public void decryptMaxContentLength() {
         final byte[] decryptedOut = new byte[decryptedOutLen];
         blockDecryptionHandler_.processBytes(outBuff.array(), 0, outBuff.array().length, decryptedOut, 0);
     }
-}
+
+    @Test(expected = AwsCryptoException.class)
+    public void processBytesCalledWhileComplete() {
+        final BlockEncryptionHandler blockEncryptionHandler = new BlockEncryptionHandler(
+                dataKey_,
+                nonceLen_,
+                cryptoAlgorithm_,
+                messageId_);
+        final byte[] in = new byte[0];
+        final int outLen = blockEncryptionHandler.estimateOutputSize(in.length);
+        final byte[] out = new byte[outLen];
+
+        blockEncryptionHandler.processBytes(in, 0, in.length, out, 0);
+        blockEncryptionHandler.doFinal(out, 0);
+
+        final byte[] decryptedOut = new byte[outLen];
+        blockDecryptionHandler_.processBytes(out, 0, outLen, decryptedOut, 0);
+        blockDecryptionHandler_.processBytes(out, 0, outLen, decryptedOut, 0);
+    }
+}
diff --git a/src/test/java/com/amazonaws/encryptionsdk/internal/DecryptionHandlerTest.java b/src/test/java/com/amazonaws/encryptionsdk/internal/DecryptionHandlerTest.java
@@ -16,6 +16,7 @@
 import java.util.Collections;
 import java.util.Map;
 
+import com.amazonaws.encryptionsdk.model.CiphertextHeaders;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -140,4 +141,18 @@ public void invalidOffsetProcessBytes() {
         final byte[] out = new byte[1];
         decryptionHandler.processBytes(in, -1, in.length, out, 0);
     }
-}
+
+    @Test(expected = BadCiphertextException.class)
+    public void incompleteCiphertext() {
+        byte[] ciphertext = getTestHeaders();
+
+        CiphertextHeaders h = new CiphertextHeaders();
+        h.deserialize(ciphertext, 0);
+
+        final DecryptionHandler<StaticMasterKey> decryptionHandler = DecryptionHandler.create(masterKeyProvider_);
+        final byte[] out = new byte[1];
+
+        decryptionHandler.processBytes(ciphertext, 0, ciphertext.length - 1, out, 0);
+        decryptionHandler.doFinal(out, 0);
+    }
+}
diff --git a/src/test/java/com/amazonaws/encryptionsdk/internal/FrameDecryptionHandlerTest.java b/src/test/java/com/amazonaws/encryptionsdk/internal/FrameDecryptionHandlerTest.java
@@ -89,4 +89,30 @@ public void finalFrameLengthTooLarge() {
 
         frameDecryptionHandler_.processBytes(in, 0, in.length, out, 0);
     }
+
+    @Test(expected = BadCiphertextException.class)
+    public void doFinalCalledWhileNotComplete() {
+        frameDecryptionHandler_.doFinal(new byte[1], 0);
+    }
+
+    @Test(expected = AwsCryptoException.class)
+    public void processBytesCalledWhileComplete() {
+        final FrameEncryptionHandler frameEncryptionHandler = new FrameEncryptionHandler(
+                dataKey_,
+                nonceLen_,
+                cryptoAlgorithm_,
+                messageId_,
+                frameSize_);
+        final byte[] in = new byte[0];
+        final int outLen = frameEncryptionHandler.estimateOutputSize(in.length);
+        final byte[] out = new byte[outLen];
+
+        frameEncryptionHandler.processBytes(in, 0, in.length, out, 0);
+        frameEncryptionHandler.doFinal(out, 0);
+
+        final byte[] decryptedOut = new byte[outLen];
+
+        frameDecryptionHandler_.processBytes(out, 0, out.length, decryptedOut, 0);
+        frameDecryptionHandler_.processBytes(out, 0, out.length, decryptedOut, 0);
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -326,6 +326,10 @@ public int doFinal(final byte[] out, final int outOff) throws BadCiphertextExcep`
`326`	`326`	`} else {`
`327`	`327`	`int result = contentCryptoHandler_.doFinal(out, outOff);`
`328`	`328`
	`329`	`+ if (!ciphertextHeaders_.isComplete() \|\| !contentCryptoHandler_.isComplete()) {`
	`330`	`+ throw new BadCiphertextException("Unable to process entire ciphertext.");`
	`331`	`+ }`
	`332`	`+`
`329`	`333`	`return result;`
`330`	`334`	`}`
`331`	`335`	`}`