Opened 7 months ago

Closed 7 months ago

#25472 closed defect (fixed)

snowflake-client's `-url` option is intolerant of a missing path

Reported by: dcf Owned by:
Priority: Medium Milestone:
Component: Obfuscation/Snowflake Version:
Severity: Minor Keywords: easy
Cc: dcf, arlolra Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I was following server-webrtc's README and I made a small mistake in the client torrc. Instead of

-url http://127.0.0.1:8080/

I had (no trailing slash):

-url http://127.0.0.1:8080

This caused the client to fail with this error:

2018/03/13 03:10:40 Negotiating via BrokerChannel...
Target URL:   
Front URL:   127.0.0.1:8080
2018/03/13 03:10:40 BrokerChannel Error: dial tcp: unknown port tcp/8080client
2018/03/13 03:10:40 Failed to retrieve answer. Retrying in 10 seconds

This is because the URL to request is built using string concatenation. It built the string "http://127.0.0.1:8080client" instead of "http://127.0.0.1:8080/client".

https://gitweb.torproject.org/pluggable-transports/snowflake.git/tree/client/rendezvous.go?id=ff8f3851082e8f7f8b4c8b99b161be35020aeb67#n83

request, err := http.NewRequest("POST", bc.url.String()+"client", data)

Child Tickets

Change History (8)

comment:1 Changed 7 months ago by dcf

Status: newneeds_review
Summary: snowflake-client's `-url` option is intolerant of a missing trailing slash.snowflake-client's `-url` option is intolerant of a missing path

Here is a patch. It uses the net.url ResolveReference function to construct a new URL whose that is the same as a base URL, except with a different path. This works for the no-path case (e.g. http://127.0.0.1:8080), and also for the case when the base URL has a path other than /, e.g. https://philanthropic.example/snowflake/. The latter example would cause client POSTS to go to https://philanthropic.example/snowflake/client.

The way we're going it already in proxy-go is similar, except we're clobbering the path, rather than resolving it relatively:

https://gitweb.torproject.org/pluggable-transports/snowflake.git/tree/proxy-go/snowflake.go?id=ff8f3851082e8f7f8b4c8b99b161be35020aeb67#n134

	broker := mustParseURL(brokerURL)
	broker.Path = "/proxy"

https://gitweb.torproject.org/pluggable-transports/snowflake.git/tree/proxy-go/snowflake.go?id=ff8f3851082e8f7f8b4c8b99b161be35020aeb67#n159

	broker := mustParseURL(brokerURL)
	broker.Path = "/answer"

If the patch looks good, I think we should change proxy-go to use ResolveReference as well, because it avoids having to repeatedly re-parse brokerURL, and allows the base URL to have a path, as in the example above.

comment:2 Changed 7 months ago by arlolra

If the patch looks good, I think we should change proxy-go to use ResolveReference as well, because it avoids having to repeatedly re-parse brokerURL, and allows the base URL to have a path, as in the example above.

I merged your patch as,
https://gitweb.torproject.org/pluggable-transports/snowflake.git/commit/?id=c61336c897b5d21cc94a21241e98b33df5dcbf78

and attached one for review with the follow up suggested.

comment:3 in reply to:  2 ; Changed 7 months ago by dcf

Replying to arlolra:

If the patch looks good, I think we should change proxy-go to use ResolveReference as well, because it avoids having to repeatedly re-parse brokerURL, and allows the base URL to have a path, as in the example above.

and attached one for review with the follow up suggested.

Your attached patch looks good.

I'm looking at how the browser proxy code does it. It also uses string concatenation, though the broker object constructor ensures that the URL ends in a slash, so you can't get the problem this ticket is about.

https://gitweb.torproject.org/pluggable-transports/snowflake.git/tree/proxy/broker.coffee?id=c61336c897b5d21cc94a21241e98b33df5dcbf78#n24

  constructor: (@url) ->
    @clients = 0
    # Ensure url has the right protocol + trailing slash.
    @url = 'http://' + @url if 0 == @url.indexOf('localhost', 0)
    @url = 'https://' + @url if 0 != @url.indexOf('http', 0)
    @url += '/' if '/' != @url.substr -1

https://gitweb.torproject.org/pluggable-transports/snowflake.git/tree/proxy/broker.coffee?id=c61336c897b5d21cc94a21241e98b33df5dcbf78#n76

  _postRequest: (id, xhr, urlSuffix, payload) =>
    try
      xhr.open 'POST', @url + urlSuffix

For uniformity, we could use the URL constructor, which allows resolving a URL against a base URL, like ResolveReference:

      xhr.open 'POST', new URL(urlSuffix, @url).href

What do you think? I attached a patch. The URL API is marked "experimental"; maybe we don't want to deal with that?

comment:4 in reply to:  3 ; Changed 7 months ago by arlolra

Your attached patch looks good.

Pushed as https://gitweb.torproject.org/pluggable-transports/snowflake.git/commit/?id=42ec097a58cd7a0bf9a6d2e24fe9beeec689c0f4

What do you think? I attached a patch. The URL API is marked "experimental"; maybe we don't want to deal with that?

Could we add a little defensive code in the shims to exit early if the api isn't available? Seems to require node.js v7 and not implemented in IE (but in Edge).

comment:5 in reply to:  4 Changed 7 months ago by dcf

Resolution: fixed
Status: needs_reviewclosed

Replying to arlolra:

Your attached patch looks good.

Pushed as https://gitweb.torproject.org/pluggable-transports/snowflake.git/commit/?id=42ec097a58cd7a0bf9a6d2e24fe9beeec689c0f4

What do you think? I attached a patch. The URL API is marked "experimental"; maybe we don't want to deal with that?

Could we add a little defensive code in the shims to exit early if the api isn't available? Seems to require node.js v7 and not implemented in IE (but in Edge).

Eh, let's not worry about it then. It's not really hurting anything as is, and we may deal with it all in a later refactoring anyway.

Note: See TracTickets for help on using tickets.