Bug 179374 – [GTK] [WPE] Add an API to set TLS settings

• Component: WebKitGTK
• Status: NEW

[Nael Ouedraogo]

Add an API to set TLS settings.

[Michael Catanzaro]

I guess a proposal is incoming?

It's going to depend on https://bugzilla.gnome.org/show_bug.cgi?id=745637 for sure. And possibly also on https://bugzilla.gnome.org/show_bug.cgi?id=711864 if you want to get any information about TLS certificates without having to parse them inside WebKit using libgcrypt. (And we might not want to do that to implement our API, since it would probably be better for our API to match whatever eventually ends up being exposed on GTlsCertificate)

The problem then becomes how to proxy information exposed by GTlsConnection and GTlsCertificate from the network process to the UI process, and designing a suitable API to expose it. This would be ambitious. I was not planning to expose TLS settings in the WebKit API at all, but I'm not opposed if you want to try.

Note: in the meantime you can use the G_TLS_GNUTLS_PRIORITY environment variable as a workaround. That would overwrite the secure defaults we set in NetworkProcessMain.cpp and WebProcessMain.cpp, so be careful if doing so.

[Michael Catanzaro]

Also note: as part of this work, we *really* need to start blocking certificate chains that use non-root certificates signed with SHA-1, by default.

[Nael Ouedraogo]

Created attachment 326211 [details]
Patch

[Nael Ouedraogo]

Yes a proposal is coming. We started a first implementation to start the discussion. Even if GLib is not providing a API to control the TLS settings yet, we think it is useful to provide an API to customize cipher-suites in WebKitGTK/WPE. The API is quite basic since relies only on G_TLS_GNUTLS_PRIORITY environment variable. 

We propose three modes to configure the cipher-suites. First mode enforces WebKit default settings. The second mode uses G_TLS_GNUTLS_PRIORITY environment variable value if set by the system. With third mode, you can enable/disable protocols. A developer could check the return value of webkit_tls_settings_enable_protocol to determine if its customized settings are considered as safe in WebKit.

[Nael Ouedraogo]

Created attachment 326212 [details]
Patch

[Michael Catanzaro]

Thanks! I'll try to review it later today. There are a couple of serious problems with using that environment variable that I'll need some more time to type up a proper explanation for, and that's going to make this rather more complicated. But this looks like a good start.

[Michael Catanzaro]

Comment on attachment 326212 [details]
Patch

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

OK, two big problems here.

First off, as you've noticed an API to set TLS settings using G_TLS_GNUTLS_PRIORITY is unsafe and crashy, due to setenv being terrible. You've documented that, which is OK (would be better to document the precise condition under which it is unsafe: when a secondary thread is running), but the problem is, it's really too limiting here. Application developers expect to be able to create a WebKitWebContext somewhere other than the very top of main, and if this function is provided they'll expect to be able to use it where they create a WebKitWebContext. But that's not possible after the first thread has been created (which would need to be clearly documented). Who knows where exactly the first thread is created in most complex applications. I don't even know if it's possible to create a WebKitWebContext without creating a secondary thread, so I'm not certain it would ever be possible to call this safely. So that's another reason that we need to do better than the environment variable.

The next problem is that, since it uses G_TLS_GNUTLS_PRIORITY, it depends on glib-networking (and obviously GnuTLS). But glib-networking is just one implementation of GIO's TLS extension point. Another possible implementation is glib-openssl, which is being used some places in practice, and which is probably more appropriate for most embedded systems. If we add API to control TLS settings, it really needs to work in a manner that's independent of which TLS extension point is in use. Hence, my opinion that the only sane way to do this is to first solve https://bugzilla.gnome.org/show_bug.cgi?id=745637. Then the WebKit API would most likely be dictated by the solution in that bug. E.g. if GTlsConnection exposes properties as a GVariant dict of strings, rather than as enums, the WebKit API should probably expose it in exactly the same way. That way, applications get maximum flexibility to control the settings, without baking the details of different TLS protocols into the WebKit API. But of course, that's all up for discussion and debate. (Especially if you're interested in implementing it. ;)

Actually, there's a third problem with the current implementation. This isn't actually a property of the WebKitWebContext, it's a global property that has to be shared by all web contexts. It's a bit of a shame that we turned off unused parameter warnings in Source/WebKit, as then the compiler would have warned about this.

> Source/WebKit/ChangeLog:14
> +        cipher-suites. WEBKIT_TLS_MODE_DEFAULT enforces WebKit default
> +        settings. The second mode WEBKIT_TLS_MODE_SYSTEM uses
> +        G_TLS_GNUTLS_PRIORITY environment variable value if set by the
> +        system. WEBKIT_TLS_MODE_CUSTOMIZE is used to enable/disable each
> +        protocols from cipher-suites settings. The return value of

Hm. What do you think WEBKIT_TLS_MODE_DEFAULT would be useful for? If the application does not want to customize the ciphersuite settings, and the environment variable is set, why not always follow that? Normally environment variables exist to override hardcoded application settings, also all. The naming is also awkward, because of course respecting the environment variable should be the default behavior, and now you have a mode WEBKIT_TLS_MODE_DEFAULT that is not used by default.

I'm not sure if we need WebKitTLSMode at all. What do you think?

> Source/WebKit/ChangeLog:30
> +        (webkitTLSSettingsGetGNUTLSInitialSuiteString):
> +        (webkitTLSSettingsGetGNUTLSString):
> +        (webkitTLSSettingGetGNUTLSPriorityString): Returns GNUTLS priority string from a WebKitTLSSettings.

GNUTLS -> GnuTLS

> Source/WebKit/UIProcess/API/glib/WebKitTLSSettings.cpp:85
> + * Returns: FALSE when WebKit default settings disable this protocol
> + * for security reason; otherwise TRUE.

Do you need this knowledge for some purpose? I know it's common in some Linux APIs for setter functions to return a value, but that's not typically done in GObject APIs because it can be quite confusing. Rather, I would prefer to add a new function to get the default value.

> Source/WebKit/UIProcess/API/glib/WebKitTLSSettings.cpp:89
> +gboolean webkit_tls_settings_enable_protocol(WebKitTLSSettings *tlsSettings, WebKitTLSProtocol protocol)

Careful: now that we're in the implementation file instead of the public header file, the * goes on the type rather than the variable name. This needs to be fixed in several places.

> Source/WebKit/UIProcess/API/glib/WebKitTLSSettingsPrivate.h:56
> +        if (protocol == WEBKIT_TLS_SSL3_0 || protocol == WEBKIT_TLS_ARCFOUR_128)
> +            return false;

Why return false?

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1686
> + * settings with %WEBKIT_TLS_MODE_CUSTOM. When @tls_mode is
> + * %WEBKIT_TLS_MODE_CUSTOM @tls_settings must be a valid
> + * #WebKitTLSSettings; otherwise, @tls_settings must be %NULL.

You should use g_return_if_fail to assert this.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1693
> + * This method **must be called before any web process has been
> + * created**, as early as possible in your application. Calling it
> + * later will make your application not safe.

I'm not sure how the **emphasized text** will appear in the output documentation. I think gtk-doc actually understands markdown nowadays, so it will probably appear bold. I would drop that.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1699
> +    g_return_if_fail(!context);

g_return_if_fail(!WEBKIT_IS_WEB_CONTEXT(context));

> Source/WebKit/UIProcess/API/gtk/WebKitTLSSettings.h:71
> + * @WEBKIT_TLS_DTLSALL: All versions of DTLS
> + * @WEBKIT_TLS_DTLS0_9: DTLS version 0.9
> + * @WEBKIT_TLS_DTLS1_0: DTLS version 1.0
> + * @WEBKIT_TLS_DTLS2_0: DTLS version 2.0

DTLS?

I'm tempted to say "WebKit should never use DTLS, so this does not need to be exposed." But that won't be true for much longer, because WebRTC is coming (which we'll need to port from OpenSSL/BoringSSL to GTlsConnection). Still, maybe it would be better to not expose the DTLS stuff until that work is done, since it won't work as expected in the meantime.

> Source/WebKit/UIProcess/API/gtk/WebKitWebContext.h:329
> +webkit_web_context_set_tls_settings                 (WebKitWebContext              *context,
> +                                                     WebKitTLSMode                 tls_mode,
> +                                                     WebKitTLSSettings             *tls_settings);

tls_mode should be one space farther to the right. The *s hang one space to the left of the variable names, so the first characters of the names are aligned. It needs fixed in WebKitTLSSettings.h as well.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:774
> +    g_setenv("G_TLS_GNUTLS_PRIORITY", systemPriorityString, 1);

But this test will probably be flaky, since I doubt it's possible to initialize the GTest environment without starting any threads. I could be wrong, but that would surprise me. It's likely impossible to write a test that won't crash randomly, which is not a good sign.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #7)
> But of course,
> that's all up for discussion and debate. (Especially if you're interested in
> implementing it. ;)

I mean, the exact implementation of the API is up for debate. The fact that it needs to be implemented in GLib is pretty clear to me, though. Dan already has a WIP implementation in https://bugzilla.gnome.org/show_bug.cgi?id=745637, so most of the work is probably already done, and just requires someone to finish the job.

[Nael Ouedraogo]

Thanks a lot Michael for the very detailed review and comments. I'm sorry for the late response, I was quite busy.
I checked the Glib bugs you mentioned but unfortunately I'm not very familiar with Glib implementation. So I will probably not be able to contribute rapidly.

I agree that fixing first Glib bug would be the best approach since avoid security and flaky tests issues .

>> Source/WebKit/ChangeLog:14
>> +        cipher-suites. WEBKIT_TLS_MODE_DEFAULT enforces WebKit default
>> +        settings. The second mode WEBKIT_TLS_MODE_SYSTEM uses
>> +        G_TLS_GNUTLS_PRIORITY environment variable value if set by the
>> +        system. WEBKIT_TLS_MODE_CUSTOMIZE is used to enable/disable each
>> +        protocols from cipher-suites settings. The return value of
> Hm. What do you think WEBKIT_TLS_MODE_DEFAULT would be useful for?
My intent was to provide a method to enforce "default" WebKitGTK configuration regardless of the environment variable but as you mentioned this is probably not useful.


> If the application does not want to customize the ciphersuite settings, and the environment variable is set, why not always follow that? Normally environment variables exist to override hardcoded application settings, also all. The naming is also awkward, because of course respecting the environment variable should be the default behavior, and now you have a mode WEBKIT_TLS_MODE_DEFAULT that is not used by default.
> I'm not sure if we need WebKitTLSMode at all. What do you think?
Yes, if we don't need to override system settings with WebKit default settings, we don't need WebKitTLSMode.

> Do you need this knowledge for some purpose? I know it's common in some Linux APIs for setter functions to return a value, but that's not typically done in GObject APIs because it can be quite confusing. Rather, I would prefer to add a new function to get the default value.
The idea is to let application developers check if their TLS configuration is considered as safe or not in WebKit.  For instance, a developer enables a cipher suite initially considered as safe. If later, WebKit considers this cipher suite as unsafe and disables it by default, we should modify webkit_tls_settings_enable_protocol to return false for this cipher suite. Thus, the developer would be warned that this cipher suite is not anymore safe.

[Michael Catanzaro]

(In reply to Nael Ouedraogo from comment #9)
> Thanks a lot Michael for the very detailed review and comments. I'm sorry
> for the late response, I was quite busy.
> I checked the Glib bugs you mentioned but unfortunately I'm not very
> familiar with Glib implementation. So I will probably not be able to
> contribute rapidly.
> 
> I agree that fixing first Glib bug would be the best approach since avoid
> security and flaky tests issues .

OK. It's inching closer to the top of my TODO list, so I'll get around to working on this sooner or later. It will not be an easy project. :(

> > I'm not sure if we need WebKitTLSMode at all. What do you think?
> Yes, if we don't need to override system settings with WebKit default
> settings, we don't need WebKitTLSMode.

Yeah, dropping that makes everything simpler.

> The idea is to let application developers check if their TLS configuration
> is considered as safe or not in WebKit.  For instance, a developer enables a
> cipher suite initially considered as safe. If later, WebKit considers this
> cipher suite as unsafe and disables it by default, we should modify
> webkit_tls_settings_enable_protocol to return false for this cipher suite.
> Thus, the developer would be warned that this cipher suite is not anymore
> safe.

Probably better to add a function to get the defaults, then.