Re: rpki-client: add check for RSA key pair modulus & public exponent (RFC 7935)
On Mon, Mar 06, 2023 at 02:50:14PM +, 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 +, 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.c21 Feb 2023 10:18:47 - 1.102
> +++ cert.c6 Mar 2023 14:28:20 -
> @@ -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, , x);
> + if (palg == NULL) {
> + cryptowarnx("%s: X509_get0_signature", p.fn);
> + goto out;
> + }
> + X509_ALGOR_get0(, 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 - 1.28
> +++ cms.c 6 Mar 2023 14:28:20 -
> @@ -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, , );
> + CMS_SignerInfo_get0_algs(si, , NULL, , );
> + if (!valid_ca_pkey(fn, pkey))
> + goto out;
> +
> X509_ALGOR_get0(, NULL, NULL, pdig);
> nid = OBJ_obj2nid(obj);
> if (nid !=
Re: rpki-client: add check for RSA key pair modulus & public exponent (RFC 7935)
On Mon, Mar 06, 2023 at 12:27:36PM +0100, Theo Buehler wrote:
> On Mon, Mar 06, 2023 at 10:52:31AM +, 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
3) Signatures (outside the TBS) in a .cer must be RSA (TODO: also check mod +
(e))
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 - 1.102
+++ cert.c 6 Mar 2023 14:28:20 -
@@ -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, , x);
+ if (palg == NULL) {
+ cryptowarnx("%s: X509_get0_signature", p.fn);
+ goto out;
+ }
+ X509_ALGOR_get0(, 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;
+}
+
+ /* 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 - 1.28
+++ cms.c 6 Mar 2023 14:28:20 -
@@ -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, , );
+ CMS_SignerInfo_get0_algs(si, , NULL, , );
+ if (!valid_ca_pkey(fn, pkey))
+ goto out;
+
X509_ALGOR_get0(, 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.h13 Jan 2023 08:58:36 - 1.167
+++ extern.h6 Mar 2023 14:28:20 -
@@ -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
Re: rpki-client: add check for RSA key pair modulus & public exponent (RFC 7935)
On Mon, Mar 06, 2023 at 10:52:31AM +, 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 - 1.28
> +++ cms.c 6 Mar 2023 10:50:33 -
> @@ -23,6 +23,7 @@
> #include
>
> #include
> +#include
> #include
>
> #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, , );
> + CMS_SignerInfo_get0_algs(si, , NULL, , );
> X509_ALGOR_get0(, 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, _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;
> }
>
>
