Bug 119436 - [curl] Improve detecting and handling of SSL related errors
Summary: [curl] Improve detecting and handling of SSL related errors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 117300 124569
  Show dependency treegraph
 
Reported: 2013-08-02 01:15 PDT by sipka
Modified: 2014-05-27 05:05 PDT (History)
5 users (show)

See Also:


Attachments
First draft (20.29 KB, patch)
2013-08-02 01:15 PDT, sipka
no flags Details | Formatted Diff | Diff
working progress patch (20.69 KB, patch)
2013-10-11 08:10 PDT, sipka
no flags Details | Formatted Diff | Diff
working progress patch v2 (25.60 KB, patch)
2013-10-16 04:47 PDT, sipka
no flags Details | Formatted Diff | Diff
working progress patch (25.58 KB, patch)
2013-10-21 00:59 PDT, sipka
no flags Details | Formatted Diff | Diff
proposed patch (25.83 KB, patch)
2013-11-13 03:55 PST, sipka
bfulgham: review-
Details | Formatted Diff | Diff
proposed patch v2 (25.82 KB, patch)
2013-11-14 03:56 PST, sipka
no flags Details | Formatted Diff | Diff
proposed patch (25.82 KB, patch)
2013-11-15 01:27 PST, sipka
bfulgham: review-
Details | Formatted Diff | Diff
proposed patch (39.56 KB, patch)
2013-11-20 08:01 PST, sipka
no flags Details | Formatted Diff | Diff
proposed patch (39.56 KB, patch)
2013-11-20 08:05 PST, sipka
bfulgham: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
proposed patch (25.86 KB, patch)
2013-11-20 12:58 PST, sipka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description sipka 2013-08-02 01:15:27 PDT
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.
Comment 1 sipka 2013-10-11 08:10:07 PDT
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.
Comment 2 Peter Gal 2013-10-14 04:36:56 PDT
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.
Comment 3 peavo 2013-10-15 01:54:34 PDT
(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.
Comment 4 sipka 2013-10-16 04:47:21 PDT
Created attachment 214354 [details]
working progress patch v2
Comment 5 sipka 2013-10-16 04:52:11 PDT
Now also Win platform is supported. 
Peavo, would you help me to test this on wincairo?
Comment 6 peavo 2013-10-16 23:32:51 PDT
(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?
Comment 7 sipka 2013-10-17 01:04:39 PDT
(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.
Comment 8 peavo 2013-10-19 04:40:34 PDT
I get the following compile error:

>..\platform\network\curl\SSLHandle.cpp(64): error C2144: syntax error : 'unsigned int' should be preceded by ';'
Comment 9 sipka 2013-10-21 00:59:07 PDT
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?
Comment 10 peavo 2013-10-21 07:25:38 PDT
> 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?
Comment 11 sipka 2013-10-21 12:10:00 PDT
(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/
Comment 12 peavo 2013-10-22 04:41:41 PDT
(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.
Comment 13 sipka 2013-10-22 05:19:34 PDT
(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?
Comment 14 peavo 2013-10-22 13:19:20 PDT
(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 :)
Comment 15 sipka 2013-10-24 03:42:37 PDT
(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.
Comment 16 peavo 2013-10-24 06:21:17 PDT
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?
Comment 17 sipka 2013-11-06 07:53:09 PST
(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.
Comment 18 sipka 2013-11-11 00:09:36 PST
ping for review
Comment 19 peavo 2013-11-11 23:31:18 PST
(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 :)
Comment 20 sipka 2013-11-12 04:30:17 PST
(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 :)
Comment 21 sipka 2013-11-13 03:55:02 PST
Created attachment 216791 [details]
proposed patch

update for review
Comment 22 Brent Fulgham 2013-11-13 09:16:36 PST
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.
Comment 23 Brent Fulgham 2013-11-13 09:18:40 PST
(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!
Comment 24 sipka 2013-11-14 03:56:54 PST
Created attachment 216915 [details]
proposed patch v2

Thanks for the detailed review! I fixed the style problems you've mentioned.
Comment 25 sipka 2013-11-15 01:27:13 PST
Created attachment 217023 [details]
proposed patch

I fixed a careless error and recheck the patch.
Comment 26 Brent Fulgham 2013-11-19 17:20:27 PST
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?
Comment 27 sipka 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.
Comment 28 sipka 2013-11-20 08:01:46 PST
Created attachment 217434 [details]
proposed patch
Comment 29 sipka 2013-11-20 08:05:00 PST
Created attachment 217435 [details]
proposed patch
Comment 30 Brent Fulgham 2013-11-20 09:55:42 PST
Comment on attachment 217435 [details]
proposed patch

Great!  Thanks for updating the patch. r=me.
Comment 31 WebKit Commit Bot 2013-11-20 09:56:25 PST
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
Comment 32 Brent Fulgham 2013-11-20 09:57:59 PST
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?
Comment 33 sipka 2013-11-20 12:58:16 PST
Created attachment 217468 [details]
proposed patch

just generate the patch one more time with all default settings
Comment 34 Brent Fulgham 2013-11-20 14:56:00 PST
Comment on attachment 217468 [details]
proposed patch

r=me
Comment 35 WebKit Commit Bot 2013-11-20 15:23:15 PST
Comment on attachment 217468 [details]
proposed patch

Clearing flags on attachment: 217468

Committed r159587: <http://trac.webkit.org/changeset/159587>
Comment 36 Darin Adler 2013-11-20 15:28:11 PST
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.
Comment 37 sipka 2013-11-21 01:52:06 PST
(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.
Comment 38 sipka 2014-05-27 05:05:36 PDT
All reviewed patches have been landed.  Closing bug.