OpenJDK / jdk8u / jdk8u / jdk
changeset 12168:31dae74ad077
8168714: Tighten ECDSA validation
Summary: Added additional checks to DER parsing code
Reviewed-by: vinnie, ahgross
author | igerasim |
---|---|
date | Wed, 23 Nov 2016 11:42:01 +0300 |
parents | a956a914f3a4 |
children | 0cb423d5df20 |
files | src/share/classes/sun/security/ec/ECDSASignature.java src/share/classes/sun/security/pkcs11/P11Signature.java src/share/classes/sun/security/provider/DSA.java src/share/classes/sun/security/rsa/RSASignature.java src/share/classes/sun/security/util/DerInputBuffer.java src/share/classes/sun/security/util/DerInputStream.java src/share/classes/sun/security/util/DerValue.java |
diffstat | 7 files changed, 119 insertions(+), 55 deletions(-) [+] |
line wrap: on
line diff
--- a/src/share/classes/sun/security/ec/ECDSASignature.java Wed Nov 23 11:41:41 2016 +0300 +++ b/src/share/classes/sun/security/ec/ECDSASignature.java Wed Nov 23 11:42:01 2016 +0300 @@ -364,18 +364,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()); @@ -389,7 +393,7 @@ return result; } catch (Exception e) { - throw new SignatureException("Could not decode signature", e); + throw new SignatureException("Invalid encoding for signature", e); } }
--- a/src/share/classes/sun/security/pkcs11/P11Signature.java Wed Nov 23 11:41:41 2016 +0300 +++ b/src/share/classes/sun/security/pkcs11/P11Signature.java Wed Nov 23 11:42:01 2016 +0300 @@ -705,17 +705,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)) { @@ -725,21 +729,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()); @@ -750,7 +758,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); } }
--- a/src/share/classes/sun/security/provider/DSA.java Wed Nov 23 11:41:41 2016 +0300 +++ b/src/share/classes/sun/security/provider/DSA.java Wed Nov 23 11:42:01 2016 +0300 @@ -267,18 +267,20 @@ 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
--- a/src/share/classes/sun/security/rsa/RSASignature.java Wed Nov 23 11:41:41 2016 +0300 +++ b/src/share/classes/sun/security/rsa/RSASignature.java Wed Nov 23 11:42:01 2016 +0300 @@ -223,9 +223,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/src/share/classes/sun/security/util/DerInputBuffer.java Wed Nov 23 11:41:41 2016 +0300 +++ b/src/share/classes/sun/security/util/DerInputBuffer.java Wed Nov 23 11:42:01 2016 +0300 @@ -1,5 +1,5 @@ /* - * 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 @@ 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/src/share/classes/sun/security/util/DerInputStream.java Wed Nov 23 11:41:41 2016 +0300 +++ b/src/share/classes/sun/security/util/DerInputStream.java Wed Nov 23 11:42:01 2016 +0300 @@ -1,5 +1,5 @@ /* - * 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 @@ * @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); @@ -233,12 +258,21 @@ * 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 @@ 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 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 @@ 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 @@ } 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 @@ 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 @@ */ 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 @@ 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/src/share/classes/sun/security/util/DerValue.java Wed Nov 23 11:41:41 2016 +0300 +++ b/src/share/classes/sun/security/util/DerValue.java Wed Nov 23 11:42:01 2016 +0300 @@ -1,5 +1,5 @@ /* - * 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 @@ 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 @@ 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