| Message ID | 20221214003401.4086781-4-eric.snowberg@oracle.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Add CA enforcement keyring restrictions | expand |
> diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h > index a299c9c56f40..7c5c0ad1c22e 100644 > --- a/crypto/asymmetric_keys/x509_parser.h > +++ b/crypto/asymmetric_keys/x509_parser.h > @@ -38,6 +38,7 @@ struct x509_certificate { > bool self_signed; /* T if self-signed (check unsupported_sig too) */ > bool unsupported_sig; /* T if signature uses unsupported crypto */ > bool blacklisted; > + bool root_ca; /* T if basic constraints CA is set */ > }; The variable "root_ca" should probably be renamed to just "ca", right?
On Tue, Dec 13, 2022 at 07:33:54PM -0500, Eric Snowberg wrote: > Parse the X.509 Basic Constraints. The basic constraints extension > identifies whether the subject of the certificate is a CA. > > BasicConstraints ::= SEQUENCE { > cA BOOLEAN DEFAULT FALSE, > pathLenConstraint INTEGER (0..MAX) OPTIONAL } > > If the CA is true, store it in the x509_certificate. This will be used > in a follow on patch that requires knowing if the public key is a CA. Please add: Link: https://www.rfc-editor.org/rfc/rfc5280 # 4.2.1.9. Basic Constraints > Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com> > --- > crypto/asymmetric_keys/x509_cert_parser.c | 9 +++++++++ > crypto/asymmetric_keys/x509_parser.h | 1 + > 2 files changed, 10 insertions(+) > > diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c > index 7a9b084e2043..b4443e507153 100644 > --- a/crypto/asymmetric_keys/x509_cert_parser.c > +++ b/crypto/asymmetric_keys/x509_cert_parser.c > @@ -586,6 +586,15 @@ int x509_process_extension(void *context, size_t hdrlen, > return 0; > } > > + if (ctx->last_oid == OID_basicConstraints) { > + if (vlen < 2 || v[0] != (ASN1_CONS_BIT | ASN1_SEQ)) > + return -EBADMSG; > + if (v[1] != vlen - 2) > + return -EBADMSG; Why this instead of either: 1. Each check in separate if-statement. 2. All in a single statement: vlen < 2 || v[0] != (ASN1_CONS_BIT | ASN1_SEQ) || v[1] != vlen - 2 It would be also nice to have some sort of explanation in a comment, given the cryptic statement and the amount of magic numbers in it. I.e. in plain English what does the check actually means. > + if (vlen >= 4 && v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1) > + ctx->cert->root_ca = true; Ditto for the explanation part. I have really hard time deciphering this. > + } > + > return 0; > } > > diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h > index a299c9c56f40..7c5c0ad1c22e 100644 > --- a/crypto/asymmetric_keys/x509_parser.h > +++ b/crypto/asymmetric_keys/x509_parser.h > @@ -38,6 +38,7 @@ struct x509_certificate { > bool self_signed; /* T if self-signed (check unsupported_sig too) */ > bool unsupported_sig; /* T if signature uses unsupported crypto */ > bool blacklisted; > + bool root_ca; /* T if basic constraints CA is set */ > }; > > /* > -- > 2.27.0 > BR, Jarkko
On Thu, Dec 15, 2022 at 06:10:04AM -0500, Mimi Zohar wrote: > > diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h > > index a299c9c56f40..7c5c0ad1c22e 100644 > > --- a/crypto/asymmetric_keys/x509_parser.h > > +++ b/crypto/asymmetric_keys/x509_parser.h > > @@ -38,6 +38,7 @@ struct x509_certificate { > > bool self_signed; /* T if self-signed (check unsupported_sig too) */ > > bool unsupported_sig; /* T if signature uses unsupported crypto */ > > bool blacklisted; > > + bool root_ca; /* T if basic constraints CA is set */ > > }; > > The variable "root_ca" should probably be renamed to just "ca", right? Perhaps is_ca? BR, Jarkko
> On Jan 4, 2023, at 5:29 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote: > > On Thu, Dec 15, 2022 at 06:10:04AM -0500, Mimi Zohar wrote: >>> diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h >>> index a299c9c56f40..7c5c0ad1c22e 100644 >>> --- a/crypto/asymmetric_keys/x509_parser.h >>> +++ b/crypto/asymmetric_keys/x509_parser.h >>> @@ -38,6 +38,7 @@ struct x509_certificate { >>> bool self_signed; /* T if self-signed (check unsupported_sig too) */ >>> bool unsupported_sig; /* T if signature uses unsupported crypto */ >>> bool blacklisted; >>> + bool root_ca; /* T if basic constraints CA is set */ >>> }; >> >> The variable "root_ca" should probably be renamed to just "ca", right? > > Perhaps is_ca? I am open to renaming this, but need an agreement on whether the “is_” should be used or not: https://lore.kernel.org/lkml/b28ea211d88e968a5487b20477236e9b507755f4.camel@linux.ibm.com/
On Wed, 2023-01-04 at 20:14 +0000, Eric Snowberg wrote: > > > On Jan 4, 2023, at 5:29 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote: > > > > On Thu, Dec 15, 2022 at 06:10:04AM -0500, Mimi Zohar wrote: > >>> diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h > >>> index a299c9c56f40..7c5c0ad1c22e 100644 > >>> --- a/crypto/asymmetric_keys/x509_parser.h > >>> +++ b/crypto/asymmetric_keys/x509_parser.h > >>> @@ -38,6 +38,7 @@ struct x509_certificate { > >>> bool self_signed; /* T if self-signed (check unsupported_sig too) */ > >>> bool unsupported_sig; /* T if signature uses unsupported crypto */ > >>> bool blacklisted; > >>> + bool root_ca; /* T if basic constraints CA is set */ > >>> }; > >> > >> The variable "root_ca" should probably be renamed to just "ca", right? > > > > Perhaps is_ca? > > I am open to renaming this, but need an agreement on whether the “is_” should be used or not: > > https://lore.kernel.org/lkml/b28ea211d88e968a5487b20477236e9b507755f4.camel@linux.ibm.com/ Examples of both functions and variables exist that are prefixed with "is_". One is a question; the other a statement. Naming the variable "is_ca" and using it like "if (cert->is_ca)" does make sense.
diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c index 7a9b084e2043..b4443e507153 100644 --- a/crypto/asymmetric_keys/x509_cert_parser.c +++ b/crypto/asymmetric_keys/x509_cert_parser.c @@ -586,6 +586,15 @@ int x509_process_extension(void *context, size_t hdrlen, return 0; } + if (ctx->last_oid == OID_basicConstraints) { + if (vlen < 2 || v[0] != (ASN1_CONS_BIT | ASN1_SEQ)) + return -EBADMSG; + if (v[1] != vlen - 2) + return -EBADMSG; + if (vlen >= 4 && v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1) + ctx->cert->root_ca = true; + } + return 0; } diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h index a299c9c56f40..7c5c0ad1c22e 100644 --- a/crypto/asymmetric_keys/x509_parser.h +++ b/crypto/asymmetric_keys/x509_parser.h @@ -38,6 +38,7 @@ struct x509_certificate { bool self_signed; /* T if self-signed (check unsupported_sig too) */ bool unsupported_sig; /* T if signature uses unsupported crypto */ bool blacklisted; + bool root_ca; /* T if basic constraints CA is set */ }; /*
Parse the X.509 Basic Constraints. The basic constraints extension identifies whether the subject of the certificate is a CA. BasicConstraints ::= SEQUENCE { cA BOOLEAN DEFAULT FALSE, pathLenConstraint INTEGER (0..MAX) OPTIONAL } If the CA is true, store it in the x509_certificate. This will be used in a follow on patch that requires knowing if the public key is a CA. Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com> --- crypto/asymmetric_keys/x509_cert_parser.c | 9 +++++++++ crypto/asymmetric_keys/x509_parser.h | 1 + 2 files changed, 10 insertions(+)