[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





--- Comment #27 from sipka at inf.u-szeged.hu  2013-11-20 08:01:15 PST ---
(In reply to comment #26)
> (From update of attachment 217023 [details])
> 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.
> 
Thanks for the detailed review. 

> > 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?
>
Yes, it's protected by #if USE(CURL). I generated the diffs with bigger lines of context, to show this part.  

> > Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:121
> > +    allowsAnyHTTPSCertificateHosts(host.lower());
> 
> This says the key is always lower-case...
> 

I want to handle/storage the host in insensitive case at here. 

> > 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?
> 

It's mixed case, I get the certificate in PEM format which is simply base64 encoded data.

> > 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?
> 
We need a case sensitive 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.

Yes, the BIO_get_mem_data returns with a long value and it's possible to be a negative value. I fixed this part. 

> 
> > Source/WebCore/platform/network/curl/SSLHandle.cpp:136
> > +    certificate = data;
> 
> Do we care about the case provided by the certificate store?
> 
Yes, mixed case is important. 

> > Source/WebCore/platform/network/curl/SSLHandle.cpp:165
> > +    ok = sslIgnoreHTTPSCertificate(host.lower(), certificate);
> 
> So should we be using "certificate.lower()" here?

No, we need to pass the certificate in mixed case to handle this base64 encoded data.

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