Bug 137859 – [SOUP] Disable SSLv3

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

[Michael Catanzaro]

We need to disable SSLv3 in order to fix the POODLE vulnerability.

Discussion on why the environment variable approach was chosen, and why it's not set in the GTK+ port instead of in shared soup code: https://bugzilla.gnome.org/show_bug.cgi?id=738633

Quick security test: https://www.ssllabs.com/ssltest/viewMyClient.html

GnuTLS security advisory: http://www.gnutls.org/security.html#GNUTLS-SA-2014-4

[Michael Catanzaro]

Created attachment 240078 [details]
Patch

[Michael Catanzaro]

If we go with this fix, then we need to notify the EFL port so it gets fixed there too.

[Carlos Garcia Campos]

Comment on attachment 240078 [details]
Patch

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

> Source/WebKit2/ChangeLog:10
> +        * NetworkProcess/gtk/NetworkProcessMainGtk.cpp:
> +        (WebKit::NetworkProcessMainUnix):
> +        Set G_TLS_GNUTLS_PRIORITY if unset.

You should do the same in the web process, since the network process is only used in multi-webprocess model

> Source/WebKit2/NetworkProcess/gtk/NetworkProcessMainGtk.cpp:70
> +    // Disable SSLv3 very early because it is practically impossible to safely
> +    // use setenv() when multiple threads are running, as another thread calling
> +    // getenv() could cause a crash, and many functions use getenv() internally.
> +    // This workaround will stop working if glib-networking switches away from
> +    // GnuTLS or simply stops parsing this variable. We intentionally do not
> +    // overwrite this priority string if it's already set by the user.
> +    // https://bugzilla.gnome.org/show_bug.cgi?id=738633
> +    g_setenv("G_TLS_GNUTLS_PRIORITY", "NORMAL:%COMPAT:!VERS-SSL3.0", FALSE);

You can do it even earlier in NetworkProcessMain.cpp and it will be disabled for EFL too.

[Michael Catanzaro]

Created attachment 240085 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 240085 [details]
Patch

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

It seems this doesn't build for EFL, you probably need to include stdlib.h. I guess it's not possible or easy to tests this?

> Source/WebKit2/ChangeLog:13
> +        * NetworkProcess/EntryPoint/unix/NetworkProcessMain.cpp:
> +        (main):
> +        Set G_TLS_GNUTLS_PRIORITY if unset.
> +        * WebProcess/EntryPoint/unix/WebProcessMain.cpp:
> +        (main):
> +        Set G_TLS_GNUTLS_PRIORITY if unset.

If the comment is the same for all files, move it after the Reviewed by line, as a global comment instead of duplicating it.

[Michael Catanzaro]

(In reply to comment #5)
> I guess it's not possible or easy to tests this?

"Manual verification" (see comment #0). :/  I don't see how we can write a real test case: the only way to ensure the test server uses SSLv3 is with the this same environment variable, but changing the environment will cause the test to randomly crash.

[Michael Catanzaro]

Created attachment 240166 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 240166 [details]
Patch

Clearing flags on attachment: 240166

Committed r174927: <http://trac.webkit.org/changeset/174927>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.