Remove attributes checks of certificate extensions in CertificateVerification class and improve list of known standard extensions

yulian-gaponenko · yulian-gaponenko · commit 795ef5e10664 · 2019-04-01T13:03:46.000+03:00
It's wrong to perform the same checks of certificate extensions attributes for all certificates from the chain or keystore, such checks must depend on what certificate is currently inspected (e.g. root certificate, dig sig certificate, timestamp provider/crl client/ocsp responder certificate, etc). For example, KEY_USAGE extension attributes can be not only DIGITAL SIGNING or NONREPUDIATION for root certificates. Additionally we only had a very limited list of checks, which was also not correct in general case even for right certificates. Moreover those checks were inconsistent in Java: certificate extensions attributes check was only performed when there were some unsupported extensions according to '#hasUnsupportedCriticalExtension' method.

While such checks should probably be performed, we should make a separate investigation on this matter.

Also, list of known standard extensions is changed to correspond to RFC specifications. In Java some of the extensions were always allowed to be critical, regardless of '#hasUnsupportedCriticalExtension' method result. However one user recently experienced that this list was too restrictive and we extended it a bit. Such list extension was too case-specific and if we do this additional check after '#hasUnsupportedCriticalExtension', it makes sense to check for all standard extensions. For now in Java both '#hasUnsupportedCriticalExtension' method and standard extensions checks are performed.

DEVSIX-2616
diff --git a/sign/src/main/java/com/itextpdf/signatures/CertificateVerification.java b/sign/src/main/java/com/itextpdf/signatures/CertificateVerification.java
@@ -115,7 +115,7 @@ public static String verifyCertificate(X509Certificate cert, Collection<CRL> crl
      * @param certs    the certificate chain
      * @param keystore the <CODE>KeyStore</CODE>
      * @param crls     the certificate revocation list or <CODE>null</CODE>
-     * @return <CODE>null</CODE> if the certificate chain could be validated or a
+     * @return empty list if the certificate chain could be validated or a
      * <CODE>Object[]{cert,error}</CODE> where <CODE>cert</CODE> is the
      * failed certificate and <CODE>error</CODE> is the error message
      */
@@ -130,7 +130,7 @@ public static List<VerificationException> verifyCertificates(Certificate[] certs
      * @param keystore the <CODE>KeyStore</CODE>
      * @param crls     the certificate revocation list or <CODE>null</CODE>
      * @param calendar the date, shall not be null
-     * @return <CODE>null</CODE> if the certificate chain could be validated or a
+     * @return empty list if the certificate chain could be validated or a
      * <CODE>Object[]{cert,error}</CODE> where <CODE>cert</CODE> is the
      * failed certificate and <CODE>error</CODE> is the error message
      */
diff --git a/sign/src/main/java/com/itextpdf/signatures/OID.java b/sign/src/main/java/com/itextpdf/signatures/OID.java
@@ -42,6 +42,11 @@ This file is part of the iText (R) project.
  */
 package com.itextpdf.signatures;
 
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.LinkedHashSet;
+import java.util.Set;
+
 /**
  * Class containing all the OID values used by iText.
  */
@@ -55,9 +60,123 @@ private OID() {
      * Contains all OIDs used by iText in the context of Certificate Extensions.
      */
     public static final class X509Extensions {
+        /**
+         * One of the standard extensions from https://tools.ietf.org/html/rfc5280
+         * <p>
+         * "Conforming CAs MUST mark this extension as non-critical."
+         */
+        public static final String AUTHORITY_KEY_IDENTIFIER = "2.5.29.35";
+        /**
+         * One of the standard extensions from https://tools.ietf.org/html/rfc5280
+         * <p>
+         * "Conforming CAs MUST mark this extension as non-critical."
+         */
+        public static final String SUBJECT_KEY_IDENTIFIER = "2.5.29.14";
+        /**
+         * One of the standard extensions from https://tools.ietf.org/html/rfc5280
+         */
+        public static final String KEY_USAGE = "2.5.29.15";
+        /**
+         * One of the standard extensions from https://tools.ietf.org/html/rfc5280
+         */
+        public static final String CERTIFICATE_POLICIES = "2.5.29.32";
+        /**
+         * One of the standard extensions from https://tools.ietf.org/html/rfc5280
+         */
+        public static final String POLICY_MAPPINGS = "2.5.29.33";
+        /**
+         * One of the standard extensions from https://tools.ietf.org/html/rfc5280
+         */
+        public static final String SUBJECT_ALTERNATIVE_NAME = "2.5.29.17";
+        /**
+         * One of the standard extensions from https://tools.ietf.org/html/rfc5280
+         */
+        public static final String ISSUER_ALTERNATIVE_NAME = "2.5.29.18";
+        /**
+         * One of the standard extensions from https://tools.ietf.org/html/rfc5280
+         * <p>
+         * "Conforming CAs MUST mark this extension as non-critical."
+         */
+        public static final String SUBJECT_DIRECTORY_ATTRIBUTES = "2.5.29.9";
+        /**
+         * One of the standard extensions from https://tools.ietf.org/html/rfc5280
+         */
         public static final String BASIC_CONSTRAINTS = "2.5.29.19";
+        /**
+         * One of the standard extensions from https://tools.ietf.org/html/rfc5280
+         */
+        public static final String NAME_CONSTRAINTS = "2.5.29.30";
+        /**
+         * One of the standard extensions from https://tools.ietf.org/html/rfc5280
+         */
+        public static final String POLICY_CONSTRAINTS = "2.5.29.36";
+        /**
+         * One of the standard extensions from https://tools.ietf.org/html/rfc5280
+         */
         public static final String EXTENDED_KEY_USAGE = "2.5.29.37";
+        /**
+         * One of the standard extensions from https://tools.ietf.org/html/rfc5280
+         */
+        public static final String CRL_DISTRIBUTION_POINTS = "2.5.29.31";
+        /**
+         * One of the standard extensions from https://tools.ietf.org/html/rfc5280
+         */
+        public static final String INHIBIT_ANY_POLICY = "2.5.29.54";
+        /**
+         * One of the standard extensions from https://tools.ietf.org/html/rfc5280
+         * <p>
+         * "The extension MUST be marked as non-critical by conforming CAs."
+         */
+        public static final String FRESHEST_CRL = "2.5.29.46";
+
+        /**
+         * One of the Internet Certificate Extensions also from https://tools.ietf.org/html/rfc5280
+         * <p>
+         * "The extension MUST be marked as non-critical by conforming CAs."
+         */
+        public static final String AUTHORITY_INFO_ACCESS = "1.3.6.1.5.5.7.1.1";
+        /**
+         * One of the Internet Certificate Extensions also from https://tools.ietf.org/html/rfc5280
+         * <p>
+         * "Conforming CAs MUST mark this extension as non-critical."
+         */
+        public static final String SUBJECT_INFO_ACCESS = "1.3.6.1.5.5.7.1.11";
+
+
+        /**
+         * One of the {@link #EXTENDED_KEY_USAGE} purposes from https://www.ietf.org/rfc/rfc2459.txt
+         */
         public static final String ID_KP_TIMESTAMPING = "1.3.6.1.5.5.7.3.8";
-        public static final String KEY_USAGE = "2.5.29.15";
+
+
+        /**
+         * Extension for OCSP responder certificate
+         * from https://www.ietf.org/rfc/rfc2560.txt.
+         */
+        public static final String ID_PKIX_OCSP_NOCHECK = "1.3.6.1.5.5.7.48.1.5";
+
+
+        /**
+         * According to https://tools.ietf.org/html/rfc5280 4.2. "Certificate Extensions":
+         * "A certificate-using system MUST reject the certificate if it encounters a critical extension it
+         * does not recognize or a critical extension that contains information that it cannot process."
+         * <p>
+         * This set consists of standard extensions which are defined in RFC specifications and are not mentioned
+         * as forbidden to be marked as critical.
+         */
+        public static final Set<String> SUPPORTED_CRITICAL_EXTENSIONS = Collections.unmodifiableSet(new LinkedHashSet<>(Arrays.asList(
+                KEY_USAGE,
+                CERTIFICATE_POLICIES,
+                POLICY_MAPPINGS,
+                SUBJECT_ALTERNATIVE_NAME,
+                ISSUER_ALTERNATIVE_NAME,
+                BASIC_CONSTRAINTS,
+                NAME_CONSTRAINTS,
+                POLICY_CONSTRAINTS,
+                EXTENDED_KEY_USAGE,
+                CRL_DISTRIBUTION_POINTS,
+                INHIBIT_ANY_POLICY,
+                ID_PKIX_OCSP_NOCHECK
+        )));
     }
 }
diff --git a/sign/src/main/java/com/itextpdf/signatures/SignUtils.java b/sign/src/main/java/com/itextpdf/signatures/SignUtils.java
@@ -73,7 +73,6 @@ This file is part of the iText (R) project.
 import java.security.cert.CertificateEncodingException;
 import java.security.cert.CertificateException;
 import java.security.cert.CertificateFactory;
-import java.security.cert.CertificateParsingException;
 import java.security.cert.X509Certificate;
 import java.text.SimpleDateFormat;
 import java.util.ArrayList;
@@ -118,9 +117,6 @@ This file is part of the iText (R) project.
 final class SignUtils {
     static final Object UNDEFINED_TIMESTAMP_DATE = null;
 
-    private static final int KEY_USAGE_DIGITAL_SIGNATURE = 0;
-    private static final int KEY_USAGE_NON_REPUDIATION = 1;
-
     static String getPrivateKeyAlgorithm(PrivateKey pk) {
         String algorithm = pk.getAlgorithm();
 
@@ -298,35 +294,26 @@ static TsaResponse getTsaResponseForUserRequest(String tsaUrl, byte[] requestByt
      *
      * @param cert X509Certificate instance to check
      * @return true if there are unsupported critical extensions, false if there are none
+     * @deprecated this behavior is different in Java and .NET, because in Java we use this
+     * two-step check: first via #hasUnsupportedCriticalExtension method, and then additionally allowing
+     * standard critical extensions; in .NET there's only second step. However, removing
+     * first step in Java can be a breaking change for some users and moreover we don't
+     * have any means of providing customization for unsupported extensions check as of right now.
+     * <p>
+     * During major release I'd suggest changing java unsupported extensions check logic to the same as in .NET,
+     * but only if it is possible to customize this logic.
      */
+    @Deprecated
     static boolean hasUnsupportedCriticalExtension(X509Certificate cert) {
         if ( cert == null ) {
             throw new IllegalArgumentException("X509Certificate can't be null.");
         }
 
         if (cert.hasUnsupportedCriticalExtension()) {
             for (String oid : cert.getCriticalExtensionOIDs()) {
-                // KEY USAGE and DIGITAL SIGNING or NONREPUDIATION is ALLOWED
-                if (OID.X509Extensions.KEY_USAGE.equals(oid)) {
-                    if(cert.getKeyUsage()[KEY_USAGE_DIGITAL_SIGNATURE] || cert.getKeyUsage()[KEY_USAGE_NON_REPUDIATION]) { // allow digSig and nonRepudiation
-                        continue;
-                    }
-                }
-
-                // BASIC CONSTRAINTS is ALLOWED
-                if (OID.X509Extensions.BASIC_CONSTRAINTS.equals(oid)) { // allow basicConstraints, can be checked later
+                if (OID.X509Extensions.SUPPORTED_CRITICAL_EXTENSIONS.contains(oid)) {
                     continue;
                 }
-
-                try {
-                    // EXTENDED KEY USAGE and TIMESTAMPING is ALLOWED
-                    if (OID.X509Extensions.EXTENDED_KEY_USAGE.equals(oid) && cert.getExtendedKeyUsage().contains(OID.X509Extensions.ID_KP_TIMESTAMPING)) {
-                        continue;
-                    }
-                } catch (CertificateParsingException e) {
-                    // DO NOTHING;
-                }
-
                 return true;
             }
         }
@@ -392,7 +379,7 @@ private void tryToGetNextCertificate() {
                         while (keyStoreAliases.hasMoreElements()) {
                             try {
                                 String alias = keyStoreAliases.nextElement();
-                                if (keyStore.isCertificateEntry(alias)) {
+                                if (keyStore.isCertificateEntry(alias) || keyStore.isKeyEntry(alias)) {
                                     nextCert = (X509Certificate) keyStore.getCertificate(alias);
                                     break;
                                 }
diff --git a/sign/src/test/java/com/itextpdf/signatures/CertificateSupportedCriticalExtensionsTest.java b/sign/src/test/java/com/itextpdf/signatures/CertificateSupportedCriticalExtensionsTest.java
@@ -109,7 +109,7 @@ public void extendedKeyUsageWithoutIdKpTimestampingTest() {
                 .setExtendedKeyUsage("Not ID KP TIMESTAMPING.")
                 .setKeyUsage(true, true);
 
-        Assert.assertTrue(SignUtils.hasUnsupportedCriticalExtension(cert));
+        Assert.assertFalse(SignUtils.hasUnsupportedCriticalExtension(cert));
     }
 
     @Test
diff --git a/sign/src/test/java/com/itextpdf/signatures/sign/PadesSigTest.java b/sign/src/test/java/com/itextpdf/signatures/sign/PadesSigTest.java
@@ -99,6 +99,13 @@ public void padesRsaSigTest01() throws IOException, GeneralSecurityException, TS
         basicCheckSignedDoc(destinationFolder + "padesRsaSigTest01.pdf", "Signature1");
     }
 
+    @Test
+    public void padesRsaSigTestWithChain01() throws IOException, GeneralSecurityException, TSPException, OperatorCreationException {
+        signApproval(certsSrc + "signCertRsaWithChain.p12", destinationFolder + "padesRsaSigTestWithChain01.pdf");
+
+        basicCheckSignedDoc(destinationFolder + "padesRsaSigTestWithChain01.pdf", "Signature1");
+    }
+
     @Test
     @Ignore("DEVSIX-1620: For some reason signatures created with the given cert (either by iText or acrobat) are considered invalid")
     public void padesDsaSigTest01() throws IOException, GeneralSecurityException, TSPException, OperatorCreationException {
diff --git a/sign/src/test/java/com/itextpdf/signatures/verify/CertificateVerificationClassTest.java b/sign/src/test/java/com/itextpdf/signatures/verify/CertificateVerificationClassTest.java
@@ -0,0 +1,95 @@
+/*
+    This file is part of the iText (R) project.
+    Copyright (c) 1998-2019 iText Group NV
+    Authors: iText Software.
+
+    This program is free software; you can redistribute it and/or modify
+    it under the terms of the GNU Affero General Public License version 3
+    as published by the Free Software Foundation with the addition of the
+    following permission added to Section 15 as permitted in Section 7(a):
+    FOR ANY PART OF THE COVERED WORK IN WHICH THE COPYRIGHT IS OWNED BY
+    ITEXT GROUP. ITEXT GROUP DISCLAIMS THE WARRANTY OF NON INFRINGEMENT
+    OF THIRD PARTY RIGHTS
+
+    This program 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 Affero General Public License for more details.
+    You should have received a copy of the GNU Affero General Public License
+    along with this program; if not, see http://www.gnu.org/licenses or write to
+    the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
+    Boston, MA, 02110-1301 USA, or download the license from the following URL:
+    http://itextpdf.com/terms-of-use/
+
+    The interactive user interfaces in modified source and object code versions
+    of this program must display Appropriate Legal Notices, as required under
+    Section 5 of the GNU Affero General Public License.
+
+    In accordance with Section 7(b) of the GNU Affero General Public License,
+    a covered work must retain the producer line in every PDF that is created
+    or manipulated using iText.
+
+    You can be released from the requirements of the license by purchasing
+    a commercial license. Buying such a license is mandatory as soon as you
+    develop commercial activities involving the iText software without
+    disclosing the source code of your own applications.
+    These activities include: offering paid services to customers as an ASP,
+    serving PDFs on the fly in a web application, shipping iText with a closed
+    source product.
+
+    For more information, please contact iText Software Corp. at this
+    address: sales@itextpdf.com
+ */
+package com.itextpdf.signatures.verify;
+
+import com.itextpdf.signatures.CertificateVerification;
+import com.itextpdf.signatures.VerificationException;
+import com.itextpdf.signatures.testutils.Pkcs12FileHelper;
+import com.itextpdf.test.ExtendedITextTest;
+import com.itextpdf.test.ITextTest;
+import com.itextpdf.test.annotations.type.UnitTest;
+import java.io.IOException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.NoSuchProviderException;
+import java.security.Security;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateException;
+import java.util.List;
+import org.bouncycastle.jce.provider.BouncyCastleProvider;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+@Category(UnitTest.class)
+public class CertificateVerificationClassTest extends ExtendedITextTest {
+    private static final String certsSrc = "./src/test/resources/com/itextpdf/signatures/certs/";
+    private static final char[] password = "testpass".toCharArray();
+
+    @BeforeClass
+    public static void before() {
+        Security.addProvider(new BouncyCastleProvider());
+        ITextTest.removeCryptographyRestrictions();
+    }
+
+    @AfterClass
+    public static void after() {
+        ITextTest.restoreCryptographyRestrictions();
+    }
+
+    @Test
+    public void validCertificateChain01() throws CertificateException, NoSuchAlgorithmException, KeyStoreException, IOException, UnrecoverableKeyException, NoSuchProviderException {
+        Certificate[] certChain = Pkcs12FileHelper.readFirstChain(certsSrc + "signCertRsaWithChain.p12", password);
+
+        String caCertFileName = certsSrc + "rootRsa.p12";
+        KeyStore caKeyStore = Pkcs12FileHelper.initStore(caCertFileName, password);
+
+        List<VerificationException> verificationExceptions = CertificateVerification.verifyCertificates(certChain, caKeyStore);
+
+        Assert.assertTrue(verificationExceptions.isEmpty());
+    }
+}

Original file line number	Diff line number	Diff line change
`@@ -109,7 +109,7 @@ public void extendedKeyUsageWithoutIdKpTimestampingTest() {`
`109`	`109`	`.setExtendedKeyUsage("Not ID KP TIMESTAMPING.")`
`110`	`110`	`.setKeyUsage(true, true);`
`111`	`111`
`112`		`- Assert.assertTrue(SignUtils.hasUnsupportedCriticalExtension(cert));`
	`112`	`+ Assert.assertFalse(SignUtils.hasUnsupportedCriticalExtension(cert));`
`113`	`113`	`}`
`114`	`114`
`115`	`115`	`@Test`