On Mon, Mar 06, 2023 at 04:35:05PM +0100, Theo Buehler wrote: > > 3) Signatures (outside the TBS) in a .cer must be RSA (TODO: also > > check mod + (e)) > > I'd prefer to skip this for now. This does not really buy us much, it > is independent and I see it as some polish that doesn't need to go in > at the same time.
OK, let's approach that part separately as per below. > Some comments/questions about this inline > > RFC 7935 explicitly allows NID_rsaEncryption. I seem to recall it was > an issue in cms.c. Why is not an issue here? I think there is a potential for confusion in that RFC 7935 differentiates between locations: 'in the certificate' and 'CMS SignedData'. In the CMS context both rsaEncryption or sha256WithRSAEncryption can appear (and both *do* appear in the wild). This is why we have to that that 'nid is NID_rsaEncryption or NID_sha256WithRSAEncryption' check in cms.c line 202. However, RFC 7935 section 2 fourth paragraph starting with "In certificates, CRLs, ..." mandates that sha256WithRSAEncryption is used: "The Object Identifier (OID) sha256WithRSAEncryption from [RFC4055] MUST be used in these products." I take that to mean that the algorithmIdentifier OID (outside the TBS) on .cer and .crl files MUST be sha256WithRSAEncryption; it seems all deployed RPKI CAs concluded the same. The below changeset adds a check to rpki-client to ensure that arbitrary .cer and .crl files were signed with the RFC-mandated sha256WithRSAEncryption algorithm. OK? Index: cert.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v retrieving revision 1.103 diff -u -p -r1.103 cert.c --- cert.c 6 Mar 2023 16:04:52 -0000 1.103 +++ cert.c 6 Mar 2023 16:28:41 -0000 @@ -647,9 +647,12 @@ cert_parse_pre(const char *fn, const uns size_t i; X509 *x = NULL; X509_EXTENSION *ext = NULL; + const X509_ALGOR *palg; + const ASN1_OBJECT *cobj; ASN1_OBJECT *obj; EVP_PKEY *pkey; struct parse p; + int nid; /* just fail for empty buffers, the warning was printed elsewhere */ if (der == NULL) @@ -673,6 +676,20 @@ cert_parse_pre(const char *fn, const uns /* Cache X509v3 extensions, see X509_check_ca(3). */ if (X509_check_purpose(x, -1, -1) <= 0) { cryptowarnx("%s: could not cache X509v3 extensions", p.fn); + goto out; + } + + /* RFC 7935 section 2 */ + X509_get0_signature(NULL, &palg, x); + if (palg == NULL) { + cryptowarnx("%s: X509_get0_signature", p.fn); + goto out; + } + X509_ALGOR_get0(&cobj, NULL, NULL, palg); + if ((nid = OBJ_obj2nid(cobj)) != NID_sha256WithRSAEncryption) { + warnx("%s: RFC 6488: wrong signature algorithm %s, want %s", + fn, OBJ_nid2ln(nid), + OBJ_nid2ln(NID_sha256WithRSAEncryption)); goto out; } Index: crl.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/crl.c,v retrieving revision 1.22 diff -u -p -r1.22 crl.c --- crl.c 21 Feb 2023 10:18:47 -0000 1.22 +++ crl.c 6 Mar 2023 16:28:41 -0000 @@ -20,6 +20,8 @@ #include <string.h> #include <unistd.h> +#include <openssl/x509.h> + #include "extern.h" struct crl * @@ -27,8 +29,10 @@ crl_parse(const char *fn, const unsigned { const unsigned char *oder; struct crl *crl; + const X509_ALGOR *palg; + const ASN1_OBJECT *cobj; const ASN1_TIME *at; - int rc = 0; + int nid, rc = 0; /* just fail for empty buffers, the warning was printed elsewhere */ if (der == NULL) @@ -44,6 +48,19 @@ crl_parse(const char *fn, const unsigned } if (der != oder + len) { warnx("%s: %td bytes trailing garbage", fn, oder + len - der); + goto out; + } + + X509_CRL_get0_signature(crl->x509_crl, NULL, &palg); + if (palg == NULL) { + cryptowarnx("%s: X509_CRL_get0_signature", fn); + goto out; + } + X509_ALGOR_get0(&cobj, NULL, NULL, palg); + if ((nid = OBJ_obj2nid(cobj)) != NID_sha256WithRSAEncryption) { + warnx("%s: RFC 6488: wrong signature algorithm %s, want %s", + fn, OBJ_nid2ln(nid), + OBJ_nid2ln(NID_sha256WithRSAEncryption)); goto out; }