8168714: Tighten ECDSA validation
authorvaleriep
Thu, 17 Nov 2016 02:08:53 +0000
changeset 43214 3dd7af9b9e05
parent 43213 dc8daa68dd31
child 43215 f3d46da34ae9
8168714: Tighten ECDSA validation Summary: Added additional checks to DER parsing code Reviewed-by: vinnie, ahgross
jdk/src/java.base/share/classes/sun/security/provider/DSA.java
jdk/src/java.base/share/classes/sun/security/rsa/RSASignature.java
jdk/src/java.base/share/classes/sun/security/util/DerInputBuffer.java
jdk/src/java.base/share/classes/sun/security/util/DerInputStream.java
jdk/src/java.base/share/classes/sun/security/util/DerValue.java
jdk/src/jdk.crypto.ec/share/classes/sun/security/ec/ECDSASignature.java
jdk/src/jdk.crypto.token/share/classes/sun/security/pkcs11/P11Signature.java
--- a/jdk/src/java.base/share/classes/sun/security/provider/DSA.java	Thu Nov 10 15:52:48 2016 +0000
+++ b/jdk/src/java.base/share/classes/sun/security/provider/DSA.java	Thu Nov 17 02:08:53 2016 +0000
@@ -322,19 +322,20 @@
         } else {
             // first decode the signature.
             try {
-                DerInputStream in = new DerInputStream(signature, offset,
-                                                       length);
+                // Enforce strict DER checking for signatures
+                DerInputStream in =
+                    new DerInputStream(signature, offset, length, false);
                 DerValue[] values = in.getSequence(2);
 
+                // check number of components in the read sequence
+                // and trailing data
+                if ((values.length != 2) || (in.available() != 0)) {
+                    throw new IOException("Invalid encoding for signature");
+                }
                 r = values[0].getBigInteger();
                 s = values[1].getBigInteger();
-
-                // Check for trailing signature data
-                if (in.available() != 0) {
-                    throw new IOException("Incorrect signature length");
-                }
             } catch (IOException e) {
-                throw new SignatureException("invalid encoding for signature");
+                throw new SignatureException("Invalid encoding for signature", e);
             }
         }
 
--- a/jdk/src/java.base/share/classes/sun/security/rsa/RSASignature.java	Thu Nov 10 15:52:48 2016 +0000
+++ b/jdk/src/java.base/share/classes/sun/security/rsa/RSASignature.java	Thu Nov 17 02:08:53 2016 +0000
@@ -226,9 +226,10 @@
      * Decode the signature data. Verify that the object identifier matches
      * and return the message digest.
      */
-    public static byte[] decodeSignature(ObjectIdentifier oid, byte[] signature)
+    public static byte[] decodeSignature(ObjectIdentifier oid, byte[] sig)
             throws IOException {
-        DerInputStream in = new DerInputStream(signature);
+        // Enforce strict DER checking for signatures
+        DerInputStream in = new DerInputStream(sig, 0, sig.length, false);
         DerValue[] values = in.getSequence(2);
         if ((values.length != 2) || (in.available() != 0)) {
             throw new IOException("SEQUENCE length error");
--- a/jdk/src/java.base/share/classes/sun/security/util/DerInputBuffer.java	Thu Nov 10 15:52:48 2016 +0000
+++ b/jdk/src/java.base/share/classes/sun/security/util/DerInputBuffer.java	Thu Nov 17 02:08:53 2016 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1996, 2014, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2016, 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
@@ -147,6 +147,11 @@
         System.arraycopy(buf, pos, bytes, 0, len);
         skip(len);
 
+        // check to make sure no extra leading 0s for DER
+        if (len >= 2 && (bytes[0] == 0) && (bytes[1] >= 0)) {
+            throw new IOException("Invalid encoding: redundant leading 0s");
+        }
+
         if (makePositive) {
             return new BigInteger(1, bytes);
         } else {
--- a/jdk/src/java.base/share/classes/sun/security/util/DerInputStream.java	Thu Nov 10 15:52:48 2016 +0000
+++ b/jdk/src/java.base/share/classes/sun/security/util/DerInputStream.java	Thu Nov 17 02:08:53 2016 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1996, 2014, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2016, 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
@@ -77,7 +77,7 @@
      * @param data the buffer from which to create the string (CONSUMED)
      */
     public DerInputStream(byte[] data) throws IOException {
-        init(data, 0, data.length);
+        init(data, 0, data.length, true);
     }
 
     /**
@@ -92,23 +92,48 @@
      *          starting at "offset"
      */
     public DerInputStream(byte[] data, int offset, int len) throws IOException {
-        init(data, offset, len);
+        init(data, offset, len, true);
+    }
+
+    /**
+     * Create a DER input stream from part of a data buffer with
+     * additional arg to indicate whether to allow constructed
+     * indefinite-length encoding.
+     * The buffer is not copied, it is shared.  Accordingly, the
+     * buffer should be treated as read-only.
+     *
+     * @param data the buffer from which to create the string (CONSUMED)
+     * @param offset the first index of <em>data</em> which will
+     *          be read as DER input in the new stream
+     * @param len how long a chunk of the buffer to use,
+     *          starting at "offset"
+     * @param allowIndefiniteLength whether to allow constructed
+     *          indefinite-length encoding
+     */
+    public DerInputStream(byte[] data, int offset, int len,
+        boolean allowIndefiniteLength) throws IOException {
+        init(data, offset, len, allowIndefiniteLength);
     }
 
     /*
      * private helper routine
      */
-    private void init(byte[] data, int offset, int len) throws IOException {
+    private void init(byte[] data, int offset, int len,
+        boolean allowIndefiniteLength) throws IOException {
         if ((offset+2 > data.length) || (offset+len > data.length)) {
             throw new IOException("Encoding bytes too short");
         }
         // check for indefinite length encoding
         if (DerIndefLenConverter.isIndefinite(data[offset+1])) {
-            byte[] inData = new byte[len];
-            System.arraycopy(data, offset, inData, 0, len);
+            if (!allowIndefiniteLength) {
+                throw new IOException("Indefinite length BER encoding found");
+            } else {
+                byte[] inData = new byte[len];
+                System.arraycopy(data, offset, inData, 0, len);
 
-            DerIndefLenConverter derIn = new DerIndefLenConverter();
-            buffer = new DerInputBuffer(derIn.convert(inData));
+                DerIndefLenConverter derIn = new DerIndefLenConverter();
+                buffer = new DerInputBuffer(derIn.convert(inData));
+            }
         } else
             buffer = new DerInputBuffer(data, offset, len);
         buffer.mark(Integer.MAX_VALUE);
@@ -239,15 +264,19 @@
          * representation.
          */
         length--;
-        int validBits = length*8 - buffer.read();
+        int excessBits = buffer.read();
+        if (excessBits < 0) {
+            throw new IOException("Unused bits of bit string invalid");
+        }
+        int validBits = length*8 - excessBits;
         if (validBits < 0) {
-            throw new IOException("valid bits of bit string invalid");
+            throw new IOException("Valid bits of bit string invalid");
         }
 
         byte[] repn = new byte[length];
 
         if ((length != 0) && (buffer.read(repn) != length)) {
-            throw new IOException("short read of DER bit string");
+            throw new IOException("Short read of DER bit string");
         }
 
         return new BitArray(validBits, repn);
@@ -263,7 +292,7 @@
         int length = getDefiniteLength(buffer);
         byte[] retval = new byte[length];
         if ((length != 0) && (buffer.read(retval) != length))
-            throw new IOException("short read of DER octet string");
+            throw new IOException("Short read of DER octet string");
 
         return retval;
     }
@@ -273,7 +302,7 @@
      */
     public void getBytes(byte[] val) throws IOException {
         if ((val.length != 0) && (buffer.read(val) != val.length)) {
-            throw new IOException("short read of DER octet string");
+            throw new IOException("Short read of DER octet string");
         }
     }
 
@@ -357,7 +386,7 @@
         DerInputStream  newstr;
 
         byte lenByte = (byte)buffer.read();
-        int len = getLength((lenByte & 0xff), buffer);
+        int len = getLength(lenByte, buffer);
 
         if (len == -1) {
            // indefinite length encoding found
@@ -403,7 +432,7 @@
         } while (newstr.available() > 0);
 
         if (newstr.available() != 0)
-            throw new IOException("extra data at end of vector");
+            throw new IOException("Extra data at end of vector");
 
         /*
          * Now stick them into the array we're returning.
@@ -494,7 +523,7 @@
         int length = getDefiniteLength(buffer);
         byte[] retval = new byte[length];
         if ((length != 0) && (buffer.read(retval) != length))
-            throw new IOException("short read of DER " +
+            throw new IOException("Short read of DER " +
                                   stringName + " string");
 
         return new String(retval, enc);
@@ -555,7 +584,11 @@
      */
     static int getLength(int lenByte, InputStream in) throws IOException {
         int value, tmp;
+        if (lenByte == -1) {
+            throw new IOException("Short read of DER length");
+        }
 
+        String mdName = "DerInputStream.getLength(): ";
         tmp = lenByte;
         if ((tmp & 0x080) == 0x00) { // short form, 1 byte datum
             value = tmp;
@@ -569,17 +602,23 @@
             if (tmp == 0)
                 return -1;
             if (tmp < 0 || tmp > 4)
-                throw new IOException("DerInputStream.getLength(): lengthTag="
-                    + tmp + ", "
+                throw new IOException(mdName + "lengthTag=" + tmp + ", "
                     + ((tmp < 0) ? "incorrect DER encoding." : "too big."));
 
-            for (value = 0; tmp > 0; tmp --) {
+            value = 0x0ff & in.read();
+            tmp--;
+            if (value == 0) {
+                // DER requires length value be encoded in minimum number of bytes
+                throw new IOException(mdName + "Redundant length bytes found");
+            }
+            while (tmp-- > 0) {
                 value <<= 8;
                 value += 0x0ff & in.read();
             }
             if (value < 0) {
-                throw new IOException("DerInputStream.getLength(): "
-                        + "Invalid length bytes");
+                throw new IOException(mdName + "Invalid length bytes");
+            } else if (value <= 127) {
+                throw new IOException(mdName + "Should use short form for length");
             }
         }
         return value;
--- a/jdk/src/java.base/share/classes/sun/security/util/DerValue.java	Thu Nov 10 15:52:48 2016 +0000
+++ b/jdk/src/java.base/share/classes/sun/security/util/DerValue.java	Thu Nov 17 02:08:53 2016 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1996, 2014, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2016, 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
@@ -248,7 +248,7 @@
 
         tag = (byte)in.read();
         byte lenByte = (byte)in.read();
-        length = DerInputStream.getLength((lenByte & 0xff), in);
+        length = DerInputStream.getLength(lenByte, in);
         if (length == -1) {  // indefinite length encoding found
             DerInputBuffer inbuf = in.dup();
             int readLen = inbuf.available();
@@ -361,7 +361,7 @@
 
         tag = (byte)in.read();
         byte lenByte = (byte)in.read();
-        length = DerInputStream.getLength((lenByte & 0xff), in);
+        length = DerInputStream.getLength(lenByte, in);
         if (length == -1) { // indefinite length encoding found
             int readLen = in.available();
             int offset = 2;     // for tag and length bytes
--- a/jdk/src/jdk.crypto.ec/share/classes/sun/security/ec/ECDSASignature.java	Thu Nov 10 15:52:48 2016 +0000
+++ b/jdk/src/jdk.crypto.ec/share/classes/sun/security/ec/ECDSASignature.java	Thu Nov 17 02:08:53 2016 +0000
@@ -455,18 +455,22 @@
     }
 
     // Convert the DER encoding of R and S into a concatenation of R and S
-    private byte[] decodeSignature(byte[] signature) throws SignatureException {
+    private byte[] decodeSignature(byte[] sig) throws SignatureException {
 
         try {
-            DerInputStream in = new DerInputStream(signature);
+            // Enforce strict DER checking for signatures
+            DerInputStream in = new DerInputStream(sig, 0, sig.length, false);
             DerValue[] values = in.getSequence(2);
+
+            // check number of components in the read sequence
+            // and trailing data
+            if ((values.length != 2) || (in.available() != 0)) {
+                throw new IOException("Invalid encoding for signature");
+            }
+
             BigInteger r = values[0].getPositiveBigInteger();
             BigInteger s = values[1].getPositiveBigInteger();
 
-            // Check for trailing signature data
-            if (in.available() != 0) {
-                throw new IOException("Incorrect signature length");
-            }
             // trim leading zeroes
             byte[] rBytes = trimZeroes(r.toByteArray());
             byte[] sBytes = trimZeroes(s.toByteArray());
@@ -480,7 +484,7 @@
             return result;
 
         } catch (Exception e) {
-            throw new SignatureException("Could not decode signature", e);
+            throw new SignatureException("Invalid encoding for signature", e);
         }
     }
 
--- a/jdk/src/jdk.crypto.token/share/classes/sun/security/pkcs11/P11Signature.java	Thu Nov 10 15:52:48 2016 +0000
+++ b/jdk/src/jdk.crypto.token/share/classes/sun/security/pkcs11/P11Signature.java	Thu Nov 17 02:08:53 2016 +0000
@@ -740,17 +740,21 @@
         }
     }
 
-    private static byte[] asn1ToDSA(byte[] signature) throws SignatureException {
+    private static byte[] asn1ToDSA(byte[] sig) throws SignatureException {
         try {
-            DerInputStream in = new DerInputStream(signature);
+            // Enforce strict DER checking for signatures
+            DerInputStream in = new DerInputStream(sig, 0, sig.length, false);
             DerValue[] values = in.getSequence(2);
+
+            // check number of components in the read sequence
+            // and trailing data
+            if ((values.length != 2) || (in.available() != 0)) {
+                throw new IOException("Invalid encoding for signature");
+            }
+
             BigInteger r = values[0].getPositiveBigInteger();
             BigInteger s = values[1].getPositiveBigInteger();
 
-            // Check for trailing signature data
-            if (in.available() != 0) {
-                throw new IOException("Incorrect signature length");
-            }
             byte[] br = toByteArray(r, 20);
             byte[] bs = toByteArray(s, 20);
             if ((br == null) || (bs == null)) {
@@ -760,21 +764,25 @@
         } catch (SignatureException e) {
             throw e;
         } catch (Exception e) {
-            throw new SignatureException("invalid encoding for signature", e);
+            throw new SignatureException("Invalid encoding for signature", e);
         }
     }
 
-    private byte[] asn1ToECDSA(byte[] signature) throws SignatureException {
+    private byte[] asn1ToECDSA(byte[] sig) throws SignatureException {
         try {
-            DerInputStream in = new DerInputStream(signature);
+            // Enforce strict DER checking for signatures
+            DerInputStream in = new DerInputStream(sig, 0, sig.length, false);
             DerValue[] values = in.getSequence(2);
+
+            // check number of components in the read sequence
+            // and trailing data
+            if ((values.length != 2) || (in.available() != 0)) {
+                throw new IOException("Invalid encoding for signature");
+            }
+
             BigInteger r = values[0].getPositiveBigInteger();
             BigInteger s = values[1].getPositiveBigInteger();
 
-            // Check for trailing signature data
-            if (in.available() != 0) {
-                throw new IOException("Incorrect signature length");
-            }
             // trim leading zeroes
             byte[] br = KeyUtil.trimZeroes(r.toByteArray());
             byte[] bs = KeyUtil.trimZeroes(s.toByteArray());
@@ -785,7 +793,7 @@
             System.arraycopy(bs, 0, res, res.length - bs.length, bs.length);
             return res;
         } catch (Exception e) {
-            throw new SignatureException("invalid encoding for signature", e);
+            throw new SignatureException("Invalid encoding for signature", e);
         }
     }