On Mon, Mar 06, 2023 at 10:52:31AM +0000, Job Snijders wrote:
> Hi,
>
> 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.
> OK?
>
> Kind regards,
>
> Job
>
> 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 10:50:33 -0000
> @@ -23,6 +23,7 @@
> #include <unistd.h>
>
> #include <openssl/bio.h>
> +#include <openssl/bn.h>
> #include <openssl/cms.h>
>
> #include "extern.h"
> @@ -76,10 +77,15 @@ 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;
> + RSA *rsa;
> + const BIGNUM *rsa_e;
> + BN_ULONG e_value;
> int i, nattrs, nid;
> int has_ct = 0, has_md = 0, has_st = 0,
> has_bst = 0;
> + int key_bits;
> int rc = 0;
>
> *xp = NULL;
> @@ -184,7 +190,7 @@ 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);
> X509_ALGOR_get0(&obj, NULL, NULL, pdig);
> nid = OBJ_obj2nid(obj);
> if (nid != NID_sha256) {
> @@ -198,6 +204,29 @@ cms_parse_validate_internal(X509 **xp, c
> if (nid != NID_rsaEncryption && nid != NID_sha256WithRSAEncryption) {
> warnx("%s: RFC 6488: wrong signature algorithm %s, want %s",
> fn, OBJ_nid2ln(nid), OBJ_nid2ln(NID_rsaEncryption));
> + goto out;
> + }
I think we should avoid making cms_parse_validate_internal() longer than
it already is. Since we anticipate that we need this check elsewhere,
perhaps it's better to split it into a helper from the start. How about
adding this to cms.c for now:
static int
validate_pkey(const EVP_PKEY *pkey)
{
}
so cms_parse_validate_internal() only gets a pkey variable and can do
if (!validate_pkey(pkey))
goto err;
This could be moved to validate.c later when we identify the other
places that need this check.
> + if ((key_bits = EVP_PKEY_bits(pkey)) <= 0) {
> + cryptowarnx("%s: failed to get cryptographic key length", fn);
> + goto out;
> + }
> + if (key_bits != 2048) {
> + warnx("%s: RFC 7935: expected 2048-bit modulus", fn);
> + goto out;
> + }
Merge the previous two checks for simplicity:
if ((key_bits = EVP_PKEY_bits(pkey)) != 2048) {
warnx("%s: RFC 7935: expected 2048-bit modulus, got %d bits",
fn, key_bits);
goto out;
}
> + if ((rsa = EVP_PKEY_get0_RSA(pkey)) == NULL) {
> + warnx("%s: failed to extract RSA public key", fn);
> + goto out;
> + }
> +
> + RSA_get0_key(rsa, NULL, &rsa_e, NULL);
I would prefer using the more explicit RSA_get0_e() (available since
LibreSSL 3.5, which is the minimum supported version in portable):
if ((rsa_e = RSA_get0_e(rsa)) == NULL) {
> + if (rsa_e == NULL) {
> + warnx("%s: failed to get RSA exponent", fn);
> + goto out;
> + }
> + e_value = BN_get_word(rsa_e);
> + if (e_value != 65537) {
No need for e_value:
if (!BN_is_word(rsa_e, 65537)) {
> + warnx("%s: incorrect exponent (e) in RSA public key", fn);
Should this error mention RFC 7935?
> goto out;
> }
>
>
