[PATCH 1/2] tls: Fix CA certificate presence check

[PATCH 1/2] tls: Fix CA certificate presence check

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 483 bytes --]

---
 ell/tls.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ell/tls.c b/ell/tls.c
index 3879b50..388efa7 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -1474,7 +1474,7 @@ static void tls_handle_certificate(struct l_tls *tls,
 	 * against our CA if we have any.
 	 */
 
-	if (ca_cert) {
+	if (tls->ca_cert_path) {
 		ca_cert = tls_cert_load_file(tls->ca_cert_path);
 		if (!ca_cert) {
 			tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR,
-- 
2.7.4

[PATCH 2/2] tls: Don't fail if root CA present in received chain

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2218 bytes --]

The certificate chain from the Server Certificate message may be a
complete chain from server's certificate to root CA.  l_keyring_link
would fail if we tried to add the self-signed root CA to the ring,
this seems to be unrelated to that certificate being the same as the
one in the trusted ring.

In the early userspace tls_cert_verify_certchain implementation the
verification would succeed if any of the certificates in the chain
was trusted by the supplied CA + the trust chain was correct, but the
RFC implies this must be the root CA (see the comment in the code).
---
 ell/tls.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/ell/tls.c b/ell/tls.c
index 388efa7..06e3341 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -2469,13 +2469,26 @@ static void tls_key_cleanup(struct l_key **p)
 	l_key_free_norevoke(*p);
 }
 
-static bool tls_cert_verify_with_keyring(struct tls_cert *cert,
-						struct l_keyring *ring)
+static int tls_cert_verify_with_keyring(struct tls_cert *cert,
+						struct l_keyring *ring,
+						struct tls_cert *root)
 {
 	if (!cert)
 		return true;
 
-	if (tls_cert_verify_with_keyring(cert->issuer, ring)) {
+	/*
+	 * RFC5246 7.4.2:
+	 * "Because certificate validation requires that root keys be
+	 * distributed independently, the self-signed certificate that
+	 * specifies the root certificate authority MAY be omitted from
+	 * the chain, under the assumption that the remote end must
+	 * already possess it in order to validate it in any case."
+	 */
+	if (!cert->issuer && root && cert->size == root->size &&
+			!memcmp(cert->asn1, root->asn1, root->size))
+		return true;
+
+	if (tls_cert_verify_with_keyring(cert->issuer, ring, root)) {
 		L_AUTO_CLEANUP_VAR(struct l_key *, key, tls_key_cleanup);
 
 		key = l_key_new(L_KEY_RSA, cert->asn1, cert->size);
@@ -2520,7 +2533,7 @@ bool tls_cert_verify_certchain(struct tls_cert *certchain,
 	if (!verify_ring)
 		return false;
 
-	return tls_cert_verify_with_keyring(certchain, verify_ring);
+	return tls_cert_verify_with_keyring(certchain, verify_ring, ca_cert);
 }
 
 void tls_cert_free_certchain(struct tls_cert *cert)
-- 
2.7.4

Re: [PATCH 2/2] tls: Don't fail if root CA present in received chain

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 812 bytes --]

Hi Andrew,

On 11/14/2016 02:16 PM, Andrew Zaborowski wrote:
> The certificate chain from the Server Certificate message may be a
> complete chain from server's certificate to root CA.  l_keyring_link
> would fail if we tried to add the self-signed root CA to the ring,
> this seems to be unrelated to that certificate being the same as the
> one in the trusted ring.
>
> In the early userspace tls_cert_verify_certchain implementation the
> verification would succeed if any of the certificates in the chain
> was trusted by the supplied CA + the trust chain was correct, but the
> RFC implies this must be the root CA (see the comment in the code).
> ---
>   ell/tls.c | 21 +++++++++++++++++----
>   1 file changed, 17 insertions(+), 4 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 1/2] tls: Fix CA certificate presence check

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 189 bytes --]

Hi Andrew,

On 11/14/2016 02:16 PM, Andrew Zaborowski wrote:
> ---
>   ell/tls.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>

Applied, thanks.

Regards,
-Denis