[Webkit-unassigned] [Bug 119436] [curl] Improve detecting and handling of SSL related errors

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=119436


Brent Fulgham <bfulgham at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #217023|review?                     |review-
               Flag|                            |




--- Comment #26 from Brent Fulgham <bfulgham at webkit.org>  2013-11-19 17:20:27 PST ---
(From update of attachment 217023)
View in context: https://bugs.webkit.org/attachment.cgi?id=217023&action=review

Looks very good.  Looking at this in more detail I had a few questions, as well as a concern about initialization of m_sslErrors.

> Source/WebCore/platform/network/ResourceHandleInternal.h:182
> +        unsigned m_sslErrors;

This will not get initialized on other platforms. Maybe this should be protected by #if USE(CURL) or something?

> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:121
> +    allowsAnyHTTPSCertificateHosts(host.lower());

This says the key is always lower-case...

> Source/WebCore/platform/network/curl/SSLHandle.cpp:40
> +void allowsAnyHTTPSCertificateHosts(const String& host)

... host always seems to be supplied as lower case.

> Source/WebCore/platform/network/curl/SSLHandle.cpp:54
> +            it->value = cert;

What is the case-ness of 'cert'?  Is it always lower case? Mixed case?

> Source/WebCore/platform/network/curl/SSLHandle.cpp:57
> +        if (it->value == cert)

This comparison assumes that the certificate state in allowedHosts is always the same as the passed cert. Do we need case insensitive compare?

> Source/WebCore/platform/network/curl/SSLHandle.cpp:134
> +    long len = BIO_get_mem_data(bio, &data);

len is signed. If it can be negative (e.g., error case) the following code will cause problems.

> Source/WebCore/platform/network/curl/SSLHandle.cpp:136
> +    certificate = data;

Do we care about the case provided by the certificate store?

> Source/WebCore/platform/network/curl/SSLHandle.cpp:165
> +    ok = sslIgnoreHTTPSCertificate(host.lower(), certificate);

So should we be using "certificate.lower()" here?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.