Bug 158785 - [SOUP] Stop setting G_TLS_GNUTLS_PRIORITY
Summary: [SOUP] Stop setting G_TLS_GNUTLS_PRIORITY
Status: RESOLVED DUPLICATE of bug 172154
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-15 08:39 PDT by Michael Catanzaro
Modified: 2019-09-28 12:35 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.81 KB, patch)
2016-06-15 08:42 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (5.05 KB, patch)
2016-06-16 07:41 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (5.66 KB, patch)
2016-06-16 07:43 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (5.67 KB, patch)
2016-06-16 07:48 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2016-06-15 08:39:10 PDT
It was needed as a crisis response to disable SSLv3, and later I used it to disable RC4. But if your GnuTLS still allows either of these by default, then you have much more serious problems than SSLv3 or RC4. We can't support outdated GnuTLS; this is a security-sensitive library that has to be kept always at the latest version.

This change brings us into compliance with Fedora crypto requirements, but it's appropriate for all distros. In the future, we will trust GnuTLS to handle TLS crisis response, and it's on distros if they don't update GnuTLS.
Comment 1 Michael Catanzaro 2016-06-15 08:42:18 PDT
Created attachment 281363 [details]
Patch
Comment 2 Carlos Garcia Campos 2016-06-15 08:51:26 PDT
Comment on attachment 281363 [details]
Patch

Shouldn't we bump the gnutls min version required then? also note that this is also used by EFL port, so please confirm EFL devs are ok with this before landing.
Comment 3 Michael Catanzaro 2016-06-15 09:11:29 PDT
(In reply to comment #2)
> Comment on attachment 281363 [details]
> Patch
> 
> Shouldn't we bump the gnutls min version required then? 

I don't think so... this is only one of various security problems that come with using old GnuTLS. Let's expect distros to use a secure version of GnuTLS, and trust GnuTLS to enable secure ciphersuites.

> also note that this
> is also used by EFL port, so please confirm EFL devs are ok with this before
> landing.

CCing.
Comment 4 Carlos Garcia Campos 2016-06-15 23:00:48 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 281363 [details]
> > Patch
> > 
> > Shouldn't we bump the gnutls min version required then? 
> 
> I don't think so... this is only one of various security problems that come
> with using old GnuTLS. Let's expect distros to use a secure version of
> GnuTLS, and trust GnuTLS to enable secure ciphersuites.

Then I don't understand why that was valid before and not now, if we still need that for old versions of gnutls and we still allow to build with those we should keep it. You are assuming WebKitGTK+ is only used by Linux distros, but people building from sources in embedded typically build whatever is required by WebKitGTK+. So, we either bump the requirement or leave whatever is needed for all the versions we still support.

> > also note that this
> > is also used by EFL port, so please confirm EFL devs are ok with this before
> > landing.
> 
> CCing.
Comment 5 Carlos Alberto Lopez Perez 2016-06-16 03:39:56 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Comment on attachment 281363 [details]
> > > Patch
> > > 
> > > Shouldn't we bump the gnutls min version required then? 
> > 
> > I don't think so... this is only one of various security problems that come
> > with using old GnuTLS. Let's expect distros to use a secure version of
> > GnuTLS, and trust GnuTLS to enable secure ciphersuites.
> 
> Then I don't understand why that was valid before and not now, if we still
> need that for old versions of gnutls and we still allow to build with those
> we should keep it. You are assuming WebKitGTK+ is only used by Linux
> distros, but people building from sources in embedded typically build
> whatever is required by WebKitGTK+. So, we either bump the requirement or
> leave whatever is needed for all the versions we still support.
> 

I never thought this was ok in the first place. I don't think is WebKitGTK+ business to modify the default cipher-suites. Neither I think we should require a specific version of GnuTLS just because that version has more secure cipher-suites.

I think this is either the business of GnuTLS, or of the distributions or of the system integrators to ensure they are shipping secure cipher-suites. They may even have legit reasons to want to enable non-secure cipher-suites like SSLv3. For example, I still have DDWRT routers where you have to access a web interface via HTTPS with SSLv3, and good luck trying to change that because is not possible without rebuilding the firmware (which is either just not possible or more work than reasonable).

I never liked this hack of setting an environment variable to force a specific ciphersuite on GnuTLS.
Comment 6 Michael Catanzaro 2016-06-16 07:37:22 PDT
We have to block access to DDWRT routers; they are broken and we can't be compatible with them: allowing access would make the entire Internet insecure. No other browsers allow SSLv3, so there's no expectation or pressure on us to do so. Users who really want to do this can always allow it by setting the environment variable themselves; WebKit only sets it if unset. (But actually, I think the main problem with old routers was usually insecure DH primes; GnuTLS solves that by ordering RSA ciphersuites before DH ciphersuites.)

(In reply to comment #4)
> You are assuming WebKitGTK+ is only used by Linux
> distros, but people building from sources in embedded typically build
> whatever is required by WebKitGTK+.

OK, let's bump the requirement. Honestly, this feels like an attempt to fix stupid -- if the developers don't realize that GnuTLS is security-critical, they probably have bigger problems -- but such as it is....
Comment 7 Michael Catanzaro 2016-06-16 07:41:09 PDT
Created attachment 281456 [details]
Patch
Comment 8 Michael Catanzaro 2016-06-16 07:43:50 PDT
Created attachment 281457 [details]
Patch
Comment 9 Michael Catanzaro 2016-06-16 07:47:19 PDT
The version bump will probably break the GTK and EFL bots, so please, check your bots and upgrade GnuTLS if needed. The bots using Debian testing should be fine, but bots using stable Debian/Ubuntu might discover that these distros are not very good about offering GnuTLS security updates (i.e. you might need to build it from source).
Comment 10 Michael Catanzaro 2016-06-16 07:48:14 PDT
Created attachment 281458 [details]
Patch
Comment 11 Carlos Alberto Lopez Perez 2016-06-16 07:56:15 PDT
(In reply to comment #9)
> The version bump will probably break the GTK and EFL bots, so please, check
> your bots and upgrade GnuTLS if needed. The bots using Debian testing should
> be fine, but bots using stable Debian/Ubuntu might discover that these
> distros are not very good about offering GnuTLS security updates (i.e. you
> might need to build it from source).

Debian stable has GnuTLS 3.3.8.
This *will* break all GTK bots because all run Debian stable.
Comment 12 Michael Catanzaro 2016-08-02 21:14:56 PDT
Let's be conservative and hold off on this until we decide to drop support for Jessie. In the meantime, I've patched it out in Fedora to comply with downstream crypto policy.
Comment 13 Michael Catanzaro 2017-05-15 18:07:54 PDT
Based on discussion with Nikos, we agreed that it's not reasonable for WebKit to expect GnuTLS or glib-networking defaults to be appropriate for a web browser. We should still stop using the G_TLS_GNUTLS_PRIORITY environment variable, but only once new API is added to GLib to allow WebKit to control the set of offered ciphersuites. We should move this code to a central location at the same time, since having it in two different places like we do right now is not good.
Comment 14 Michael Catanzaro 2017-05-15 18:15:14 PDT
(In reply to Michael Catanzaro from comment #13)
> We should move this code to a
> central location at the same time, since having it in two different places
> like we do right now is not good.

Actually, since network process is mandatory nowadays, we can rather stop setting it in the web process. Bug #172154
Comment 15 Adrian Perez 2019-09-28 12:35:47 PDT
This was done ultimately in bug #172154 so I am marking this
one as duplicate.

*** This bug has been marked as a duplicate of bug 172154 ***