Re: The _gnutls_x509_verify_certificate fix

[Simon Josefsson]

Simon Josefsson <address@hidden> writes:

> Simon Josefsson <address@hidden> writes:
>
>> Tomas Mraz <address@hidden> writes:
>>
>>> On Mon, 2008-11-10 at 21:04 +0100, Nikos Mavrogiannopoulos wrote:
>>>> On Mon, Nov 10, 2008 at 2:47 PM, Tomas Mraz <address@hidden> wrote:
>>>> > Hello,
>>>> > given the recent fix in the _gnutls_x509_verify_certificate I have been
>>>> > looking at the function. I see there are currently some limitations in
>>>> > it. For example it now doesn't allow verification of explicitely trusted
>>>> > self-signed site certificate. Is there some other method how this could
>>>> > be achieved?
>>>> You can achieve it by associating an address of a website with the
>>>> keyid of the given
>>>> certificate. This is more generic of trusting a self-signed
>>>> certificate. You can trust any
>>>> certificate first presented when accessing a website that way (ssh 
>>>> security).
>>>
>>> But the patch should be modified anyway because in case the server
>>> presents just a self-signed site certificate there will be a dereference
>>> of the certificate_list[-1].
>>>
>>> It is also questionable whether the function should not also check for
>>> clist_size of 0 before calling _gnutls_verify_certificate2().
>>
>> Indeed.  This may explain:
>>
>> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=505279
>
> On reading more code, probably not, since all calls to
> _gnutls_x509_verify_certificate goes through gnutls_x509_crt_list_verify
> which does check for clist_size==0.  For readability, the two functions
> should probably be merged into one.

I was mistaken, the problem is when clist_size==1, then the function
decrements it and then references clist_size-1 which is bad.

So the patch in v2.6.1 and in the advisory causes crashes when the chain
contains just one self-signed certificate.  Ouch.  Thinking more about
this, I suspect the entire optimization to remove the final certificate
is pointless.  So this patch (against v2.6.1) should also work:

diff --git a/lib/x509/verify.c b/lib/x509/verify.c
index 8fa90dc..92ef722 100644
--- a/lib/x509/verify.c
+++ b/lib/x509/verify.c
@@ -374,17 +374,6 @@ _gnutls_x509_verify_certificate (const gnutls_x509_crt_t * 
certificate_list,
   int i = 0, ret;
   unsigned int status = 0, output;
 
-  /* Check if the last certificate in the path is self signed.
-   * In that case ignore it (a certificate is trusted only if it
-   * leads to a trusted party by us, not the server's).
-   */
-  if (gnutls_x509_crt_check_issuer (certificate_list[clist_size - 1],
-                                   certificate_list[clist_size - 1]) > 0
-      && clist_size > 0)
-    {
-      clist_size--;
-    }
-
   /* Verify the last certificate in the certificate path
    * against the trusted CA certificate list.
    *

The self-test needs to be modified slightly to accommodate this, because
now the validation returns without the GNUTLS_CERT_SIGNER_NOT_FOUND
flag, which is arguable more correct (?) since the problem is actually
because of failing verification.

Nikos, any thoughts?

Btw, I've updated cve-2008-4989.c in git, now it tests both the chain
that caused incorrect validation and a self-signed chain that causes a
crash.

/Simon