Bug 79657 – [WK2] [GTK] [libsoup] SoupSession should use system CA

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Sergio Villar Senin]

As stated here https://bugs.webkit.org/show_bug.cgi?id=68602#c24 libsoup should use the available system CA list to validate SSL certificates. Enabling that caused several regressions (see https://bugs.webkit.org/show_bug.cgi?id=68602#c27).

[Sergio Villar Senin]

Created attachment 129008 [details]
Patch

[Martin Robinson]

Comment on attachment 129008 [details]
Patch

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

Looks good, but this needs to be expanded for WKTR.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:597
> +    /* Do not use strict certificate validation for tests. */
> +    g_object_set(session, SOUP_SESSION_SSL_STRICT, FALSE, NULL);
> +

You'll also need to do this for WebKitTestRunner.

[Sergio Villar Senin]

Created attachment 129254 [details]
Patch

[Martin Robinson]

Comment on attachment 129254 [details]
Patch

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

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:236
> +    g_object_set(session, SOUP_SESSION_SSL_STRICT, FALSE, NULL);

The more I think this, the more I wonder if this should be in ResourceHandleSoup.cpp as well. Sergio and Dan, any thoughts about this?

[Sergio Villar Senin]

(In reply to comment #4)
> (From update of attachment 129254 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129254&action=review
> 
> > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:236
> > +    g_object_set(session, SOUP_SESSION_SSL_STRICT, FALSE, NULL);
> 
> The more I think this, the more I wonder if this should be in ResourceHandleSoup.cpp as well. Sergio and Dan, any thoughts about this?

If we do that then we must add some sort of "This certificate seems to be invalid/expired/whatever. Do you want to accept this certificate anyway?" dialog (as we will be accepting invalid certificates). IMO we do not need to do that as I haven't seen any major issue related to certificate handling.

[Martin Robinson]

(In reply to comment #5)

> If we do that then we must add some sort of "This certificate seems to be invalid/expired/whatever. Do you want to accept this certificate anyway?" dialog (as we will be accepting invalid certificates). IMO we do not need to do that as I haven't seen any major issue related to certificate handling.

Doesn't Soup support this kind of operation? 

My concern with this patch is that by flipping this flag in ResouceHandleSoup, we'll be preventing sites from loading that used to load before. This will break downstream applications until they turn off strict SSL handling. Is this a valid concern or have I just misunderstood the situation?

[Dan Winship]

(In reply to comment #6)
> (In reply to comment #5)
> 
> > If we do that then we must add some sort of "This certificate seems to be invalid/expired/whatever. Do you want to accept this certificate anyway?" dialog (as we will be accepting invalid certificates).
> 
> Doesn't Soup support this kind of operation? 

libsoup doesn't do any UI stuff. If the app knows that SSL_STRICT has been set FALSE, then it can watch for various signals and such to know when it needs to present such a dialog itself, but if not, then setting SSL_STRICT to FALSE is exactly the same as just not setting USE_SYSTEM_CA_FILE. Just slower.

Moreover, if the app was previously setting its own SSL_CA_FILE, and just assuming that SSL_STRICT had its default value of TRUE, then having WebKit set it FALSE by default would render the app insecure (since it would be expecting invalid certificates to be rejected, but they wouldn't be).

So I guess we can't do this without breaking API... maybe do it in wk2 only?

[Martin Robinson]

(In reply to comment #7)

> So I guess we can't do this without breaking API... maybe do it in wk2 only?

Sounds like WebKit2-only is the best option. :( Let's keep this in mind if we ever break WebKit1 API.

[Sergio Villar Senin]

(In reply to comment #7)
> Moreover, if the app was previously setting its own SSL_CA_FILE, and just assuming that SSL_STRICT had its default value of TRUE, then having WebKit set it FALSE by default would render the app insecure (since it would be expecting invalid certificates to be rejected, but they wouldn't be).
> 
> So I guess we can't do this without breaking API... maybe do it in wk2 only?

Let's clarify this, this patch *does not* set SSL strict to FALSE for WebKit clients. It will do it only for testing purpouses, i.e, it will only be set to FALSE for WebKitTestRunner and DumpRenderTree.

[Martin Robinson]

(In reply to comment #9)
> > So I guess we can't do this without breaking API... maybe do it in wk2 only?
> 
> Let's clarify this, this patch *does not* set SSL strict to FALSE for WebKit clients. It will do it only for testing purpouses, i.e, it will only be set to FALSE for WebKitTestRunner and DumpRenderTree.

From what I understand this is an API break, because applications will fail to load sites that they could load just fine in the past -- or is that not the case?

[Sergio Villar Senin]

(In reply to comment #10)
> (In reply to comment #9)
> > > So I guess we can't do this without breaking API... maybe do it in wk2 only?
> > 
> > Let's clarify this, this patch *does not* set SSL strict to FALSE for WebKit clients. It will do it only for testing purpouses, i.e, it will only be set to FALSE for WebKitTestRunner and DumpRenderTree.
> 
> From what I understand this is an API break, because applications will fail to load sites that they could load just fine in the past -- or is that not the case?

Unless I'm missing something I don't think this will break any application.

[Dan Winship]

(In reply to comment #11)
> Unless I'm missing something I don't think this will break any application.

It will. Note that SSL_STRICT was tacked on after the fact and has confusing semantics because we didn't think it through enough at the time. SSL_STRICT=TRUE doesn't mean "be strict", it means "be strict if SSL_CA_FILE or USE_SYSTEM_CA_FILE is also set, and non-strict if not".

So, with WebKit's current behavior, an app that doesn't set any SSL-related options will accept all certificates.

If we commit your patch as is, then an app that doesn't set any SSL-related options will now start rejecting all certificates that aren't signed by one of the system CAs, which is essentially an API break. (It doesn't break epiphany, because epiphany unsets SSL_STRICT itself.)

If we commit your patch and also unset SSL_STRICT (as Martin suggested in comment 4), then that avoids that problem, but causes the opposite problem; apps that were previously setting SSL_CA_FILE or USE_SYSTEM_CA_FILE but not changing the setting of SSL_STRICT will now suddenly start accepting all certificates, instead of only accepting acceptable ones.

[Sergio Villar Senin]

(In reply to comment #12)
> (In reply to comment #11)
> > Unless I'm missing something I don't think this will break any application.
> 
> It will. Note that SSL_STRICT was tacked on after the fact and has confusing semantics because we didn't think it through enough at the time. SSL_STRICT=TRUE doesn't mean "be strict", it means "be strict if SSL_CA_FILE or USE_SYSTEM_CA_FILE is also set, and non-strict if not".
> 
> So, with WebKit's current behavior, an app that doesn't set any SSL-related options will accept all certificates.

Yeah indeed, I was wrongly assuming that rejecting certificates was the default policy.

So, I guess we should remove the USE_SYSTEM_CA_FILE from ResourceHandleSoup and move it to WK2 initialization specific code. How does it sound Martin?

[Martin Robinson]

(In reply to comment #13)

> So, I guess we should remove the USE_SYSTEM_CA_FILE from ResourceHandleSoup and move it to WK2 initialization specific code. How does it sound Martin?

Yeah, let's do that. Now that I understand the scope of the problem, this sounds like the best plan.

[Sergio Villar Senin]

Created attachment 129453 [details]
Patch

[Martin Robinson]

Comment on attachment 129453 [details]
Patch

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

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:236
> +    SoupSession* session = WebCore::ResourceHandle::defaultSession();
> +    g_object_set(session, SOUP_SESSION_SSL_STRICT, FALSE, NULL);

I'm really sorry about this back and forth in this patch, I really am. Before landing I think we should move this to Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp. Later we can add API to WebKit2 to expose certificate errors to the client. By default it would accept broken certificates.

[Sergio Villar Senin]

Committed r109338: <http://trac.webkit.org/changeset/109338>