8296742: Illegal X509 Extension should not be created

Reviewed-by: mullan

src/java.base/share/classes/sun/security/tools/keytool/Main.java

@@ -1582,7 +1582,11 @@ public final class Main {
             int d = id.indexOf(':');
             if (d >= 0) {
                 CRLExtensions ext = new CRLExtensions();
-                ext.setExtension("Reason", new CRLReasonCodeExtension(Integer.parseInt(id.substring(d+1))));
+                int code = Integer.parseInt(id.substring(d+1));
+                if (code <= 0) {
+                    throw new Exception("Reason code must be positive");
+                }
+                ext.setExtension("Reason", new CRLReasonCodeExtension(code));
                 badCerts[i] = new X509CRLEntryImpl(new BigInteger(id.substring(0, d)),
                         firstDate, ext);
             } else {
@@ -4632,6 +4636,9 @@ public final class Main {
                     continue;
                 }
                 int exttype = oneOf(name, extSupported);
+                if (exttype != -1 && value != null && value.isEmpty()) {
+                    throw new Exception(rb.getString("Illegal.value.") + extstr);
+                }
                 switch (exttype) {
                     case 0:     // BC
                         int pathLen = -1;

src/java.base/share/classes/sun/security/x509/AuthorityInfoAccessExtension.java

@@ -74,11 +74,15 @@ public class AuthorityInfoAccessExtension extends Extension {
      * Create an AuthorityInfoAccessExtension from a List of
      * AccessDescription; the criticality is set to false.
      *
-     * @param accessDescriptions the List of AccessDescription
+     * @param accessDescriptions the List of AccessDescription,
+     *                           cannot be null or empty.
      * @throws IOException on error
      */
     public AuthorityInfoAccessExtension(
             List<AccessDescription> accessDescriptions) throws IOException {
+        if (accessDescriptions == null || accessDescriptions.isEmpty()) {
+            throw new IllegalArgumentException("accessDescriptions is null or empty");
+        }
         this.extensionId = PKIXExtensions.AuthInfoAccess_Id;
         this.critical = false;
         this.accessDescriptions = accessDescriptions;

src/java.base/share/classes/sun/security/x509/AuthorityKeyIdentifierExtension.java

@@ -99,8 +99,8 @@ public class AuthorityKeyIdentifierExtension extends Extension {
     }
 
     /**
-     * The default constructor for this extension.  Null parameters make
-     * the element optional (not present).
+     * The default constructor for this extension. At least one parameter
+     * must be non null. Null parameters make the element optional (not present).
      *
      * @param kid the KeyIdentifier associated with this extension.
      * @param names the GeneralNames associated with this extension
@@ -110,7 +110,11 @@ public class AuthorityKeyIdentifierExtension extends Extension {
      */
     public AuthorityKeyIdentifierExtension(KeyIdentifier kid, GeneralNames names,
                                            SerialNumber sn)
-    throws IOException {
+            throws IOException {
+        if (kid == null && names == null && sn == null) {
+            throw new IllegalArgumentException(
+                    "AuthorityKeyIdentifierExtension cannot be empty");
+        }
         this.id = kid;
         this.names = names;
         this.serialNum = sn;

src/java.base/share/classes/sun/security/x509/CRLDistributionPointsExtension.java

@@ -106,7 +106,8 @@ public class CRLDistributionPointsExtension extends Extension {
      * DistributionPoint.
      *
      * @param isCritical the criticality setting.
-     * @param distributionPoints the list of distribution points
+     * @param distributionPoints the list of distribution points,
+     *                           cannot be null or empty.
      * @throws IOException on error
      */
     public CRLDistributionPointsExtension(boolean isCritical,
@@ -120,9 +121,14 @@ public class CRLDistributionPointsExtension extends Extension {
      * Creates the extension (also called by the subclass).
      */
     protected CRLDistributionPointsExtension(ObjectIdentifier extensionId,
-        boolean isCritical, List<DistributionPoint> distributionPoints,
+            boolean isCritical, List<DistributionPoint> distributionPoints,
             String extensionName) throws IOException {
 
+        if (distributionPoints == null || distributionPoints.isEmpty()) {
+            throw new IllegalArgumentException(
+                    "distribution points cannot be null or empty");
+        }
+
         this.extensionId = extensionId;
         this.critical = isCritical;
         this.distributionPoints = distributionPoints;

src/java.base/share/classes/sun/security/x509/CRLNumberExtension.java

@@ -78,7 +78,7 @@ public class CRLNumberExtension extends Extension {
      * Create a CRLNumberExtension with the BigInteger value .
      * The criticality is set to false.
      *
-     * @param crlNum the value to be set for the extension.
+     * @param crlNum the value to be set for the extension, cannot be null
      */
     public CRLNumberExtension(BigInteger crlNum) throws IOException {
         this(PKIXExtensions.CRLNumber_Id, false, crlNum, NAME, LABEL);
@@ -91,6 +91,9 @@ public class CRLNumberExtension extends Extension {
         boolean isCritical, BigInteger crlNum, String extensionName,
         String extensionLabel) throws IOException {
 
+        if (crlNum == null) {
+            throw new IllegalArgumentException("CRL number cannot be null");
+        }
         this.extensionId = extensionId;
         this.critical = isCritical;
         this.crlNumber = crlNum;

src/java.base/share/classes/sun/security/x509/CRLReasonCodeExtension.java

@@ -69,10 +69,13 @@ public class CRLReasonCodeExtension extends Extension {
      * Create a CRLReasonCodeExtension with the passed in reason.
      *
      * @param critical true if the extension is to be treated as critical.
-     * @param reason the enumerated value for the reason code.
+     * @param reason the enumerated value for the reason code, must be positive.
      */
     public CRLReasonCodeExtension(boolean critical, int reason)
-    throws IOException {
+            throws IOException {
+        if (reason <= 0) {
+            throw new IllegalArgumentException("reason code must be positive");
+        }
         this.extensionId = PKIXExtensions.ReasonCode_Id;
         this.critical = critical;
         this.reasonCode = reason;

src/java.base/share/classes/sun/security/x509/CertificateIssuerExtension.java

@@ -80,10 +80,13 @@ public class CertificateIssuerExtension extends Extension {
      * Create a CertificateIssuerExtension containing the specified issuer name.
      * Criticality is automatically set to true.
      *
-     * @param issuer the certificate issuer
+     * @param issuer the certificate issuer, cannot be null or empty.
      * @throws IOException on error
      */
     public CertificateIssuerExtension(GeneralNames issuer) throws IOException {
+        if (issuer == null || issuer.isEmpty()) {
+            throw new IllegalArgumentException("issuer cannot be null or empty");
+        }
         this.extensionId = PKIXExtensions.CertificateIssuer_Id;
         this.critical = true;
         this.names = issuer;

src/java.base/share/classes/sun/security/x509/CertificatePoliciesExtension.java

@@ -106,10 +106,14 @@ public class CertificatePoliciesExtension extends Extension {
      * a List of PolicyInformation with specified criticality.
      *
      * @param critical true if the extension is to be treated as critical.
-     * @param certPolicies the List of PolicyInformation.
+     * @param certPolicies the List of PolicyInformation, cannot be null or empty.
      */
     public CertificatePoliciesExtension(Boolean critical,
             List<PolicyInformation> certPolicies) throws IOException {
+        if (certPolicies == null || certPolicies.isEmpty()) {
+            throw new IllegalArgumentException(
+                    "certificate policies cannot be null or empty");
+        }
         this.certPolicies = certPolicies;
         this.extensionId = PKIXExtensions.CertificatePolicies_Id;
         this.critical = critical.booleanValue();

src/java.base/share/classes/sun/security/x509/CertificatePolicyId.java

@@ -26,6 +26,8 @@
 package sun.security.x509;
 
 import java.io.IOException;
+import java.util.Objects;
+
 import sun.security.util.*;
 
 
@@ -44,7 +46,7 @@ public class CertificatePolicyId {
      * @param id the ObjectIdentifier for the policy id.
      */
     public CertificatePolicyId(ObjectIdentifier id) {
-        this.id = id;
+        this.id = Objects.requireNonNull(id);
     }
 
     /**

src/java.base/share/classes/sun/security/x509/CertificatePolicyMap.java

@@ -26,6 +26,7 @@
 package sun.security.x509;
 
 import java.io.IOException;
+import java.util.Objects;
 
 import sun.security.util.*;
 
@@ -47,8 +48,8 @@ public class CertificatePolicyMap {
      */
     public CertificatePolicyMap(CertificatePolicyId issuer,
                                 CertificatePolicyId subject) {
-        this.issuerDomain = issuer;
-        this.subjectDomain = subject;
+        this.issuerDomain = Objects.requireNonNull(issuer);
+        this.subjectDomain = Objects.requireNonNull(subject);
     }
 
     /**

src/java.base/share/classes/sun/security/x509/ExtendedKeyUsageExtension.java

@@ -118,10 +118,15 @@ public class ExtendedKeyUsageExtension extends Extension {
      * a Vector of KeyUsages with specified criticality.
      *
      * @param critical true if the extension is to be treated as critical.
-     * @param keyUsages the Vector of KeyUsages (ObjectIdentifiers)
+     * @param keyUsages the Vector of KeyUsages (ObjectIdentifiers),
+     *                  cannot be null or empty.
      */
     public ExtendedKeyUsageExtension(Boolean critical, Vector<ObjectIdentifier> keyUsages)
-    throws IOException {
+            throws IOException {
+        if (keyUsages == null || keyUsages.isEmpty()) {
+            throw new IllegalArgumentException(
+                    "key usages cannot be null or empty");
+        }
         this.keyUsages = keyUsages;
         this.extensionId = PKIXExtensions.ExtendedKeyUsage_Id;
         this.critical = critical.booleanValue();

src/java.base/share/classes/sun/security/x509/Extension.java

@@ -28,6 +28,8 @@ package sun.security.x509;
 import java.io.IOException;
 import java.io.OutputStream;
 import java.util.Arrays;
+import java.util.Objects;
+
 import sun.security.util.*;
 
 /**
@@ -172,10 +174,10 @@ public class Extension implements java.security.cert.Extension, DerEncoder {
     @Override
     public void encode(DerOutputStream out) throws IOException {
 
-        if (extensionId == null)
-            throw new IOException("Null OID to encode for the extension!");
-        if (extensionValue == null)
-            throw new IOException("No value to encode for the extension!");
+        Objects.requireNonNull(extensionId,
+                "No OID to encode for the extension");
+        Objects.requireNonNull(extensionValue,
+                "No value to encode for the extension");
 
         DerOutputStream dos = new DerOutputStream();
 

src/java.base/share/classes/sun/security/x509/GeneralSubtree.java

@@ -26,6 +26,7 @@
 package sun.security.x509;
 
 import java.io.*;
+import java.util.Objects;
 
 import sun.security.util.*;
 
@@ -61,7 +62,7 @@ public class GeneralSubtree {
      * @param max the maximum BaseDistance
      */
     public GeneralSubtree(GeneralName name, int min, int max) {
-        this.name = name;
+        this.name = Objects.requireNonNull(name);
         this.minimum = min;
         this.maximum = max;
     }

src/java.base/share/classes/sun/security/x509/InhibitAnyPolicyExtension.java

@@ -83,7 +83,7 @@ public class InhibitAnyPolicyExtension extends Extension {
      */
     public InhibitAnyPolicyExtension(int skipCerts) throws IOException {
         if (skipCerts < -1)
-            throw new IOException("Invalid value for skipCerts");
+            throw new IllegalArgumentException("Invalid value for skipCerts");
         if (skipCerts == -1)
             this.skipCerts = Integer.MAX_VALUE;
         else

src/java.base/share/classes/sun/security/x509/InvalidityDateExtension.java

@@ -88,10 +88,13 @@ public class InvalidityDateExtension extends Extension {
      * Create a InvalidityDateExtension with the passed in date.
      *
      * @param critical true if the extension is to be treated as critical.
-     * @param date the invalidity date
+     * @param date the invalidity date, cannot be null.
      */
     public InvalidityDateExtension(boolean critical, Date date)
-    throws IOException {
+            throws IOException {
+        if (date == null) {
+            throw new IllegalArgumentException("date cannot be null");
+        }
         this.extensionId = PKIXExtensions.InvalidityDate_Id;
         this.critical = critical;
         this.date = date;

src/java.base/share/classes/sun/security/x509/IssuerAlternativeNameExtension.java

@@ -69,11 +69,8 @@ public class IssuerAlternativeNameExtension extends Extension {
      * @exception IOException on error.
      */
     public IssuerAlternativeNameExtension(GeneralNames names)
-    throws IOException {
-        this.names = names;
-        this.extensionId = PKIXExtensions.IssuerAlternativeName_Id;
-        this.critical = false;
-        encodeThis();
+            throws IOException {
+        this(false, names);
     }
 
     /**
@@ -81,26 +78,20 @@ public class IssuerAlternativeNameExtension extends Extension {
      * and GeneralNames.
      *
      * @param critical true if the extension is to be treated as critical.
-     * @param names the GeneralNames for the issuer.
+     * @param names the GeneralNames for the issuer, cannot be null or empty.
      * @exception IOException on error.
      */
     public IssuerAlternativeNameExtension(Boolean critical, GeneralNames names)
-    throws IOException {
+            throws IOException {
+        if (names == null || names.isEmpty()) {
+            throw new IllegalArgumentException("names cannot be null or empty");
+        }
         this.names = names;
         this.extensionId = PKIXExtensions.IssuerAlternativeName_Id;
         this.critical = critical.booleanValue();
         encodeThis();
     }
 
-    /**
-     * Create a default IssuerAlternativeNameExtension.
-     */
-    public IssuerAlternativeNameExtension() {
-        extensionId = PKIXExtensions.IssuerAlternativeName_Id;
-        critical = false;
-        names = new GeneralNames();
-    }
-
     /**
      * Create the extension from the passed DER encoded value.
      *

src/java.base/share/classes/sun/security/x509/IssuingDistributionPointExtension.java

@@ -110,7 +110,8 @@ public class IssuingDistributionPointExtension extends Extension {
      *        issuer CRL entry extension.
      * @throws IllegalArgumentException if more than one of
      *        <code>hasOnlyUserCerts</code>, <code>hasOnlyCACerts</code>,
-     *        <code>hasOnlyAttributeCerts</code> is set to <code>true</code>.
+     *        <code>hasOnlyAttributeCerts</code> is set to <code>true</code>,
+     *        or all arguments are either <code>null</code> or <code>false</code>.
      * @throws IOException on encoding error.
      */
     public IssuingDistributionPointExtension(
@@ -119,6 +120,14 @@ public class IssuingDistributionPointExtension extends Extension {
         boolean hasOnlyAttributeCerts, boolean isIndirectCRL)
             throws IOException {
 
+        if (distributionPoint == null &&
+                revocationReasons == null &&
+                !hasOnlyUserCerts &&
+                !hasOnlyCACerts &&
+                !hasOnlyAttributeCerts &&
+                !isIndirectCRL) {
+            throw new IllegalArgumentException("elements cannot be empty");
+        }
         if ((hasOnlyUserCerts && (hasOnlyCACerts || hasOnlyAttributeCerts)) ||
             (hasOnlyCACerts && (hasOnlyUserCerts || hasOnlyAttributeCerts)) ||
             (hasOnlyAttributeCerts && (hasOnlyUserCerts || hasOnlyCACerts))) {

src/java.base/share/classes/sun/security/x509/NameConstraintsExtension.java

@@ -127,8 +127,8 @@ public class NameConstraintsExtension extends Extension
     }
 
     /**
-     * The default constructor for this class. Both parameters
-     * are optional and can be set to null.  The extension criticality
+     * The default constructor for this class. Both parameters are optional
+     * but at least one should be non null.  The extension criticality
      * is set to true.
      *
      * @param permitted the permitted GeneralSubtrees (null for optional).
@@ -136,7 +136,11 @@ public class NameConstraintsExtension extends Extension
      */
     public NameConstraintsExtension(GeneralSubtrees permitted,
                                     GeneralSubtrees excluded)
-    throws IOException {
+            throws IOException {
+        if (permitted == null && excluded == null) {
+            throw new IllegalArgumentException(
+                    "permitted and excluded cannot both be null");
+        }
         this.permitted = permitted;
         this.excluded = excluded;
 
@@ -280,6 +284,8 @@ public class NameConstraintsExtension extends Extension
             return;
         }
 
+        boolean updated = false;
+
         /*
          * If excludedSubtrees is present in the certificate, set the
          * excluded subtrees state variable to the union of its previous
@@ -288,12 +294,15 @@ public class NameConstraintsExtension extends Extension
 
         GeneralSubtrees newExcluded = newConstraints.getExcludedSubtrees();
         if (excluded == null) {
-            excluded = (newExcluded != null) ?
-                        (GeneralSubtrees)newExcluded.clone() : null;
+            if (newExcluded != null) {
+                excluded = (GeneralSubtrees) newExcluded.clone();
+                updated = true;
+            }
         } else {
             if (newExcluded != null) {
                 // Merge new excluded with current excluded (union)
                 excluded.union(newExcluded);
+                updated = true;
             }
         }
 
@@ -305,8 +314,10 @@ public class NameConstraintsExtension extends Extension
 
         GeneralSubtrees newPermitted = newConstraints.getPermittedSubtrees();
         if (permitted == null) {
-            permitted = (newPermitted != null) ?
-                        (GeneralSubtrees)newPermitted.clone() : null;
+            if (newPermitted != null) {
+                permitted = (GeneralSubtrees) newPermitted.clone();
+                updated = true;
+            }
         } else {
             if (newPermitted != null) {
                 // Merge new permitted with current permitted (intersection)
@@ -319,6 +330,7 @@ public class NameConstraintsExtension extends Extension
                     } else {
                         excluded = (GeneralSubtrees)newExcluded.clone();
                     }
+                    updated = true;
                 }
             }
         }
@@ -329,12 +341,14 @@ public class NameConstraintsExtension extends Extension
         // less space.
         if (permitted != null) {
             permitted.reduce(excluded);
+            updated = true;
         }
 
         // The NameConstraints have been changed, so re-encode them.  Methods in
         // this class assume that the encodings have already been done.
-        encodeThis();
-
+        if (updated) {
+            encodeThis();
+        }
     }
 
     /**

src/java.base/share/classes/sun/security/x509/PolicyConstraintsExtension.java

@@ -102,14 +102,18 @@ public class PolicyConstraintsExtension extends Extension {
     /**
      * Create a PolicyConstraintsExtension object with specified
      * criticality and both require explicit policy and inhibit
-     * policy mapping.
+     * policy mapping. At least one should be provided (not -1).
      *
      * @param critical true if the extension is to be treated as critical.
      * @param require require explicit policy (-1 for optional).
      * @param inhibit inhibit policy mapping (-1 for optional).
      */
     public PolicyConstraintsExtension(Boolean critical, int require, int inhibit)
-    throws IOException {
+            throws IOException {
+        if (require == -1 && inhibit == -1) {
+            throw new IllegalArgumentException(
+                    "require and inhibit cannot both be -1");
+        }
         this.require = require;
         this.inhibit = inhibit;
         this.extensionId = PKIXExtensions.PolicyConstraints_Id;

src/java.base/share/classes/sun/security/x509/PolicyInformation.java

@@ -29,6 +29,7 @@ import java.io.IOException;
 import java.security.cert.PolicyQualifierInfo;
 import java.util.Collections;
 import java.util.LinkedHashSet;
+import java.util.Objects;
 import java.util.Set;
 
 import sun.security.util.DerValue;
@@ -87,7 +88,7 @@ public class PolicyInformation {
         }
         this.policyQualifiers =
                 new LinkedHashSet<>(policyQualifiers);
-        this.policyIdentifier = policyIdentifier;
+        this.policyIdentifier = Objects.requireNonNull(policyIdentifier);
     }
 
     /**

src/java.base/share/classes/sun/security/x509/PolicyMappingsExtension.java

@@ -75,25 +75,19 @@ public class PolicyMappingsExtension extends Extension {
     /**
      * Create a PolicyMappings with the List of CertificatePolicyMap.
      *
-     * @param maps the List of CertificatePolicyMap.
+     * @param maps the List of CertificatePolicyMap, cannot be null or empty.
      */
     public PolicyMappingsExtension(List<CertificatePolicyMap> maps)
             throws IOException {
+        if (maps == null || maps.isEmpty()) {
+            throw new IllegalArgumentException("maps cannot be null or empty");
+        }
         this.maps = maps;
         this.extensionId = PKIXExtensions.PolicyMappings_Id;
         this.critical = true;
         encodeThis();
     }
 
-    /**
-     * Create a default PolicyMappingsExtension.
-     */
-    public PolicyMappingsExtension() {
-        extensionId = PKIXExtensions.PolicyMappings_Id;
-        critical = true;
-        maps = Collections.emptyList();
-    }
-
     /**
      * Create the extension from the passed DER encoded value.
      *

src/java.base/share/classes/sun/security/x509/PrivateKeyUsageExtension.java

@@ -93,15 +93,20 @@ public class PrivateKeyUsageExtension extends Extension {
     }
 
     /**
-     * The default constructor for PrivateKeyUsageExtension.
+     * The default constructor for PrivateKeyUsageExtension. At least one
+     * of the arguments must be non null.
      *
      * @param notBefore the date/time before which the private key
-     *         should not be used.
+     *         should not be used
      * @param notAfter the date/time after which the private key
      *         should not be used.
      */
     public PrivateKeyUsageExtension(Date notBefore, Date notAfter)
-    throws IOException {
+            throws IOException {
+        if (notBefore == null && notAfter == null) {
+            throw new IllegalArgumentException(
+                    "notBefore and notAfter cannot both be null");
+        }
         this.notBefore = notBefore;
         this.notAfter = notAfter;
 

src/java.base/share/classes/sun/security/x509/SubjectAlternativeNameExtension.java

@@ -84,27 +84,20 @@ public class SubjectAlternativeNameExtension extends Extension {
      * criticality and GeneralNames.
      *
      * @param critical true if the extension is to be treated as critical.
-     * @param names the GeneralNames for the subject.
+     * @param names the GeneralNames for the subject, cannot be null or empty.
      * @exception IOException on error.
      */
     public SubjectAlternativeNameExtension(Boolean critical, GeneralNames names)
-    throws IOException {
+            throws IOException {
+        if (names == null || names.isEmpty()) {
+            throw new IllegalArgumentException("names cannot be null or empty");
+        }
         this.names = names;
         this.extensionId = PKIXExtensions.SubjectAlternativeName_Id;
         this.critical = critical.booleanValue();
         encodeThis();
     }
 
-    /**
-     * Create a default SubjectAlternativeNameExtension. The extension
-     * is marked non-critical.
-     */
-    public SubjectAlternativeNameExtension() {
-        extensionId = PKIXExtensions.SubjectAlternativeName_Id;
-        critical = false;
-        names = new GeneralNames();
-    }
-
     /**
      * Create the extension from the passed DER encoded value.
      *

src/java.base/share/classes/sun/security/x509/SubjectInfoAccessExtension.java

@@ -78,11 +78,16 @@ public class SubjectInfoAccessExtension extends Extension {
      * Create an SubjectInfoAccessExtension from a List of
      * AccessDescription; the criticality is set to false.
      *
-     * @param accessDescriptions the List of AccessDescription
+     * @param accessDescriptions the List of AccessDescription,
+     *                           cannot be null or empty.
      * @throws IOException on error
      */
     public SubjectInfoAccessExtension(
             List<AccessDescription> accessDescriptions) throws IOException {
+        if (accessDescriptions == null || accessDescriptions.isEmpty()) {
+            throw new IllegalArgumentException(
+                    "accessDescriptions cannot be null or empty");
+        }
         this.extensionId = PKIXExtensions.SubjectInfoAccess_Id;
         this.critical = false;
         this.accessDescriptions = accessDescriptions;

src/java.base/share/classes/sun/security/x509/SubjectKeyIdentifierExtension.java

@@ -73,7 +73,7 @@ public class SubjectKeyIdentifierExtension extends Extension {
      * @param octetString the octet string identifying the key identifier.
      */
     public SubjectKeyIdentifierExtension(byte[] octetString)
-    throws IOException {
+            throws IOException {
         id = new KeyIdentifier(octetString);
 
         this.extensionId = PKIXExtensions.SubjectKey_Id;