Bug 126384 (CVE-2018-11713) - [SOUP] WebSockets must use system proxy settings
Summary: [SOUP] WebSockets must use system proxy settings
Status: RESOLVED FIXED
Alias: CVE-2018-11713
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Critical
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-02 08:11 PST by Dirkjan Ochtman
Modified: 2018-06-04 08:09 PDT (History)
9 users (show)

See Also:


Attachments
Use new libsoup function to get a proxied GIOStream (7.26 KB, patch)
2014-01-02 08:19 PST, Dirkjan Ochtman
no flags Details | Formatted Diff | Diff
Use new libsoup function to get a proxied GIOStream (6.24 KB, patch)
2014-01-02 23:50 PST, Dirkjan Ochtman
no flags Details | Formatted Diff | Diff
Version of the patch that applies to recent WebKit SVN (6.16 KB, patch)
2014-01-03 07:46 PST, Dirkjan Ochtman
no flags Details | Formatted Diff | Diff
WebKit patches for libsoup WebSockets over proxy (9.77 KB, text/plain)
2017-11-17 02:30 PST, Dirkjan Ochtman
no flags Details
libsoup patches for libsoup WebSockets over proxy (22.47 KB, text/plain)
2017-11-17 02:31 PST, Dirkjan Ochtman
no flags Details
Patch (9.89 KB, patch)
2018-01-05 00:15 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch with unit tests (19.50 KB, patch)
2018-01-08 04:28 PST, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirkjan Ochtman 2014-01-02 08:11:22 PST
Currently, it doesn't appear like WebSockets will work with proxies. I'd like to fix that.
Comment 1 Dirkjan Ochtman 2014-01-02 08:19:01 PST
Created attachment 220225 [details]
Use new libsoup function to get a proxied GIOStream

Here's a WIP patch. I'd like for this to get a preliminary review. It probably doesn't apply cleanly to current HEAD, but I'd like to gather some comments on the design.

The patch relies on a new function in libsoup. That work is being tracked in https://bugzilla.gnome.org/show_bug.cgi?id=721343. I was advised there that extracting a GIOStream from libsoup made the most sense, which is why I also replaced the GSocketConnection in SocketStreamHandle by a GIOStream. I'm aware of the changes from bug 88094, so I'm also wondering how that would make sense to integrate.
Comment 2 Carlos Garcia Campos 2014-01-02 08:59:18 PST
(In reply to comment #1)
> Created an attachment (id=220225) [details]
> Use new libsoup function to get a proxied GIOStream

This looks like a libsoup patch, you should use the GNOME bugzilla instead.

> Here's a WIP patch. I'd like for this to get a preliminary review. It probably doesn't apply cleanly to current HEAD, but I'd like to gather some comments on the design.

It's ok to submit WIP patches to bugzilla for early reviews, but do not set the r? flag in such case.

> The patch relies on a new function in libsoup. That work is being tracked in https://bugzilla.gnome.org/show_bug.cgi?id=721343. I was advised there that extracting a GIOStream from libsoup made the most sense, which is why I also replaced the GSocketConnection in SocketStreamHandle by a GIOStream. I'm aware of the changes from bug 88094, so I'm also wondering how that would make sense to integrate.

hmm, I think you submitted the libsoup patch here instead by mistake :-)
Comment 3 Dirkjan Ochtman 2014-01-02 23:50:43 PST
Created attachment 220284 [details]
Use new libsoup function to get a proxied GIOStream
Comment 4 Dirkjan Ochtman 2014-01-02 23:51:30 PST
(In reply to comment #2)
> This looks like a libsoup patch, you should use the GNOME bugzilla instead.

Doh! You're right, of course. I've now attached the correct patch.

> It's ok to submit WIP patches to bugzilla for early reviews, but do not set the r? flag in such case.

Okay, sorry about that!
Comment 5 Martin Robinson 2014-01-03 06:40:38 PST
Comment on attachment 220284 [details]
Use new libsoup function to get a proxied GIOStream

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

> WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:-94
> -    if (url.protocolIs("wss"))
> -        g_socket_client_set_tls(socketClient.get(), TRUE);

Why are you removing this code? Doesn't this break secure web sockets?

> WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:94
> +    soup_session_proxy_connect(session, uri, connectedCallback, m_id);

Is this the only change related to proxy support?

> WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:238
> -static void connectedCallback(GSocketClient* client, GAsyncResult* result, void* id)
> +static void connectedCallback(GIOStream *iostream, GError *error, void* id)

Is the GIOStream here actually a GSocketClient? If so, perhaps it would make more sense to cast it and avoid the rest of your changes.
Comment 6 Dirkjan Ochtman 2014-01-03 06:51:36 PST
(In reply to comment #5)
> (From update of attachment 220284 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220284&action=review
> 
> > WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:-94
> > -    if (url.protocolIs("wss"))
> > -        g_socket_client_set_tls(socketClient.get(), TRUE);
> 
> Why are you removing this code? Doesn't this break secure web sockets?

I doesn't seem to in my testing. Before my patch, SocketStreamHandles rely on the pretty low-level GSocketClient API. By using more libsoup infrastructure, this kind of thing is handled for us. In this case, I think (but have not verified) that libsoup can figure out from the URL that TLS support is required.

> 
> > WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:94
> > +    soup_session_proxy_connect(session, uri, connectedCallback, m_id);
> 
> Is this the only change related to proxy support?

Yes, this is the core of the motivating change.

> > WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:238
> > -static void connectedCallback(GSocketClient* client, GAsyncResult* result, void* id)
> > +static void connectedCallback(GIOStream *iostream, GError *error, void* id)
> 
> Is the GIOStream here actually a GSocketClient? If so, perhaps it would make more sense to cast it and avoid the rest of your changes.

Probably! IIRC danw suggested the use of GIOStream in the soup_session_proxy_connect() API. It seems to me like maybe GSocketConnection makes more sense there. If not, then casting on the WebKit side would be fine as far as I'm concerned.
Comment 7 Dan Winship 2014-01-03 07:02:54 PST
(In reply to comment #5)
> Is the GIOStream here actually a GSocketClient? If so, perhaps it would make more sense to cast it and avoid the rest of your changes.

You mean "actually a GSocketConnection". And yes, it is, but the point of returning a GIOStream rather than a GSocketConnection is that you mostly don't care that it's a GSocketConnection specifically. ie, in the current code, every single place in SocketStreamHandleSoup that uses it casts it to a GIOStream first. So it makes more sense to just have it be an iostream.
Comment 8 Dirkjan Ochtman 2014-01-03 07:46:00 PST
Created attachment 220303 [details]
Version of the patch that applies to recent WebKit SVN
Comment 9 Silvia Pfeiffer 2017-08-09 05:41:49 PDT
Corporate networks often have machines behind a Web proxy. Is this bug going to get addressed, so websockets can be used in corporate networks? I'm particularly curious because WebRTC is about to be released in WebKit/Safari and many WebRTC applications use a websocket signalling server.
Comment 10 Carlos Alberto Lopez Perez 2017-08-09 06:13:19 PDT
(In reply to Silvia Pfeiffer from comment #9)
> Corporate networks often have machines behind a Web proxy. Is this bug going
> to get addressed, so websockets can be used in corporate networks? I'm
> particularly curious because WebRTC is about to be released in WebKit/Safari
> and many WebRTC applications use a websocket signalling server.

The Mac or iOS ports of WebKit (the ones used in Safari) don't use the soup backend for network access. So this bug doesn't affect them.

The libsoup network backend is only used by the GTK+ and WPE ports of WebKit, and WebRTC support on this ports is still a WIP.
Comment 11 Michael Catanzaro 2017-08-09 08:23:11 PDT
Oh wow, if WebKit's implementation of WebSockets does not respect GNOME's proxy settings, that's very bad. These kinds of bugs are what get people arrested or killed.

Dirkjan, are you still watching your bugmail? Do you know what the status of this is?
Comment 12 Michael Catanzaro 2017-08-09 08:24:48 PDT
I've also considered if maybe we should be using libsoup's implementation of WebSockets rather than WebKit's implementation. I always assumed that WebKit's implementation was likely to have fewer bugs. Silly me.
Comment 13 Carlos Alberto Lopez Perez 2017-08-09 09:48:49 PDT
(In reply to Michael Catanzaro from comment #12)
> I've also considered if maybe we should be using libsoup's implementation of
> WebSockets rather than WebKit's implementation. I always assumed that
> WebKit's implementation was likely to have fewer bugs. Silly me.

Do you know something about the quality of the libsoup WebSocket implementation and/or about any project that uses it? 
Keep in mind _any_ implementation is going to have bugs.
Comment 14 Dan Winship 2017-08-09 11:02:23 PDT
(In reply to Carlos Alberto Lopez Perez from comment #13)
> (In reply to Michael Catanzaro from comment #12)
> > I've also considered if maybe we should be using libsoup's implementation of
> > WebSockets rather than WebKit's implementation. I always assumed that
> > WebKit's implementation was likely to have fewer bugs. Silly me.

(Of course, this bug predates libsoup's WebSockets implementation.)

> Do you know something about the quality of the libsoup WebSocket
> implementation and/or about any project that uses it? 
> Keep in mind _any_ implementation is going to have bugs.

The actual WebSockets implementation isn't all that complicated. The code for letting SoupSession make a (possibly-proxied) connection to a server, and then stealing that GIOStream from SoupSession to use as a websockets connection is kind of tricky, but of course, that's exactly the code that the patch here makes use of, so you'd be using it either way. (Well, the patch here is written against Dirkjan's original proposal that isn't quite how things eventually got implemented, but it would be similar.)

Anyway, a handful of people are using libsoup's websockets code now, (and it was originally based on cockpit's code that had been in use for a few years before that). And I assume WebKit has some WebSockets tests anyway that would validate that it was good enough for WebKit purposes?
Comment 15 Carlos Garcia Campos 2017-08-10 04:14:10 PDT
(In reply to Dan Winship from comment #14)
> (In reply to Carlos Alberto Lopez Perez from comment #13)
> > (In reply to Michael Catanzaro from comment #12)
> > > I've also considered if maybe we should be using libsoup's implementation of
> > > WebSockets rather than WebKit's implementation. I always assumed that
> > > WebKit's implementation was likely to have fewer bugs. Silly me.
> 
> (Of course, this bug predates libsoup's WebSockets implementation.)

Yes, that's why we don't use the libsoup one, not because it's more or less buggy, we always prefer to use libsoup (well, except in the case of the disk cache, because the webkit implementation is much better and shared with all other ports).

> > Do you know something about the quality of the libsoup WebSocket
> > implementation and/or about any project that uses it? 
> > Keep in mind _any_ implementation is going to have bugs.
> 
> The actual WebSockets implementation isn't all that complicated. The code
> for letting SoupSession make a (possibly-proxied) connection to a server,
> and then stealing that GIOStream from SoupSession to use as a websockets
> connection is kind of tricky, but of course, that's exactly the code that
> the patch here makes use of, so you'd be using it either way. (Well, the
> patch here is written against Dirkjan's original proposal that isn't quite
> how things eventually got implemented, but it would be similar.)
> 
> Anyway, a handful of people are using libsoup's websockets code now, (and it
> was originally based on cockpit's code that had been in use for a few years
> before that). And I assume WebKit has some WebSockets tests anyway that
> would validate that it was good enough for WebKit purposes?

WebSockets were added to libsoup in 2.50, IIRC, and we depend on 2.40, so we still need to fix our implementation. Then we can use libsoup, but we will have to use ifdefs.
Comment 16 Dirkjan Ochtman 2017-08-10 04:29:29 PDT
Michael: yeah, I'm still reading my bug mail.

We've had this working in our internal WebKit fork, based on an also-forked libsoup pinned to somewhere 2.45ish. If there's interest I can try to dig out our patches.
Comment 17 Michael Catanzaro 2017-08-10 08:19:36 PDT
Yes we're definitely interested, thanks.
Comment 18 Dirkjan Ochtman 2017-11-17 02:30:46 PST
Created attachment 327158 [details]
WebKit patches for libsoup WebSockets over proxy
Comment 19 Dirkjan Ochtman 2017-11-17 02:31:11 PST
Created attachment 327159 [details]
libsoup patches for libsoup WebSockets over proxy
Comment 20 Dirkjan Ochtman 2017-11-17 02:31:58 PST
I made a quick attempt at extracting the relevant patches from our libsoup and WebKit repositories. Hope this helps. If you have questions or something seems missing, let me know and I can have another look.
Comment 21 Claudio Saavedra 2017-11-20 07:11:43 PST
Would you mind submitting the libsoup patches to GNOME bugzilla? Please use separate bug reports if they tackle different issues, as it seems to be the case, then we can see about getting them reviewed and in.
Comment 22 Dirkjan Ochtman 2017-11-20 11:03:14 PST
See also the libsoup bug mentioned in comment 1. I have no further desire to chase these issues, so I'll leave any new work in creating libsoup bugs to others.
Comment 23 Carlos Garcia Campos 2018-01-03 04:45:26 PST
If we need new libsoup API in both cases, then I think it's better to keep current impl for older soup versions and use soup web sockets for newer versions then.
Comment 24 Carlos Garcia Campos 2018-01-04 01:06:12 PST
I've looked at this and we can't really use the libsoup websockets API, because WebKit already implements websockets protocol, the only platform specific part is creating a network connection to send and receive data from. That's lower level than the libsoup websockets API. We can't use soup_session_steal_connection() either, because you are expected to steal the connection when upgrading the protocol once 101 response is returned from the server. However, we need the connection before to properly send the upgrade message. So, we indeed need new API in libsoup to create a connection using ssl and proxy as with any other HTTP connection, but returning the GIOStream for the socket *before* any data is sent.
Comment 25 Carlos Garcia Campos 2018-01-04 01:07:18 PST
Dirkjan, why do you need to force the tunneling in libsoup when creating a connection for the websockets?
Comment 26 Carlos Garcia Campos 2018-01-05 00:15:53 PST
Created attachment 330537 [details]
Patch
Comment 27 Michael Catanzaro 2018-01-05 12:49:20 PST
Comment on attachment 330537 [details]
Patch

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

Thanks for fixing this exceptionally serious issue.

We definitely need to request a CVE. I can handle that.

How hard would it be to extend testWebContextProxySettings in TestWebKitWebContext.cpp to test this?

> Source/WebCore/platform/network/soup/SocketStreamHandleImplSoup.cpp:58
> +#if SOUP_CHECK_VERSION(2, 61, 2)

I see a few different options here:

 (a) We could stick with what you implemented. But I suggest we do not allow web socket connections to ignore proxy settings under any circumstances, certainly not just because libsoup is too old.
 (b) We could ignore our usual dependency policy and require newer libsoup. This would be justified by the severity of this issue. We'd have to send a notice and apology to distributors-list and inform them of the need to update libsoup. Debian would probably refuse. Not great.
 (c) We could disable WebSocket support if libsoup is not new enough. This will break loads of websites.
 (d) We could disable WebSocket support if libsoup is not new enough *and* a system proxy is configured. Websites only break if a proxy is configured.

(d) seems like the best approach to me. What do you think? If we go with (a) or (b) or (c), then we'll certainly need to be backport your new API to libsoup 2.60. But I'm OK with temporarily breaking web sockets for proxy users, so we wouldn't need to do that if we take approach (d).
Comment 28 Carlos Garcia Campos 2018-01-08 04:28:46 PST
Created attachment 330682 [details]
Patch with unit tests
Comment 29 Carlos Garcia Campos 2018-01-08 04:30:45 PST
(In reply to Michael Catanzaro from comment #27)
> Comment on attachment 330537 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330537&action=review
> 
> Thanks for fixing this exceptionally serious issue.
> 
> We definitely need to request a CVE. I can handle that.
> 
> How hard would it be to extend testWebContextProxySettings in
> TestWebKitWebContext.cpp to test this?
> 
> > Source/WebCore/platform/network/soup/SocketStreamHandleImplSoup.cpp:58
> > +#if SOUP_CHECK_VERSION(2, 61, 2)
> 
> I see a few different options here:
> 
>  (a) We could stick with what you implemented. But I suggest we do not allow
> web socket connections to ignore proxy settings under any circumstances,
> certainly not just because libsoup is too old.
>  (b) We could ignore our usual dependency policy and require newer libsoup.
> This would be justified by the severity of this issue. We'd have to send a
> notice and apology to distributors-list and inform them of the need to
> update libsoup. Debian would probably refuse. Not great.
>  (c) We could disable WebSocket support if libsoup is not new enough. This
> will break loads of websites.
>  (d) We could disable WebSocket support if libsoup is not new enough *and* a
> system proxy is configured. Websites only break if a proxy is configured.
> 
> (d) seems like the best approach to me. What do you think? If we go with (a)
> or (b) or (c), then we'll certainly need to be backport your new API to
> libsoup 2.60. But I'm OK with temporarily breaking web sockets for proxy
> users, so we wouldn't need to do that if we take approach (d).

I don't think we should backport any new API to libsoup stable branches. I'm not sure it's possible to do d) either, there might be proxy settings that are acceptable, for example if the websockets host used is blacklisted, or if only https proxy is used. We would need to check the actual proxy settings to decide whether to allow the websocket connection or not.
Comment 30 Michael Catanzaro 2018-01-08 09:00:58 PST
(In reply to Carlos Garcia Campos from comment #29)
> I don't think we should backport any new API to libsoup stable branches. I'm
> not sure it's possible to do d) either, there might be proxy settings that
> are acceptable,

We should fail safe: assume that if the user has set a proxy, no WebSockets connection should ever be attempted.

> for example if the websockets host used is blacklisted,

OK, in that case it really would be safe, but that seems like a very unlikely configuration. Since this is only a fallback codepath, I would not worry about this unlikely case at all.

> or
> if only https proxy is used. 

Another odd case... in that case, wss sockets should still use the https proxy settings, but I guess it's not very much of a privacy issue if no http proxy is set. I suppose it would make sense to block use of ws WebSockets only when an http proxy is set, and to block wss WebSockets only when an https proxy is set.

> We would need to check the actual proxy
> settings to decide whether to allow the websocket connection or not.

We certainly need to check the proxy settings, but I don't think it's important to do as thorough checks as you propose. The question is: how hard is it to check the proxy settings from WebCore/platform? We really should.
Comment 31 Michael Catanzaro 2018-01-08 09:19:41 PST
Comment on attachment 330682 [details]
Patch with unit tests

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

r=me because I can't find any problems with the code, and I like the new test. But I'm still concerned about not blocking WebSocket connections when a proxy is in use in the fallback codepath.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:700
> +    void webSockedConnected(WebSocketServerType serverType)

webSocketConnected

> Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.cpp:56
> +#if SOUP_CHECK_VERSION(2, 50, 0)

Why do we switch to (2, 50, 0) here? What requires libsoup 2.50.0?

> Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.cpp:117
> +    soup_uri_free(uri);

Why are you not able to use GUniquePtr<SoupURI>?

> Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.h:55
> +    SoupURI* m_baseURI { nullptr };
> +#if SOUP_CHECK_VERSION(2, 50, 0)
> +    SoupURI* m_baseWebSocketURI { nullptr };
> +#endif

I'd convert these to use GUniquePtr.
Comment 32 Carlos Garcia Campos 2018-01-09 00:26:51 PST
(In reply to Michael Catanzaro from comment #31)
> Comment on attachment 330682 [details]
> Patch with unit tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330682&action=review
> 
> r=me because I can't find any problems with the code, and I like the new
> test. But I'm still concerned about not blocking WebSocket connections when
> a proxy is in use in the fallback codepath.
> 
> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:700
> > +    void webSockedConnected(WebSocketServerType serverType)
> 
> webSocketConnected

Oops.

> > Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.cpp:56
> > +#if SOUP_CHECK_VERSION(2, 50, 0)
> 
> Why do we switch to (2, 50, 0) here? What requires libsoup 2.50.0?

Because this is generic code, not specific to the web sockets over proxy tests, even if it's only used by that test ATM. WebSockets API used in WebKitTestServer was added in libsoup 2.50

> > Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.cpp:117
> > +    soup_uri_free(uri);
> 
> Why are you not able to use GUniquePtr<SoupURI>?

Because the unit tests don't use WebCore, only the public API and WTF. Soup smart pointers are defined in WebCore, because WTF doesn't depend on soup.

> > Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.h:55
> > +    SoupURI* m_baseURI { nullptr };
> > +#if SOUP_CHECK_VERSION(2, 50, 0)
> > +    SoupURI* m_baseWebSocketURI { nullptr };
> > +#endif
> 
> I'd convert these to use GUniquePtr.

It's not possible.
Comment 33 Carlos Garcia Campos 2018-01-09 08:50:02 PST
libsoup 2.61.2 has been released so I'll have to update the ifdefs.
Comment 34 Carlos Garcia Campos 2018-02-05 01:02:35 PST
Committed r228088: <https://trac.webkit.org/changeset/228088>