Moat: add support for obfsproxy's meek_lite

Trac:
Parent Ticket: #29430 (moved)

added TorBrowserTeam201905R component::applications/tor launcher owner::brade parent::29430 priority::medium resolution::fixed severity::normal status::closed type::defect labels

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.

Trac:
Status: new to needs_review
Keywords: TorBrowserTeam201903 deleted, TorBrowserTeam201903R added

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

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.

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

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.

Moving review tickets to April.

Trac:
Keywords: TorBrowserTeam201903R deleted, TorBrowserTeam201904R added

No April anymore, moving review tickets to May.

Trac:
Keywords: TorBrowserTeam201904R deleted, TorBrowserTeam201905R added

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?

Trac:
Status: needs_review to needs_revision
Keywords: TorBrowserTeam201905R deleted, TorBrowserTeam201905 added

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?

Trac:
Status: needs_revision to needs_information

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.

Trac:
Keywords: TorBrowserTeam201905 deleted, TorBrowserTeam201905R added
Status: needs_information to needs_review

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.