8224997: ChaCha20-Poly1305 TLS cipher suite decryption throws ShortBufferException

Reviewed-by: xuelei

src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Cipher.java

@@ -33,12 +33,11 @@ import java.nio.ByteBuffer;
 import java.nio.ByteOrder;
 import java.security.*;
 import java.security.spec.AlgorithmParameterSpec;
-import java.util.Arrays;
 import java.util.Objects;
+import javax.crypto.*;
 import javax.crypto.spec.ChaCha20ParameterSpec;
 import javax.crypto.spec.IvParameterSpec;
 import javax.crypto.spec.SecretKeySpec;
-import javax.crypto.*;
 import sun.security.util.DerValue;
 
 /**
@@ -134,8 +133,7 @@ abstract class ChaCha20Cipher extends CipherSpi {
     /**
      * Default constructor.
      */
-    protected ChaCha20Cipher() {
+    protected ChaCha20Cipher() { }
-    }
 
     /**
      * Set the mode of operation.  Since this is a stream cipher, there
@@ -185,11 +183,13 @@ abstract class ChaCha20Cipher extends CipherSpi {
     }
 
     /**
-     * Get the output size based on an input length.  In simple stream-cipher
+     * Get the output size required to hold the result of the next update or
+     * doFinal operation.  In simple stream-cipher
      * mode, the output size will equal the input size.  For ChaCha20-Poly1305
      * for encryption the output size will be the sum of the input length
-     * and tag length.  For decryption, the output size will be the input
+     * and tag length.  For decryption, the output size will be  the input
-     * length less the tag length or zero, whichever is larger.
+     * length plus any previously unprocessed data minus the tag
+     * length, minimum zero.
      *
      * @param inputLen the length in bytes of the input
      *
@@ -197,17 +197,7 @@ abstract class ChaCha20Cipher extends CipherSpi {
      */
     @Override
     protected int engineGetOutputSize(int inputLen) {
-        int outLen = 0;
+        return engine.getOutputSize(inputLen, true);
-
-        if (mode == MODE_NONE) {
-            outLen = inputLen;
-        } else if (mode == MODE_AEAD) {
-            outLen = (direction == Cipher.ENCRYPT_MODE) ?
-                    Math.addExact(inputLen, TAG_LENGTH) :
-                    Integer.max(inputLen - TAG_LENGTH, 0);
-        }
-
-        return outLen;
     }
 
     /**
@@ -237,13 +227,10 @@ abstract class ChaCha20Cipher extends CipherSpi {
         AlgorithmParameters params = null;
         if (mode == MODE_AEAD) {
             try {
-                // Force the 12-byte nonce into a DER-encoded OCTET_STRING
+                // Place the 12-byte nonce into a DER-encoded OCTET_STRING
-                byte[] derNonce = new byte[nonce.length + 2];
-                derNonce[0] = 0x04;                 // OCTET_STRING tag
-                derNonce[1] = (byte)nonce.length;   // 12-byte length;
-                System.arraycopy(nonce, 0, derNonce, 2, nonce.length);
                 params = AlgorithmParameters.getInstance("ChaCha20-Poly1305");
-                params.init(derNonce);
+                params.init((new DerValue(
+                        DerValue.tag_OctetString, nonce).toByteArray()));
             } catch (NoSuchAlgorithmException | IOException exc) {
                 throw new RuntimeException(exc);
             }
@@ -638,7 +625,7 @@ abstract class ChaCha20Cipher extends CipherSpi {
      */
     @Override
     protected byte[] engineUpdate(byte[] in, int inOfs, int inLen) {
-        byte[] out = new byte[inLen];
+        byte[] out = new byte[engine.getOutputSize(inLen, false)];
         try {
             engine.doUpdate(in, inOfs, inLen, out, 0);
         } catch (ShortBufferException | KeyException exc) {
@@ -696,7 +683,7 @@ abstract class ChaCha20Cipher extends CipherSpi {
     @Override
     protected byte[] engineDoFinal(byte[] in, int inOfs, int inLen)
             throws AEADBadTagException {
-        byte[] output = new byte[engineGetOutputSize(inLen)];
+        byte[] output = new byte[engine.getOutputSize(inLen, true)];
         try {
             engine.doFinal(in, inOfs, inLen, output, 0);
         } catch (ShortBufferException | KeyException exc) {
@@ -1157,6 +1144,17 @@ abstract class ChaCha20Cipher extends CipherSpi {
      * Interface for the underlying processing engines for ChaCha20
      */
     interface ChaChaEngine {
+        /**
+         * Size an output buffer based on the input and where applicable
+         * the current state of the engine in a multipart operation.
+         *
+         * @param inLength the input length.
+         * @param isFinal true if this is invoked from a doFinal call.
+         *
+         * @return the recommended size for the output buffer.
+         */
+        int getOutputSize(int inLength, boolean isFinal);
+
         /**
          * Perform a multi-part update for ChaCha20.
          *
@@ -1200,6 +1198,12 @@ abstract class ChaCha20Cipher extends CipherSpi {
 
         private EngineStreamOnly () { }
 
+        @Override
+        public int getOutputSize(int inLength, boolean isFinal) {
+            // The isFinal parameter is not relevant in this kind of engine
+            return inLength;
+        }
+
         @Override
         public int doUpdate(byte[] in, int inOff, int inLen, byte[] out,
                 int outOff) throws ShortBufferException, KeyException {
@@ -1234,6 +1238,11 @@ abstract class ChaCha20Cipher extends CipherSpi {
 
     private final class EngineAEADEnc implements ChaChaEngine {
 
+        @Override
+        public int getOutputSize(int inLength, boolean isFinal) {
+            return (isFinal ? Math.addExact(inLength, TAG_LENGTH) : inLength);
+        }
+
         private EngineAEADEnc() throws InvalidKeyException {
             initAuthenticator();
             counter = 1;
@@ -1294,6 +1303,18 @@ abstract class ChaCha20Cipher extends CipherSpi {
         private final ByteArrayOutputStream cipherBuf;
         private final byte[] tag;
 
+        @Override
+        public int getOutputSize(int inLen, boolean isFinal) {
+            // If we are performing a decrypt-update we should always return
+            // zero length since we cannot return any data until the tag has
+            // been consumed and verified.  CipherSpi.engineGetOutputSize will
+            // always set isFinal to true to get the required output buffer
+            // size.
+            return (isFinal ?
+                    Integer.max(Math.addExact((inLen - TAG_LENGTH),
+                            cipherBuf.size()), 0) : 0);
+        }
+
         private EngineAEADDec() throws InvalidKeyException {
             initAuthenticator();
             counter = 1;

test/jdk/com/sun/crypto/provider/Cipher/ChaCha20/OutputSizeTest.java

@@ -0,0 +1,181 @@
+/*
+ * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/**
+ * @test
+ * @bug 8224997
+ * @summary ChaCha20-Poly1305 TLS cipher suite decryption throws ShortBufferException
+ * @library /test/lib
+ * @build jdk.test.lib.Convert
+ * @run main OutputSizeTest
+ */
+
+import java.nio.ByteBuffer;
+import java.security.GeneralSecurityException;
+import java.security.Key;
+import java.security.SecureRandom;
+import javax.crypto.Cipher;
+import javax.crypto.KeyGenerator;
+import javax.crypto.spec.ChaCha20ParameterSpec;
+import javax.crypto.spec.IvParameterSpec;
+
+public class OutputSizeTest {
+
+    private static final SecureRandom SR = new SecureRandom();
+
+    public static void main(String args[]) throws Exception {
+        testCC20GetOutSize();
+        testCC20P1305GetOutSize();
+        testMultiPartAEADDec();
+    }
+
+    private static void testCC20GetOutSize()
+            throws GeneralSecurityException {
+        boolean result = true;
+        KeyGenerator kg = KeyGenerator.getInstance("ChaCha20", "SunJCE");
+        kg.init(256);
+
+        // ChaCha20 encrypt
+        Cipher cc20 = Cipher.getInstance("ChaCha20", "SunJCE");
+        cc20.init(Cipher.ENCRYPT_MODE, kg.generateKey(),
+                new ChaCha20ParameterSpec(getRandBuf(12), 10));
+
+        testOutLen(cc20, 0, 0);
+        testOutLen(cc20, 5, 5);
+        testOutLen(cc20, 5120, 5120);
+        // perform an update, then test with a final block
+        byte[] input = new byte[5120];
+        SR.nextBytes(input);
+        cc20.update(input);
+        testOutLen(cc20, 1024, 1024);
+
+        // Decryption lengths should be calculated the same way as encryption
+        cc20.init(Cipher.DECRYPT_MODE, kg.generateKey(),
+                new ChaCha20ParameterSpec(getRandBuf(12), 10));
+        testOutLen(cc20, 0, 0);
+        testOutLen(cc20, 5, 5);
+        testOutLen(cc20, 5120, 5120);
+        // perform an update, then test with a final block
+        cc20.update(input);
+        testOutLen(cc20, 1024, 1024);
+    }
+
+    private static void testCC20P1305GetOutSize()
+            throws GeneralSecurityException {
+        KeyGenerator kg = KeyGenerator.getInstance("ChaCha20", "SunJCE");
+        kg.init(256);
+
+        // ChaCha20 encrypt
+        Cipher cc20 = Cipher.getInstance("ChaCha20-Poly1305", "SunJCE");
+        cc20.init(Cipher.ENCRYPT_MODE, kg.generateKey(),
+                new IvParameterSpec(getRandBuf(12)));
+
+        // Encryption lengths are calculated as the input length plus the tag
+        // length (16).
+        testOutLen(cc20, 0, 16);
+        testOutLen(cc20, 5, 21);
+        testOutLen(cc20, 5120, 5136);
+        // perform an update, then test with a final block
+        byte[] input = new byte[5120];
+        SR.nextBytes(input);
+        cc20.update(input);
+        testOutLen(cc20, 1024, 1040);
+
+        // Decryption lengths are handled differently for AEAD mode.  The length
+        // should be zero for anything up to and including the first 16 bytes
+        // (since that's the tag).  Anything above that should be the input
+        // length plus any unprocessed input (via update calls), minus the
+        // 16 byte tag.
+        cc20.init(Cipher.DECRYPT_MODE, kg.generateKey(),
+                new IvParameterSpec(getRandBuf(12)));
+        testOutLen(cc20, 0, 0);
+        testOutLen(cc20, 5, 0);
+        testOutLen(cc20, 16, 0);
+        testOutLen(cc20, 5120, 5104);
+        // Perform an update, then test with a the length of a final chunk
+        // of data.
+        cc20.update(input);
+        testOutLen(cc20, 1024, 6128);
+    }
+
+    private static void testMultiPartAEADDec() throws GeneralSecurityException {
+        KeyGenerator kg = KeyGenerator.getInstance("ChaCha20", "SunJCE");
+        kg.init(256);
+        Key key = kg.generateKey();
+        IvParameterSpec ivps = new IvParameterSpec(getRandBuf(12));
+
+        // Encrypt some data so we can test decryption.
+        byte[] pText = getRandBuf(2048);
+        ByteBuffer pTextBase = ByteBuffer.wrap(pText);
+
+        Cipher enc = Cipher.getInstance("ChaCha20-Poly1305", "SunJCE");
+        enc.init(Cipher.ENCRYPT_MODE, key, ivps);
+        ByteBuffer ctBuf = ByteBuffer.allocateDirect(
+                enc.getOutputSize(pText.length));
+        enc.doFinal(pTextBase, ctBuf);
+
+        // Create a new direct plain text ByteBuffer which will catch the
+        // decrypted data.
+        ByteBuffer ptBuf = ByteBuffer.allocateDirect(pText.length);
+
+        // Set the cipher text buffer limit to roughly half the data so we can
+        // do an update/final sequence.
+        ctBuf.position(0).limit(1024);
+
+        Cipher dec = Cipher.getInstance("ChaCha20-Poly1305", "SunJCE");
+        dec.init(Cipher.DECRYPT_MODE, key, ivps);
+        dec.update(ctBuf, ptBuf);
+        System.out.println("CTBuf: " + ctBuf);
+        System.out.println("PTBuf: " + ptBuf);
+        ctBuf.limit(ctBuf.capacity());
+        dec.doFinal(ctBuf, ptBuf);
+
+        ptBuf.flip();
+        pTextBase.flip();
+        System.out.println("PT Base:" + pTextBase);
+        System.out.println("PT Actual:" + ptBuf);
+
+        if (pTextBase.compareTo(ptBuf) != 0) {
+            StringBuilder sb = new StringBuilder();
+            sb.append("Plaintext mismatch: Original: ").
+                    append(pTextBase.toString()).append("\nActual :").
+                    append(ptBuf);
+            throw new RuntimeException(sb.toString());
+        }
+    }
+
+    private static void testOutLen(Cipher c, int inLen, int expOut) {
+        int actualOut = c.getOutputSize(inLen);
+        if (actualOut != expOut) {
+            throw new RuntimeException("Cipher " + c + ", in: " + inLen +
+                    ", expOut: " + expOut + ", actual: " + actualOut);
+        }
+    }
+
+    private static byte[] getRandBuf(int len) {
+        byte[] buf = new byte[len];
+        SR.nextBytes(buf);
+        return buf;
+    }
+}
+