8168714: Tighten ECDSA validation

Summary: Added additional checks to DER parsing code
Reviewed-by: vinnie, ahgross

src/share/classes/sun/security/ec/ECDSASignature.java

@@ -364,18 +364,22 @@ abstract class ECDSASignature extends SignatureSpi {
     }
 
     // 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());
@@ -389,7 +393,7 @@ abstract class ECDSASignature extends SignatureSpi {
             return result;
 
         } catch (Exception e) {
-            throw new SignatureException("Could not decode signature", e);
+            throw new SignatureException("Invalid encoding for signature", e);
         }
     }
 

src/share/classes/sun/security/pkcs11/P11Signature.java

@@ -705,17 +705,21 @@ final class P11Signature extends SignatureSpi {
         }
     }
 
-    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)) {
@@ -725,21 +729,25 @@ final class P11Signature extends SignatureSpi {
         } 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());
@@ -750,7 +758,7 @@ final class P11Signature extends SignatureSpi {
             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);
         }
     }
 

src/share/classes/sun/security/provider/DSA.java

@@ -267,18 +267,20 @@ abstract class DSA extends SignatureSpi {
         BigInteger s = null;
         // 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);
         }
 
         // some implementations do not correctly encode values in the ASN.1

src/share/classes/sun/security/rsa/RSASignature.java

@@ -223,9 +223,10 @@ public abstract class RSASignature extends SignatureSpi {
      * 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");

src/share/classes/sun/security/util/DerInputBuffer.java

 /*
- * Copyright (c) 1996, 2006, 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 @@ class DerInputBuffer extends ByteArrayInputStream implements Cloneable {
         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 {

src/share/classes/sun/security/util/DerInputStream.java

 /*
- * Copyright (c) 1996, 2006, 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 @@ public class DerInputStream {
      * @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 @@ public class DerInputStream {
      *          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);
-
-            DerIndefLenConverter derIn = new DerIndefLenConverter();
-            buffer = new DerInputBuffer(derIn.convert(inData));
+            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));
+            }
         } else
             buffer = new DerInputBuffer(data, offset, len);
         buffer.mark(Integer.MAX_VALUE);
@@ -233,12 +258,21 @@ public class DerInputStream {
          * First byte = number of excess bits in the last octet of the
          * representation.
          */
-        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");
+        }
 
         byte[] repn = new byte[length];
 
-        if ((length != 0) && (buffer.read(repn) != length))
-            throw new IOException("short read of DER bit string");
+        if ((length != 0) && (buffer.read(repn) != length)) {
+            throw new IOException("Short read of DER bit string");
+        }
+
         return new BitArray(validBits, repn);
     }
 
@@ -252,7 +286,7 @@ public class DerInputStream {
         int length = getLength(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;
     }
@@ -262,7 +296,7 @@ public class DerInputStream {
      */
     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");
         }
     }
 
@@ -346,7 +380,7 @@ public class DerInputStream {
         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
@@ -392,7 +426,7 @@ public class DerInputStream {
         } 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.
@@ -483,7 +517,7 @@ public class DerInputStream {
         int length = getLength(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);
@@ -544,7 +578,11 @@ public class DerInputStream {
      */
     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;
@@ -558,17 +596,23 @@ public class DerInputStream {
             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;

src/share/classes/sun/security/util/DerValue.java

 /*
- * Copyright (c) 1996, 2009, 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
@@ -249,7 +249,7 @@ public class DerValue {
 
         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();
@@ -362,7 +362,7 @@ public class DerValue {
 
         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