Opened 4 years ago

Closed 3 years ago

#13788 closed defect (fixed)

Meek bridges don't work in TB 4.5alpha1

Reported by: cypherpunks Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: TorBrowserTeam201501R, tbb-4.5-alpha-3, MikePerry201501R
Cc: dcf, mcs, arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Meek bridges (amazon and azure) don't work on both windows7 and wheezy.

Tor launcher error messages:
Tor failed to establish a Tor network connection.
Loading network status failed (done).

Tor log:
[NOTICE] Bootstrapped 5%: Connecting to directory server
[NOTICE] Bootstrapped 10%: Finishing handshake with directory server
[WARN] Problem bootstrapping. Stuck at 10%: Finishing handshake with
directory server. (DONE; DONE; count 1; recommendation warn)
[WARN] 1 connections have failed:
[WARN] 1 connections died in state handshaking (TLS) with SSL state
SSLv2/v3 read server hello A in HANDSHAKE

Meek client Debug messages on Windows:

using helper on 127.0.0.1:49929
listening on 127.0.0.1:49932
error in handling request: WSARecv tcp 127.0.0.1:49934: i/o timeout

Child Tickets

Attachments (5)

nspr-4.0.log (45.7 KB) - added by dcf 4 years ago.
NSPR_LOG_MODULES for Tor Browser 4.0 with meek-azure.
nspr-4.5-alpha-1.log (164.7 KB) - added by dcf 4 years ago.
NSPR_LOG_MODULES for Tor Browser 4.5-alpha-1 with meek-azure.
0001-fixup-Bug-3455.2.-Allow-RFC1929-authentication-usern.patch (14.6 KB) - added by arthuredelstein 4 years ago.
nspr-4.5-alpha-2-comment15.log (149.4 KB) - added by dcf 4 years ago.
NSPR_LOG_MODULES for Tor Browser 4.5-alpha-2 and attachment:0001-fixup-Bug-3455.2.-Allow-RFC1929-authentication-usern.patch​.
0001-fixup-Bug-3455.2.-Allow-RFC1929-authentication-usern.2.patch (15.1 KB) - added by arthuredelstein 3 years ago.

Download all attachments as: .zip

Change History (39)

comment:1 Changed 4 years ago by mcs

Cc: dcf mcs added

I tried all three built-in meek PT options on a Mac OS 10.9.5 system and each one failed (same tor log as above).

Cc: dcf (You can probably reproduce this problem easily, but let me know if you want me to gather any Tor or meek debug logs)

comment:2 Changed 4 years ago by dcf

In the Browser Console (ctrl+J) of the headless helper browser, I see this error:

getFirstPartyURI failed for https://www.torproject.org/dist/torbrowser/update_2/alpha/Linux_x86_64-gcc3/4.5-alpha-1/en-US: 0x80070057
getFirstPartyURI failed for https://ajax.aspnetcdn.com/: 0x80070057
[Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIHttpChannel.responseStatus]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: file:///home/david/tor-browser_en-US/Browser/TorBrowser/Data/Browser/profile.meek-http-helper/extensions/meek-http-helper@bamsoftware.com/components/main.js :: MeekHTTPHelper.HttpStreamListener.prototype.onStopRequest :: line 396"  data: no]

The last line refers to main.js:396:

            status: context.responseStatus,

I found the commit that adds getFirstPartyURI, it's one of the patches on top of esr31:

comment:3 Changed 4 years ago by dcf

I tried setting privacy.thirdparty.isolate=0 in profile.meek-http-helper. It made the getFirstPartyURI errors go away, but the "Component returned failure code: 0x80040111" one remained and it still didn't work.

comment:4 Changed 4 years ago by mcs

I am not sure the getFirstPartyURI log messages are relevant; that is probably just "noise."

Regarding the other problem, looking here:
http://mxr.mozilla.org/mozilla-esr31/source/netwerk/protocol/http/HttpBaseChannel.cpp#1232
it seems like the request did not complete successfully. I am not sure why that would be the case, but I think the onStopRequest code should execute a test like this:

if (Components.isSuccessCode(status)) {

before trying to access responseStatus.

comment:5 in reply to:  4 Changed 4 years ago by dcf

Replying to mcs:

Regarding the other problem, looking here:
http://mxr.mozilla.org/mozilla-esr31/source/netwerk/protocol/http/HttpBaseChannel.cpp#1232
it seems like the request did not complete successfully. I am not sure why that would be the case, but I think the onStopRequest code should execute a test like this:

if (Components.isSuccessCode(status)) {

before trying to access responseStatus.

We already do the isSuccessCode(status) check a couple of lines later, at main.js:398. The error is happening at main.js:396 when trying to index context, not when reading status.

There's something internal to Tor Browser that is blocking the request. It's not just a remote server error—those already happen frequently in normal use and the code handles them. Something (something new in 4.5-alpha-1) is preventing the request from even being sent. We can make the code handle this case gracefully, but we'll also have to find out what prevents the requests from being sent in order for the pluggable transport to work.

Changed 4 years ago by dcf

Attachment: nspr-4.0.log added

NSPR_LOG_MODULES for Tor Browser 4.0 with meek-azure.

Changed 4 years ago by dcf

Attachment: nspr-4.5-alpha-1.log added

NSPR_LOG_MODULES for Tor Browser 4.5-alpha-1 with meek-azure.

comment:6 Changed 4 years ago by dcf

I attached some NSPR logs for 4.0 (works) and 4.5-alpha-1 (doesn't work).

I made them with this patch to meek-client-torbrowser:

@@ -79,6 +79,8 @@ func runFirefox() (cmd *exec.Cmd, stdout io.Reader, err error) {
        if err != nil {
                return
        }
+       os.Setenv("NSPR_LOG_MODULES", "nsHttp:5,nsSocketTransport:5,nsStreamPump:5,nsHostResolver:5")
+       os.Setenv("NSPR_LOG_FILE", "nspr.log")
        cmd = exec.Command(firefoxPath, "-no-remote", "-profile", profilePath)
        cmd.Stderr = os.Stderr
        stdout, err = cmd.StdoutPipe()

I don't really know what I'm looking at, but this looks promising. The 4.0 log has this:

nsSocketTransport::ResolveHost [this=7fbe015e6140 ajax.aspnetcdn.com:443]
nsSocketTransport::SendStatus [this=7fbe015e6140 status=804b0003]
Resolving host [ajax.aspnetcdn.com].
  No usable address in cache for [ajax.aspnetcdn.com]
  DNS thread counters: total=1 any-live=0 idle=0 pending=1
  DNS lookup for host [ajax.aspnetcdn.com] blocking pending 'getaddrinfo' query: callback [7fbe00207650] 
  advancing to STATE_RESOLVING

while the 4.5-alpha-1 log has this:

nsSocketTransport::ResolveHost [this=7fc7bcda9c60 :65535] 
nsSocketTransport::SendStatus [this=7fc7bcda9c60 status=804b0003]
  after event [this=7fc7bcda9c60 cond=8000ffff]

Status 804b003 is NS_BINDING_REDIRECTED and 8000ffff is NS_ERROR_UNEXPECTED (https://developer.mozilla.org/en-US/docs/Mozilla/Errors).

The diff between 4.0.1 and 4.5-alpha-1 has a bunch of changes related to name resolution and proxies.

comment:7 Changed 4 years ago by dcf

I suspect it's related to the changes made for #3455, namely either of

For example comment:39:ticket:3455 says "anyone implementing nsIProtocolProxy is forced to re-examine their code."

comment:8 Changed 4 years ago by gk

Is there an easy way to get at the output of the meek helper extension? Like what it is writing to stdout?

comment:9 Changed 4 years ago by dcf

Here are the essential debugging patches.

This one makes the headless browser not headless, so you can Ctrl+J and view the Browser Console. To apply it, you need to first unzip the xpi.

cd Browser/TorBrowser/Data/Browser/profile.meek-http-helper/extensions
mkdir meek-http-helper@bamsoftware.com
cd meek-http-helper@bamsoftware.com
unzip ../meek-http-helper@bamsoftware.com.xpi
rm ../meek-http-helper@bamsoftware.com
# Now edit meek-http-helper@bamsoftware.com/components/main.js.
--- a/components/main.js
+++ b/components/main.js
@@ -83,13 +83,17 @@ MeekHTTPHelper.prototype = {
 
             // Block forever.
             // https://developer.mozilla.org/en-US/Add-ons/Code_snippets/Threads#Waiting_for_a_background_task_to_complete
+            /*
             var thread = Components.classes["@mozilla.org/thread-manager;1"].getService().currentThread;
             while (true)
                 thread.processNextEvent(true);
+            */
         } finally {
+            /*
             var app = Components.classes["@mozilla.org/toolkit/app-startup;1"]
                 .getService(Components.interfaces.nsIAppStartup);
             app.quit(app.eForceQuit);
+            */
         }
     },

Unfortunately I don't know of a way to view the browser dump messages out of the box, but you can do it by patching meek-client-torbrowser to log those messages. You also have to enable logging by adding the --log option to the ClientTransportPlugin line (note meek-client-torbrowser and meek-client have separate logs). The log will be written to Browser/meek-client-torbrowser.log.

ClientTransportPlugin meek exec ./TorBrowser/Tor/PluggableTransports/meek-client-torbrowser --log meek-client-torbrowser.log -- ./TorBrowser/Tor/PluggableTransports/meek-client
--- a/meek-client-torbrowser/meek-client-torbrowser.go
+++ b/meek-client-torbrowser/meek-client-torbrowser.go
@@ -102,6 +102,7 @@ func grepHelperAddr(r io.Reader) (string, error) {
        scanner := bufio.NewScanner(r)
        for scanner.Scan() {
                line := scanner.Text()
+               log.Print(line)
                if m := helperAddrPattern.FindStringSubmatch(line); m != nil {
                        helperAddr = m[1]
                        break
@@ -116,7 +117,12 @@ func grepHelperAddr(r io.Reader) (string, error) {
                return "", io.EOF
        }
        // Keep reading from the browser to avoid its output buffer filling.
-       go io.Copy(ioutil.Discard, r)
+       go func() {
+               for scanner.Scan() {
+                       line := scanner.Text()
+                       log.Print(line)
+               }
+       }()
        return helperAddr, nil
 }

I usually just symlink Browser/TorBrowser/Tor/PluggableTransports/meek-client-torbrowser to my development build of meek-client-torbrowser when I'm testing something like this. To build meek-client-torbrowser,

export GOPATH=~/go
cd meek/meek-client-torbrowser
go build

There are also some commented log.Printf calls in meek-client itself that log when it tries to make a request. However I think the error happening somewhere inside Firefox; none of the meek code changed since 4.0.

comment:10 in reply to:  9 Changed 4 years ago by dcf

Replying to dcf:

Unfortunately I don't know of a way to view the browser dump messages out of the box, but you can do it by patching meek-client-torbrowser to log those messages. You also have to enable logging by adding the --log option to the ClientTransportPlugin line (note meek-client-torbrowser and meek-client have separate logs). The log will be written to Browser/meek-client-torbrowser.log.

When I do this, I just get the same message on stdout that I got in the browser console.

2014/11/24 15:41:04 running firefox command ["./firefox" "-no-remote" "-profile" "/home/david/tor-browser_en-US/Browser/TorBrowser/Data/Browser/profile.meek-http-helper"]
2014/11/24 15:41:04 firefox started with pid 31954
2014/11/24 15:41:04 meek-http-helper: listen 127.0.0.1:51311
2014/11/24 15:41:04 running meek-client command ["./TorBrowser/Tor/PluggableTransports/meek-client" "--helper" "127.0.0.1:51311"]
2014/11/24 15:41:04 meek-client started with pid 31968
2014/11/24 15:41:06 ************************************************************
2014/11/24 15:41:06 * Call to xpconnect wrapped JSObject produced this error:  *
2014/11/24 15:41:06 [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIHttpChannel.responseStatus]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: file:///home/david/tor-browser_en-US/Browser/TorBrowser/Data/Browser/profile.meek-http-helper/extensions/meek-http-helper@bamsoftware.com/components/main.js :: MeekHTTPHelper.HttpStreamListener.prototype.onStopRequest :: line 396"  data: no]
2014/11/24 15:41:06 ************************************************************

comment:11 Changed 4 years ago by gk

Cc: arthuredelstein added

The offending patch is: https://gitweb.torproject.org/tor-browser.git/commitdiff/e0eaf6b471ae3bbc06066232a00f3b27c2bedeee

And I think I know what is going on: nsSocketTransport::SocketHost() is using the host of the nsIProxyInfo if there is any. And per default meek is creating an HTTP channel with a nsIProxyInfo which is instructing the browser to make a direct connection (type DIRECT). Thus, the host is empty but is nevertheless selected as the socket host as

if (mProxyInfo && !mProxyTransparent) {

is true. I guess we should just check that we not only have an nsIProxyInfo but a non-empty host as well (nsSocketTransport::SocketPort() is affected, too). Arthur?

(dcf - thanks for the logs they were pretty helpful and saved me some time :) )

comment:12 in reply to:  11 Changed 4 years ago by dcf

Replying to gk:

The offending patch is: https://gitweb.torproject.org/tor-browser.git/commitdiff/e0eaf6b471ae3bbc06066232a00f3b27c2bedeee

And I think I know what is going on: nsSocketTransport::SocketHost() is using the host of the nsIProxyInfo if there is any. And per default meek is creating an HTTP channel with a nsIProxyInfo which is instructing the browser to make a direct connection (type DIRECT). Thus, the host is empty but is nevertheless selected as the socket host as

if (mProxyInfo && !mProxyTransparent) {

is true. I guess we should just check that we not only have an nsIProxyInfo but a non-empty host as well (nsSocketTransport::SocketPort() is affected, too). Arthur?

Thanks gk, that's great.

In case anyone is wondering why the code works like that (setting a proxy with a "direct" type instead of simply setting no proxy), it is just a failsafe; see #12674. Just in case something goes wrong and the extension shows a browser window, the global proxy setting in the extension profile is a non-working "blackhole" proxy. That is, the default proxy setting is blackholed, and the extension has to specifically ask to turn off the proxy for every request that it makes.

comment:13 Changed 4 years ago by dcf

I tried this patch (copying the host and port from the URI into the "direct" nsIProxyInfo, rather than using empty values), but it didn't work. It also didnt' work if I hardcoded 443 for the port.

--- a/firefox/components/main.js
+++ b/firefox/components/main.js
@@ -149,3 +149,3 @@ MeekHTTPHelper.lookupStatus = function(status) {
 //   {"type": "socks4a", "host": "example.com", "port": 1080}
-MeekHTTPHelper.buildProxyInfo = function(spec) {
+MeekHTTPHelper.buildProxyInfo = function(spec, uri) {
     // https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIProxyInfo#Constants
@@ -154,3 +154,3 @@ MeekHTTPHelper.buildProxyInfo = function(spec) {
         // "direct"; i.e., no proxy. This is the default.
-        return MeekHTTPHelper.proxyProtocolService.newProxyInfo("direct", "", 0, flags, 0xffffffff, null);
+        return MeekHTTPHelper.proxyProtocolService.newProxyInfo("direct", uri.host, uri.port, flags, 0xffffffff, null);
     } else if (spec.type === "http") {
@@ -198,5 +198,7 @@ MeekHTTPHelper.LocalConnectionHandler.prototype = {
 
+        var uri = MeekHTTPHelper.ioService.newURI(req.url, null, null);
+
         // Check what proxy to use, if any.
         // dump("using proxy " + JSON.stringify(req.proxy) + "\n");
-        var proxyInfo = MeekHTTPHelper.buildProxyInfo(req.proxy);
+        var proxyInfo = MeekHTTPHelper.buildProxyInfo(req.proxy, uri);
         if (proxyInfo === null) {
@@ -208,3 +210,2 @@ MeekHTTPHelper.LocalConnectionHandler.prototype = {
         // https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIHttpChannel
-        var uri = MeekHTTPHelper.ioService.newURI(req.url, null, null);
         this.channel = MeekHTTPHelper.httpProtocolHandler.newProxiedChannel(uri, proxyInfo, 0, null)

It seems that previously the code used !mProxyHost.IsEmpty() as the signal for a proxy being set, not merely mProxyHost being true. Maybe that old condition should be restored?
https://gitweb.torproject.org/tor-browser.git/diff/netwerk/base/src/nsSocketTransport2.h?h=tor-browser-31.2.0esr-4.5-1&id=e0eaf6b471ae3bbc06066232a00f3b27c2bedeee

comment:14 in reply to:  13 ; Changed 4 years ago by arthuredelstein

Replying to dcf:
[snip]

It seems that previously the code used !mProxyHost.IsEmpty() as the signal for a proxy being set, not merely mProxyHost being true. Maybe that old condition should be restored?
https://gitweb.torproject.org/tor-browser.git/diff/netwerk/base/src/nsSocketTransport2.h?h=tor-browser-31.2.0esr-4.5-1&id=e0eaf6b471ae3bbc06066232a00f3b27c2bedeee

I'm posting a fixup here that simplifies that patch (e0eaf6b4) and restores the use of !mProxyHost.IsEmpty() in nsSocketTransport2.*. I did this work in trying to get the patch to land in mozilla-central (still working on that). The patch works for domain isolation, but I haven't had a chance to test it with meek. If anyone wants to give it a try, let me know if this helps.

comment:15 in reply to:  14 ; Changed 4 years ago by dcf

Replying to arthuredelstein:

Replying to dcf:
[snip]

It seems that previously the code used !mProxyHost.IsEmpty() as the signal for a proxy being set, not merely mProxyHost being true. Maybe that old condition should be restored?
https://gitweb.torproject.org/tor-browser.git/diff/netwerk/base/src/nsSocketTransport2.h?h=tor-browser-31.2.0esr-4.5-1&id=e0eaf6b471ae3bbc06066232a00f3b27c2bedeee

I'm posting a fixup here that simplifies that patch (e0eaf6b4) and restores the use of !mProxyHost.IsEmpty() in nsSocketTransport2.*. I did this work in trying to get the patch to land in mozilla-central (still working on that). The patch works for domain isolation, but I haven't had a chance to test it with meek. If anyone wants to give it a try, let me know if this helps.

Thanks, Arthur. I'm starting a build that includes attachment:0001-fixup-Bug-3455.2.-Allow-RFC1929-authentication-usern.patch. (I did "git am" of the patch in tor-browser.git on top of tor-browser-31.3.0esr-4.5-1-build1 and then set "VERIFY_TAGS=0" and "TORBROWSER_TAG=commit hash after git am" in versions.alpha.) I'll test it and see how it works.

If you want to try yourself, it's pretty easy (doesn't need bridge addresses). See https://trac.torproject.org/projects/tor/wiki/doc/meek#Quickstart. If bootstrapping gets past 10%, that means it's working.

comment:16 in reply to:  15 Changed 4 years ago by dcf

Replying to dcf:

Thanks, Arthur. I'm starting a build that includes attachment:0001-fixup-Bug-3455.2.-Allow-RFC1929-authentication-usern.patch. (I did "git am" of the patch in tor-browser.git on top of tor-browser-31.3.0esr-4.5-1-build1 and then set "VERIFY_TAGS=0" and "TORBROWSER_TAG=commit hash after git am" in versions.alpha.) I'll test it and see how it works.

My build is stalled because I hit #13608.

comment:17 in reply to:  15 Changed 4 years ago by dcf

Replying to dcf:

Thanks, Arthur. I'm starting a build that includes attachment:0001-fixup-Bug-3455.2.-Allow-RFC1929-authentication-usern.patch. (I did "git am" of the patch in tor-browser.git on top of tor-browser-31.3.0esr-4.5-1-build1 and then set "VERIFY_TAGS=0" and "TORBROWSER_TAG=commit hash after git am" in versions.alpha.) I'll test it and see how it works.

My build finished; unfortunately it still doesn't work. The NSPR log (attachment:nspr-4.5-alpha-2-comment15.log) at least shows something different though. The name resolution seems to go okay:

Resolving host [ajax.aspnetcdn.com].
STS dispatch [7f227aa8bfc0]
  No usable address in cache for [ajax.aspnetcdn.com]
  DNS thread counters: total=3 any-live=0 idle=0 pending=1
 DNS lookup thread - starting execution.
  DNS lookup for host [ajax.aspnetcdn.com] blocking pending 'getaddrinfo' query: callback [7f227aaa0470]
  advancing to STATE_RESOLVING

But then it quickly gets into a very tight loop of:

STS poll iter [1]
  active [2] { handler=7f227ab6f6e0 condition=0 pollflags=7 }
  active [1] { handler=7f227ab6f500 condition=0 pollflags=5 }
  active [0] { handler=7f2288567e00 condition=0 pollflags=5 }
  calling PR_Poll [active=3 idle=0]
poll timeout: none
    timeout = -1 milliseconds  
    ...returned after 0 milliseconds
nsSocketTransport::OnSocketReady [this=7f227ab6f6e0 outFlags=2]
  advancing to STATE_TRANSFERRING
JIMB: ReleaseFD_Locked: mFDref = 2
nsSocketTransport::SendStatus [this=7f227ab6f6e0 status=804b0004]
nsHttpTransaction::OnSocketStatus [this=7f227d22d000 status=804b0004 progress=0]

And I mean really tight—it made 800 MB of log file in about 20 seconds.

comment:18 in reply to:  14 Changed 4 years ago by gk

Replying to arthuredelstein:

Replying to dcf:
[snip]

It seems that previously the code used !mProxyHost.IsEmpty() as the signal for a proxy being set, not merely mProxyHost being true. Maybe that old condition should be restored?
https://gitweb.torproject.org/tor-browser.git/diff/netwerk/base/src/nsSocketTransport2.h?h=tor-browser-31.2.0esr-4.5-1&id=e0eaf6b471ae3bbc06066232a00f3b27c2bedeee

I'm posting a fixup here that simplifies that patch (e0eaf6b4) and restores the use of !mProxyHost.IsEmpty() in nsSocketTransport2.*. I did this work in trying to get the patch to land in mozilla-central (still working on that). The patch works for domain isolation, but I haven't had a chance to test it with meek. If anyone wants to give it a try, let me know if this helps.

No, alas not. Meek is still not working for me if applying the patch.

Changed 4 years ago by dcf

NSPR_LOG_MODULES for Tor Browser 4.5-alpha-2 and attachment:0001-fixup-Bug-3455.2.-Allow-RFC1929-authentication-usern.patch​.

comment:19 Changed 3 years ago by mikeperry

Keywords: TorBrowserTeam201501 added; meek removed

comment:20 Changed 3 years ago by arthuredelstein

Status: newneeds_review

I'm posting a patch that fixes the meek problem, and also includes the simplification of the SOCKs patch given in my previous attachment.

comment:21 Changed 3 years ago by arthuredelstein

Keywords: TorBrowserTeam201501R added; TorBrowserTeam201501 removed

comment:22 Changed 3 years ago by gk

Works across all platforms, nice. I hope to get to the review today (if not then on Monday).

comment:23 in reply to:  20 ; Changed 3 years ago by dcf

Replying to arthuredelstein:

I'm posting a patch that fixes the meek problem, and also includes the simplification of the SOCKs patch given in my previous attachment.

I compared attachment:0001-fixup-Bug-3455.2.-Allow-RFC1929-authentication-usern.patch and attachment:0001-fixup-Bug-3455.2.-Allow-RFC1929-authentication-usern.2.patch. I guess the important change (wrt this ticket) is the last one, from proxy != NULL to !proxyHost.IsEmpty()?

  {
    nsNSSShutDownPreventionLock locker;
 -  if (forSTARTTLS || haveProxy) {
-+  if (forSTARTTLS || proxy) {
++  nsCString proxyHost;
++  if (proxy) {
++    proxy->GetHost(proxyHost);
++  }
++  if (forSTARTTLS || !proxyHost.IsEmpty()) {
      if (SECSuccess != SSL_OptionSet(fd, SSL_SECURITY, false)) {
        return NS_ERROR_FAILURE;
      }
+   nsresult rv;
+   PRStatus stat;
+ 
++  nsCString proxyHost;
++  if (proxy) {
++    proxy->GetHost(proxyHost);
++  }
++
+   SharedSSLState* sharedState =
+     providerFlags & nsISocketProvider::NO_PERMANENT_STORAGE ? PrivateSSLState() : PublicSSLState();
+   nsNSSSocketInfo* infoObject = new nsNSSSocketInfo(*sharedState, providerFlags);
    // We are going use a clear connection first //
 -  if (forSTARTTLS || haveProxy) {
-+  if (forSTARTTLS || proxy) {
++  if (forSTARTTLS || !proxyHost.IsEmpty()) {
      infoObject->SetHandshakeNotPending();
    }

comment:24 in reply to:  23 Changed 3 years ago by arthuredelstein

Replying to dcf:

Replying to arthuredelstein:

I'm posting a patch that fixes the meek problem, and also includes the simplification of the SOCKs patch given in my previous attachment.


I compared attachment:0001-fixup-Bug-3455.2.-Allow-RFC1929-authentication-usern.patch and attachment:0001-fixup-Bug-3455.2.-Allow-RFC1929-authentication-usern.2.patch. I guess the important change (wrt this ticket) is the last one, from proxy != NULL to !proxyHost.IsEmpty()?

[...]

Yes, exactly.

comment:25 Changed 3 years ago by mikeperry

Keywords: tbb-4.5-alpha-3 added

comment:26 Changed 3 years ago by mikeperry

Keywords: MikePerry201501R added

comment:27 Changed 3 years ago by mikeperry

I am curious about the simplification changes unrelated to meek in this patch. Were those requested by mozilla? It looks like they disable the use of SOCKS username and password pref updates in the proxy service. Was this due to some potential conflict with the nsIProxyInfo members, or for some other reason?

The rest of it seems OK to me.

comment:28 in reply to:  27 Changed 3 years ago by arthuredelstein

Replying to mikeperry:

I am curious about the simplification changes unrelated to meek in this patch. Were those requested by mozilla? It looks like they disable the use of SOCKS username and password pref updates in the proxy service. Was this due to some potential conflict with the nsIProxyInfo members, or for some other reason?

The rest of it seems OK to me.

The simplifications were requested by Mozilla. On the other hand, I probably shouldn't have introduced those changes here as they are unrelated to this bug. So I will post a new patch shortly (after testing) with just the meek-relevant changes.

comment:29 Changed 3 years ago by arthuredelstein

I have now figured out that the first patch I posted here (the "simplifications") fixed the meek breakage in one place, but introduced a new meek breakage in a new place. Argh.

So here's a new patch with the just the necessary changes to restore meek, without the additional breakage:
https://github.com/arthuredelstein/tor-browser/commit/a06da5112ab3a05e2c87fe40ae26d52cfe8bcca9

I've tested this patch and meek is working. Virtually all changes in this latest patch revert lines in our original "Bug #3455.2. Allow RFC1929 authentication (username/password) to SOCKS servers" commit.

comment:30 in reply to:  29 ; Changed 3 years ago by gk

Replying to arthuredelstein:

I have now figured out that the first patch I posted here (the "simplifications") fixed the meek breakage in one place, but introduced a new meek breakage in a new place. Argh.

I am curious: How did it break as it worked for me while testing?

comment:31 Changed 3 years ago by mikeperry

I also noticed that all of these patches demote the nsIProxyInfo member of nsSocketTransport2 from a nsCOMPtr to an unmanaged bare pointer. What was the reason for this change? Unless I'm missing some other far flung magic, I think this is a likely UAF due to the missing refcount management?

comment:32 in reply to:  31 Changed 3 years ago by arthuredelstein

Replying to mikeperry:

I also noticed that all of these patches demote the nsIProxyInfo member of nsSocketTransport2 from a nsCOMPtr to an unmanaged bare pointer. What was the reason for this change? Unless I'm missing some other far flung magic, I think this is a likely UAF due to the missing refcount management?

You're right -- that was a mistake. Here's the new version:
https://github.com/arthuredelstein/tor-browser/commit/bcbe56c40ae214449d4269de63cf9a219e5ec7df

(Tested with meek.)

comment:33 in reply to:  30 Changed 3 years ago by arthuredelstein

Replying to gk:

Replying to arthuredelstein:

I have now figured out that the first patch I posted here (the "simplifications") fixed the meek breakage in one place, but introduced a new meek breakage in a new place. Argh.

I am curious: How did it break as it worked for me while testing?

The first patch I posted here (discussed in comment:14) was written for other purposes (simplification), and you (comment:18) reported that it didn't fix meek. It turns out that patch did (accidentally) fix one meek breakage, but introduce another one.

comment:34 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

This is fixed by commit b491f6d4bacdbc07d8003f8d6a79e4ffeb775988.

Note: See TracTickets for help on using tickets.