[SOUP] WebSockets must use system proxy settings

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Dirkjan Ochtman]

Currently, it doesn't appear like WebSockets will work with proxies. I'd like to fix that.

[Dirkjan Ochtman]

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.

[Carlos Garcia Campos]

(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 :-)

[Dirkjan Ochtman]

Created attachment 220284 [details]
Use new libsoup function to get a proxied GIOStream

[Dirkjan Ochtman]

(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!

[Martin Robinson]

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.

[Dirkjan Ochtman]

(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.

[Dan Winship]

(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.

[Dirkjan Ochtman]

Created attachment 220303 [details]
Version of the patch that applies to recent WebKit SVN

[Silvia Pfeiffer]

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.

[Carlos Alberto Lopez Perez]

(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.

[Michael Catanzaro]

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?

[Michael Catanzaro]

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.

[Carlos Alberto Lopez Perez]

(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.

[Dan Winship]

(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?

[Carlos Garcia Campos]

(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.

[Dirkjan Ochtman]

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.

[Michael Catanzaro]

Yes we're definitely interested, thanks.

[Dirkjan Ochtman]

Created attachment 327158 [details]
WebKit patches for libsoup WebSockets over proxy

[Dirkjan Ochtman]

Created attachment 327159 [details]
libsoup patches for libsoup WebSockets over proxy

[Dirkjan Ochtman]

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.

[Claudio Saavedra]

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.

[Dirkjan Ochtman]

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.

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Dirkjan, why do you need to force the tunneling in libsoup when creating a connection for the websockets?

[Carlos Garcia Campos]

Created attachment 330537 [details]
Patch

[Michael Catanzaro]

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).

[Carlos Garcia Campos]

Created attachment 330682 [details]
Patch with unit tests

[Carlos Garcia Campos]

(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.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

libsoup 2.61.2 has been released so I'll have to update the ifdefs.

[Carlos Garcia Campos]

Committed r228088: <https://trac.webkit.org/changeset/228088>