9:45 a.m.
In the case that the trusted CA certificates are provided to
l_certchain_verify() and one of them binary-matches the top CA
certificate in the chain being verified, we'd skip linking it to the
keyring and we'd link the next certificate directly. As a result
that second certificate would be linked to an empty unrestricted keyring
and would not be verified at all.
Instead add the top CA to the keyring and restrict it the same way we
proceed when no trusted CAs are provided.
---
ell/cert.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/ell/cert.c b/ell/cert.c
index d1f58d7..dc6b845 100644
--- a/ell/cert.c
+++ b/ell/cert.c
@@ -454,6 +454,7 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
struct l_cert *cert;
struct l_key *prev_key = NULL;
int verified = 0;
+ bool ca_match;
static char error_buf[200];
if (unlikely(!chain || !chain->leaf))
@@ -464,6 +465,7 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
RETURN_ERROR("Can't create verify keyring");
cert = chain->ca;
+ ca_match = cert_is_in_set(cert, ca_certs);
/*
* For TLS compatibility the trusted root CA certificate is
@@ -480,16 +482,9 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
* cert in the chain if it is identical to one of the trusted CA
* certificates. It also happens to work around a kernel issue
* preventing self-signed certificates missing the AKID
- * extension from being linked to a keyring.
+ * extension from being linked to a restricted keyring.
*/
- if (cert_is_in_set(cert, ca_certs)) {
- verified++;
- cert = cert->issued;
- if (!cert)
- return true;
-
- prev_key = cert_try_link(cert, verify_ring);
- } else if (ca_certs) {
+ if (ca_certs && !ca_match) {
ca_ring = cert_set_to_keyring(ca_certs, error_buf);
if (!ca_ring) {
if (error)
@@ -550,8 +545,8 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
"verified against trusted CA(s) and the "
"following %i top certificates verified ok",
verified + 1, total,
- ca_certs && verified ? "" : "not ",
- verified ? verified - 1 : 0);
+ ca_certs && (ca_match || verified) ? "" :
+ "not ", verified ? verified - 1 : 0);
}
l_key_free(prev_key);
--
2.20.1
9:45 a.m.
New subject: [PATCH 2/3] cert: Relax trused CA matching in l_certchain_verify
Check if any of the certificates in the chain matched any of the
trusted CA certificates (if given) and treat the chain as trusted even
if the matching certificate was not the top CA in the chain.
We also don't require that the trusted certificates or the root in the
chain are self-signed.
This allows a user to pin a specific certificate to a server/client or
trust a subtree of all of the certificates emitted by the root CA.
---
ell/cert.c | 30 ++++++++++++++++++++++--------
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/ell/cert.c b/ell/cert.c
index dc6b845..cb3bda7 100644
--- a/ell/cert.c
+++ b/ell/cert.c
@@ -454,7 +454,8 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
struct l_cert *cert;
struct l_key *prev_key = NULL;
int verified = 0;
- bool ca_match;
+ int ca_match = 0;
+ int i = 0;
static char error_buf[200];
if (unlikely(!chain || !chain->leaf))
@@ -464,8 +465,13 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
if (!verify_ring)
RETURN_ERROR("Can't create verify keyring");
+ for (cert = chain->ca; cert; cert = cert->issued, i++)
+ if (cert_is_in_set(cert, ca_certs)) {
+ ca_match = i + 1;
+ break;
+ }
+
cert = chain->ca;
- ca_match = cert_is_in_set(cert, ca_certs);
/*
* For TLS compatibility the trusted root CA certificate is
@@ -539,14 +545,22 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
if (!prev_key) {
int total = 0;
+ char str[100];
for (cert = chain->ca; cert; cert = cert->issued, total++);
- RETURN_ERROR("Linking certificate %i / %i failed, root %s"
- "verified against trusted CA(s) and the "
- "following %i top certificates verified ok",
- verified + 1, total,
- ca_certs && (ca_match || verified) ? "" :
- "not ", verified ? verified - 1 : 0);
+
+ if (ca_match)
+ snprintf(str, sizeof(str), "%i / %i matched a trusted "
+ "certificate, root not verified",
+ ca_match, total);
+ else
+ snprintf(str, sizeof(str), "root %sverified against "
+ "trusted CA(s)",
+ ca_certs && !ca_match && verified ? "" :
+ "not ");
+
+ RETURN_ERROR("Linking certificate %i / %i failed, %s",
+ verified + 1, total, str);
}
l_key_free(prev_key);
--
2.20.1
9:45 a.m.
New subject: [PATCH 3/3] unit: Relax test-tls certificate chain checks
Make the l_certchain_verify() failure tests pass after the previous
commit.
---
unit/test-tls.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/unit/test-tls.c b/unit/test-tls.c
index 3519376..f4beee8 100644
--- a/unit/test-tls.c
+++ b/unit/test-tls.c
@@ -228,6 +228,7 @@ static void test_certificates(const void *data)
{
struct l_queue *cacert;
struct l_queue *wrongca;
+ struct l_queue *wrongca2;
struct l_queue *twocas;
struct l_certchain *chain;
struct l_certchain *chain2;
@@ -240,6 +241,9 @@ static void test_certificates(const void *data)
wrongca = l_pem_load_certificate_list(CERTDIR "cert-intca.pem");
assert(wrongca && !l_queue_isempty(wrongca));
+ wrongca2 = l_pem_load_certificate_list(CERTDIR "cert-server.pem");
+ assert(wrongca2 && !l_queue_isempty(wrongca2));
+
twocas = l_pem_load_certificate_list(CERTDIR "cert-chain.pem");
assert(twocas);
@@ -254,7 +258,7 @@ static void test_certificates(const void *data)
chain2 = l_pem_load_certificate_chain(CERTDIR "cert-chain.pem");
assert(chain2);
- assert(!l_certchain_verify(chain2, wrongca, NULL));
+ assert(!l_certchain_verify(chain2, wrongca2, NULL));
assert(l_certchain_verify(chain2, cacert, NULL));
assert(l_certchain_verify(chain2, NULL, NULL));
assert(l_certchain_verify(chain2, twocas, NULL));
@@ -282,7 +286,7 @@ static void test_certificates(const void *data)
load_cert_file(CERTDIR "cert-ca.pem"));
assert(chain4);
- assert(!l_certchain_verify(chain4, wrongca, NULL));
+ assert(!l_certchain_verify(chain4, wrongca2, NULL));
assert(l_certchain_verify(chain4, cacert, NULL));
assert(l_certchain_verify(chain4, NULL, NULL));
assert(l_certchain_verify(chain4, twocas, NULL));
@@ -293,6 +297,7 @@ static void test_certificates(const void *data)
l_certchain_free(chain4);
l_queue_destroy(cacert, (l_queue_destroy_func_t) l_cert_free);
l_queue_destroy(wrongca, (l_queue_destroy_func_t) l_cert_free);
+ l_queue_destroy(wrongca2, (l_queue_destroy_func_t) l_cert_free);
l_queue_destroy(twocas, (l_queue_destroy_func_t) l_cert_free);
}
--
2.20.1
7:09 a.m.
New subject: [PATCH 1/3] cert: Fix verification of the 2nd certificate in
chain
Hi Andrew,
On 4/7/20 12:45 PM, Andrew Zaborowski wrote:
In the case that the trusted CA certificates are provided to
l_certchain_verify() and one of them binary-matches the top CA
certificate in the chain being verified, we'd skip linking it to the
keyring and we'd link the next certificate directly. As a result
that second certificate would be linked to an empty unrestricted keyring
and would not be verified at all.
Instead add the top CA to the keyring and restrict it the same way we
proceed when no trusted CAs are provided.
---
ell/cert.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/ell/cert.c b/ell/cert.c
index d1f58d7..dc6b845 100644
--- a/ell/cert.c
+++ b/ell/cert.c
@@ -454,6 +454,7 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
struct l_cert *cert;
struct l_key *prev_key = NULL;
int verified = 0;
+ bool ca_match;
static char error_buf[200];
if (unlikely(!chain || !chain->leaf))
@@ -464,6 +465,7 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
RETURN_ERROR("Can't create verify keyring");
cert = chain->ca;
+ ca_match = cert_is_in_set(cert, ca_certs);
/*
* For TLS compatibility the trusted root CA certificate is
@@ -480,16 +482,9 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
* cert in the chain if it is identical to one of the trusted CA
* certificates. It also happens to work around a kernel issue
* preventing self-signed certificates missing the AKID
- * extension from being linked to a keyring.
+ * extension from being linked to a restricted keyring.
*/
So is this comment still valid now that you've taken out the if block
that takes care of this? At the very least it seems completely out of
place now.
Do we have a unit test with a certchain where the root cert doesn't have
AKID extension?
- if (cert_is_in_set(cert, ca_certs)) {
- verified++;
- cert = cert->issued;
- if (!cert)
- return true;
-
- prev_key = cert_try_link(cert, verify_ring);
So remind me again what the actual problem was? The code here is
'optimizing' by not trying to link the root cert at all, supposedly
because it might be self-signed and missing the AKID extension.
- } else if (ca_certs) {
+ if (ca_certs && !ca_match) {
ca_ring = cert_set_to_keyring(ca_certs, error_buf);
if (!ca_ring) {
if (error)
But now you link it always?
Regards,
-Denis
10:37 a.m.
Hi Denis!
On Wed, 8 Apr 2020 at 17:09, Denis Kenzior <denkenz(a)gmail.com> wrote:
On 4/7/20 12:45 PM, Andrew Zaborowski wrote:
> In the case that the trusted CA certificates are provided to
> l_certchain_verify() and one of them binary-matches the top CA
> certificate in the chain being verified, we'd skip linking it to the
> keyring and we'd link the next certificate directly. As a result
> that second certificate would be linked to an empty unrestricted keyring
> and would not be verified at all.
>
> Instead add the top CA to the keyring and restrict it the same way we
> proceed when no trusted CAs are provided.
> ---
> ell/cert.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/ell/cert.c b/ell/cert.c
> index d1f58d7..dc6b845 100644
> --- a/ell/cert.c
> +++ b/ell/cert.c
> @@ -454,6 +454,7 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
> struct l_cert *cert;
> struct l_key *prev_key = NULL;
> int verified = 0;
> + bool ca_match;
> static char error_buf[200];
>
> if (unlikely(!chain || !chain->leaf))
> @@ -464,6 +465,7 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
> RETURN_ERROR("Can't create verify keyring");
>
> cert = chain->ca;
> + ca_match = cert_is_in_set(cert, ca_certs);
>
> /*
> * For TLS compatibility the trusted root CA certificate is
> @@ -480,16 +482,9 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain *chain,
> * cert in the chain if it is identical to one of the trusted CA
> * certificates. It also happens to work around a kernel issue
> * preventing self-signed certificates missing the AKID
> - * extension from being linked to a keyring.
> + * extension from being linked to a restricted keyring.
> */
So is this comment still valid now that you've taken out the if block
that takes care of this? At the very least it seems completely out of
place now.
We have the if (ca_caerts && !ca_match) line right after this comment,
and what the comment refers to now is the !ca_match check.
Do we have a unit test with a certchain where the root cert doesn't have
AKID extension?
Since we still need this workaound I'd prefer that we don't commit to
supporting this for now. We don't know if we may need to make an
incompatible change at some point...
Also if we generate such certificates in the Makefile we'd have to
hide the command pretty well so that nobody thinks it's the right
thing to do.
> - if (cert_is_in_set(cert, ca_certs)) {
> - verified++;
> - cert = cert->issued;
> - if (!cert)
> - return true;
> -
> - prev_key = cert_try_link(cert, verify_ring);
So remind me again what the actual problem was? The code here is
'optimizing' by not trying to link the root cert at all, supposedly
because it might be self-signed and missing the AKID extension.
Previously if we detected that the top certificate was an exact copy
of one of the trusted CAs, we'd save time by not pushing any of the
trusted CAs into the kernel, building the CA keyring, etc., and not
even pushing that top certificate into the kernel... but that was
wrong because the second certificate was completely unchecked, I think
we had a serious security problem there.
Now we still skip building the whole ca_ring but we do link the first
certificate to verify_ring so that the second certificate can be
verified like the rest of the chain. The code for this is common with
the case where no trusted CA list is provided.
In terms of the AKID extension, when we link the first certificate to
verify_ring it is not restricted so the restrict functions don't run
and there should be no issue. Previously we didn't even link that
certificate so that also worked...
Best regards
9:09 a.m.
New subject: [PATCH 1/3] cert: Fix verification of the 2nd certificate in
chain
Hi Andrew,
On 4/8/20 1:37 PM, Andrew Zaborowski wrote:
Hi Denis!
On Wed, 8 Apr 2020 at 17:09, Denis Kenzior <denkenz(a)gmail.com> wrote:
> On 4/7/20 12:45 PM, Andrew Zaborowski wrote:
>> In the case that the trusted CA certificates are provided to
>> l_certchain_verify() and one of them binary-matches the top CA
>> certificate in the chain being verified, we'd skip linking it to the
>> keyring and we'd link the next certificate directly. As a result
>> that second certificate would be linked to an empty unrestricted keyring
>> and would not be verified at all.
>>
>> Instead add the top CA to the keyring and restrict it the same way we
>> proceed when no trusted CAs are provided.
>> ---
>> ell/cert.c | 17 ++++++-----------
>> 1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/ell/cert.c b/ell/cert.c
>> index d1f58d7..dc6b845 100644
>> --- a/ell/cert.c
>> +++ b/ell/cert.c
>> @@ -454,6 +454,7 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain
*chain,
>> struct l_cert *cert;
>> struct l_key *prev_key = NULL;
>> int verified = 0;
>> + bool ca_match;
>> static char error_buf[200];
>>
>> if (unlikely(!chain || !chain->leaf))
>> @@ -464,6 +465,7 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain
*chain,
>> RETURN_ERROR("Can't create verify keyring");
>>
>> cert = chain->ca;
>> + ca_match = cert_is_in_set(cert, ca_certs);
>>
>> /*
>> * For TLS compatibility the trusted root CA certificate is
>> @@ -480,16 +482,9 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain
*chain,
>> * cert in the chain if it is identical to one of the trusted CA
>> * certificates. It also happens to work around a kernel issue
>> * preventing self-signed certificates missing the AKID
>> - * extension from being linked to a keyring.
>> + * extension from being linked to a restricted keyring.
>> */
>
> So is this comment still valid now that you've taken out the if block
> that takes care of this? At the very least it seems completely out of
> place now.
We have the if (ca_caerts && !ca_match) line right after this comment,
and what the comment refers to now is the !ca_match check.
I guess I can kind of agree with this, but see below... I think the
comment should be rewritten for clarity
>
> Do we have a unit test with a certchain where the root cert doesn't have
> AKID extension?
Since we still need this workaound I'd prefer that we don't commit to
supporting this for now. We don't know if we may need to make an
incompatible change at some point...
I'm not following? We have CA Certs that might be missing the AKID
extension that are supposed to work in all situations. Why do we not
have a test for this that can detect regressions, etc?
Also if we generate such certificates in the Makefile we'd have to
hide the command pretty well so that nobody thinks it's the right
thing to do.
I will have to disagree completely here. This is a valid use case we
should be testing. If someone uses iwd as a guide for 'best practices
of certificate generation', that is their problem ;)
>
>> - if (cert_is_in_set(cert, ca_certs)) {
>> - verified++;
>> - cert = cert->issued;
>> - if (!cert)
>> - return true;
>> -
>> - prev_key = cert_try_link(cert, verify_ring);
>
> So remind me again what the actual problem was? The code here is
> 'optimizing' by not trying to link the root cert at all, supposedly
> because it might be self-signed and missing the AKID extension.
Previously if we detected that the top certificate was an exact copy
of one of the trusted CAs, we'd save time by not pushing any of the
trusted CAs into the kernel, building the CA keyring, etc., and not
I get this part. By the way, this should probably be part of the
comment above.
even pushing that top certificate into the kernel... but that was
wrong because the second certificate was completely unchecked, I think
we had a serious security problem there.
Right...
Now we still skip building the whole ca_ring but we do link the first
certificate to verify_ring so that the second certificate can be
verified like the rest of the chain. The code for this is common with
the case where no trusted CA list is provided.
In terms of the AKID extension, when we link the first certificate to
verify_ring it is not restricted so the restrict functions don't run
and there should be no issue. Previously we didn't even link that
certificate so that also worked...
So to be clear, the issue with AKID extension is only in the following
scenario:
CACerts = ca1 ca2 ca3
PeerCertchain = ca2 int1 int2 leaf
Lets say ca2 is self-signed and missing AKID.
If we load the CACerts into the ca_ring, link it to the verify ring and
restrict the verify ring, the linking of ca2 from the PeerCertchain
would fail. This is the limitation in the kernel we're working around,
right?
So the optimization is that since we know that ca2 is a bitwise copy, we
can load ca2 into the verify_ring, restrict the verify ring. Any
subsequent linking of int1 would then succeed.
Right? So why don't we have a unit test of this case? ;)
Regards,
-Denis
1:34 p.m.
On Wed, 8 Apr 2020 at 21:32, Denis Kenzior <denkenz(a)gmail.com> wrote:
On 4/8/20 1:37 PM, Andrew Zaborowski wrote:
> On Wed, 8 Apr 2020 at 17:09, Denis Kenzior <denkenz(a)gmail.com> wrote:
>> On 4/7/20 12:45 PM, Andrew Zaborowski wrote:
>>> /*
>>> * For TLS compatibility the trusted root CA certificate is
>>> @@ -480,16 +482,9 @@ LIB_EXPORT bool l_certchain_verify(struct l_certchain
*chain,
>>> * cert in the chain if it is identical to one of the trusted CA
>>> * certificates. It also happens to work around a kernel issue
>>> * preventing self-signed certificates missing the AKID
>>> - * extension from being linked to a keyring.
>>> + * extension from being linked to a restricted keyring.
>>> */
>>
>> So is this comment still valid now that you've taken out the if block
>> that takes care of this? At the very least it seems completely out of
>> place now.
>
> We have the if (ca_caerts && !ca_match) line right after this comment,
> and what the comment refers to now is the !ca_match check.
I guess I can kind of agree with this, but see below... I think the
comment should be rewritten for clarity
>
>>
>> Do we have a unit test with a certchain where the root cert doesn't have
>> AKID extension?
>
> Since we still need this workaound I'd prefer that we don't commit to
> supporting this for now. We don't know if we may need to make an
> incompatible change at some point...
I'm not following? We have CA Certs that might be missing the AKID
extension that are supposed to work in all situations. Why do we not
have a test for this that can detect regressions, etc?
Ok, let me add it then. I guess it's a question of whether we care
about this use case enough to add some certificate parsing of our own
in a scenario where for some reason we can't load them into the kernel
or find any workaround. Previously we tried to avoid this at any cost.
>
> Also if we generate such certificates in the Makefile we'd have to
> hide the command pretty well so that nobody thinks it's the right
> thing to do.
>
I will have to disagree completely here. This is a valid use case we
should be testing. If someone uses iwd as a guide for 'best practices
of certificate generation', that is their problem ;)
>>
>>> - if (cert_is_in_set(cert, ca_certs)) {
>>> - verified++;
>>> - cert = cert->issued;
>>> - if (!cert)
>>> - return true;
>>> -
>>> - prev_key = cert_try_link(cert, verify_ring);
>>
>> So remind me again what the actual problem was? The code here is
>> 'optimizing' by not trying to link the root cert at all, supposedly
>> because it might be self-signed and missing the AKID extension.
>
> Previously if we detected that the top certificate was an exact copy
> of one of the trusted CAs, we'd save time by not pushing any of the
> trusted CAs into the kernel, building the CA keyring, etc., and not
I get this part. By the way, this should probably be part of the
comment above.
Ok.
> even pushing that top certificate into the kernel... but that was
> wrong because the second certificate was completely unchecked, I think
> we had a serious security problem there.
Right...
>
> Now we still skip building the whole ca_ring but we do link the first
> certificate to verify_ring so that the second certificate can be
> verified like the rest of the chain. The code for this is common with
> the case where no trusted CA list is provided.
>
> In terms of the AKID extension, when we link the first certificate to
> verify_ring it is not restricted so the restrict functions don't run
> and there should be no issue. Previously we didn't even link that
> certificate so that also worked...
So to be clear, the issue with AKID extension is only in the following
scenario:
CACerts = ca1 ca2 ca3
PeerCertchain = ca2 int1 int2 leaf
Lets say ca2 is self-signed and missing AKID.
If we load the CACerts into the ca_ring, link it to the verify ring and
restrict the verify ring, the linking of ca2 from the PeerCertchain
would fail. This is the limitation in the kernel we're working around,
right?
Yep.
So the optimization is that since we know that ca2 is a bitwise copy, we
can load ca2 into the verify_ring, restrict the verify ring. Any
subsequent linking of int1 would then succeed.
Exactly, so if ca2 is self-signed, at this point we've verified that
ca2 is trusted... we don't check if it's self signed but if the user
trusts it then it's fine either way.
Right? So why don't we have a unit test of this case? ;)
Well we don't test optimizations ;) but we can test this missing AKID
scenario just in case...
Best regards
853
days inactive
854
days old
6 comments
2 participants
participants (2)
-
Andrew Zaborowski -
Denis Kenzior