Bug 137859

Summary: [SOUP] Disable SSLv3
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Severity: Major CC: cgarcia, commit-queue, gyuyoung.kim, mrobinson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Description Flags
Patch none

Description Michael Catanzaro 2014-10-18 17:36:35 PDT
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
Comment 1 Michael Catanzaro 2014-10-18 17:39:27 PDT
Created attachment 240078 [details]
Comment 2 Michael Catanzaro 2014-10-18 17:43:15 PDT
If we go with this fix, then we need to notify the EFL port so it gets fixed there too.
Comment 3 Carlos Garcia Campos 2014-10-18 23:53:14 PDT
Comment on attachment 240078 [details]

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

You can do it even earlier in NetworkProcessMain.cpp and it will be disabled for EFL too.
Comment 4 Michael Catanzaro 2014-10-19 08:12:57 PDT
Created attachment 240085 [details]
Comment 5 Carlos Garcia Campos 2014-10-19 23:55:26 PDT
Comment on attachment 240085 [details]

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.
Comment 6 Michael Catanzaro 2014-10-20 17:48:19 PDT
(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.
Comment 7 Michael Catanzaro 2014-10-20 17:49:41 PDT
Created attachment 240166 [details]
Comment 8 WebKit Commit Bot 2014-10-21 00:35:04 PDT
Comment on attachment 240166 [details]

Clearing flags on attachment: 240166

Committed r174927: <http://trac.webkit.org/changeset/174927>
Comment 9 WebKit Commit Bot 2014-10-21 00:35:09 PDT
All reviewed patches have been landed.  Closing bug.