1200802 - Accept SOCKS credentials in proxyInfo object

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Arthur Edelstein [:arthur]]

Attachment: 0001-Accept-RFC1929-SOCKS-credentials-in-proxyInfo.patch

Tor Browser uses SOCKs credentials (RFC 1929) when communicating with the Tor process. For each new connection, a proxyFilter has the opportunity to assign a new SOCKS username/password via the proxyInfo object. We would like to propose upstreaming this patch to mozilla-central.

The attached patch includes Tor Browser's implementation for SOCKS authentication. In this patch, the proxyInfo object is checked for the presence of a SOCKS username/password before connects to the SOCKS server.

This patch does not include a user interface for SOCKS auth, nor a database of credentials for different servers, as discussed in 122752, comment 80.

Comment 1

[Arthur Edelstein [:arthur]]

(The last sentence should have linked to bug 122752, comment 80.)

Comment 2

[Michal Novotny [:michal]]

Comment on attachment 8655622 [details] [diff] [review]
0001-Accept-RFC1929-SOCKS-credentials-in-proxyInfo.patch

Review of attachment 8655622 [details] [diff] [review]:
-----------------------------------------------------------------

So, if I understand correctly the patch is mostly the same as in bug 122752. As I already wrote, user/pass for normal socks proxy (i.e. not Tor) should be obtained from some auth manager or user prompt once we find out in nsSOCKSSocketInfo that the proxy supports it. I.e. user/pass in nsIProxyInfo will be used only by Tor browser. Let's see what Patrick thinks about this change. I'll review the socks part once he review all the proxy changes.

Comment 3

[Patrick McManus [:mcmanus]]

Comment on attachment 8655622 [details] [diff] [review]
0001-Accept-RFC1929-SOCKS-credentials-in-proxyInfo.patch

Review of attachment 8655622 [details] [diff] [review]:
-----------------------------------------------------------------

overall I'm ok with the notion that we have an optional set of default bearer credentials that get sent on first use.. and its even ok that its part of nsIProxyInfo. That ought to work for both socks and http proxies though.. as for where to store those credentials, I'm not sure. prefs seems wonky, but I think you should ask sworkman to help figure out if that's right.

::: netwerk/base/nsIProtocolProxyService.idl
@@ +140,4 @@
>                                in nsIProxyInfo aFailoverProxy);
>  
>      /**
> +     * This method may be called to construct a nsIProxyInfo instance for

you didn't bump the uuid

@@ +161,5 @@
> +     * @param aFailoverProxy
> +     *        Specifies the next proxy to try if this proxy fails.  This
> +     *        parameter may be null.
> +     */
> +    nsIProxyInfo newSOCKSProxyInfo(in AUTF8String aHost, in long aPort,

why isn't this just newProxyInfo2 with auth information added to the old one?

::: netwerk/base/nsIProxyInfo.idl
@@ +51,4 @@
>    readonly attribute unsigned long resolveFlags;
>  
>    /**
> +   * Specifies a SOCKS5 username.

probly not socks5 only, right?

::: netwerk/base/nsProtocolProxyService.cpp
@@ +628,5 @@
>      if (!pref || !strcmp(pref, PROXY_PREF("socks_port")))
>          proxy_GetIntPref(prefBranch, PROXY_PREF("socks_port"), mSOCKSProxyPort);
>  
> +    if (!pref || !strcmp(pref, PROXY_PREF("socks_username")))
> +        proxy_GetStringPref(prefBranch, PROXY_PREF("socks_username"), mSOCKSProxyUsername);

I'm not sure about storing credentials in prefs.. I would want to find someone to help with that design. Maybe ni sworkman.

but as to the prefs themselves, again - not socks specific.

::: netwerk/protocol/http/nsHttpConnectionInfo.cpp
@@ +167,5 @@
>          mHashKey.Append(')');
> +        mHashKey.Append('[');
> +        mHashKey.Append(ProxyUsername());
> +        mHashKey.Append(':');
> +        mHashKey.Append(ProxyPassword());

this hashkey will end up in a bunch of debug logging. not sure if that's a problem, but I'll note it.

Comment 4

[Arthur Edelstein [:arthur]]

Attachment: 0001-Accept-RFC1929-SOCKS-credentials-in-proxyInfo.patch

(In reply to Patrick McManus [:mcmanus] from comment #3)
> Comment on attachment 8655622 [details] [diff] [review]
> 0001-Accept-RFC1929-SOCKS-credentials-in-proxyInfo.patch
> 
> Review of attachment 8655622 [details] [diff] [review]:
> -----------------------------------------------------------------
[...]
> ::: netwerk/base/nsIProtocolProxyService.idl
> @@ +140,4 @@
> >                                in nsIProxyInfo aFailoverProxy);
> >  
> >      /**
> > +     * This method may be called to construct a nsIProxyInfo instance for
> 
> you didn't bump the uuid

Done.

> @@ +161,5 @@
> > +     * @param aFailoverProxy
> > +     *        Specifies the next proxy to try if this proxy fails.  This
> > +     *        parameter may be null.
> > +     */
> > +    nsIProxyInfo newSOCKSProxyInfo(in AUTF8String aHost, in long aPort,
> 
> why isn't this just newProxyInfo2 with auth information added to the old one?

For most proxies, auth info isn't needed, so it's convenient to have a function without them, rather than requiring the caller to provide empty strings.

> ::: netwerk/base/nsIProxyInfo.idl
> @@ +51,4 @@
> >    readonly attribute unsigned long resolveFlags;
> >  
> >    /**
> > +   * Specifies a SOCKS5 username.
> 
> probly not socks5 only, right?

We (Tor Browser) only need a username/password for the SOCKS proxy. I could try to implement username/password for the HTTP proxy, but would it be better to simply throw a "not implemented" exception instead, on the YAGNI principle?

> ::: netwerk/base/nsProtocolProxyService.cpp
> @@ +628,5 @@
> >      if (!pref || !strcmp(pref, PROXY_PREF("socks_port")))
> >          proxy_GetIntPref(prefBranch, PROXY_PREF("socks_port"), mSOCKSProxyPort);
> >  
> > +    if (!pref || !strcmp(pref, PROXY_PREF("socks_username")))
> > +        proxy_GetStringPref(prefBranch, PROXY_PREF("socks_username"), mSOCKSProxyUsername);
> 
> I'm not sure about storing credentials in prefs.. I would want to find
> someone to help with that design. Maybe ni sworkman.

These prefs are not needed by Tor, so I have removed them.

> but as to the prefs themselves, again - not socks specific.
> 
> ::: netwerk/protocol/http/nsHttpConnectionInfo.cpp
> @@ +167,5 @@
> >          mHashKey.Append(')');
> > +        mHashKey.Append('[');
> > +        mHashKey.Append(ProxyUsername());
> > +        mHashKey.Append(':');
> > +        mHashKey.Append(ProxyPassword());
> 
> this hashkey will end up in a bunch of debug logging. not sure if that's a
> problem, but I'll note it.

Good point -- I suppose we could sha256-hash these if you think it is worth it.

Comment 5

[Patrick McManus [:mcmanus]]

> > > +     * @param aFailoverProxy
> > > +     *        Specifies the next proxy to try if this proxy fails.  This
> > > +     *        parameter may be null.
> > > +     */
> > > +    nsIProxyInfo newSOCKSProxyInfo(in AUTF8String aHost, in long aPort,
> > 
> > why isn't this just newProxyInfo2 with auth information added to the old one?
> 
> For most proxies, auth info isn't needed, so it's convenient to have a
> function without them, rather than requiring the caller to provide empty
> strings.

so the function without them would be newProxyInfo and the function with them would be newProxyInfo2 (or newProxyInfoWithAuth())

> 
> > ::: netwerk/base/nsIProxyInfo.idl
> > @@ +51,4 @@
> > >    readonly attribute unsigned long resolveFlags;
> > >  
> > >    /**
> > > +   * Specifies a SOCKS5 username.
> > 
> > probly not socks5 only, right?
> 
> We (Tor Browser) only need a username/password for the SOCKS proxy. I could
> try to implement username/password for the HTTP proxy, but would it be
> better to simply throw a "not implemented" exception instead, on the YAGNI
> principle?
> 

so I think throwing not implemented is fine, but for a different reason - http auth requires more parameters (digest/ntlm/basic) etc..

> > 
> > ::: netwerk/protocol/http/nsHttpConnectionInfo.cpp
> > @@ +167,5 @@
> > >          mHashKey.Append(')');
> > > +        mHashKey.Append('[');
> > > +        mHashKey.Append(ProxyUsername());
> > > +        mHashKey.Append(':');
> > > +        mHashKey.Append(ProxyPassword());
> > 
> > this hashkey will end up in a bunch of debug logging. not sure if that's a
> > problem, but I'll note it.
> 
> Good point -- I suppose we could sha256-hash these if you think it is worth
> it.


I think that makes sense - thanks!

Comment 6

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1200802-Accept-RFC1929-SOCKS-credentials-in-prox.patch

(In reply to Patrick McManus [:mcmanus] from comment #5)
> > > > +     * @param aFailoverProxy
> > > > +     *        Specifies the next proxy to try if this proxy fails.  This
> > > > +     *        parameter may be null.
> > > > +     */
> > > > +    nsIProxyInfo newSOCKSProxyInfo(in AUTF8String aHost, in long aPort,
> > > 
> > > why isn't this just newProxyInfo2 with auth information added to the old one?
> > 
> > For most proxies, auth info isn't needed, so it's convenient to have a
> > function without them, rather than requiring the caller to provide empty
> > strings.
> 
> so the function without them would be newProxyInfo and the function with
> them would be newProxyInfo2 (or newProxyInfoWithAuth())

OK, I have called it newProxyInfoWithAuth().

> > 
> > > ::: netwerk/base/nsIProxyInfo.idl
> > > @@ +51,4 @@
> > > >    readonly attribute unsigned long resolveFlags;
> > > >  
> > > >    /**
> > > > +   * Specifies a SOCKS5 username.
> > > 
> > > probly not socks5 only, right?
> > 
> > We (Tor Browser) only need a username/password for the SOCKS proxy. I could
> > try to implement username/password for the HTTP proxy, but would it be
> > better to simply throw a "not implemented" exception instead, on the YAGNI
> > principle?
> > 
> 
> so I think throwing not implemented is fine, but for a different reason -
> http auth requires more parameters (digest/ntlm/basic) etc..

OK, done.

> > > 
> > > ::: netwerk/protocol/http/nsHttpConnectionInfo.cpp
> > > @@ +167,5 @@
> > > >          mHashKey.Append(')');
> > > > +        mHashKey.Append('[');
> > > > +        mHashKey.Append(ProxyUsername());
> > > > +        mHashKey.Append(':');
> > > > +        mHashKey.Append(ProxyPassword());
> > > 
> > > this hashkey will end up in a bunch of debug logging. not sure if that's a
> > > problem, but I'll note it.
> > 
> > Good point -- I suppose we could sha256-hash these if you think it is worth
> > it.
> 
> 
> I think that makes sense - thanks!

I've added sha256 hashing to the password.

Comment 7

[Patrick McManus [:mcmanus]]

Comment on attachment 8671760 [details] [diff] [review]
0001-Bug-1200802-Accept-RFC1929-SOCKS-credentials-in-prox.patch

Review of attachment 8671760 [details] [diff] [review]:
-----------------------------------------------------------------

I'm ready to r+ this - thanks!

BUT before you check it in, will you do some regression testing of http proxies with and witout basic auth on them. quid would be fine here. It would be good if you tried both configuring explicitly and via pac. Our automated testing of real proxies is almost non existant (partly because most of our infrastructure uses a fake proxy and getting the two to coexist has been more trouble than its worth).

Comment 8

[Arthur Edelstein [:arthur]]

Comment on attachment 8671760 [details] [diff] [review]
0001-Bug-1200802-Accept-RFC1929-SOCKS-credentials-in-prox.patch

(In reply to Patrick McManus [:mcmanus] from comment #7)
> Comment on attachment 8671760 [details] [diff] [review]
> 0001-Bug-1200802-Accept-RFC1929-SOCKS-credentials-in-prox.patch
> 
> Review of attachment 8671760 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm ready to r+ this - thanks!
> 
> BUT before you check it in, will you do some regression testing of http
> proxies with and witout basic auth on them. quid would be fine here. It
> would be good if you tried both configuring explicitly and via pac. Our
> automated testing of real proxies is almost non existant (partly because
> most of our infrastructure uses a fake proxy and getting the two to coexist
> has been more trouble than its worth).

Thanks for the review!

As requested, I ran manual regression tests using squid3. I set it up using the following config file:

  auth_param digest program /usr/lib/squid3/digest_file_auth -c /etc/squid3/passwords
  auth_param digest realm proxy
  acl authenticated proxy_auth REQUIRED
  http_access allow authenticated
  http_port 3128

I entered a proxy user and password with

  htdigest -c /etc/squid3/passwords proxy user

Then I used the Firefox dialog at
  about:preferences > Advanced > Network > Connection Settings
to set the Proxy to "Manual proxy configuration", where I entered an HTTP Proxy with 127.0.0.1:3128.

When I attempted to browse to a web page, I was presented with a password prompt. Entering the correct password resulted in the page successfully loading, and I could monitor pages loading by tailing the squid3 access log file:

  tail -f /var/log/squid3/access.log

I then restarted Firefox and tested the following minimal PAC file (instead of Manual proxy configuration):

  function FindProxyForURL(url, host) {
    return "PROXY 127.0.0.1:3128";
  }

And I got the same behavior: an authentication prompt, followed by normal proxyied connections.

So the next step is to see what :michal thinks of the SOCKS part of this patch.

Comment 9

[Michal Novotny [:michal]]

Comment on attachment 8671760 [details] [diff] [review]
0001-Bug-1200802-Accept-RFC1929-SOCKS-credentials-in-prox.patch

Review of attachment 8671760 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the assertion fixed

::: netwerk/socket/nsSOCKSIOLayer.cpp
@@ +619,4 @@
>  
>      LOGDEBUG(("socks4: checking connection reply"));
> + 
> +    if (mDataLength != 8) {

Why did you change the assertion to a normal if check? This should never happen so we shouldn't try to handle such situation. If it ever happens then ReadFromSocket() doesn't work correctly and assertion is the best way to get notified about it. Please change it back on all places including the new code.

@@ +656,5 @@
>      mDataLength = Buffer<BUFFER_SIZE>(mData)
>                    .WriteUint8(0x05) // version -- 5
> +                  .WriteUint8(0x01) // # of auth methods -- 1
> +                  // Use authenticate iff we have a proxy username.
> +                  .WriteUint8(mProxyUsername.IsEmpty() ? 0x00 : 0x02)

See my comment #80 in bug 122752. I'm not going to block the review on this, but expect this change to happen if we ever support normal socks proxy authentication.

Comment 10

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1200802-Accept-RFC1929-SOCKS-credentials-in-prox.patch

(In reply to Michal Novotny (:michal) from comment #9)
> Comment on attachment 8671760 [details] [diff] [review]
> 0001-Bug-1200802-Accept-RFC1929-SOCKS-credentials-in-prox.patch
> 
> Review of attachment 8671760 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the assertion fixed

Thank you for the review.

> ::: netwerk/socket/nsSOCKSIOLayer.cpp
> @@ +619,4 @@
> >  
> >      LOGDEBUG(("socks4: checking connection reply"));
> > + 
> > +    if (mDataLength != 8) {
> 
> Why did you change the assertion to a normal if check? This should never
> happen so we shouldn't try to handle such situation. If it ever happens then
> ReadFromSocket() doesn't work correctly and assertion is the best way to get
> notified about it. Please change it back on all places including the new
> code.

Good point -- I changed them back and used asserts in similar places in the new code.

> @@ +656,5 @@
> >      mDataLength = Buffer<BUFFER_SIZE>(mData)
> >                    .WriteUint8(0x05) // version -- 5
> > +                  .WriteUint8(0x01) // # of auth methods -- 1
> > +                  // Use authenticate iff we have a proxy username.
> > +                  .WriteUint8(mProxyUsername.IsEmpty() ? 0x00 : 0x02)
> 
> See my comment #80 in bug 122752. I'm not going to block the review on this,
> but expect this change to happen if we ever support normal socks proxy
> authentication.

That sounds reasonable to me.

Attaching the new version, with original author's name. Here are the try server results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ebc410e5c7e

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76cee391a698

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/76cee391a698

Comment 13

[Philip Chee]

(In reply to Carsten Book [:Tomcat] from comment #12)
> https://hg.mozilla.org/mozilla-central/rev/76cee391a698
This changeset lists as author: Ben Bucksch <[e-mail]>
Shouldn't the author be: Arthur Edelstein ?

Comment 14

[Philip Chee]

Oops

Comment 15

[alpha3]

I just tried Firefox 45.  It does not support authentication for SOCKS5 - there is no option to enter username/password, nor does any dialog box pop up to prompt you for the username/password - or am I missing something?

Where do you enter the SOCKS5 username/password on FF 45?

How do I populate the ProxyInfo object with my username/password?

Comment 16

[Ben Bucksch (:BenB)]

Philip Chee, I'm the original author of the patch, so that's fair.
Tomcat, thanks for crediting me in the git commit!
Arthur, thanks for picking up my patch and bringing it to land.
Ben

Comment 17

[Ben Bucksch (:BenB)]

alpha3, as mentioned above, there is no UI for this patch. My patch had at least an about:config preference socks_username and socks_password, but it was removed here in comment 4. So, this feature currently can only be used from an XUL extension using the XPCOM API.

Comment 18

[alpha3]

@benB: ok thanks --- so how do us mere mortals get the XUL extension working... is there an add-on or something?  Or is there some code you can share?

@Arthur Edelstein --- any chance we can reinstate the about:config preference socks_username and socks_password?  This would make this patch more a lot more useful to the general public.

Look forward to seeing FF 46.

Comment 19

[sevenever]

(In reply to Ben Bucksch (:BenB) from comment #17)
> alpha3, as mentioned above, there is no UI for this patch. My patch had at
> least an about:config preference socks_username and socks_password, but it
> was removed here in comment 4. So, this feature currently can only be used
> from an XUL extension using the XPCOM API.

@benB: Is this XPCOM API available in FF50? I am getting error when trying in FF50 developer edition:

const Cc = Components.classes, Ci = Components.interfaces
protocolProxyService = Cc["@mozilla.org/network/protocol-proxy-service;1"].getService(Ci.nsIProtocolProxyService);
protocolProxyService.newProxyInfoWithAuth('socks', 'host', '888', 'user', 'pass', 0, 0, null)
[Exception... "JavaScript component does not have a method named: "newProxyInfoWithAuth"'JavaScript component does not have a method named: "newProxyInfoWithAuth"' when calling method: [nsIProtocolProxyService::newProxyInfoWithAuth]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: debugger eval code :: <TOP_LEVEL> :: line 1"  data: no]

Comment 20

[Eric Jung [:ericjung]]

(In reply to sevenever from comment #19)

> Is this XPCOM API available in FF50? I am getting error when trying
> in FF50 developer edition:

[Exception... "JavaScript component does not have a method named: "newProxyInfoWithAuth"'JavaScript component does not have a method named: "newProxyInfoWithAuth"' when calling method: [nsIProtocolProxyService::newProxyInfoWithAuth]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"

Confirmed with 52.0a2 (2016-12-15) (64-bit) Firefox Developer Edition Aurora Update Channel.