Bug 121548

Summary: [SOUP] TLSErrors do not cause page load to fail when not ignored
Product: WebKit Reporter: Brian Holt <brian.holt>
Component: WebKitGTKAssignee: Brian Holt <brian.holt>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, cdumez, cgarcia, commit-queue, danw, gns, mario, mrobinson, rakuco, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 131104    
Attachments:
Description Flags
Patch
none
Patch
cgarcia: review-
Alternative patch
none
Updated patch including GTK unit test svillar: review+

Description Brian Holt 2013-09-18 03:36:34 PDT
When the m_ignoreTLSErrors in WebContext is set to false and a page like https://www.aeat.com is loaded, page loading does not fail (i.e. webkitWebViewLoadFailedWithTLSErrors is not called).

Investigation into this issue revealed that the GTtlCertificateFlags in ResourceResponseSoup.cpp updateFromSoupMessage() are not persisted for more than a single update.
Comment 1 Brian Holt 2013-09-18 03:46:19 PDT
Created attachment 211983 [details]
Patch
Comment 2 Carlos Garcia Campos 2013-09-18 03:50:57 PDT
Is this a regression or has always happened? Could you add a unit test for this?
Comment 3 Brian Holt 2013-09-18 03:57:35 PDT
(In reply to comment #2)
> Is this a regression or has always happened? Could you add a unit test for this?

TestSSL passes without it, but I think this is because there is only one soup update, whereas https://www.aeat.com results in many updates and the error is overwritten on the second update.  So I think it's always been like this, but I'm surprised you didn't encounter it in https://bugs.webkit.org/show_bug.cgi?id=90267#c12.
Comment 4 Sergio Villar Senin 2013-09-18 04:00:40 PDT
Are you sure https://www.aeat.com is showing that behaviour, it redirects to a non secure http site. Anyway...

so the tls errors (if any) in the SoupMessage are set once the SSL connection is completed. When reading the description I was wondering why they would be cleared. When you say "are not persisted for more than a single update" do you mean that they are no longer there on reload?
Comment 5 Carlos Garcia Campos 2013-09-18 04:14:34 PDT
The thing is why updateFromSoupMessage is called multiple times. Then try to reproduce that in a unit test.
Comment 6 Brian Holt 2013-09-18 05:47:28 PDT
(In reply to comment #4)
> Are you sure https://www.aeat.com is showing that behaviour, it redirects to a non secure http site. Anyway...
> 

Try it in Firefox/Chrome and compare to WK2, you should expect to see 3 errors: expired + bad identity + unknown CA

> so the tls errors (if any) in the SoupMessage are set once the SSL connection is completed. When reading the description I was wondering why they would be cleared. When you say "are not persisted for more than a single update" do you mean that they are no longer there on reload?

I mean that there are multiple calls to 
soup_message_get_https_status(soupMessage, &certificate, &m_tlsFlags); and its only in the first one that m_tlsFlags is set to 11.  On the second and subsequent calls it is set to 0 by soup_message_get_https_status().
Comment 7 Carlos Garcia Campos 2013-09-18 05:51:33 PDT
(In reply to comment #6)
> (In reply to comment #4)
> > Are you sure https://www.aeat.com is showing that behaviour, it redirects to a non secure http site. Anyway...
> > 
> 
> Try it in Firefox/Chrome and compare to WK2, you should expect to see 3 errors: expired + bad identity + unknown CA
> 
> > so the tls errors (if any) in the SoupMessage are set once the SSL connection is completed. When reading the description I was wondering why they would be cleared. When you say "are not persisted for more than a single update" do you mean that they are no longer there on reload?
> 
> I mean that there are multiple calls to 
> soup_message_get_https_status(soupMessage, &certificate, &m_tlsFlags); and its only in the first one that m_tlsFlags is set to 11.  On the second and subsequent calls it is set to 0 by soup_message_get_https_status().

The patch makes sense, the question is why this is called multiple times for this page and not others, is it because of the redirection? can we emulate that in a unit test?
Comment 8 Brian Holt 2013-09-18 05:58:36 PDT
(In reply to comment #5)
> [...]
> The patch makes sense, the question is why this is called multiple times for this page and not others, is it because of the redirection? can we emulate that in a unit test?

Sure, I'll try to reproduce it in the TestSSL unit test and upload a new patch.
Comment 9 Carlos Garcia Campos 2013-09-18 06:01:29 PDT
(In reply to comment #8)
> (In reply to comment #5)
> > [...]
> > The patch makes sense, the question is why this is called multiple times for this page and not others, is it because of the redirection? can we emulate that in a unit test?
> 
> Sure, I'll try to reproduce it in the TestSSL unit test and upload a new patch.

Thanks!
Comment 10 Sergio Villar Senin 2013-09-18 07:23:07 PDT
(In reply to comment #6)
> (In reply to comment #4)
> > Are you sure https://www.aeat.com is showing that behaviour, it redirects to a non secure http site. Anyway...
> > 
> 
> Try it in Firefox/Chrome and compare to WK2, you should expect to see 3 errors: expired + bad identity + unknown CA

Yeah but that's because they warn the user, probably we should do the same, but currently we just go on with the redirection. So it's normal that the errors are cleared because they belong to the original https site not to the redirected one. IMO it makes no sense to show the errors in the redirected page, we either ask the user for confirmation or just completely forget about the errors.
Comment 11 Brian Holt 2013-09-18 07:44:55 PDT
(In reply to comment #10)
> [...]
> Yeah but that's because they warn the user, probably we should do the same, but currently we just go on with the redirection. 

Should we fail page loading at this point with TLS errors and not allow the redirection to go ahead?  Why does this not happen now and what is the best way of doing this?
Comment 12 Sergio Villar Senin 2013-09-18 07:56:15 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > [...]
> > Yeah but that's because they warn the user, probably we should do the same, but currently we just go on with the redirection. 
> 
> Should we fail page loading at this point with TLS errors and not allow the redirection to go ahead?  Why does this not happen now and what is the best way of doing this?

I think we definitely should, that redirection is a huge security issue because users won't notice that a compromised "secure" site (like your bank) is redirecting you to some let-me-steal-your-password pirate site.

In any case I think that's a matter of the webkit embeders and not the engine itself. So in your example, epiphany should get the "decide-policy" signal which carries a WebKitNavigationPolicyDecision which has the request. I think in that case epiphany should get the request, check that there are some errors and ask the user whether they want to complete the navigation or cancel the redirection.
Comment 13 Carlos Garcia Campos 2013-09-18 08:01:54 PDT
We don't warn because we are currently ignoring TLS errors by default, but Brian said that webkitWebViewLoadFailedWithTLSErrors is not called even when not ignoring TLS errors, if that's true, it's a bug. 
The plan is to shown an error page, but first we need to add support to allow the user to ignore individual TLS errors, and that's the API Brian is currently working on.
Comment 14 Brian Holt 2013-09-18 08:04:23 PDT
> In any case I think that's a matter of the webkit embeders and not the engine itself. So in your example, epiphany should get the "decide-policy" signal which carries a WebKitNavigationPolicyDecision which has the request. I think in that case epiphany should get the request, check that there are some errors and ask the user whether they want to complete the navigation or cancel the redirection.

That's exactly what I'm aiming to support in https://bugs.webkit.org/show_bug.cgi?id=120160.  The only problem is that webkitWebViewLoadFailedWithTLSErrors is not called when a site is loaded that has TLS errors...
Comment 15 Sergio Villar Senin 2013-09-18 08:08:43 PDT
(In reply to comment #13)
> We don't warn because we are currently ignoring TLS errors by default, but Brian said that webkitWebViewLoadFailedWithTLSErrors is not called even when not ignoring TLS errors, if that's true, it's a bug. 
> The plan is to shown an error page, but first we need to add support to allow the user to ignore individual TLS errors, and that's the API Brian is currently working on.

Ah OK then I didn't understand the bug correctly. In any case I think the real issue is to know why the errors are being removed something that is not clear to me.
Comment 16 Carlos Garcia Campos 2013-09-18 08:15:10 PDT
(In reply to comment #15)
> (In reply to comment #13)
> > We don't warn because we are currently ignoring TLS errors by default, but Brian said that webkitWebViewLoadFailedWithTLSErrors is not called even when not ignoring TLS errors, if that's true, it's a bug. 
> > The plan is to shown an error page, but first we need to add support to allow the user to ignore individual TLS errors, and that's the API Brian is currently working on.
> 
> Ah OK then I didn't understand the bug correctly. In any case I think the real issue is to know why the errors are being removed something that is not clear to me.

And that's why I'm asking for a unit test :-)
Comment 17 Brian Holt 2013-10-02 10:03:30 PDT
Removing the dependency, as the TLS permission API patch works without this.  I will investigate this soon.
Comment 18 Carlos Garcia Campos 2014-04-30 03:11:51 PDT
Comment on attachment 211983 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

You should remove this, commit-queue will refuse to land this patch with this line here.

> Source/WebCore/platform/network/soup/ResourceResponseSoup.cpp:71
> +    GTlsCertificateFlags tlsFlags = static_cast<GTlsCertificateFlags>(0);

I would use tldErrors instead of tlsFlasg for consistency, making clear that this is the same as m_tlsErrors
Comment 19 Brian Holt 2014-04-30 09:00:28 PDT
Created attachment 230480 [details]
Patch
Comment 20 Brian Holt 2014-04-30 09:05:40 PDT
I have a unit test, but I'm not convinced that it tests the issue properly. So in order to make progress with this issue I've rebased and uploaded the patch again and created a new bug for the GTK unit test (bug 132384).

This fix should probably also be backported to the 2.4 (and possibly the 2.2) releases.
Comment 21 Carlos Garcia Campos 2014-04-30 09:14:00 PDT
(In reply to comment #20)
> I have a unit test, but I'm not convinced that it tests the issue properly. 

why? what's the issue? You should have a failing test (maybe timing out because the expected signal is not emitted) without the patch and a passing test with the patch.

> So in order to make progress with this issue I've rebased and uploaded the patch again and created a new bug for the GTK unit test (bug 132384).

Well, having a unit tests reproducing the issue would help me to understand the problem, the fact that you have a unit test that you don't know if it tests the issue makes me doubt, because I don't understand the EFL test.

> This fix should probably also be backported to the 2.4 (and possibly the 2.2) releases.

Yes, I'll backport it.
Comment 22 Brian Holt 2014-05-01 03:05:16 PDT
Carlos, if you were wondering if this really is a bug, you can follow these steps to reproduce with the problem in the MiniBrowser:

1) Modify main.c of MiniBrowser to set the TLS policy to fail (default is ignore)

webkit_web_context_set_tls_errors_policy(webkit_web_context_get_default(), WEBKIT_TLS_ERRORS_POLICY_FAIL);

2) Build webkit and run $ ./bin/MiniBrowser https://www.aeat.com/

The site https://www.aeat.com/ has an invalid certificate so if it did not redirect, it would have failed. However, because it redirects to an http site http://www.ricardo-aea.com/cms/, the new site finishes the load without reporting an error. This is the bug.

3) Apply the patch attached to this bug report, rebuild and run $ ./bin/MiniBrowser https://www.aeat.com/

Observe that webkit now reports a TLS error.
Comment 23 Mario Sanchez Prada 2014-05-02 04:05:56 PDT
(In reply to comment #22)
> Carlos, if you were wondering if this really is a bug, you can follow these steps to reproduce with the problem in the MiniBrowser:
> 
> 1) Modify main.c of MiniBrowser to set the TLS policy to fail (default is ignore)
> 
> webkit_web_context_set_tls_errors_policy(webkit_web_context_get_default(), WEBKIT_TLS_ERRORS_POLICY_FAIL);
> 
> 2) Build webkit and run $ ./bin/MiniBrowser https://www.aeat.com/
> 
> The site https://www.aeat.com/ has an invalid certificate so if it did not redirect, it would have failed. However, because it redirects to an http site http://www.ricardo-aea.com/cms/, the new site finishes the load without reporting an error. This is the bug.
> 
> 3) Apply the patch attached to this bug report, rebuild and run $ ./bin/MiniBrowser https://www.aeat.com/
> 
> Observe that webkit now reports a TLS error.

Thanks Brian for the step-by-step guide to reproduce the problem (and for the unit test attached to bug 132384).

Carlos, do you think that this is enough to land the fix now, providing an unit test (EFL, GTK or both) will follow? I'll comment on bug 131104 to see if we can get the EFL unit test reviewed promptly, assuming that the real fix would be landed through this bug anyway
Comment 24 Carlos Garcia Campos 2014-05-03 03:51:33 PDT
Comment on attachment 230480 [details]
Patch

Thanks!
Comment 25 Carlos Garcia Campos 2014-05-03 04:26:48 PDT
Comment on attachment 230480 [details]
Patch

hmm, after trying the patch I've realized that this is not the right fix. This patch is setting the tls errors to the response after the redirection, which is not even a HTTPS response. I think we should fail *before* the redirection, so that we don't see the redirected URL in the browser either.
Comment 26 Carlos Garcia Campos 2014-05-03 04:33:41 PDT
Created attachment 230752 [details]
Alternative patch

Checks the TLS errors before starting a redirection.
Comment 27 Brian Holt 2014-05-06 02:53:57 PDT
(In reply to comment #25)
> (From update of attachment 230480 [details])
> hmm, after trying the patch I've realized that this is not the right fix. This patch is setting the tls errors to the response after the redirection, which is not even a HTTPS response. I think we should fail *before* the redirection, so that we don't see the redirected URL in the browser either.

Thinking about it I agree with you that we should fail before the redirection. I wasn't sure about expected behaviour except to know that at some point the load should fail and the error should be reported. Failing earlier makes sense (and probably doesn't require it's own unit test then?).
Comment 28 Carlos Garcia Campos 2014-05-07 02:37:31 PDT
Created attachment 230980 [details]
Updated patch including GTK unit test

Before:

/webkit2/WebKitWebView/tls-errors-redirect-to-http: **
ERROR:../../Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestSSL.cpp:142:void testTLSErrorsRedirect(SSLTest*, gconstpointer): assertion failed: (test->m_loadFailed)

After:

/webkit2/WebKitWebView/tls-errors-redirect-to-http: OK
Comment 29 Sergio Villar Senin 2014-05-07 02:50:48 PDT
Comment on attachment 230980 [details]
Updated patch including GTK unit test

Nice fix!
Comment 30 Carlos Garcia Campos 2014-05-07 03:12:08 PDT
Committed r168417: <http://trac.webkit.org/changeset/168417>