From 81a4dcb74c8c8aa57c25d85d551bf436338e4745 Mon Sep 17 00:00:00 2001 From: kares Date: Sat, 16 Oct 2021 19:00:40 +0200 Subject: [PATCH 1/5] DRAFT: 'minimal' legacy multi-path cert verification an attempt to retrofit verification to consider alt-chains --- .../ext/openssl/x509store/StoreContext.java | 236 +++++++++++++++--- .../ext/openssl/x509store/X509Utils.java | 4 + 2 files changed, 205 insertions(+), 35 deletions(-) diff --git a/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java b/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java index 235fcc40..54187d82 100644 --- a/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java +++ b/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java @@ -37,7 +37,9 @@ import java.util.Collection; import java.util.Date; import java.util.HashSet; +import java.util.LinkedList; import java.util.List; +import java.util.ListIterator; import java.util.Set; import org.bouncycastle.asn1.ASN1InputStream; @@ -45,6 +47,8 @@ import org.bouncycastle.asn1.ASN1Sequence; import org.jruby.ext.openssl.SecurityHelper; +import static org.jruby.ext.openssl.x509store.X509Utils.*; + /** * c: X509_STORE_CTX * @@ -134,7 +138,7 @@ public Object getApplicationData() { /** * c: X509_STORE_CTX_get1_issuer */ - int getFirstIssuer(final X509AuxCertificate[] issuers, final X509AuxCertificate x) throws Exception { + int getFirstIssuer(final X509AuxCertificate[] _issuer, final X509AuxCertificate x) throws Exception { final Name xn = new Name( x.getIssuerX500Principal() ); final X509Object[] s_obj = new X509Object[1]; int ok = store == null ? 0 : getBySubject(X509Utils.X509_LU_X509, xn, s_obj); @@ -148,33 +152,96 @@ else if ( ok != X509Utils.X509_LU_FAIL ) { } return 0; } - + /* If certificate matches all OK */ X509Object obj = s_obj[0]; if ( checkIssued.call(this, x, ((Certificate) obj).cert) != 0 ) { - issuers[0] = ((Certificate) obj).cert; - return 1; + X509AuxCertificate issuer = ((Certificate) obj).cert; + if (x509_check_cert_time(issuer, -1)) { + _issuer[0] = issuer; + return 1; + } } List objects = store.getObjects(); int idx = X509Object.indexBySubject(objects, X509Utils.X509_LU_X509, xn); if ( idx == -1 ) return 0; + int ret = 0; /* Look through all matching certificates for a suitable issuer */ for ( int i = idx; i < objects.size(); i++ ) { final X509Object pobj = objects.get(i); if ( pobj.type() != X509Utils.X509_LU_X509 ) { - return 0; + break; // return 0 } final X509AuxCertificate x509 = ((Certificate) pobj).cert; if ( ! xn.equalTo( x509.getSubjectX500Principal() ) ) { - return 0; + break; // return 0 } if ( checkIssued.call(this, x, x509) != 0 ) { - issuers[0] = x509; - return 1; + _issuer[0] = x509; + ret = 1; + /* + * If times check, exit with match, otherwise keep looking. + * Leave last match in issuer so we return nearest + * match if no certificate time is OK. + */ + if (x509_check_cert_time(x509, -1)) break; // return 1; } } - return 0; + return ret; + } + + // NOTE: not based on OpenSSL - self invented (till JOSSL 1.1.1 port) + private int getValidIssuers(final X509AuxCertificate x, final List _issuers) + throws Exception { + final Name xn = new Name( x.getIssuerX500Principal() ); + final X509Object[] s_obj = new X509Object[1]; + int ok = store == null ? 0 : getBySubject(X509Utils.X509_LU_X509, xn, s_obj); + if ( ok != X509Utils.X509_LU_X509 ) { + if ( ok == X509Utils.X509_LU_RETRY ) { + X509Error.addError(X509Utils.X509_R_SHOULD_RETRY); + return -1; + } + else if ( ok != X509Utils.X509_LU_FAIL ) { + return -1; + } + return 0; + } + int ret = 0; + /* If certificate matches all OK */ + X509Object obj = s_obj[0]; + if ( checkIssued.call(this, x, ((Certificate) obj).cert) != 0 ) { + X509AuxCertificate issuer = ((Certificate) obj).cert; + if (x509_check_cert_time(issuer, -1)) { + _issuers.add(issuer); + ret = 1; + } + } + + List objects = store.getObjects(); + + int idx = X509Object.indexBySubject(objects, X509Utils.X509_LU_X509, xn); + if ( idx == -1 ) return ret; + + /* Look through all matching certificates for a suitable issuer */ + for ( int i = idx; i < objects.size(); i++ ) { + final X509Object pobj = objects.get(i); + if ( pobj.type() != X509Utils.X509_LU_X509 ) { + continue; + } + final X509AuxCertificate x509 = ((Certificate) pobj).cert; + if ( ! xn.equalTo( x509.getSubjectX500Principal() ) ) { + continue; + } + + if ( checkIssued.call(this, x, x509) != 0 ) { + if (x509_check_cert_time(x509, -1)) { + _issuers.add(x509); + ret = 1; + } + } + } + return ret; } public static List ensureAux(final Collection input) { @@ -658,10 +725,8 @@ public int verifyCertificate() throws Exception { // We use a temporary STACK so we can chop and hack at it - List sktmp = null; - if ( untrusted != null ) { - sktmp = new ArrayList(untrusted); - } + LinkedList sktmp = untrusted != null ? new LinkedList<>(untrusted) : null; + num = chain.size(); x = chain.get(num - 1); depth = verifyParameter.depth; @@ -671,7 +736,7 @@ public int verifyCertificate() throws Exception { if ( checkIssued.call(this, x, x) != 0 ) break; if ( sktmp != null ) { - xtmp = findIssuer(sktmp, x); + xtmp = findIssuer(sktmp, x, true); if ( xtmp != null ) { chain.add(xtmp); sktmp.remove(xtmp); @@ -725,6 +790,41 @@ public int verifyCertificate() throws Exception { } } // We now lookup certs from the certificate store + // JOSSL specific re-invention with semi alternate chain search + if (false && checkIssued.call(this, x, x) == 0) { // TODO + final int[] p_num = new int[] { num }; + + if (getIssuer == getFirstIssuer) { + LinkedList xtmps = new LinkedList<>(); + int ok = getValidIssuers(x, xtmps); + if ( ok < 0 ) return ok; // error + if ( ok != 0 ) { // at least one issuer for given name + while (!xtmps.isEmpty()) { + chain.add(x = xtmps.removeFirst()); + p_num[0] = num + 1; + + ok = finishChain(x, depth, p_num); + if ( ok < 0 ) return ok; // error + if ( ok == 1 ) { + x = chain.get(chain.size() - 1); + if (x509_check_cert_time(x, -1)) break; + } + // we're going to retry thus reset chain back: + int added = p_num[0] - num; + while (added-- > 0) chain.remove(chain.size() - 1); + } + } + } else { + int ok = finishChain(x, depth, p_num); + if ( ok < 0 ) return ok; // error + } + + if (p_num[0] != num) { + x = chain.get(chain.size() - 1); + } + num = p_num[0]; + } + // We now lookup certs from the certificate store for(;;) { // If we have enough, we break if ( depth < num ) break; @@ -801,6 +901,40 @@ public int verifyCertificate() throws Exception { return ok; } + // JOSSL specific re-invention with semi alternate chain search + private int finishChain(X509AuxCertificate x, final int depth, final int[] _num) + throws Exception { + X509AuxCertificate[] p_xtmp = new X509AuxCertificate[] { null }; + int num = _num[0]; + for(;;) { + // If we have enough, we break + if ( depth < num ) break; + // If we are self signed, we break + if ( checkIssued.call(this, x, x) != 0 ) break; + + int ok = getIssuer.call(this, p_xtmp, x); + if ( ok < 0 ) return ok; // error + if ( ok == 0 ) return 0; // no issuer for given name + + x = p_xtmp[0]; + + chain.add(x); + num++; + } + + _num[0] = num; + return 1; + } + + public X509AuxCertificate findIssuer(final List certs, + final X509AuxCertificate cert, final boolean check_time) throws Exception { + for ( X509AuxCertificate issuer : certs ) { + if ( checkIssued.call(this, cert, issuer) != 0 ) { + if (!check_time || x509_check_cert_time(issuer, -1)) return issuer; + } + } + return null; + } private final static Set CRITICAL_EXTENSIONS = new HashSet(8); static { @@ -960,32 +1094,48 @@ public int checkTrust() throws Exception { return verifyCallback.call(this, ZERO); } - /** - * c: check_cert_time + /*- + * Check certificate validity times. + * If depth >= 0, invoke verification callbacks on error, otherwise just return + * the validation status. + * + * Return 1 on success, 0 otherwise. */ - public int checkCertificateTime(X509AuxCertificate x) throws Exception { + boolean x509_check_cert_time(X509AuxCertificate x, final int depth) throws Exception { final Date pTime; - if ( (verifyParameter.flags & X509Utils.V_FLAG_USE_CHECK_TIME) != 0 ) { - pTime = this.verifyParameter.checkTime; + if ((getParam().flags & V_FLAG_USE_CHECK_TIME) != 0) { + pTime = getParam().checkTime; + } else if ((getParam().flags & V_FLAG_NO_CHECK_TIME) != 0) { + return true; } else { pTime = Calendar.getInstance().getTime(); } - if ( ! x.getNotBefore().before(pTime) ) { - error = X509Utils.V_ERR_CERT_NOT_YET_VALID; - currentCertificate = x; - if ( verifyCallback.call(this, ZERO) == 0 ) { - return 0; - } + int i = x.getNotBefore().compareTo(pTime); + if (i >= 0 && depth < 0) { + + return false; } - if ( ! x.getNotAfter().after(pTime) ) { - error = X509Utils.V_ERR_CERT_HAS_EXPIRED; - currentCertificate = x; - if ( verifyCallback.call(this, ZERO) == 0 ) { - return 0; - } + if (i == 0 && verify_cb_cert(x, depth, V_ERR_ERROR_IN_CERT_NOT_BEFORE_FIELD) == 0) { + return false; } - return 1; + if (i > 0 && verify_cb_cert(x, depth, V_ERR_CERT_NOT_YET_VALID) == 0) { + return false; + } + + i = x.getNotAfter().compareTo(pTime); + if (i >= 0 && depth < 0) { + + return false; + } + if (i == 0 && verify_cb_cert(x, depth, V_ERR_ERROR_IN_CERT_NOT_AFTER_FIELD) == 0) { + return false; + } + if (i < 0 && verify_cb_cert(x, depth, V_ERR_CERT_HAS_EXPIRED) == 0) { + return false; + } + + return true; } /** @@ -1068,6 +1218,22 @@ public int getCRLStack(X509CRL[] pcrl, Name name, List crls) throws Exc return 0; } + /* + * Inform the verify callback of an error. + * If B is not NULL it is the error cert, otherwise use the chain cert at + * B. + * If B is not X509_V_OK, that's the error value, otherwise leave + * unchanged (presumably set by the caller). + * + * Returns 0 to abort verification with an error, non-zero to continue. + */ + private int verify_cb_cert(X509AuxCertificate x, int depth, int err) throws Exception { + this.errorDepth = depth; + this.currentCertificate = x != null ? x : chain.get(depth); + if (err != V_OK) this.error = err; + return verifyCallback.call(this, 0); // ctx->verify_cb(0, ctx) + } + final static Store.GetIssuerFunction getFirstIssuer = new Store.GetIssuerFunction() { public int call(StoreContext context, X509AuxCertificate[] issuer, X509AuxCertificate cert) throws Exception { return context.getFirstIssuer(issuer, cert); @@ -1079,7 +1245,7 @@ public int call(StoreContext context, X509AuxCertificate[] issuer, X509AuxCertif */ final static Store.GetIssuerFunction getIssuerStack = new Store.GetIssuerFunction() { public int call(StoreContext context, X509AuxCertificate[] issuer, X509AuxCertificate x) throws Exception { - issuer[0] = context.findIssuer(context.otherContext, x); + issuer[0] = context.findIssuer(context.otherContext, x, false); if ( issuer[0] != null ) { return 1; } else { @@ -1169,8 +1335,8 @@ public int call(final StoreContext context) throws Exception { } xs.setValid(true); - ok = context.checkCertificateTime(xs); - if ( ok == 0 ) return ok; + + if (!context.x509_check_cert_time(xs, n)) return 0; context.currentIssuer = xi; context.currentCertificate = xs; diff --git a/src/main/java/org/jruby/ext/openssl/x509store/X509Utils.java b/src/main/java/org/jruby/ext/openssl/x509store/X509Utils.java index a1980e8e..d2934e81 100644 --- a/src/main/java/org/jruby/ext/openssl/x509store/X509Utils.java +++ b/src/main/java/org/jruby/ext/openssl/x509store/X509Utils.java @@ -427,6 +427,10 @@ else if (maybeCertFile != null && new File(maybeCertFile).exists()) { public static final int V_FLAG_INHIBIT_MAP = 0x400; public static final int V_FLAG_NOTIFY_POLICY = 0x800; + + /* Do not check certificate/CRL validity against current time */ + public static final int V_FLAG_NO_CHECK_TIME = 0x200000; + public static final int VP_FLAG_DEFAULT = 0x1; public static final int VP_FLAG_OVERWRITE = 0x2; public static final int VP_FLAG_RESET_FLAGS = 0x4; From 1b9d2345944870bfebf7bf2576f8f12a22879964 Mon Sep 17 00:00:00 2001 From: kares Date: Sun, 17 Oct 2021 13:05:50 +0200 Subject: [PATCH 2/5] TEMP --- .../org/jruby/ext/openssl/x509store/StoreContext.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java b/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java index 54187d82..2d67d675 100644 --- a/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java +++ b/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java @@ -156,10 +156,10 @@ else if ( ok != X509Utils.X509_LU_FAIL ) { X509Object obj = s_obj[0]; if ( checkIssued.call(this, x, ((Certificate) obj).cert) != 0 ) { X509AuxCertificate issuer = ((Certificate) obj).cert; - if (x509_check_cert_time(issuer, -1)) { + //if (x509_check_cert_time(issuer, -1)) { TODO back _issuer[0] = issuer; return 1; - } + //} } List objects = store.getObjects(); @@ -172,10 +172,12 @@ else if ( ok != X509Utils.X509_LU_FAIL ) { final X509Object pobj = objects.get(i); if ( pobj.type() != X509Utils.X509_LU_X509 ) { break; // return 0 + //return 0; } final X509AuxCertificate x509 = ((Certificate) pobj).cert; if ( ! xn.equalTo( x509.getSubjectX500Principal() ) ) { break; // return 0 + //return 0; } if ( checkIssued.call(this, x, x509) != 0 ) { _issuer[0] = x509; @@ -185,7 +187,8 @@ else if ( ok != X509Utils.X509_LU_FAIL ) { * Leave last match in issuer so we return nearest * match if no certificate time is OK. */ - if (x509_check_cert_time(x509, -1)) break; // return 1; + //if (x509_check_cert_time(x509, -1)) break; // return 1; TODO back + return 1; } } return ret; From f6cf4765e10194b414ec9825e3a9457054c80bd3 Mon Sep 17 00:00:00 2001 From: kares Date: Sun, 17 Oct 2021 13:07:16 +0200 Subject: [PATCH 3/5] revert temporary (debugging) hacks --- .../org/jruby/ext/openssl/x509store/StoreContext.java | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java b/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java index 2d67d675..fda38b85 100644 --- a/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java +++ b/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java @@ -156,10 +156,10 @@ else if ( ok != X509Utils.X509_LU_FAIL ) { X509Object obj = s_obj[0]; if ( checkIssued.call(this, x, ((Certificate) obj).cert) != 0 ) { X509AuxCertificate issuer = ((Certificate) obj).cert; - //if (x509_check_cert_time(issuer, -1)) { TODO back + if (x509_check_cert_time(issuer, -1)) { _issuer[0] = issuer; return 1; - //} + } } List objects = store.getObjects(); @@ -172,12 +172,10 @@ else if ( ok != X509Utils.X509_LU_FAIL ) { final X509Object pobj = objects.get(i); if ( pobj.type() != X509Utils.X509_LU_X509 ) { break; // return 0 - //return 0; } final X509AuxCertificate x509 = ((Certificate) pobj).cert; if ( ! xn.equalTo( x509.getSubjectX500Principal() ) ) { break; // return 0 - //return 0; } if ( checkIssued.call(this, x, x509) != 0 ) { _issuer[0] = x509; @@ -187,8 +185,7 @@ else if ( ok != X509Utils.X509_LU_FAIL ) { * Leave last match in issuer so we return nearest * match if no certificate time is OK. */ - //if (x509_check_cert_time(x509, -1)) break; // return 1; TODO back - return 1; + if (x509_check_cert_time(x509, -1)) break; } } return ret; From e70148c1587dff4d975f6608d6531b8c6379c317 Mon Sep 17 00:00:00 2001 From: kares Date: Sun, 17 Oct 2021 13:08:11 +0200 Subject: [PATCH 4/5] [fix] SIGHT the usual < > copy pasta mess! --- .../java/org/jruby/ext/openssl/x509store/StoreContext.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java b/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java index fda38b85..fa6340fb 100644 --- a/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java +++ b/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java @@ -1113,7 +1113,6 @@ boolean x509_check_cert_time(X509AuxCertificate x, final int depth) throws Excep int i = x.getNotBefore().compareTo(pTime); if (i >= 0 && depth < 0) { - return false; } if (i == 0 && verify_cb_cert(x, depth, V_ERR_ERROR_IN_CERT_NOT_BEFORE_FIELD) == 0) { @@ -1124,8 +1123,7 @@ boolean x509_check_cert_time(X509AuxCertificate x, final int depth) throws Excep } i = x.getNotAfter().compareTo(pTime); - if (i >= 0 && depth < 0) { - + if (i <= 0 && depth < 0) { return false; } if (i == 0 && verify_cb_cert(x, depth, V_ERR_ERROR_IN_CERT_NOT_AFTER_FIELD) == 0) { From 8c04b7102f049ae9311812d3d08c3b338241fb7d Mon Sep 17 00:00:00 2001 From: kares Date: Sun, 17 Oct 2021 14:11:39 +0200 Subject: [PATCH 5/5] fix error reporting compat with multi issuers in case or a store.verify failure - report error as before --- .../ext/openssl/x509store/StoreContext.java | 59 +++++++++---------- 1 file changed, 27 insertions(+), 32 deletions(-) diff --git a/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java b/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java index fa6340fb..d9d608d4 100644 --- a/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java +++ b/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java @@ -191,8 +191,8 @@ else if ( ok != X509Utils.X509_LU_FAIL ) { return ret; } - // NOTE: not based on OpenSSL - self invented (till JOSSL 1.1.1 port) - private int getValidIssuers(final X509AuxCertificate x, final List _issuers) + // NOTE: JOSSL specific - semi alternate chain search (drop once OpenSSL 1.1.1 verify ported) + private int getValidIssuersOrFirst(final X509AuxCertificate x, final List _issuers) throws Exception { final Name xn = new Name( x.getIssuerX500Principal() ); final X509Object[] s_obj = new X509Object[1]; @@ -223,6 +223,10 @@ else if ( ok != X509Utils.X509_LU_FAIL ) { int idx = X509Object.indexBySubject(objects, X509Utils.X509_LU_X509, xn); if ( idx == -1 ) return ret; + // Leave last match in issuer so we return nearest match if no certificate time is OK. + // NOTE: due store.error reporting compatibility (with plain-old getFirstIssuer) + X509AuxCertificate firstTimeCheckFailed = null; + /* Look through all matching certificates for a suitable issuer */ for ( int i = idx; i < objects.size(); i++ ) { final X509Object pobj = objects.get(i); @@ -238,9 +242,16 @@ else if ( ok != X509Utils.X509_LU_FAIL ) { if (x509_check_cert_time(x509, -1)) { _issuers.add(x509); ret = 1; + } else { + if (firstTimeCheckFailed == null) firstTimeCheckFailed = x509; + ret = 1; } } } + + if (firstTimeCheckFailed != null && _issuers.isEmpty()) { + _issuers.add(firstTimeCheckFailed); + } return ret; } @@ -790,17 +801,17 @@ public int verifyCertificate() throws Exception { } } // We now lookup certs from the certificate store - // JOSSL specific re-invention with semi alternate chain search - if (false && checkIssued.call(this, x, x) == 0) { // TODO - final int[] p_num = new int[] { num }; + // NOTE: JOSSL specific - semi alternate chain search (drop once OpenSSL 1.1.1 verify ported) + if ( checkIssued.call(this, x, x) == 0 ) { + final int[] p_num = new int[] { num }; if (getIssuer == getFirstIssuer) { - LinkedList xtmps = new LinkedList<>(); - int ok = getValidIssuers(x, xtmps); + ArrayList xtmps = new ArrayList<>(4); + int ok = getValidIssuersOrFirst(x, xtmps); if ( ok < 0 ) return ok; // error if ( ok != 0 ) { // at least one issuer for given name - while (!xtmps.isEmpty()) { - chain.add(x = xtmps.removeFirst()); + for (int t = 0; t < xtmps.size(); t++) { + chain.add(x = xtmps.get(t)); p_num[0] = num + 1; ok = finishChain(x, depth, p_num); @@ -809,9 +820,12 @@ public int verifyCertificate() throws Exception { x = chain.get(chain.size() - 1); if (x509_check_cert_time(x, -1)) break; } - // we're going to retry thus reset chain back: - int added = p_num[0] - num; - while (added-- > 0) chain.remove(chain.size() - 1); + + if (t < xtmps.size() - 1) { + // did not break - going to retry thus reset chain back : + int added = p_num[0] - num; + while (added-- > 0) chain.remove(chain.size() - 1); + } } } } else { @@ -824,25 +838,6 @@ public int verifyCertificate() throws Exception { } num = p_num[0]; } - // We now lookup certs from the certificate store - for(;;) { - // If we have enough, we break - if ( depth < num ) break; - //xn = new X509_NAME(x.getIssuerX500Principal()); - // If we are self signed, we break - if ( checkIssued.call(this, x, x) != 0 ) break; - - X509AuxCertificate[] p_xtmp = new X509AuxCertificate[]{ xtmp }; - int ok = getIssuer.call(this, p_xtmp, x); - xtmp = p_xtmp[0]; - - if ( ok < 0 ) return ok; - if ( ok == 0 ) break; - - x = xtmp; - chain.add(x); - num++; - } /* we now have our chain, lets check it... */ @@ -901,7 +896,7 @@ public int verifyCertificate() throws Exception { return ok; } - // JOSSL specific re-invention with semi alternate chain search + // NOTE: JOSSL specific - semi alternate chain search (drop once OpenSSL 1.1.1 verify ported) private int finishChain(X509AuxCertificate x, final int depth, final int[] _num) throws Exception { X509AuxCertificate[] p_xtmp = new X509AuxCertificate[] { null };