On Mon, Mar 06, 2023 at 02:50:14PM +0000, Job Snijders wrote:
> On Mon, Mar 06, 2023 at 12:27:36PM +0100, Theo Buehler wrote:
> > On Mon, Mar 06, 2023 at 10:52:31AM +0000, Job Snijders wrote:
> > > RFC 7935 states in section 3: "The RSA key pairs used to compute the
> > > signatures MUST have a 2048-bit modulus and a public exponent (e) of
> > > 65,537."
> > >
> > > The below adds a check for that.
> >
> > That's a good first step. See comments below.
>
> Compliance checks that follow from reading RFC 7935 section 3 can be
> applied in at least three places:
>
> 1) The SPKI inside a CA's .cer TBS must be RSA, with mod 2048 & (e) 0x10001
> 2) Signers wrapped in CMS must be RSA, with mod 2048 & (e) 0x10001
These two parts are ok modulo a comment in valid_ca_pkey(). I think this
already gives us what we want.
> 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.
Some comments/questions about this inline
>
> While (1) and (2) can conveniently share some code by passing the
> to-be-checked information as EVP_PKEY to valid_ca_pkey(); to fully check
> (3) we'd need to transform 'psig' (an ASN1_BIT_STRING) from
> X509_get0_signature() into an EVP_PKEY. I didn't see a super easy way to
> do that in libcrypto. Leaving it for now.
>
> Kind regards,
>
> Job
>
> Index: cert.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.102
> diff -u -p -r1.102 cert.c
> --- cert.c 21 Feb 2023 10:18:47 -0000 1.102
> +++ cert.c 6 Mar 2023 14:28:20 -0000
> @@ -647,8 +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)
> @@ -675,6 +679,22 @@ cert_parse_pre(const char *fn, const uns
> 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) {
Please use tabs for indentation.
RFC 7935 explicitly allows NID_rsaEncryption. I seem to recall it was an
issue in cms.c. Why is not an issue here?
> + warnx("%s: RFC 6488: wrong signature algorithm %s, want %s",
> + fn, OBJ_nid2ln(nid),
> + OBJ_nid2ln(NID_sha256WithRSAEncryption));
> + goto out;
> + }
tabs
> +
> + /* XXX: also check if the above RSA signature is mod 2048 (e) 0x10001 */
> +
> /* Look for X509v3 extensions. */
>
> if ((extsz = X509_get_ext_count(x)) < 0)
> @@ -747,6 +767,13 @@ cert_parse_pre(const char *fn, const uns
>
> switch (p.res->purpose) {
> case CERT_PURPOSE_CA:
> + if ((pkey = X509_get0_pubkey(x)) == NULL) {
> + warnx("%s: X509_get0_pubkey failed", p.fn);
> + goto out;
> + }
> + if (!valid_ca_pkey(p.fn, pkey))
> + goto out;
> +
> if (X509_get_key_usage(x) != (KU_KEY_CERT_SIGN | KU_CRL_SIGN)) {
> warnx("%s: RFC 6487 section 4.8.4: key usage violation",
> p.fn);
> Index: cms.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cms.c,v
> retrieving revision 1.28
> diff -u -p -r1.28 cms.c
> --- cms.c 6 Mar 2023 09:14:29 -0000 1.28
> +++ cms.c 6 Mar 2023 14:28:20 -0000
> @@ -76,6 +76,7 @@ cms_parse_validate_internal(X509 **xp, c
> STACK_OF(X509_CRL) *crls;
> STACK_OF(CMS_SignerInfo) *sinfos;
> CMS_SignerInfo *si;
> + EVP_PKEY *pkey;
> X509_ALGOR *pdig, *psig;
> int i, nattrs, nid;
> int has_ct = 0, has_md = 0, has_st = 0,
> @@ -184,7 +185,10 @@ cms_parse_validate_internal(X509 **xp, c
> }
>
> /* Check digest and signature algorithms */
> - CMS_SignerInfo_get0_algs(si, NULL, NULL, &pdig, &psig);
> + CMS_SignerInfo_get0_algs(si, &pkey, NULL, &pdig, &psig);
> + if (!valid_ca_pkey(fn, pkey))
> + goto out;
> +
> X509_ALGOR_get0(&obj, NULL, NULL, pdig);
> nid = OBJ_obj2nid(obj);
> if (nid != NID_sha256) {
> Index: extern.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.167
> diff -u -p -r1.167 extern.h
> --- extern.h 13 Jan 2023 08:58:36 -0000 1.167
> +++ extern.h 6 Mar 2023 14:28:20 -0000
> @@ -660,6 +660,7 @@ int valid_econtent_version(const char
> int valid_aspa(const char *, struct cert *, struct aspa *);
> int valid_geofeed(const char *, struct cert *, struct geofeed *);
> int valid_uuid(const char *);
> +int valid_ca_pkey(const char *, EVP_PKEY *);
>
> /* Working with CMS. */
> unsigned char *cms_parse_validate(X509 **, const char *,
> Index: validate.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/validate.c,v
> retrieving revision 1.54
> diff -u -p -r1.54 validate.c
> --- validate.c 18 Jan 2023 18:12:20 -0000 1.54
> +++ validate.c 6 Mar 2023 14:28:20 -0000
> @@ -588,3 +588,43 @@ valid_uuid(const char *s)
> }
> }
>
> +/*
> + * Validate whether the CA's public key (SPKI) conforms to RFC 7935.
> + * Returns 1 if valid, 0 otherwise.
> + */
> +int
> +valid_ca_pkey(const char *fn, EVP_PKEY *pkey)
> +{
> + RSA *rsa;
> + const BIGNUM *rsa_e;
> + int key_bits;
I am not entirely sure libcrypto guarantees pkey != NULL here in the
cert_parse_pre() path, we know it's true and I assume that CMS_verify()
success implies the existence of a pkey, but who really knows what
happens in the guts of cms/... I'd add a check that pkey != NULL here
otherwise we crash.
> +
> + if (EVP_PKEY_base_id(pkey) != EVP_PKEY_RSA) {
> + warnx("%s: Expected EVP_PKEY_RSA, got %d", fn,
> + EVP_PKEY_base_id(pkey));
> + return 0;
> + }
> +
> + if ((key_bits = EVP_PKEY_bits(pkey)) != 2048) {
> + warnx("%s: RFC 7935: expected 2048-bit modulus, got %d bits",
> + fn, key_bits);
> + return 0;
> + }
> +
> + if ((rsa = EVP_PKEY_get0_RSA(pkey)) == NULL) {
> + warnx("%s: failed to extract RSA public key", fn);
> + return 0;
> + }
> +
> + if ((rsa_e = RSA_get0_e(rsa)) == NULL) {
> + warnx("%s: failed to get RSA exponent", fn);
> + return 0;
> + }
> +
> + if (!BN_is_word(rsa_e, 65537)) {
> + warnx("%s: incorrect exponent (e) in RSA public key", fn);
> + return 0;
> + }
> +
> + return 1;
> +}
>
