subject:"Re\: LibreSSL\: handle EXFLAG

Re: LibreSSL: handle EXFLAG_INVALID

2021-03-13 Thread Theo Buehler

On Sat, Mar 13, 2021 at 09:20:32PM +0100, Tobias Heider wrote:
> On Wed, Mar 03, 2021 at 05:36:12PM +0100, Theo Buehler wrote:
> > On Thu, Feb 25, 2021 at 09:34:30PM +0100, Tobias Heider wrote:
> > > Hi,
> > > 
> > > while testing different x509 validator corner cases i found that a bunch 
> > > of
> > > errors are currently not handled in libcrypto.
> > > 
> > > In particular duplicate or undecodable extensions are ignored.
> > > The diff below sets EXFLAG_INVALID whenever X509_get_ext_d2i() returns
> > > an error (other than "not found") and later throws X509_V_ERR_UNSPECIFIED
> > > if EXFLAG_INVALID is set.
> > 
> > Yes, we want this in some form. Apart from the X509_vfy bit this is a
> > subset of the changes in
> > 
> > https://github.com/openssl/openssl/pull/10756
> > 
> > While I'd be happy to land this in a few smaller pieces, I think we want
> > most of those changes.  In particular, I think we want to check for
> > EXFLAG_INVALID after all calls to x509v3_cache_extensions().
> 
> Below is an updated diff with checks for EXFLAG_INVALID after all calls
> to x509v3_cache_extensions().  I agree that we should also port the other
> changes but i'd rather do it in a follow-up diff.

Sure.

> > 
> > I don't currently understand why your x509_vfy change was not done (and
> > why they did not add a check to check_ca()).
> 
> Also no idea, but they changed it again in:
> https://github.com/openssl/openssl/commit/33328581b83e8e9f573f08f0e2e0d6b32d095857

I see.

> With this, x509v3_cache_extensions() returns (x->ex_flags & EXFLAG_INVALID) 
> == 0
> and x509_check_ca() gets a check for the return value.

Yes, I was going to suggest something along these lines as a follow-up.
Having a void function which requires us to check for a flag is just
silly.

> This change also seems like something we should consider for the future imho.

We need to be a bit careful due to the licensing...  I don't think we
want this as a public interface if we can help it. However, changing the
signature of x509v3_cache_extensions() and absorbing the flag checks you
add here would be great after the rest of the changes from #10756 is
incorporated.

ok tb

> Index: x509/x509_purp.c
> ===
> RCS file: /cvs/src/lib/libcrypto/x509/x509_purp.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 x509_purp.c
> --- x509/x509_purp.c  13 Sep 2020 15:06:17 -  1.2
> +++ x509/x509_purp.c  13 Mar 2021 19:49:01 -
> @@ -132,6 +132,8 @@ X509_check_purpose(X509 *x, int id, int 
>   CRYPTO_w_lock(CRYPTO_LOCK_X509);
>   x509v3_cache_extensions(x);
>   CRYPTO_w_unlock(CRYPTO_LOCK_X509);
> + if (x->ex_flags & EXFLAG_INVALID)
> + return X509_V_ERR_UNSPECIFIED;
>   }
>   if (id == -1)
>   return 1;
> @@ -421,7 +423,12 @@ setup_crldp(X509 *x)
>  {
>   int i;
>  
> - x->crldp = X509_get_ext_d2i(x, NID_crl_distribution_points, NULL, NULL);
> + x->crldp = X509_get_ext_d2i(x, NID_crl_distribution_points, , NULL);
> + if (x->crldp == NULL && i != -1) {
> + x->ex_flags |= EXFLAG_INVALID;
> + return;
> + }
> +
>   for (i = 0; i < sk_DIST_POINT_num(x->crldp); i++)
>   setup_dp(x, sk_DIST_POINT_value(x->crldp, i));
>  }
> @@ -449,7 +456,7 @@ x509v3_cache_extensions(X509 *x)
>   x->ex_flags |= EXFLAG_V1;
>  
>   /* Handle basic constraints */
> - if ((bs = X509_get_ext_d2i(x, NID_basic_constraints, NULL, NULL))) {
> + if ((bs = X509_get_ext_d2i(x, NID_basic_constraints, , NULL))) {
>   if (bs->ca)
>   x->ex_flags |= EXFLAG_CA;
>   if (bs->pathlen) {
> @@ -463,10 +470,12 @@ x509v3_cache_extensions(X509 *x)
>   x->ex_pathlen = -1;
>   BASIC_CONSTRAINTS_free(bs);
>   x->ex_flags |= EXFLAG_BCONS;
> + } else if (i != -1) {
> + x->ex_flags |= EXFLAG_INVALID;
>   }
>  
>   /* Handle proxy certificates */
> - if ((pci = X509_get_ext_d2i(x, NID_proxyCertInfo, NULL, NULL))) {
> + if ((pci = X509_get_ext_d2i(x, NID_proxyCertInfo, , NULL))) {
>   if (x->ex_flags & EXFLAG_CA ||
>   X509_get_ext_by_NID(x, NID_subject_alt_name, -1) >= 0 ||
>   X509_get_ext_by_NID(x, NID_issuer_alt_name, -1) >= 0) {
> @@ -485,10 +494,12 @@ x509v3_cache_extensions(X509 *x)
>   x->ex_pcpathlen = -1;
>   PROXY_CERT_INFO_EXTENSION_free(pci);
>   x->ex_flags |= EXFLAG_PROXY;
> + } else if (i != -1) {
> + x->ex_flags |= EXFLAG_INVALID;
>   }
>  
>   /* Handle key usage */
> - if ((usage = X509_get_ext_d2i(x, NID_key_usage, NULL, NULL))) {
> + if ((usage = X509_get_ext_d2i(x, NID_key_usage, , NULL))) {
>   if (usage->length > 0) {
>   x->ex_kusage = usage->data[0];
>   if

Re: LibreSSL: handle EXFLAG_INVALID

2021-03-13 Thread Tobias Heider

On Wed, Mar 03, 2021 at 05:36:12PM +0100, Theo Buehler wrote:
> On Thu, Feb 25, 2021 at 09:34:30PM +0100, Tobias Heider wrote:
> > Hi,
> > 
> > while testing different x509 validator corner cases i found that a bunch of
> > errors are currently not handled in libcrypto.
> > 
> > In particular duplicate or undecodable extensions are ignored.
> > The diff below sets EXFLAG_INVALID whenever X509_get_ext_d2i() returns
> > an error (other than "not found") and later throws X509_V_ERR_UNSPECIFIED
> > if EXFLAG_INVALID is set.
> 
> Yes, we want this in some form. Apart from the X509_vfy bit this is a
> subset of the changes in
> 
> https://github.com/openssl/openssl/pull/10756
> 
> While I'd be happy to land this in a few smaller pieces, I think we want
> most of those changes.  In particular, I think we want to check for
> EXFLAG_INVALID after all calls to x509v3_cache_extensions().

Below is an updated diff with checks for EXFLAG_INVALID after all calls
to x509v3_cache_extensions().  I agree that we should also port the other
changes but i'd rather do it in a follow-up diff.

> 
> I don't currently understand why your x509_vfy change was not done (and
> why they did not add a check to check_ca()).

Also no idea, but they changed it again in:
https://github.com/openssl/openssl/commit/33328581b83e8e9f573f08f0e2e0d6b32d095857

With this, x509v3_cache_extensions() returns (x->ex_flags & EXFLAG_INVALID) == 0
and x509_check_ca() gets a check for the return value.
This change also seems like something we should consider for the future imho.

Index: x509/x509_purp.c
===
RCS file: /cvs/src/lib/libcrypto/x509/x509_purp.c,v
retrieving revision 1.2
diff -u -p -r1.2 x509_purp.c
--- x509/x509_purp.c13 Sep 2020 15:06:17 -  1.2
+++ x509/x509_purp.c13 Mar 2021 19:49:01 -
@@ -132,6 +132,8 @@ X509_check_purpose(X509 *x, int id, int 
CRYPTO_w_lock(CRYPTO_LOCK_X509);
x509v3_cache_extensions(x);
CRYPTO_w_unlock(CRYPTO_LOCK_X509);
+   if (x->ex_flags & EXFLAG_INVALID)
+   return X509_V_ERR_UNSPECIFIED;
}
if (id == -1)
return 1;
@@ -421,7 +423,12 @@ setup_crldp(X509 *x)
 {
int i;
 
-   x->crldp = X509_get_ext_d2i(x, NID_crl_distribution_points, NULL, NULL);
+   x->crldp = X509_get_ext_d2i(x, NID_crl_distribution_points, , NULL);
+   if (x->crldp == NULL && i != -1) {
+   x->ex_flags |= EXFLAG_INVALID;
+   return;
+   }
+
for (i = 0; i < sk_DIST_POINT_num(x->crldp); i++)
setup_dp(x, sk_DIST_POINT_value(x->crldp, i));
 }
@@ -449,7 +456,7 @@ x509v3_cache_extensions(X509 *x)
x->ex_flags |= EXFLAG_V1;
 
/* Handle basic constraints */
-   if ((bs = X509_get_ext_d2i(x, NID_basic_constraints, NULL, NULL))) {
+   if ((bs = X509_get_ext_d2i(x, NID_basic_constraints, , NULL))) {
if (bs->ca)
x->ex_flags |= EXFLAG_CA;
if (bs->pathlen) {
@@ -463,10 +470,12 @@ x509v3_cache_extensions(X509 *x)
x->ex_pathlen = -1;
BASIC_CONSTRAINTS_free(bs);
x->ex_flags |= EXFLAG_BCONS;
+   } else if (i != -1) {
+   x->ex_flags |= EXFLAG_INVALID;
}
 
/* Handle proxy certificates */
-   if ((pci = X509_get_ext_d2i(x, NID_proxyCertInfo, NULL, NULL))) {
+   if ((pci = X509_get_ext_d2i(x, NID_proxyCertInfo, , NULL))) {
if (x->ex_flags & EXFLAG_CA ||
X509_get_ext_by_NID(x, NID_subject_alt_name, -1) >= 0 ||
X509_get_ext_by_NID(x, NID_issuer_alt_name, -1) >= 0) {
@@ -485,10 +494,12 @@ x509v3_cache_extensions(X509 *x)
x->ex_pcpathlen = -1;
PROXY_CERT_INFO_EXTENSION_free(pci);
x->ex_flags |= EXFLAG_PROXY;
+   } else if (i != -1) {
+   x->ex_flags |= EXFLAG_INVALID;
}
 
/* Handle key usage */
-   if ((usage = X509_get_ext_d2i(x, NID_key_usage, NULL, NULL))) {
+   if ((usage = X509_get_ext_d2i(x, NID_key_usage, , NULL))) {
if (usage->length > 0) {
x->ex_kusage = usage->data[0];
if (usage->length > 1)
@@ -497,9 +508,12 @@ x509v3_cache_extensions(X509 *x)
x->ex_kusage = 0;
x->ex_flags |= EXFLAG_KUSAGE;
ASN1_BIT_STRING_free(usage);
+   } else if (i != -1) {
+   x->ex_flags |= EXFLAG_INVALID;
}
+
x->ex_xkusage = 0;
-   if ((extusage = X509_get_ext_d2i(x, NID_ext_key_usage, NULL, NULL))) {
+   if ((extusage = X509_get_ext_d2i(x, NID_ext_key_usage, , NULL))) {
x->ex_flags |= EXFLAG_XKUSAGE;
for (i = 0; i < sk_ASN1_OBJECT_num(extusage); i++) {
switch

Re: LibreSSL: handle EXFLAG_INVALID

2021-03-03 Thread Theo Buehler

On Thu, Feb 25, 2021 at 09:34:30PM +0100, Tobias Heider wrote:
> Hi,
> 
> while testing different x509 validator corner cases i found that a bunch of
> errors are currently not handled in libcrypto.
> 
> In particular duplicate or undecodable extensions are ignored.
> The diff below sets EXFLAG_INVALID whenever X509_get_ext_d2i() returns
> an error (other than "not found") and later throws X509_V_ERR_UNSPECIFIED
> if EXFLAG_INVALID is set.

Yes, we want this in some form. Apart from the X509_vfy bit this is a
subset of the changes in

https://github.com/openssl/openssl/pull/10756

While I'd be happy to land this in a few smaller pieces, I think we want
most of those changes.  In particular, I think we want to check for
EXFLAG_INVALID after all calls to x509v3_cache_extensions().

I don't currently understand why your x509_vfy change was not done (and
why they did not add a check to check_ca()).

> 
> Index: x509/x509_purp.c
> ===
> RCS file: /mount/openbsd/cvs/src/lib/libcrypto/x509/x509_purp.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 x509_purp.c
> --- x509/x509_purp.c  13 Sep 2020 15:06:17 -  1.2
> +++ x509/x509_purp.c  25 Feb 2021 20:25:11 -
> @@ -421,7 +421,12 @@ setup_crldp(X509 *x)
>  {
>   int i;
>  
> - x->crldp = X509_get_ext_d2i(x, NID_crl_distribution_points, NULL, NULL);
> + x->crldp = X509_get_ext_d2i(x, NID_crl_distribution_points, , NULL);
> + if (x->crldp == NULL && i != -1) {
> + x->ex_flags |= EXFLAG_INVALID;
> + return;
> + }
> +
>   for (i = 0; i < sk_DIST_POINT_num(x->crldp); i++)
>   setup_dp(x, sk_DIST_POINT_value(x->crldp, i));
>  }
> @@ -449,7 +454,7 @@ x509v3_cache_extensions(X509 *x)
>   x->ex_flags |= EXFLAG_V1;
>  
>   /* Handle basic constraints */
> - if ((bs = X509_get_ext_d2i(x, NID_basic_constraints, NULL, NULL))) {
> + if ((bs = X509_get_ext_d2i(x, NID_basic_constraints, , NULL))) {
>   if (bs->ca)
>   x->ex_flags |= EXFLAG_CA;
>   if (bs->pathlen) {
> @@ -463,10 +468,12 @@ x509v3_cache_extensions(X509 *x)
>   x->ex_pathlen = -1;
>   BASIC_CONSTRAINTS_free(bs);
>   x->ex_flags |= EXFLAG_BCONS;
> + } else if (i != -1) {
> + x->ex_flags |= EXFLAG_INVALID;
>   }
>  
>   /* Handle proxy certificates */
> - if ((pci = X509_get_ext_d2i(x, NID_proxyCertInfo, NULL, NULL))) {
> + if ((pci = X509_get_ext_d2i(x, NID_proxyCertInfo, , NULL))) {
>   if (x->ex_flags & EXFLAG_CA ||
>   X509_get_ext_by_NID(x, NID_subject_alt_name, -1) >= 0 ||
>   X509_get_ext_by_NID(x, NID_issuer_alt_name, -1) >= 0) {
> @@ -485,10 +492,12 @@ x509v3_cache_extensions(X509 *x)
>   x->ex_pcpathlen = -1;
>   PROXY_CERT_INFO_EXTENSION_free(pci);
>   x->ex_flags |= EXFLAG_PROXY;
> + } else if (i != -1) {
> + x->ex_flags |= EXFLAG_INVALID;
>   }
>  
>   /* Handle key usage */
> - if ((usage = X509_get_ext_d2i(x, NID_key_usage, NULL, NULL))) {
> + if ((usage = X509_get_ext_d2i(x, NID_key_usage, , NULL))) {
>   if (usage->length > 0) {
>   x->ex_kusage = usage->data[0];
>   if (usage->length > 1)
> @@ -497,9 +506,12 @@ x509v3_cache_extensions(X509 *x)
>   x->ex_kusage = 0;
>   x->ex_flags |= EXFLAG_KUSAGE;
>   ASN1_BIT_STRING_free(usage);
> + } else if (i != -1) {
> + x->ex_flags |= EXFLAG_INVALID;
>   }
> +
>   x->ex_xkusage = 0;
> - if ((extusage = X509_get_ext_d2i(x, NID_ext_key_usage, NULL, NULL))) {
> + if ((extusage = X509_get_ext_d2i(x, NID_ext_key_usage, , NULL))) {
>   x->ex_flags |= EXFLAG_XKUSAGE;
>   for (i = 0; i < sk_ASN1_OBJECT_num(extusage); i++) {
>   switch (OBJ_obj2nid(sk_ASN1_OBJECT_value(extusage, i))) 
> {
> @@ -538,19 +550,27 @@ x509v3_cache_extensions(X509 *x)
>   }
>   }
>   sk_ASN1_OBJECT_pop_free(extusage, ASN1_OBJECT_free);
> + } else if (i != -1) {
> + x->ex_flags |= EXFLAG_INVALID;
>   }
>  
> - if ((ns = X509_get_ext_d2i(x, NID_netscape_cert_type, NULL, NULL))) {
> + if ((ns = X509_get_ext_d2i(x, NID_netscape_cert_type, , NULL))) {
>   if (ns->length > 0)
>   x->ex_nscert = ns->data[0];
>   else
>   x->ex_nscert = 0;
>   x->ex_flags |= EXFLAG_NSCERT;
>   ASN1_BIT_STRING_free(ns);
> + } else if (i != -1) {
> + x->ex_flags |= EXFLAG_INVALID;
>   }
>  
> - x->skid = X509_get_ext_d2i(x, NID_subject_key_identifier, NULL, NULL);
> - x->akid = X509_get_ext_d2i(x, NID_authority_key_identifier, NULL, NULL);
> + x->skid =

Re: LibreSSL: handle EXFLAG_INVALID

Re: LibreSSL: handle EXFLAG_INVALID

Re: LibreSSL: handle EXFLAG_INVALID

3 matches

Site Navigation

Mail list logo

Footer information