Bug 119436 – [curl] Improve detecting and handling of SSL related errors

• Component: WebCore Misc.
• Status: RESOLVED FIXED

Created attachment 207993 [details]
First draft

Enable SSL verification when we have a way of shipping certs and/or reporting SSL errors to the user.
The certificate verification result reported for the user. The enabled domains are stored in a non-persistent way and the certificate are saved into a pem format file. 
Future plan is to stop the communication until the user gives an answer to enable or disable those sites, which have invalid certificates and separate the enable domains and save function of the certs.

Created attachment 213988 [details]
working progress patch

Enable SSL verification when we have a way of shipping certs and/or reporting SSL errors to the user.
Set the exact SSL verification error on CURL and store the enabled domain with certificate.

[Peter Gal]

peavo could you help us a bit: on wincairo does the libcurl uses openssl? or it uses some other ssl library? We would like to extract additional informations from bad certificates.

(In reply to comment #2)
> peavo could you help us a bit: on wincairo does the libcurl uses openssl? or it uses some other ssl library? We would like to extract additional informations from bad certificates.

Yes, I believe wincairo uses openssl. It is built with the define USE_SSLEAY, and depends on the dll ssleay32.dll.

Created attachment 214354 [details]
working progress patch v2

Now also Win platform is supported. 
Peavo, would you help me to test this on wincairo?

(In reply to comment #5)
> Now also Win platform is supported. 
> Peavo, would you help me to test this on wincairo?

Yes, will do :) Which cases do you want me to test?

(In reply to comment #6)
> (In reply to comment #5)
> > Now also Win platform is supported. 
> > Peavo, would you help me to test this on wincairo?
> 
> Yes, will do :) Which cases do you want me to test?

There are some websites that throw  an SSL/TLS handshake fail, could you please run some tests and find out whether or not the security certificate of these sites can be accepted in order to load the content?  It is also important to have MiniBrowser remember these certificate informations in runtime, so it wont ask the user for permission every single time.

I get the following compile error:

>..\platform\network\curl\SSLHandle.cpp(64): error C2144: syntax error : 'unsigned int' should be preceded by ';'

Created attachment 214710 [details]
working progress patch

peavo: Yes, it's careless error that has been corrected with this patch. Could you repeat the test please?

> static int certVerifyCallback(int ok, X509_STORE_CTX *ctx)
> {
>    // whether the verification of the certificate in question was passed (preverify_ok=1) or not (preverify_ok=0)

>    unsigned err = X509_STORE_CTX_get_error(ctx);
>    if (!err)
>        return 1;

I never get past this, err is always 0, is that correct?

(In reply to comment #10)
> > static int certVerifyCallback(int ok, X509_STORE_CTX *ctx)
> > {
> >    // whether the verification of the certificate in question was passed (preverify_ok=1) or not (preverify_ok=0)
> 
> >    unsigned err = X509_STORE_CTX_get_error(ctx);
> >    if (!err)
> >        return 1;
> 
> I never get past this, err is always 0, is that correct?

In those cases too, where the certificate is untrusted?
On the website below you can test the error messages you receive self-signed SSL certificate:
https://www.pcwebshop.co.uk/

(In reply to comment #11)
> > 
> > I never get past this, err is always 0, is that correct?
> 
> In those cases too, where the certificate is untrusted?
> On the website below you can test the error messages you receive self-signed SSL certificate:
> https://www.pcwebshop.co.uk/

Yes, I have tried https://duckduckgo.com, without a pem file, and err == 0.

(In reply to comment #12)
> (In reply to comment #11)
> > > 
> > > I never get past this, err is always 0, is that correct?
> > 
> > In those cases too, where the certificate is untrusted?
> > On the website below you can test the error messages you receive self-signed SSL certificate:
> > https://www.pcwebshop.co.uk/
> 
> Yes, I have tried https://duckduckgo.com, without a pem file, and err == 0.

Have you got any SSL warnings on any sites where the content didn't load? (e.g.: SSL handshake error) If so, can you ignore them?

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > > 
> > > > I never get past this, err is always 0, is that correct?
> > > 
> > > In those cases too, where the certificate is untrusted?
> > > On the website below you can test the error messages you receive self-signed SSL certificate:
> > > https://www.pcwebshop.co.uk/
> > 
> > Yes, I have tried https://duckduckgo.com, without a pem file, and err == 0.
> 
> Have you got any SSL warnings on any sites where the content didn't load? (e.g.: SSL handshake error) If so, can you ignore them?

OK, I think I know why I am getting err == 0 now.
I am running with the env variable CURL_CA_BUNDLE_PATH set to a .pem file.
When I visit e.g. https://duckduckgo.com, the certificate is trusted, and err will  of course be zero then. The page then loads fine, which it should :)

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (In reply to comment #11)
> > > > > 
> > > > > I never get past this, err is always 0, is that correct?
> > > > 
> > > > In those cases too, where the certificate is untrusted?
> > > > On the website below you can test the error messages you receive self-signed SSL certificate:
> > > > https://www.pcwebshop.co.uk/
> > > 
> > > Yes, I have tried https://duckduckgo.com, without a pem file, and err == 0.
> > 
> > Have you got any SSL warnings on any sites where the content didn't load? (e.g.: SSL handshake error) If so, can you ignore them?
> 
> OK, I think I know why I am getting err == 0 now.
> I am running with the env variable CURL_CA_BUNDLE_PATH set to a .pem file.
> When I visit e.g. https://duckduckgo.com, the certificate is trusted, and err will  of course be zero then. The page then loads fine, which it should :)

Ok, thanks a lot. So it seems to work fine on wincairo too.

I now tested without a .pem file locally, and when I visit a secure site, I get err == 20 (X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY), which I assume is correct. But the host is not found in the list, so the load fails. How will I add allowed hosts to the host list?

Also, the function bool pemData(X509_STORE_CTX *ctx, String& certificate) compiles fine on Windows, maybe it should be used here as well?

(In reply to comment #16)
> I now tested without a .pem file locally, and when I visit a secure site, I get err == 20 (X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY), which I assume is correct. But the host is not found in the list, so the load fails. How will I add allowed hosts to the host list?
>
How can you skipped ssl errors currently in WinCairo? With this patch it should be the same. 
 
> Also, the function bool pemData(X509_STORE_CTX *ctx, String& certificate) compiles fine on Windows, maybe it should be used here as well?

This required to extract additional ssl error informations and currently we cannot test it on windows so we will skip it for now.

ping for review

(In reply to comment #17)
> (In reply to comment #16)
> > I now tested without a .pem file locally, and when I visit a secure site, I get err == 20 (X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY), which I assume is correct. But the host is not found in the list, so the load fails. How will I add allowed hosts to the host list?
> >
> How can you skipped ssl errors currently in WinCairo? With this patch it should be the same. 
> 

I did not skip ssl errors, the load failed. Is there a way to accept the new certificate, to successfully load the page?

> > Also, the function bool pemData(X509_STORE_CTX *ctx, String& certificate) compiles fine on Windows, maybe it should be used here as well?
> 
> This required to extract additional ssl error informations and currently we cannot test it on windows so we will skip it for now.

Ok, it seemed to manage to extract the certificate when I tested :)

(In reply to comment #19)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > I now tested without a .pem file locally, and when I visit a secure site, I get err == 20 (X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY), which I assume is correct. But the host is not found in the list, so the load fails. How will I add allowed hosts to the host list?
> > >
> > How can you skipped ssl errors currently in WinCairo? With this patch it should be the same. 
> > 

seems like the ssl error handler is not yet implemented on win platform and the user can not opt to skip the certificate verification either.

> 
> I did not skip ssl errors, the load failed. Is there a way to accept the new certificate, to successfully load the page?
> 
> > > Also, the function bool pemData(X509_STORE_CTX *ctx, String& certificate) compiles fine on Windows, maybe it should be used here as well?
> > 
> > This required to extract additional ssl error informations and currently we cannot test it on windows so we will skip it for now.
> 
> Ok, it seemed to manage to extract the certificate when I tested :)

Created attachment 216791 [details]
proposed patch

update for review

[Brent Fulgham]

Comment on attachment 216791 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216791&action=review

Looks very good!  A few style problems need to be cleaned up, but otherwise this looks great!  Thanks for getting this functionality in place.

> Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:618
> +                ResourceError resourceError = ResourceError(String(), msg->data.result, String(url), String(curl_easy_strerror(msg->data.result)));

This construct is a little funny.  Why not just declare resourceError with construction in place?

ResourceError resourceError(String(), msg->data.result, String(url), String(curl_easy_strerror(msg->data.result));

> Source/WebCore/platform/network/curl/SSLHandle.cpp:122
> +bool pemData(X509_STORE_CTX *ctx, String& certificate)

We write this as X509_STORE_CTX*

> Source/WebCore/platform/network/curl/SSLHandle.cpp:142
> +static int certVerifyCallback(int ok, X509_STORE_CTX *ctx)

X509_STORE_CTX*

> Source/WebCore/platform/network/curl/SSLHandle.cpp:150
> +    SSL* ssl = (SSL*)X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx());

This should be a C++-style cast (prob. a reinterpret_cast<SSL*>)

> Source/WebCore/platform/network/curl/SSLHandle.cpp:152
> +    ResourceHandle* job  = (ResourceHandle*)SSL_CTX_get_app_data(sslctx);

C++ style cast.

> Source/WebCore/platform/network/curl/SSLHandle.cpp:170
> +        // so don't need to curl verifies the authenticity of the peer's certificate

Because of this large comment, you should wrap the comment and curl_easy_setopt in brackets to make the structure clear:

if (ok) {
    // blah blah
    curl_easy_setopt(...)
}

> Source/WebCore/platform/network/curl/SSLHandle.cpp:175
> +static CURLcode sslctxfun(CURL * curl, void * sslctx, void * parm)

CURL* curl, void* sslctx, void* parm

> Source/WebCore/platform/network/curl/SSLHandle.cpp:177
> +    SSL_CTX_set_app_data((SSL_CTX*)sslctx, parm);

C++-style casts, please.

[Brent Fulgham]

(In reply to comment #22)
> (From update of attachment 216791 [details])
> > Source/WebCore/platform/network/curl/SSLHandle.cpp:150
> > +    SSL* ssl = (SSL*)X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx());
> 
> This should be a C++-style cast (prob. a reinterpret_cast<SSL*>)

I didn't do an exhaustive check for C-style casts, so please double-check that all of your casting is done using C++ style.  Sometimes this (annoyingly) requires you to chain a couple of casts together, but it's still preferred so that it's explicit what type conversions you are doing.

Thanks!

Created attachment 216915 [details]
proposed patch v2

Thanks for the detailed review! I fixed the style problems you've mentioned.

Created attachment 217023 [details]
proposed patch

I fixed a careless error and recheck the patch.

[Brent Fulgham]

Comment on attachment 217023 [details]
proposed patch

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?

(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.

Created attachment 217434 [details]
proposed patch

Created attachment 217435 [details]
proposed patch

[Brent Fulgham]

Comment on attachment 217435 [details]
proposed patch

Great!  Thanks for updating the patch. r=me.

[WebKit Commit Bot]

Comment on attachment 217435 [details]
proposed patch

Rejecting attachment 217435 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 217435, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ching file Source/WebCore/platform/network/curl/ResourceError.h
patching file Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp
patching file Source/WebCore/platform/network/curl/ResourceHandleManager.cpp
patching file Source/WebCore/platform/network/curl/SSLHandle.cpp
patching file Source/WebCore/platform/network/curl/SSLHandle.h

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Brent Fulgham']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/30648001

[Brent Fulgham]

It looks like the revised format of your patch confused the landing system. Can you just generate the patch one more time with all default settings and I'll r+ it for landing?

Created attachment 217468 [details]
proposed patch

just generate the patch one more time with all default settings

[Brent Fulgham]

Comment on attachment 217468 [details]
proposed patch

r=me

[WebKit Commit Bot]

Comment on attachment 217468 [details]
proposed patch

Clearing flags on attachment: 217468

Committed r159587: <http://trac.webkit.org/changeset/159587>

[Darin Adler]

Comment on attachment 217468 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217468&action=review

> Source/WebCore/platform/network/curl/SSLHandle.cpp:46
> +    HashMap<String, String>::iterator it = allowedHosts.find(host);
> +    if (it != allowedHosts.end())
> +        it->value = String();
> +    else
> +        allowedHosts.add(host, String());

This should be written like this:

    allowedHosts.set(host, String());

It doesn’t require multiple lines of code.

(In reply to comment #36)
> (From update of attachment 217468 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217468&action=review
> 
> > Source/WebCore/platform/network/curl/SSLHandle.cpp:46
> > +    HashMap<String, String>::iterator it = allowedHosts.find(host);
> > +    if (it != allowedHosts.end())
> > +        it->value = String();
> > +    else
> > +        allowedHosts.add(host, String());
> 
> This should be written like this:
> 
>     allowedHosts.set(host, String());
> 
> It doesn’t require multiple lines of code.

Yes, you are right, thanks. I paid attention to it in Bug124569.

All reviewed patches have been landed.  Closing bug.