changeset 12682:de451d90c53e

8134364: Add defensive copies to get/set methods for OCSPNonceExtension Summary: Make OCSPNonceExtension immutable, add defensive copies Reviewed-by: xuelei, mullan
author jnimeh
date Fri, 04 Sep 2015 09:31:47 -0700
parents 387dcffa2a0a
children a08b835d32ba
files src/java.base/share/classes/sun/security/provider/certpath/OCSPNonceExtension.java test/sun/security/provider/certpath/Extensions/OCSPNonceExtensionTests.java
diffstat 2 files changed, 179 insertions(+), 386 deletions(-) [+]
line wrap: on
line diff
--- a/src/java.base/share/classes/sun/security/provider/certpath/OCSPNonceExtension.java	Fri Sep 04 15:28:01 2015 +0300
+++ b/src/java.base/share/classes/sun/security/provider/certpath/OCSPNonceExtension.java	Fri Sep 04 09:31:47 2015 -0700
@@ -26,15 +26,13 @@
 package sun.security.provider.certpath;
 
 import java.io.IOException;
-import java.io.OutputStream;
-import java.util.Enumeration;
+import java.util.Objects;
 import java.security.SecureRandom;
 
-import sun.security.x509.AttributeNameEnumeration;
-import sun.security.x509.CertAttrSet;
 import sun.security.x509.Extension;
 import sun.security.x509.PKIXExtensions;
-import sun.security.util.*;
+import sun.security.util.Debug;
+import sun.security.util.DerValue;
 
 /**
  * Represent the OCSP Nonce Extension.
@@ -43,252 +41,139 @@
  * and help to prevent replay attacks (see RFC 6960, section 4.4.1).
  *
  * @see Extension
- * @see CertAttrSet
  */
-public class OCSPNonceExtension extends Extension
-implements CertAttrSet<String> {
+public final class OCSPNonceExtension extends Extension {
 
     /**
      * Attribute name.
      */
-    public static final String NAME = "OCSPNonce";
-    public static final String NONCE = "nonce";
-
+    private static final String EXTENSION_NAME = "OCSPNonce";
     private byte[] nonceData = null;
-    private String extensionName;
-
-    /**
-     * Encode this extension value to DER and assign it to the
-     * {@code extensionName} data member.
-     *
-     * @throws IOException if any errors occur during DER encoding
-     */
-    private void encodeInternal() throws IOException {
-        if (nonceData == null) {
-            this.extensionValue = null;
-            return;
-        }
-        DerOutputStream os = new DerOutputStream();
-        os.putOctetString(this.nonceData);
-        this.extensionValue = os.toByteArray();
-    }
 
     /**
      * Create a {@code OCSPNonceExtension} by providing the nonce length.
-     * The criticality is set to false.  The random bytes will be generated
-     * using the SUN provider.
+     * The criticality is set to false, and the OID for the extension will
+     * be the value defined by "id-pkix-ocsp-nonce" from RFC 6960.
      *
      * @param length the number of random bytes composing the nonce
      *
      * @throws IOException if any errors happen during encoding of the
      *      extension.
+     * @throws IllegalArgumentException if length is not a positive integer.
      */
     public OCSPNonceExtension(int length) throws IOException {
-        this(PKIXExtensions.OCSPNonce_Id, false, length, NAME);
+        this(false, length);
     }
 
     /**
-     * Creates the extension (also called by the subclass).
+     * Create a {@code OCSPNonceExtension} by providing the nonce length and
+     * criticality setting.  The OID for the extension will
+     * be the value defined by "id-pkix-ocsp-nonce" from RFC 6960.
      *
-     * @param extensionId the {@code ObjectIdentifier} for the OCSP Nonce
-     *      extension
-     * @param isCritical a boolean flag indicating if the criticality bit
-     *      is to be set for this extension
-     * @param length the length of the nonce in bytes
-     * @param extensionName the name of the extension
+     * @param isCritical a boolean flag indicating whether the criticality bit
+     *      is set for this extension
+     * @param length the number of random bytes composing the nonce
      *
      * @throws IOException if any errors happen during encoding of the
      *      extension.
+     * @throws IllegalArgumentException if length is not a positive integer.
      */
-    protected OCSPNonceExtension(ObjectIdentifier extensionId,
-            boolean isCritical, int length, String extensionName)
+    public OCSPNonceExtension(boolean isCritical, int length)
             throws IOException {
-        SecureRandom rng = new SecureRandom();
-        this.nonceData = new byte[length];
-        rng.nextBytes(nonceData);
-        this.extensionId = extensionId;
+        this.extensionId = PKIXExtensions.OCSPNonce_Id;
         this.critical = isCritical;
-        this.extensionName = extensionName;
-        encodeInternal();
-    }
 
-    /**
-     * Create the extension using the provided criticality bit setting and
-     * DER encoding.
-     *
-     * @param critical true if the extension is to be treated as critical.
-     * @param value an array of DER encoded bytes of the extnValue for the
-     *      extension.  It must not include the encapsulating OCTET STRING
-     *      tag and length.  For an {@code OCSPNonceExtension} the data value
-     *      should be a simple OCTET STRING containing random bytes
-     *      (see RFC 6960, section 4.4.1).
-     *
-     * @throws ClassCastException if value is not an array of bytes
-     * @throws IOException if any errors happen during encoding of the
-     *      extension
-     */
-    public OCSPNonceExtension(Boolean critical, Object value)
-            throws IOException {
-        this(PKIXExtensions.OCSPNonce_Id, critical, value, NAME);
-    }
-
-    /**
-     * Creates the extension (also called by the subclass).
-     *
-     * @param extensionId the {@code ObjectIdentifier} for the OCSP Nonce
-     *      extension
-     * @param critical a boolean flag indicating if the criticality bit
-     *      is to be set for this extension
-     * @param value an array of DER encoded bytes of the extnValue for the
-     *      extension.  It must not include the encapsulating OCTET STRING
-     *      tag and length.  For an {@code OCSPNonceExtension} the data value
-     *      should be a simple OCTET STRING containing random bytes
-     *      (see RFC 6960, section 4.4.1).
-     * @param extensionName the name of the extension
-     *
-     * @throws ClassCastException if value is not an array of bytes
-     * @throws IOException if any errors happen during encoding of the
-     *      extension
-     */
-    protected OCSPNonceExtension(ObjectIdentifier extensionId,
-            Boolean critical, Object value, String extensionName)
-            throws IOException {
-        this.extensionId = extensionId;
-        this.critical = critical;
-        this.extensionValue = (byte[]) value;
-        DerValue val = new DerValue(this.extensionValue);
-        this.nonceData = val.getOctetString();
-        this.extensionName = extensionName;
-    }
-
-    /**
-     * Set the attribute value.
-     *
-     * @param name the name of the attribute.
-     * @param obj an array of nonce bytes for the extension.  It must not
-     *      contain any DER tags or length.
-     *
-     * @throws IOException if an unsupported name is provided or the supplied
-     *      {@code obj} is not a byte array
-     */
-    @Override
-    public void set(String name, Object obj) throws IOException {
-        if (name.equalsIgnoreCase(NONCE)) {
-            if (!(obj instanceof byte[])) {
-                throw new IOException("Attribute must be of type byte[].");
-            }
-            nonceData = (byte[])obj;
+        if (length > 0) {
+            SecureRandom rng = new SecureRandom();
+            this.nonceData = new byte[length];
+            rng.nextBytes(nonceData);
+            this.extensionValue = new DerValue(DerValue.tag_OctetString,
+                    nonceData).toByteArray();
         } else {
-            throw new IOException("Attribute name not recognized by"
-                    + " CertAttrSet:" + extensionName + ".");
-        }
-        encodeInternal();
-    }
-
-    /**
-     * Get the attribute value.
-     *
-     * @param name the name of the attribute to retrieve.  Only "OCSPNonce"
-     *      is currently supported.
-     *
-     * @return an array of bytes that are the nonce data.  It will not contain
-     *      any DER tags or length, only the random nonce bytes.
-     *
-     * @throws IOException if an unsupported name is provided.
-     */
-    @Override
-    public Object get(String name) throws IOException {
-        if (name.equalsIgnoreCase(NONCE)) {
-            return nonceData;
-        } else {
-            throw new IOException("Attribute name not recognized by"
-                    + " CertAttrSet:" + extensionName + ".");
+            throw new IllegalArgumentException(
+                    "Length must be a positive integer");
         }
     }
 
     /**
-     * Delete the attribute value.
+     * Create a {@code OCSPNonceExtension} by providing a nonce value.
+     * The criticality is set to false, and the OID for the extension will
+     * be the value defined by "id-pkix-ocsp-nonce" from RFC 6960.
      *
-     * @param name the name of the attribute to retrieve.  Only "OCSPNonce"
-     *      is currently supported.
+     * @param incomingNonce The nonce data to be set for the extension.  This
+     *      must be a non-null array of at least one byte long.
      *
-     * @throws IOException if an unsupported name is provided or an error
-     *      occurs during re-encoding of the extension.
+     * @throws IOException if any errors happen during encoding of the
+     *      extension.
+     * @throws IllegalArgumentException if the incomingNonce length is not a
+     *      positive integer.
+     * @throws NullPointerException if the incomingNonce is null.
      */
-    @Override
-    public void delete(String name) throws IOException {
-        if (name.equalsIgnoreCase(NONCE)) {
-            nonceData = null;
+    public OCSPNonceExtension(byte[] incomingNonce) throws IOException {
+        this(false, incomingNonce);
+    }
+
+    /**
+     * Create a {@code OCSPNonceExtension} by providing a nonce value and
+     * criticality setting.  The OID for the extension will
+     * be the value defined by "id-pkix-ocsp-nonce" from RFC 6960.
+     *
+     * @param isCritical a boolean flag indicating whether the criticality bit
+     *      is set for this extension
+     * @param incomingNonce The nonce data to be set for the extension.  This
+     *      must be a non-null array of at least one byte long.
+     *
+     * @throws IOException if any errors happen during encoding of the
+     *      extension.
+     * @throws IllegalArgumentException if the incomingNonce length is not a
+     *      positive integer.
+     * @throws NullPointerException if the incomingNonce is null.
+     */
+    public OCSPNonceExtension(boolean isCritical, byte[] incomingNonce)
+            throws IOException {
+        this.extensionId = PKIXExtensions.OCSPNonce_Id;
+        this.critical = isCritical;
+
+        Objects.requireNonNull(incomingNonce, "Nonce data must be non-null");
+        if (incomingNonce.length > 0) {
+            this.nonceData = incomingNonce.clone();
+            this.extensionValue = new DerValue(DerValue.tag_OctetString,
+                    nonceData).toByteArray();
         } else {
-            throw new IOException("Attribute name not recognized by"
-                  + " CertAttrSet:" + extensionName + ".");
+            throw new IllegalArgumentException(
+                    "Nonce data must be at least 1 byte in length");
         }
-        encodeInternal();
+    }
+
+    /**
+     * Return the nonce bytes themselves, without any DER encoding.
+     *
+     * @return A copy of the underlying nonce bytes
+     */
+    public byte[] getNonceValue() {
+        return nonceData.clone();
     }
 
     /**
      * Returns a printable representation of the {@code OCSPNonceExtension}.
+     *
+     * @return a string representation of the extension.
      */
     @Override
     public String toString() {
-        String s = super.toString() + extensionName + ": " +
-                ((nonceData == null) ? "" : Debug.toString(nonceData))
-                + "\n";
-        return (s);
+        StringBuilder sb = new StringBuilder();
+        sb.append(super.toString()).append(EXTENSION_NAME).append(": ");
+        sb.append((nonceData == null) ? "" : Debug.toString(nonceData));
+        sb.append("\n");
+        return sb.toString();
     }
 
     /**
-     * Write the extension to an {@code OutputStream}
+     * Return the name of the extension as a {@code String}
      *
-     * @param out the {@code OutputStream} to write the extension to.
-     *
-     * @throws IOException on encoding errors.
+     * @return the name of the extension
      */
-    @Override
-    public void encode(OutputStream out) throws IOException {
-        encode(out, PKIXExtensions.OCSPNonce_Id, this.critical);
-    }
-
-    /**
-     * Write the extension to the DerOutputStream.
-     *
-     * @param out the {@code OutputStream} to write the extension to.
-     * @param extensionId the {@code ObjectIdentifier} used for this extension
-     * @param isCritical a flag indicating if the criticality bit is set for
-     *      this extension.
-     *
-     * @throws IOException on encoding errors.
-     */
-    protected void encode(OutputStream out, ObjectIdentifier extensionId,
-            boolean isCritical) throws IOException {
-
-        DerOutputStream tmp = new DerOutputStream();
-
-        if (this.extensionValue == null) {
-            this.extensionId = extensionId;
-            this.critical = isCritical;
-            encodeInternal();
-        }
-        super.encode(tmp);
-        out.write(tmp.toByteArray());
-    }
-
-    /**
-     * Return an enumeration of names of attributes existing within this
-     * attribute.
-     */
-    @Override
-    public Enumeration<String> getElements() {
-        AttributeNameEnumeration elements = new AttributeNameEnumeration();
-        elements.addElement(NONCE);
-        return (elements.elements());
-    }
-
-    /**
-     * Return the name of this attribute.
-     */
-    @Override
     public String getName() {
-        return (extensionName);
+        return EXTENSION_NAME;
     }
 }
--- a/test/sun/security/provider/certpath/Extensions/OCSPNonceExtensionTests.java	Fri Sep 04 15:28:01 2015 +0300
+++ b/test/sun/security/provider/certpath/Extensions/OCSPNonceExtensionTests.java	Fri Sep 04 09:31:47 2015 -0700
@@ -79,14 +79,11 @@
         Map<String, TestCase> testList =
                 new LinkedHashMap<String, TestCase>() {{
             put("CTOR Test (provide length)", testCtorByLength);
+            put("CTOR Test (provide nonce bytes)", testCtorByValue);
+            put("CTOR Test (set criticality forms)", testCtorCritForms);
             put("CTOR Test (provide extension DER encoding)",
                     testCtorSuperByDerValue);
-            put("Use set() call to provide random data", testResetValue);
-            put("Test get() method", testGet);
-            put("test set() method", testSet);
-            put("Test getElements() method", testGetElements);
             put("Test getName() method", testGetName);
-            put("Test delete() method", testDelete);
         }};
 
         System.out.println("============ Tests ============");
@@ -179,6 +176,20 @@
             Boolean pass = Boolean.FALSE;
             String message = null;
             try (ByteArrayOutputStream baos = new ByteArrayOutputStream()) {
+                // Try sending in a negative length
+                try {
+                    Extension negLenNonce = new OCSPNonceExtension(-8);
+                    throw new RuntimeException(
+                            "Accepted a negative length nonce");
+                } catch (IllegalArgumentException iae) { }
+
+                // How about a zero length?
+                try {
+                    Extension zeroLenNonce = new OCSPNonceExtension(0);
+                    throw new RuntimeException("Accepted a zero length nonce");
+                } catch (IllegalArgumentException iae) { }
+
+                // Valid input to constructor
                 Extension nonceByLen = new OCSPNonceExtension(32);
 
                 // Verify overall encoded extension structure
@@ -216,6 +227,82 @@
         }
     };
 
+    public static final TestCase testCtorByValue = new TestCase() {
+        @Override
+        public Map.Entry<Boolean, String> runTest() {
+            Boolean pass = Boolean.FALSE;
+            String message = null;
+            try (ByteArrayOutputStream baos = new ByteArrayOutputStream()) {
+
+                // Try giving a null value for the nonce
+                try {
+                    Extension nullNonce = new OCSPNonceExtension(null);
+                    throw new RuntimeException("Accepted a null nonce");
+                } catch (NullPointerException npe) { }
+
+                // How about a zero-length byte array?
+                try {
+                    Extension zeroLenNonce =
+                            new OCSPNonceExtension(new byte[0]);
+                    throw new RuntimeException("Accepted a zero length nonce");
+                } catch (IllegalArgumentException iae) { }
+
+                OCSPNonceExtension nonceByValue =
+                        new OCSPNonceExtension(DEADBEEF_16);
+
+                // Verify overall encoded extension structure
+                nonceByValue.encode(baos);
+                verifyExtStructure(baos.toByteArray());
+
+                // Verify the name, elements, and data conform to
+                // expected values for this specific object.
+                boolean crit = nonceByValue.isCritical();
+                String oid = nonceByValue.getId();
+                byte[] nonceData = nonceByValue.getNonceValue();
+
+                if (crit) {
+                    message = "Extension incorrectly marked critical";
+                } else if (!oid.equals(OCSP_NONCE_OID)) {
+                    message = "Incorrect OID (Got " + oid + ", Expected " +
+                            OCSP_NONCE_OID + ")";
+                } else if (!Arrays.equals(nonceData, DEADBEEF_16)) {
+                    message = "Returned nonce value did not match input";
+                } else {
+                    pass = Boolean.TRUE;
+                }
+            } catch (Exception e) {
+                e.printStackTrace(System.out);
+                message = e.getClass().getName();
+            }
+
+            return new AbstractMap.SimpleEntry<>(pass, message);
+        }
+    };
+
+    public static final TestCase testCtorCritForms = new TestCase() {
+        @Override
+        public Map.Entry<Boolean, String> runTest() {
+            Boolean pass = Boolean.FALSE;
+            String message = null;
+            try (ByteArrayOutputStream baos = new ByteArrayOutputStream()) {
+                Extension nonceByLength = new OCSPNonceExtension(true, 32);
+                Extension nonceByValue =
+                        new OCSPNonceExtension(true, DEADBEEF_16);
+                pass = nonceByLength.isCritical() && nonceByValue.isCritical();
+                if (!pass) {
+                    message = "nonceByLength or nonceByValue was not marked " +
+                            "critical as expected";
+                }
+            }  catch (Exception e) {
+                e.printStackTrace(System.out);
+                message = e.getClass().getName();
+            }
+
+            return new AbstractMap.SimpleEntry<>(pass, message);
+        }
+    };
+
+
     public static final TestCase testCtorSuperByDerValue = new TestCase() {
         @Override
         public Map.Entry<Boolean, String> runTest() {
@@ -260,145 +347,6 @@
         }
     };
 
-    public static final TestCase testResetValue = new TestCase() {
-        @Override
-        public Map.Entry<Boolean, String> runTest() {
-            Boolean pass = Boolean.FALSE;
-            String message = null;
-            try (ByteArrayOutputStream baos = new ByteArrayOutputStream()) {
-                OCSPNonceExtension nonce = new OCSPNonceExtension(32);
-
-                // Reset the nonce data to reflect 16 bytes of DEADBEEF
-                nonce.set(OCSPNonceExtension.NONCE, (Object)DEADBEEF_16);
-
-                // Verify overall encoded extension content
-                nonce.encode(baos);
-                dumpHexBytes(OCSP_NONCE_DB16);
-                System.out.println();
-                dumpHexBytes(baos.toByteArray());
-
-                pass = Arrays.equals(baos.toByteArray(), OCSP_NONCE_DB16);
-            } catch (Exception e) {
-                e.printStackTrace(System.out);
-                message = e.getClass().getName();
-            }
-
-            return new AbstractMap.SimpleEntry<>(pass, message);
-        }
-    };
-
-    public static final TestCase testSet = new TestCase() {
-        @Override
-        public Map.Entry<Boolean, String> runTest() {
-            Boolean pass = Boolean.FALSE;
-            String message = null;
-            try {
-                OCSPNonceExtension nonceByLen = new OCSPNonceExtension(32);
-
-                // Set the nonce data to 16 bytes of DEADBEEF
-                nonceByLen.set(ELEMENT_NONCE, DEADBEEF_16);
-                byte[] nonceData = (byte[])nonceByLen.get(ELEMENT_NONCE);
-                if (!Arrays.equals(nonceData, DEADBEEF_16)) {
-                    throw new RuntimeException("Retuned nonce data does not " +
-                            "match expected result");
-                }
-
-                // Now try to set a value using an object that is not a byte
-                // array
-                int[] INT_DB_16 = {
-                    0xDEADBEEF, 0xDEADBEEF, 0xDEADBEEF, 0xDEADBEEF
-                };
-                try {
-                    nonceByLen.set(ELEMENT_NONCE, INT_DB_16);
-                    throw new RuntimeException("Accepted get() for " +
-                            "unsupported element name");
-                } catch (IOException ioe) { }     // Expected result
-
-                // And try setting a value using an unknown element name
-                try {
-                    nonceByLen.set("FOO", DEADBEEF_16);
-                    throw new RuntimeException("Accepted get() for " +
-                            "unsupported element name");
-                } catch (IOException ioe) { }     // Expected result
-
-                pass = Boolean.TRUE;
-            } catch (Exception e) {
-                e.printStackTrace(System.out);
-                message = e.getClass().getName();
-            }
-
-            return new AbstractMap.SimpleEntry<>(pass, message);
-        }
-    };
-
-        public static final TestCase testGet = new TestCase() {
-        @Override
-        public Map.Entry<Boolean, String> runTest() {
-            Boolean pass = Boolean.FALSE;
-            String message = null;
-            try {
-                OCSPNonceExtension nonceByLen = new OCSPNonceExtension(32);
-
-                // Grab the nonce data by its correct element name
-                byte[] nonceData = (byte[])nonceByLen.get(ELEMENT_NONCE);
-                if (nonceData == null || nonceData.length != 32) {
-                    throw new RuntimeException("Unexpected return value from " +
-                            "get() method: either null or incorrect length");
-                }
-
-                // Now try to get any kind of data using an element name that
-                // doesn't exist for this extension.
-                try {
-                    nonceByLen.get("FOO");
-                    throw new RuntimeException("Accepted get() for " +
-                            "unsupported element name");
-                } catch (IOException ioe) { }     // Expected result
-
-                pass = Boolean.TRUE;
-            } catch (Exception e) {
-                e.printStackTrace(System.out);
-                message = e.getClass().getName();
-            }
-
-            return new AbstractMap.SimpleEntry<>(pass, message);
-        }
-    };
-
-    public static final TestCase testGetElements = new TestCase() {
-        @Override
-        public Map.Entry<Boolean, String> runTest() {
-            Boolean pass = Boolean.FALSE;
-            String message = null;
-            try {
-                OCSPNonceExtension nonceByLen = new OCSPNonceExtension(32);
-
-                int elementCount = 0;
-                boolean foundElement = false;
-
-                // There should be exactly one element and its name should
-                // be "nonce"
-                for (Enumeration<String> elements = nonceByLen.getElements();
-                        elements.hasMoreElements(); elementCount++) {
-                    if (elements.nextElement().equals(ELEMENT_NONCE)) {
-                        foundElement = true;
-                    }
-                }
-
-                if (!foundElement || elementCount != 1) {
-                    throw new RuntimeException("Unexpected or missing " +
-                            "Enumeration element");
-                }
-
-                pass = Boolean.TRUE;
-            } catch (Exception e) {
-                e.printStackTrace(System.out);
-                message = e.getClass().getName();
-            }
-
-            return new AbstractMap.SimpleEntry<>(pass, message);
-        }
-    };
-
     public static final TestCase testGetName = new TestCase() {
         @Override
         public Map.Entry<Boolean, String> runTest() {
@@ -415,44 +363,4 @@
             return new AbstractMap.SimpleEntry<>(pass, message);
         }
     };
-
-    public static final TestCase testDelete = new TestCase() {
-        @Override
-        public Map.Entry<Boolean, String> runTest() {
-            Boolean pass = Boolean.FALSE;
-            String message = null;
-            try {
-                OCSPNonceExtension nonceByLen = new OCSPNonceExtension(32);
-
-                // First verify that there's data to begin with
-                byte[] nonceData = (byte[])nonceByLen.get(ELEMENT_NONCE);
-                if (nonceData == null || nonceData.length != 32) {
-                    throw new RuntimeException("Unexpected return value from " +
-                            "get() method: either null or incorrect length");
-                }
-
-                // Attempt to delete using an element name that doesn't exist
-                // for this extension.
-                try {
-                    nonceByLen.delete("FOO");
-                    throw new RuntimeException("Accepted delete() for " +
-                            "unsupported element name");
-                } catch (IOException ioe) { }     // Expected result
-
-                // Now attempt to properly delete the extension data
-                nonceByLen.delete(ELEMENT_NONCE);
-                nonceData = (byte[])nonceByLen.get(ELEMENT_NONCE);
-                if (nonceData != null) {
-                    throw new RuntimeException("Unexpected non-null return");
-                }
-
-                pass = Boolean.TRUE;
-            } catch (Exception e) {
-                e.printStackTrace(System.out);
-                message = e.getClass().getName();
-            }
-
-            return new AbstractMap.SimpleEntry<>(pass, message);
-        }
-    };
 }