8175251: Failed to load RSA private key from pkcs12

Summary: Enhanced DER library with extra arg to control leading-0 check
Reviewed-by: mullan

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

 /*
- * Copyright (c) 2003, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2017, 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
@@ -191,14 +191,22 @@ public final class RSAPrivateCrtKeyImpl
             if (version != 0) {
                 throw new IOException("Version must be 0");
             }
-            n = getBigInteger(data);
-            e = getBigInteger(data);
-            d = getBigInteger(data);
-            p = getBigInteger(data);
-            q = getBigInteger(data);
-            pe = getBigInteger(data);
-            qe = getBigInteger(data);
-            coeff = getBigInteger(data);
+
+            /*
+             * Some implementations do not correctly encode ASN.1 INTEGER values
+             * in 2's complement format, resulting in a negative integer when
+             * decoded. Correct the error by converting it to a positive integer.
+             *
+             * See CR 6255949
+             */
+            n = data.getPositiveBigInteger();
+            e = data.getPositiveBigInteger();
+            d = data.getPositiveBigInteger();
+            p = data.getPositiveBigInteger();
+            q = data.getPositiveBigInteger();
+            pe = data.getPositiveBigInteger();
+            qe = data.getPositiveBigInteger();
+            coeff = data.getPositiveBigInteger();
             if (derValue.data.available() != 0) {
                 throw new IOException("Extra data available");
             }
@@ -206,23 +214,4 @@ public final class RSAPrivateCrtKeyImpl
             throw new InvalidKeyException("Invalid RSA private key", e);
         }
     }
-
-    /**
-     * Read a BigInteger from the DerInputStream.
-     */
-    static BigInteger getBigInteger(DerInputStream data) throws IOException {
-        BigInteger b = data.getBigInteger();
-
-        /*
-         * Some implementations do not correctly encode ASN.1 INTEGER values
-         * in 2's complement format, resulting in a negative integer when
-         * decoded. Correct the error by converting it to a positive integer.
-         *
-         * See CR 6255949
-         */
-        if (b.signum() < 0) {
-            b = new BigInteger(1, b.toByteArray());
-        }
-        return b;
-    }
 }

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

 /*
- * Copyright (c) 2003, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2017, 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
@@ -111,8 +111,8 @@ public final class RSAPublicKeyImpl extends X509Key implements RSAPublicKey {
                 throw new IOException("Not a SEQUENCE");
             }
             DerInputStream data = derValue.data;
-            n = RSAPrivateCrtKeyImpl.getBigInteger(data);
-            e = RSAPrivateCrtKeyImpl.getBigInteger(data);
+            n = data.getPositiveBigInteger();
+            e = data.getPositiveBigInteger();
             if (derValue.data.available() != 0) {
                 throw new IOException("Extra data available");
             }

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

 /*
- * Copyright (c) 1996, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2017, 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
@@ -44,16 +44,26 @@ import sun.util.calendar.CalendarSystem;
  */
 class DerInputBuffer extends ByteArrayInputStream implements Cloneable {
 
-    DerInputBuffer(byte[] buf) { super(buf); }
+    boolean allowBER = true;
 
-    DerInputBuffer(byte[] buf, int offset, int len) {
+    // used by sun/security/util/DerInputBuffer/DerInputBufferEqualsHashCode.java
+    DerInputBuffer(byte[] buf) {
+        this(buf, true);
+    }
+
+    DerInputBuffer(byte[] buf, boolean allowBER) {
+        super(buf);
+        this.allowBER = allowBER;
+    }
+
+    DerInputBuffer(byte[] buf, int offset, int len, boolean allowBER) {
         super(buf, offset, len);
+        this.allowBER = allowBER;
     }
 
     DerInputBuffer dup() {
         try {
             DerInputBuffer retval = (DerInputBuffer)clone();
-
             retval.mark(Integer.MAX_VALUE);
             return retval;
         } catch (CloneNotSupportedException e) {
@@ -147,8 +157,8 @@ 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)) {
+        // BER allows leading 0s but DER does not
+        if (!allowBER && (len >= 2 && (bytes[0] == 0) && (bytes[1] >= 0))) {
             throw new IOException("Invalid encoding: redundant leading 0s");
         }
 

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

 /*
- * Copyright (c) 1996, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2017, 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
@@ -81,7 +81,8 @@ public class DerInputStream {
     }
 
     /**
-     * Create a DER input stream from part of a data buffer.
+     * Create a DER input stream from part of a data buffer with
+     * additional arg to control whether DER checks are enforced.
      * The buffer is not copied, it is shared.  Accordingly, the
      * buffer should be treated as read-only.
      *
@@ -90,15 +91,16 @@ public class DerInputStream {
      *          be read as DER input in the new stream
      * @param len how long a chunk of the buffer to use,
      *          starting at "offset"
+     * @param allowBER whether to allow constructed indefinite-length
+     *          encoding as well as tolerate leading 0s
      */
-    public DerInputStream(byte[] data, int offset, int len) throws IOException {
-        init(data, offset, len, true);
+    public DerInputStream(byte[] data, int offset, int len,
+        boolean allowBER) throws IOException {
+        init(data, offset, len, allowBER);
     }
 
     /**
-     * Create a DER input stream from part of a data buffer with
-     * additional arg to indicate whether to allow constructed
-     * indefinite-length encoding.
+     * Create a DER input stream from part of a data buffer.
      * The buffer is not copied, it is shared.  Accordingly, the
      * buffer should be treated as read-only.
      *
@@ -107,35 +109,32 @@ public class DerInputStream {
      *          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);
+    public DerInputStream(byte[] data, int offset, int len) throws IOException {
+        init(data, offset, len, true);
     }
 
     /*
      * private helper routine
      */
-    private void init(byte[] data, int offset, int len,
-        boolean allowIndefiniteLength) throws IOException {
+    private void init(byte[] data, int offset, int len, boolean allowBER) 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])) {
-            if (!allowIndefiniteLength) {
+            if (!allowBER) {
                 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));
+                buffer = new DerInputBuffer(derIn.convert(inData), allowBER);
             }
-        } else
-            buffer = new DerInputBuffer(data, offset, len);
+        } else {
+            buffer = new DerInputBuffer(data, offset, len, allowBER);
+        }
         buffer.mark(Integer.MAX_VALUE);
     }
 
@@ -156,7 +155,7 @@ public class DerInputStream {
      */
     public DerInputStream subStream(int len, boolean do_skip)
     throws IOException {
-        DerInputBuffer  newbuf = buffer.dup();
+        DerInputBuffer newbuf = buffer.dup();
 
         newbuf.truncate(len);
         if (do_skip) {
@@ -393,7 +392,8 @@ public class DerInputStream {
            dis.readFully(indefData, offset, readLen);
            dis.close();
            DerIndefLenConverter derIn = new DerIndefLenConverter();
-           buffer = new DerInputBuffer(derIn.convert(indefData));
+           buffer = new DerInputBuffer(derIn.convert(indefData), buffer.allowBER);
+
            if (tag != buffer.read())
                 throw new IOException("Indefinite length encoding" +
                         " not supported");
@@ -421,7 +421,7 @@ public class DerInputStream {
         DerValue value;
 
         do {
-            value = new DerValue(newstr.buffer);
+            value = new DerValue(newstr.buffer, buffer.allowBER);
             vec.addElement(value);
         } while (newstr.available() > 0);
 

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

-/*
- * Copyright (c) 1996, 2016, Oracle and/or its affiliates. All rights reserved.
+/**
+ * Copyright (c) 1996, 2017, 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
@@ -226,6 +226,16 @@ public class DerValue {
         data = init(stringTag, value);
     }
 
+    // Creates a DerValue from a tag and some DER-encoded data w/ additional
+    // arg to control whether DER checks are enforced.
+    DerValue(byte tag, byte[] data, boolean allowBER) {
+        this.tag = tag;
+        buffer = new DerInputBuffer(data.clone(), allowBER);
+        length = data.length;
+        this.data = new DerInputStream(buffer);
+        this.data.mark(Integer.MAX_VALUE);
+    }
+
     /**
      * Creates a DerValue from a tag and some DER-encoded data.
      *
@@ -233,20 +243,16 @@ public class DerValue {
      * @param data the DER-encoded data
      */
     public DerValue(byte tag, byte[] data) {
-        this.tag = tag;
-        buffer = new DerInputBuffer(data.clone());
-        length = data.length;
-        this.data = new DerInputStream(buffer);
-        this.data.mark(Integer.MAX_VALUE);
+        this(tag, data, true);
     }
 
     /*
      * package private
      */
     DerValue(DerInputBuffer in) throws IOException {
+
         // XXX must also parse BER-encoded constructed
         // values such as sequences, sets...
-
         tag = (byte)in.read();
         byte lenByte = (byte)in.read();
         length = DerInputStream.getLength(lenByte, in);
@@ -261,7 +267,7 @@ public class DerValue {
             dis.readFully(indefData, offset, readLen);
             dis.close();
             DerIndefLenConverter derIn = new DerIndefLenConverter();
-            inbuf = new DerInputBuffer(derIn.convert(indefData));
+            inbuf = new DerInputBuffer(derIn.convert(indefData), in.allowBER);
             if (tag != inbuf.read())
                 throw new IOException
                         ("Indefinite length encoding not supported");
@@ -283,6 +289,12 @@ public class DerValue {
         }
     }
 
+    // Get an ASN.1/DER encoded datum from a buffer w/ additional
+    // arg to control whether DER checks are enforced.
+    DerValue(byte[] buf, boolean allowBER) throws IOException {
+        data = init(true, new ByteArrayInputStream(buf), allowBER);
+    }
+
     /**
      * Get an ASN.1/DER encoded datum from a buffer.  The
      * entire buffer must hold exactly one datum, including
@@ -291,7 +303,14 @@ public class DerValue {
      * @param buf buffer holding a single DER-encoded datum.
      */
     public DerValue(byte[] buf) throws IOException {
-        data = init(true, new ByteArrayInputStream(buf));
+        this(buf, true);
+    }
+
+    // Get an ASN.1/DER encoded datum from part of a buffer w/ additional
+    // arg to control whether DER checks are enforced.
+    DerValue(byte[] buf, int offset, int len, boolean allowBER)
+        throws IOException {
+        data = init(true, new ByteArrayInputStream(buf, offset, len), allowBER);
     }
 
     /**
@@ -304,7 +323,13 @@ public class DerValue {
      * @param length how many bytes are in the encoded datum
      */
     public DerValue(byte[] buf, int offset, int len) throws IOException {
-        data = init(true, new ByteArrayInputStream(buf, offset, len));
+        this(buf, offset, len, true);
+    }
+
+    // Get an ASN1/DER encoded datum from an input stream w/ additional
+    // arg to control whether DER checks are enforced.
+    DerValue(InputStream in, boolean allowBER) throws IOException {
+        data = init(false, in, allowBER);
     }
 
     /**
@@ -317,10 +342,11 @@ public class DerValue {
      *  which may be followed by additional data
      */
     public DerValue(InputStream in) throws IOException {
-        data = init(false, in);
+        this(in, true);
     }
 
-    private DerInputStream init(byte stringTag, String value) throws IOException {
+    private DerInputStream init(byte stringTag, String value)
+        throws IOException {
         String enc = null;
 
         tag = stringTag;
@@ -348,7 +374,7 @@ public class DerValue {
 
         byte[] buf = value.getBytes(enc);
         length = buf.length;
-        buffer = new DerInputBuffer(buf);
+        buffer = new DerInputBuffer(buf, true);
         DerInputStream result = new DerInputStream(buffer);
         result.mark(Integer.MAX_VALUE);
         return result;
@@ -357,8 +383,8 @@ public class DerValue {
     /*
      * helper routine
      */
-    private DerInputStream init(boolean fullyBuffered, InputStream in)
-            throws IOException {
+    private DerInputStream init(boolean fullyBuffered, InputStream in,
+        boolean allowBER) throws IOException {
 
         tag = (byte)in.read();
         byte lenByte = (byte)in.read();
@@ -385,7 +411,7 @@ public class DerValue {
 
         byte[] bytes = IOUtils.readFully(in, length, true);
 
-        buffer = new DerInputBuffer(bytes);
+        buffer = new DerInputBuffer(bytes, allowBER);
         return new DerInputStream(buffer);
     }
 
@@ -480,7 +506,8 @@ public class DerValue {
         if (buffer.read(bytes) != length)
             throw new IOException("short read on DerValue buffer");
         if (isConstructed()) {
-            DerInputStream in = new DerInputStream(bytes);
+            DerInputStream in = new DerInputStream(bytes, 0, bytes.length,
+                buffer.allowBER);
             bytes = null;
             while (in.available() != 0) {
                 bytes = append(bytes, in.getOctetString());

test/sun/security/pkcs/pkcs8/PKCS8Test.java

 /*
- * Copyright (c) 2015, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2015, 2017, 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
@@ -191,12 +191,12 @@ public class PKCS8Test {
     public static void main(String[] args)
             throws IOException, InvalidKeyException {
 
-        BigInteger p = BigInteger.valueOf(1);
-        BigInteger q = BigInteger.valueOf(2);
-        BigInteger g = BigInteger.valueOf(3);
-        BigInteger x = BigInteger.valueOf(4);
+        BigInteger x = BigInteger.valueOf(1);
+        BigInteger p = BigInteger.valueOf(2);
+        BigInteger q = BigInteger.valueOf(3);
+        BigInteger g = BigInteger.valueOf(4);
 
-        DSAPrivateKey priv = new DSAPrivateKey(p, q, g, x);
+        DSAPrivateKey priv = new DSAPrivateKey(x, p, q, g);
 
         byte[] encodedKey = priv.getEncoded();
         byte[] expectedBytes = new byte[EXPECTED.length];

test/sun/security/pkcs/pkcs8/TestLeadingZeros.java

+/*
+ * Copyright (c) 2017, 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 8175251
+ * @summary ensure that PKCS8-encoded private key with leading 0s
+ * can be loaded.
+ * @run main TestLeadingZeros
+ */
+
+import java.io.*;
+import java.security.*;
+import java.security.spec.PKCS8EncodedKeySpec;
+import java.security.interfaces.*;
+import java.util.*;
+
+public class TestLeadingZeros {
+
+    // The following test vectors are various BER encoded PKCS8 bytes
+    static final String[] PKCS8_ENCODINGS  = {
+        // first is the original one from PKCS8Test
+        "301e020100301206052b0e03020c30090201020201030201040403020101A000",
+        // changed original to version w/ 1 leading 0
+        "301f02020000301206052b0e03020c30090201020201030201040403020101A000",
+        // changed original to P w/ 1 leading 0
+        "301f020100301306052b0e03020c300a020200020201030201040403020101A000",
+        // changed original to X w/ 2 leading 0s
+        "3020020100301206052b0e03020c300902010202010302010404050203000001A000"
+    };
+
+    public static void main(String[] argv) throws Exception {
+        KeyFactory factory = KeyFactory.getInstance("DSA", "SUN");
+
+        for (String encodings : PKCS8_ENCODINGS) {
+            byte[] encodingBytes = hexToBytes(encodings);
+            PKCS8EncodedKeySpec encodedKeySpec =
+                new PKCS8EncodedKeySpec(encodingBytes);
+            DSAPrivateKey privKey2 = (DSAPrivateKey)
+                factory.generatePrivate(encodedKeySpec);
+            System.out.println("key: " + privKey2);
+        }
+        System.out.println("Test Passed");
+    }
+
+    private static byte[] hexToBytes(String hex) {
+        if (hex.length() % 2 != 0) {
+            throw new RuntimeException("Input should be even length");
+        }
+        int size = hex.length() / 2;
+        byte[] result = new byte[size];
+        for (int i = 0; i < size; i++) {
+            int hi = Character.digit(hex.charAt(2 * i), 16);
+            int lo = Character.digit(hex.charAt(2 * i + 1), 16);
+            if ((hi == -1) || (lo == -1)) {
+                throw new RuntimeException("Input should be hexadecimal");
+            }
+            result[i] = (byte) (16 * hi + lo);
+        }
+        return result;
+    }
+}