Opened 3 months ago

Closed 12 days ago

#29627 closed defect (fixed)

Moat: add support for obfsproxy's meek_lite

Reported by: mcs Owned by: brade
Priority: Medium Milestone:
Component: Applications/Tor Launcher Version:
Severity: Normal Keywords: TorBrowserTeam201905R
Cc: Actual Points:
Parent ID: #29430 Points:
Reviewer: Sponsor:

Description

We should improve the Moat client support in Tor Launcher so it will work with obfsproxy's meek_lite implementation (as well as with dcf's meek implementation). The main reason it does not currently work is because the code inside Tor Launcher that interacts with the PT program relies on command line parameters instead of SOCKS args to pass info to the PT.

For more background, see ticket:29430#comment:5 and other related comments within #29430.

Child Tickets

Change History (13)

comment:1 Changed 3 months ago by mcs

Keywords: TorBrowserTeam201903R added; TorBrowserTeam201903 removed
Status: newneeds_review

Here is a patch:
https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug29627-01&id=9ddc818a394623bc21c900fa473a6d0802537733

With these changes, Moat works with both meek-client-torbrowser and obfs4proxy.

comment:2 Changed 3 months ago by dcf

I looked over the patch and it looks reasonable with respect to escaping and everything.

comment:3 Changed 3 months ago by gk

We have meekTransport, meekClientPath, and meekClientArgs as parameters in network-settings.js. I wonder what happened to the meek part in meekTransport in tl-bridgedb.jsm as we only have aTransport/mTransport there. Maybe we don't need meekTransport but just transport in network-seetings.js?

It's not that a big deal, I got just confused while reading over the code.

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

Replying to gk:

We have meekTransport, meekClientPath, and meekClientArgs as parameters in network-settings.js. I wonder what happened to the meek part in meekTransport in tl-bridgedb.jsm as we only have aTransport/mTransport there. Maybe we don't need meekTransport but just transport in network-seetings.js?

It's not that a big deal, I got just confused while reading over the code.

Kathy and I think this is worth fixing. Here is a revised patch:
https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug29627-02&id=99035eb7b6069c60fe4f97291abbea5af4fee886

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

Replying to mcs:

Kathy and I think this is worth fixing. Here is a revised patch:
https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug29627-02&id=99035eb7b6069c60fe4f97291abbea5af4fee886

I incidentally tested this revised patch (with an additional patch to send the utls= SOCKS) arg while testing for comment:12:ticket:29430. Moat worked fine, and had the same TLS fingerprints as meek.

comment:6 Changed 7 weeks ago by gk

Keywords: TorBrowserTeam201904R added; TorBrowserTeam201903R removed

Moving review tickets to April.

comment:7 Changed 3 weeks ago by gk

Keywords: TorBrowserTeam201905R added; TorBrowserTeam201904R removed

No April anymore, moving review tickets to May.

comment:8 Changed 3 weeks ago by gk

Keywords: TorBrowserTeam201905 added; TorBrowserTeam201905R removed
Status: needs_reviewneeds_revision

Okay, I gave it another look. Just one final bit: You write

+    //      If the encoded argument list is less than 255 bytes in

which seems correct to me, but then you do

+      if (this.mMeekClientEscapedArgs.length <= 255)

I guess you want if (this.mMeekClientEscapedArgs.length < 255) instead?

comment:9 in reply to:  8 ; Changed 13 days ago by mcs

Status: needs_revisionneeds_information

Replying to gk:

Okay, I gave it another look. Just one final bit: You write

+    //      If the encoded argument list is less than 255 bytes in

which seems correct to me, but then you do

+      if (this.mMeekClientEscapedArgs.length <= 255)

I guess you want if (this.mMeekClientEscapedArgs.length < 255) instead?

That is a good question. We think the code is correct but the spec is wrong. SOCKS5 supports up to 255 bytes in each auth field. The obfs4proxy code reads a byte to get the length and does not have any other limitations, so up to and including 255 is supported. Kathy and I wanted to maximize the space available for args, so we used <=. Do you think this is OK? Should we file a bug against the PT spec?

comment:10 in reply to:  9 Changed 13 days ago by gk

Keywords: TorBrowserTeam201905R added; TorBrowserTeam201905 removed
Status: needs_informationneeds_review

Replying to mcs:

Replying to gk:

Okay, I gave it another look. Just one final bit: You write

+    //      If the encoded argument list is less than 255 bytes in

which seems correct to me, but then you do

+      if (this.mMeekClientEscapedArgs.length <= 255)

I guess you want if (this.mMeekClientEscapedArgs.length < 255) instead?

That is a good question. We think the code is correct but the spec is wrong. SOCKS5 supports up to 255 bytes in each auth field. The obfs4proxy code reads a byte to get the length and does not have any other limitations, so up to and including 255 is supported. Kathy and I wanted to maximize the space available for args, so we used <=. Do you think this is OK? Should we file a bug against the PT spec?

Yes, please file a spec bug and "<=" is okay.

Last edited 13 days ago by gk (previous) (diff)

comment:11 in reply to:  9 Changed 12 days ago by dcf

Replying to mcs:

That is a good question. We think the code is correct but the spec is wrong. SOCKS5 supports up to 255 bytes in each auth field. The obfs4proxy code reads a byte to get the length and does not have any other limitations, so up to and including 255 is supported. Kathy and I wanted to maximize the space available for args, so we used <=. Do you think this is OK? Should we file a bug against the PT spec?

There's for sure a small ambiguity in the PT spec, when the length of the username field is exactly 255. I almost mentioned it before but thought I was being too nitpicky. Suppose the PT receives a connection with a 255-byte username k=aaa...aaa and a 1-byte password \x00. There are two possible strings with that encoding, and no way to distinguish between them:

  • the 255-byte string k=aaa...aaa
  • the 256-byte string k=aaa...aaa\x00

Obviously in the PT context \x00 is an unlikely byte to appear, so in this case both goptlib and obfs4proxy disambiguate by taking the first interpretation.

To eliminate the ambiguous case, whenever the length of this.mMeekClientEscapedArgs is exactly 255, you could put 254 bytes in the username field and 1 byte in the password field.

Though now that I check, technically the spec doesn't even say how the username and password fields are supposed to be combined, or whether a decoder is even required to look at the password field if the username field is not full. But the existing implementations work by concatenation without regard to the length of the fields, except that a password field consisting of a single \x00 is treated as an empty string. An older version of the spec stated the the fields should be joined by concatenation, but didn't say what to do with an empty password field.

comment:12 Changed 12 days ago by mcs

At the same time dcf made comment:11, I opened #30442.

comment:13 Changed 12 days ago by gk

Resolution: fixed
Status: needs_reviewclosed

Merged with commit 0ce8339e490bb99096ff69cac17d648af028c951 to master. (And thanks dcf for the digging, it's not nitpicky :) ).

Note: See TracTickets for help on using tickets.