Index | Thread | Search

From:
Theo Buehler <tb@theobuehler.org>
Subject:
Re: rpki-client: more complete signature algorithm checks
To:
tech@openbsd.org
Date:
Mon, 7 Jul 2025 07:11:38 +0200

Download raw body.

Thread
On Mon, Jul 07, 2025 at 06:48:19AM +0200, Theo Buehler wrote:
> So far we relied on the X.509 verifier to check that the two
> signature algorithm identifiers in a certificate are identical.
> Let's do it ourselves since this is very cheap (in well-formed
> certificates this is a few NULL checks plus comparing nid and
> type).
> 
> An algorithm identifier contains an OID and optional parameters.
> We've ignored the parameters so far, so let's check them. There is
> some fun ASN.1 history here so there are two possible encodings
> we need to accept for RSA, which is annoying. In fact, roughly
> 1600 ROAs issued by ARIN before end of 2020 have an EE cert with
> incorrect encoding.
> 
> The bulk of this code can be reused for CRLs, I'll factor this
> into a helper and do that as a follow-up.

And here's that follow-up for CRLs (which depends on the previous diff).

This uses the LibreSSL-specific X509_CRL_get0_tbs_sigalg() which I
sent to OpenSSL, but we'll need a little bit of plumbing in regress
and portable for this anyway.

As mentioned, this splits the bulk of cert_check_sigalg() out into
a helper called from both cert_check_sigalg() and crl_check_sigalg().

This also fixes a small gap in the CRL verification in that the two
algorithm identifiers in CRLs have not been checked to be identical
in the X.509 verifier so far (I'll fix this soon).

Another fun API consistency bit is how X509_get0_signature() has
the cert last while X509_CRL_get0_signature() has the crl first.
You can't make this stuff up.

diff --git a/regress/usr.sbin/rpki-client/openssl/unistd.h b/regress/usr.sbin/rpki-client/openssl/unistd.h
index 13b89aa431a..95735420c69 100644
--- a/regress/usr.sbin/rpki-client/openssl/unistd.h
+++ b/regress/usr.sbin/rpki-client/openssl/unistd.h
@@ -27,3 +27,12 @@ CMS_SignerInfo_get_version(CMS_SignerInfo *si, long *version)
 	*version = 3;
 	return 1;
 }
+
+static inline const X509_ALGOR *
+X509_CRL_get0_tbs_sigalg(const X509_CRL *crl)
+{
+	const X509_ALGOR *alg = NULL;
+
+	X509_CRL_get0_signature(crl, NULL, &alg);
+	return alg;
+}
diff --git a/usr.sbin/rpki-client/cert.c b/usr.sbin/rpki-client/cert.c
index b341f41b6cd..5b8c5faa4b2 100644
--- a/usr.sbin/rpki-client/cert.c
+++ b/usr.sbin/rpki-client/cert.c
@@ -1106,9 +1106,6 @@ cert_check_sigalg(const char *fn, const struct cert *cert)
 {
 	const X509		*x = cert->x509;
 	const X509_ALGOR	*alg = NULL, *tbsalg;
-	const ASN1_OBJECT	*aobj = NULL;
-	int			 ptype = 0;
-	int nid;
 
 	/* Retrieve AlgorithmIdentifiers from Certificate and TBSCertificate. */
 	X509_get0_signature(NULL, &alg, x);
@@ -1124,48 +1121,7 @@ cert_check_sigalg(const char *fn, const struct cert *cert)
 		return 0;
 	}
 
-	X509_ALGOR_get0(&aobj, &ptype, NULL, tbsalg);
-	if ((nid = OBJ_obj2nid(aobj)) == NID_undef) {
-		warnx("%s: unknown signature type", fn);
-		return 0;
-	}
-
-	if (nid == NID_sha256WithRSAEncryption) {
-		/*
-		 * Correct encoding of parameters is explicit ASN.1 NULL
-		 * (V_ASN1_NULL), but implementations MUST accept absent
-		 * parameters due to an ASN.1 syntax translation mishap,
-		 * see, e.g., RFC 4055, 2.1.
-		 */
-		if (ptype != V_ASN1_NULL && ptype != V_ASN1_UNDEF) {
-			warnx("%s: RFC 4055, 5: wrong ASN.1 parameters for %s",
-			    fn, LN_sha256WithRSAEncryption);
-			return 0;
-		}
-		/*
-		 * In July 2025, there were ~1600 EE certs in ROAs with this
-		 * faulty encoding, all issued by ARIN before September 2020.
-		 */
-		if (verbose > 1 && ptype == V_ASN1_UNDEF)
-			warnx("%s: RFC 4055, 5: %s without ASN.1 parameters",
-			    fn, LN_sha256WithRSAEncryption);
-		return 1;
-	}
-
-	if (experimental && nid == NID_ecdsa_with_SHA256) {
-		if (ptype != V_ASN1_UNDEF) {
-			warnx("%s: RFC 5758, 3.2: %s encoding MUST omit "
-			    "the parameters", fn, SN_ecdsa_with_SHA256);
-			return 0;
-		}
-		if (verbose)
-			warnx("%s: P-256 support is experimental", fn);
-		return 1;
-	}
-
-	warnx("%s: RFC 7935: wrong signature algorithm %s, want %s",
-	    fn, nid2str(nid), LN_sha256WithRSAEncryption);
-	return 0;
+	return x509_check_tbs_sigalg(fn, tbsalg);
 }
 
 static int
diff --git a/usr.sbin/rpki-client/crl.c b/usr.sbin/rpki-client/crl.c
index 2c78bcd6118..983b774dd17 100644
--- a/usr.sbin/rpki-client/crl.c
+++ b/usr.sbin/rpki-client/crl.c
@@ -172,6 +172,29 @@ crl_check_revoked(const char *fn, X509_CRL *x509_crl)
 	return 1;
 }
 
+static int
+crl_check_sigalg(const char *fn, const struct crl *crl)
+{
+	const X509_CRL		*x = crl->x509_crl;
+	const X509_ALGOR	*alg = NULL, *tbsalg;
+
+	/* Retrieve AlgorithmIdentifier from CertificateList and TBSCertList. */
+	X509_CRL_get0_signature(x, NULL, &alg);
+	if (alg == NULL || (tbsalg = X509_CRL_get0_tbs_sigalg(x)) == NULL) {
+		warnx("%s: missing signatureAlgorithm in certificate", fn);
+		return 0;
+	}
+
+	/* Unlike X509_verify(), X509_CRL_verify() does not check this. */
+	if (X509_ALGOR_cmp(alg, tbsalg) != 0) {
+		warnx("%s: RFC 5280, 5.1.1.2: signatureAlgorithm and signature "
+		    "AlgorithmIdentifier mismatch", fn);
+		return 0;
+	}
+
+	return x509_check_tbs_sigalg(fn, tbsalg);
+}
+
 struct crl *
 crl_parse(const char *fn, const unsigned char *der, size_t len)
 {
@@ -179,7 +202,7 @@ crl_parse(const char *fn, const unsigned char *der, size_t len)
 	struct crl		*crl;
 	const X509_NAME		*name;
 	const ASN1_TIME		*at;
-	int			 count, nid, rc = 0;
+	int			 count, rc = 0;
 
 	/* just fail for empty buffers, the warning was printed elsewhere */
 	if (der == NULL)
@@ -210,18 +233,8 @@ crl_parse(const char *fn, const unsigned char *der, size_t len)
 	if (!x509_valid_name(fn, "issuer", name))
 		goto out;
 
-	if ((nid = X509_CRL_get_signature_nid(crl->x509_crl)) == NID_undef) {
-		warnx("%s: unknown signature type", fn);
+	if (!crl_check_sigalg(fn, crl))
 		goto out;
-	}
-	if (experimental && nid == NID_ecdsa_with_SHA256) {
-		if (verbose)
-			warnx("%s: P-256 support is experimental", fn);
-	} else if (nid != NID_sha256WithRSAEncryption) {
-		warnx("%s: RFC 7935: wrong signature algorithm %s, want %s",
-		    fn, nid2str(nid), LN_sha256WithRSAEncryption);
-		goto out;
-	}
 
 	/*
 	 * RFC 6487, section 5: AKI and crlNumber MUST be present, no other
diff --git a/usr.sbin/rpki-client/extern.h b/usr.sbin/rpki-client/extern.h
index 34d4f2ec3e9..58177460633 100644
--- a/usr.sbin/rpki-client/extern.h
+++ b/usr.sbin/rpki-client/extern.h
@@ -958,6 +958,7 @@ char		*x509_convert_seqnum(const char *, const char *,
 		    const ASN1_INTEGER *);
 int		 x509_valid_seqnum(const char *, const char *,
 		    const ASN1_INTEGER *);
+int		 x509_check_tbs_sigalg(const char *, const X509_ALGOR *);
 int		 x509_location(const char *, const char *, GENERAL_NAME *,
 		    char **);
 int		 x509_inherits(X509 *);
diff --git a/usr.sbin/rpki-client/x509.c b/usr.sbin/rpki-client/x509.c
index 6243e7751ae..cf463b02240 100644
--- a/usr.sbin/rpki-client/x509.c
+++ b/usr.sbin/rpki-client/x509.c
@@ -971,6 +971,63 @@ x509_valid_seqnum(const char *fn, const char *descr, const ASN1_INTEGER *i)
 	return 1;
 }
 
+/*
+ * Helper to check the signature algorithm in the signed part of a cert
+ * or CRL matches expectations. Only accept sha256WithRSAEncryption by
+ * default and in experimental mode also accept ecdsa-with-SHA256.
+ * Check compliance of the parameter encoding as well.
+ */
+int
+x509_check_tbs_sigalg(const char *fn, const X509_ALGOR *tbsalg)
+{
+	const ASN1_OBJECT *aobj = NULL;
+	int ptype = 0;
+	int nid;
+
+	X509_ALGOR_get0(&aobj, &ptype, NULL, tbsalg);
+	if ((nid = OBJ_obj2nid(aobj)) == NID_undef) {
+		warnx("%s: unknown signature type", fn);
+		return 0;
+	}
+
+	if (nid == NID_sha256WithRSAEncryption) {
+		/*
+		 * Correct encoding of parameters is explicit ASN.1 NULL
+		 * (V_ASN1_NULL), but implementations MUST accept absent
+		 * parameters due to an ASN.1 syntax translation mishap,
+		 * see, e.g., RFC 4055, 2.1.
+		 */
+		if (ptype != V_ASN1_NULL && ptype != V_ASN1_UNDEF) {
+			warnx("%s: RFC 4055, 5: wrong ASN.1 parameters for %s",
+			    fn, LN_sha256WithRSAEncryption);
+			return 0;
+		}
+		/*
+		 * In July 2025, there were ~1600 EE certs in ROAs with this
+		 * faulty encoding, all issued by ARIN before September 2020.
+		 */
+		if (verbose > 1 && ptype == V_ASN1_UNDEF)
+			warnx("%s: RFC 4055, 5: %s without ASN.1 parameters",
+			    fn, LN_sha256WithRSAEncryption);
+		return 1;
+	}
+
+	if (experimental && nid == NID_ecdsa_with_SHA256) {
+		if (ptype != V_ASN1_UNDEF) {
+			warnx("%s: RFC 5758, 3.2: %s encoding MUST omit "
+			    "the parameters", fn, SN_ecdsa_with_SHA256);
+			return 0;
+		}
+		if (verbose)
+			warnx("%s: P-256 support is experimental", fn);
+		return 1;
+	}
+
+	warnx("%s: RFC 7935: wrong signature algorithm %s, want %s",
+	    fn, nid2str(nid), LN_sha256WithRSAEncryption);
+	return 0;
+}
+
 /*
  * Find the closest expiry moment by walking the chain of authorities.
  */