After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 784786 - WebSockets wss:// URIs try to connect to the server without TLS
WebSockets wss:// URIs try to connect to the server without TLS
Status: RESOLVED FIXED
Product: libsoup
Classification: Core
Component: HTTP Transport
unspecified
Other Linux
: Normal normal
: ---
Assigned To: libsoup-maint@gnome.bugs
libsoup-maint@gnome.bugs
Depends on:
Blocks:
 
 
Reported: 2017-07-11 10:42 UTC by Nirbheek Chauhan
Modified: 2017-08-07 18:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Always use "wss" as an alias for "https" (2.79 KB, patch)
2017-07-12 14:36 UTC, Nirbheek Chauhan
none Details | Review
Always use "wss" as an alias for "https" (6.29 KB, patch)
2017-07-20 07:55 UTC, Nirbheek Chauhan
committed Details | Review

Description Nirbheek Chauhan 2017-07-11 10:42:27 UTC
Secure websockets server running at wss://localhost:8443

> GET  HTTP/1.1
> Soup-Debug-Timestamp: 1499766668
> Soup-Debug: SoupSession 1 (0x7efbf0009100), SoupMessage 1 (0x7efbf000f8c0), SoupSocket 1 (0x7efbf0010d10)
> Host: localhost:8443
> Upgrade: websocket
> Connection: Upgrade
> Sec-WebSocket-Key: NezfMdOSADKw7yBQGZvxeg==
> Sec-WebSocket-Version: 13
> Accept-Encoding: gzip, deflate
> Connection: Keep-Alive
  
< HTTP/1.1 8 Message Corrupt
< Soup-Debug-Timestamp: 1499766668
< Soup-Debug: SoupMessage 1 (0x7efbf000f8c0)


Most likely because there are a number of places in the codebase where TLS is only used if the URI scheme == HTTPS or via soup_uri_is_https(), and "wss" is not an alias for "https".

The workaround is to always use https:// or to set the "https-alias" property on SoupSession.

I was working on a patch, but I am not sure whether to change the relevant instances of soup_uri_is_https() to a new function called soup_uri_is_secure() or to add "wss" as an https alias by default, or to translate "wss" to "https" in soup_session_host_new(), or something else.

Thanks!
Comment 1 Dan Winship 2017-07-12 13:44:46 UTC
I think that changing the default value of https-aliases could cause problems; people assuming it defaults to NULL might just set it to ["https", "davs"] or something without looking at it, and then lose the "wss" value...

But we can change the *meaning* of https-aliases=NULL to be "only TLS-based protocols that SoupSession handles natively are treated as https" (and similarly for non-TLS for http-aliases), and then update soup_uri_is_http/soup_uri_is_https to explicitly consider "ws" and "wss" as well, the same way they handle "http"/"https", separately from the alias values. (So is_http() would return TRUE for ws and FALSE for wss, and is_https() would do the reverse.)
Comment 2 Nirbheek Chauhan 2017-07-12 14:36:01 UTC
Created attachment 355436 [details] [review]
Always use "wss" as an alias for "https"

Also fix a bunch of places that were comparing uri->scheme directly
with SOUP_URI_SCHEME_HTTPS instead of using the private function
soup_uri_is_https().
Comment 3 Dan Winship 2017-07-12 14:44:27 UTC
Comment on attachment 355436 [details] [review]
Always use "wss" as an alias for "https"

>@@ -99,7 +100,7 @@ soup_connection_set_property (GObject *object, guint prop_id,
> 	case PROP_REMOTE_URI:
> 		priv->remote_uri = g_value_dup_boxed (value);
> 		if (priv->remote_uri)
>-			priv->ssl = (priv->remote_uri->scheme == SOUP_URI_SCHEME_HTTPS);
>+			priv->ssl = soup_uri_is_https (priv->remote_uri, NULL);

Hm... this is an improvement, but it's not enough; if SoupSession:http-aliases is set then there might be connections that SoupSession thinks are https, but SoupConnection thinks are plain http.

Probably the fix is to add a PROP_SSL to SoupConnection and have SoupSession set it explicitly rather than having SoupConnection "guess" based on remote_uri

>@@ -1357,9 +1357,11 @@ soup_uri_is_https (SoupURI *uri, char **aliases)

Please update soup_uri_is_http() as well
Comment 4 Nirbheek Chauhan 2017-07-20 07:55:15 UTC
Created attachment 356011 [details] [review]
Always use "wss" as an alias for "https"

Also fix a bunch of places that were comparing uri->scheme directly
with SOUP_URI_SCHEME_HTTPS instead of using the private function
soup_uri_is_https().
Comment 5 Dan Winship 2017-08-07 17:34:58 UTC
Comment on attachment 356011 [details] [review]
Always use "wss" as an alias for "https"

>+out:
>+	g_object_set (conn,
>+		      SOUP_CONNECTION_SSL, soup_uri_is_https (host->uri, priv->https_aliases),
>+		      NULL);
> 	return conn;

This only needs to be set in the code path that first creates the connection.

>-	else if (uri->scheme == SOUP_URI_SCHEME_HTTPS)
>+	else if (uri->scheme == SOUP_URI_SCHEME_HTTPS ||
>+		 uri->scheme == SOUP_URI_SCHEME_WS)

WSS, not WS
Comment 6 Dan Winship 2017-08-07 17:35:31 UTC
pushed with those fixes. Thanks for the patch.

Attachment 356011 [details] pushed as b2f9275 - Always use "wss" as an alias for "https"
Comment 7 Nirbheek Chauhan 2017-08-07 18:25:41 UTC
Thanks, Dan!