Bug 121548 – [SOUP] TLSErrors do not cause page load to fail when not ignored

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Brian Holt]

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.

[Brian Holt]

Created attachment 211983 [details]
Patch

[Carlos Garcia Campos]

Is this a regression or has always happened? Could you add a unit test for this?

[Brian Holt]

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

[Sergio Villar Senin]

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?

[Carlos Garcia Campos]

The thing is why updateFromSoupMessage is called multiple times. Then try to reproduce that in a unit test.

[Brian Holt]

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

[Carlos Garcia Campos]

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

[Brian Holt]

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

[Carlos Garcia Campos]

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

[Sergio Villar Senin]

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

[Brian Holt]

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

[Sergio Villar Senin]

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

[Carlos Garcia Campos]

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.

[Brian Holt]

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

[Sergio Villar Senin]

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

[Carlos Garcia Campos]

(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 :-)

[Brian Holt]

Removing the dependency, as the TLS permission API patch works without this.  I will investigate this soon.

[Carlos Garcia Campos]

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

[Brian Holt]

Created attachment 230480 [details]
Patch

[Brian Holt]

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.

[Carlos Garcia Campos]

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

[Brian Holt]

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.

[Mario Sanchez Prada]

(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

[Carlos Garcia Campos]

Comment on attachment 230480 [details]
Patch

Thanks!

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 230752 [details]
Alternative patch

Checks the TLS errors before starting a redirection.

[Brian Holt]

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

[Carlos Garcia Campos]

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

[Sergio Villar Senin]

Comment on attachment 230980 [details]
Updated patch including GTK unit test

Nice fix!

[Carlos Garcia Campos]

Committed r168417: <http://trac.webkit.org/changeset/168417>