On Tue, Jan 18, 2022 at 06:46:35PM +0100, Theo Buehler wrote:
> On Tue, Jan 18, 2022 at 06:38:46PM +0100, Claudio Jeker wrote:
> > This is a follow up to the valid_x509() commit form earlier today.
> > tb@ suggested that the crl check should be grouped together.
> > After some thought I decided to do this all different.
> > First of all introduce a checkcrl flag which turns on
> > X509_V_FLAG_CRL_CHECK. This prevents code that expects a CRL to accept a
> > cert where the CRL is NULL. Apart from this build_crls(),
> > X509_STORE_CTX_set0_crls() and sk_X509_CRL_free() handle NULL inputs just
> > fine so drop the if (crl != NULL) check for them.
> >
> > I think this is better and more secure
>
> Yes, this is much better and easier to understand. I generally dislike
> such boolean flag values for functions (since you have no idea what they
> mean at the caller), but I think here it is for the better.
I was tempted to pass X509_V_FLAG_CRL_CHECK instead of 1. I should
probably do that instead. I agree that the 0 and 1 in random places of
functions is not very developer friendly.
> ok tb
>
> > @@ -361,7 +360,7 @@ proc_parser_mft(char *file, const unsign
> >
> > a = valid_ski_aki(file, &auths, mft->ski, mft->aki);
> >
>
> Perhaps it's worth reinstating the comment that was removed in an
> earlier commit?
I added that back.
> /* CRL checks disabled here because CRL is referenced from mft */
>
> > - if (!valid_x509(file, x509, a, NULL)) {
> > + if (!valid_x509(file, x509, a, NULL, 0)) {
> > mft_free(mft);
> > X509_free(x509);
> > return NULL;
>
I will commit this version in a bit (once rpki-client finished its run).
--
:wq Claudio
Index: parser.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
retrieving revision 1.43
diff -u -p -r1.43 parser.c
--- parser.c 18 Jan 2022 16:36:49 -0000 1.43
+++ parser.c 18 Jan 2022 18:03:37 -0000
@@ -204,15 +204,15 @@ verify_cb(int ok, X509_STORE_CTX *store_
* Returns 1 for valid certificates, returns 0 if there is a verify error
*/
static int
-valid_x509(char *file, X509 *x509, struct auth *a, struct crl *crl)
+valid_x509(char *file, X509 *x509, struct auth *a, struct crl *crl,
+ unsigned long flags)
{
STACK_OF(X509) *chain;
STACK_OF(X509_CRL) *crls = NULL;
int c;
build_chain(a, &chain);
- if (crl != NULL)
- build_crls(crl, &crls);
+ build_crls(crl, &crls);
assert(x509 != NULL);
if (!X509_STORE_CTX_init(ctx, NULL, x509, NULL))
@@ -221,12 +221,11 @@ valid_x509(char *file, X509 *x509, struc
X509_STORE_CTX_set_verify_cb(ctx, verify_cb);
if (!X509_STORE_CTX_set_app_data(ctx, file))
cryptoerrx("X509_STORE_CTX_set_app_data");
- if (crl != NULL)
- X509_STORE_CTX_set_flags(ctx, X509_V_FLAG_CRL_CHECK);
+ if (flags != 0)
+ X509_STORE_CTX_set_flags(ctx, flags);
X509_STORE_CTX_set_depth(ctx, MAX_CERT_DEPTH);
X509_STORE_CTX_set0_trusted_stack(ctx, chain);
- if (crl != NULL)
- X509_STORE_CTX_set0_crls(ctx, crls);
+ X509_STORE_CTX_set0_crls(ctx, crls);
if (X509_verify_cert(ctx) <= 0) {
c = X509_STORE_CTX_get_error(ctx);
@@ -262,7 +261,7 @@ proc_parser_roa(char *file, const unsign
a = valid_ski_aki(file, &auths, roa->ski, roa->aki);
crl = get_crl(a);
- if (!valid_x509(file, x509, a, crl)) {
+ if (!valid_x509(file, x509, a, crl, X509_V_FLAG_CRL_CHECK)) {
X509_free(x509);
roa_free(roa);
return NULL;
@@ -361,7 +360,8 @@ proc_parser_mft(char *file, const unsign
a = valid_ski_aki(file, &auths, mft->ski, mft->aki);
- if (!valid_x509(file, x509, a, NULL)) {
+ /* CRL checks disabled here because CRL is referenced from mft */
+ if (!valid_x509(file, x509, a, NULL, 0)) {
mft_free(mft);
X509_free(x509);
return NULL;
@@ -405,7 +405,7 @@ proc_parser_cert(char *file, const unsig
a = valid_ski_aki(file, &auths, cert->ski, cert->aki);
crl = get_crl(a);
- if (!valid_x509(file, cert->x509, a, crl)) {
+ if (!valid_x509(file, cert->x509, a, crl, X509_V_FLAG_CRL_CHECK)) {
cert_free(cert);
return NULL;
}
@@ -569,7 +569,7 @@ proc_parser_gbr(char *file, const unsign
crl = get_crl(a);
/* return value can be ignored since nothing happens here */
- valid_x509(file, x509, a, crl);
+ valid_x509(file, x509, a, crl, X509_V_FLAG_CRL_CHECK);
X509_free(x509);
gbr_free(gbr);
