Re: [tor-bugs] #12930 [Obfuscation/Pluggable transport]: Someone, somewhere needs to unescape pluggable transport "SMETHOD ARGS" arguments.

[Tor Bug Tracker & Wiki]

#12930: Someone, somewhere needs to unescape pluggable transport "SMETHOD ARGS"
arguments.
-+-
 Reporter:  yawning  |  Owner:  asn
 Type:  defect   | Status:  new
 Priority:  Medium   |  Milestone:
Component:  Obfuscation/Pluggable transport  |Version:
 Severity:  Normal   | Resolution:
 Keywords:  goptlib  |  Actual Points:
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+-

Comment (by catalyst):

 Replying to [comment:11 dcf]:
 > Replying to [comment:10 catalyst]:
 > > 1.  `SMETHOD ARGS`
 >
 > Is there a place where goptlib doesn't escape `\`? If so it is probably
 a bug. `\` has to be escaped in order to make the escaping reversible,
 even though the spec doesn't explicitly call for it ("Equal signs and
 commas MUST be escaped with a backslash"); my comment in goptlib
 interpolates "[and backslashes]".
 I think we're actually in agreement.  I meant that goptlib does something
 (escaping `\`) that the spec doesn't call for.  I think the escaping is
 reversible even if `\` isn't escaped, as long as everyone is consistent
 (which I think they aren't).  It's more robust to have a syntax where `\`
 gets escaped though, particularly if characters can be optionally escaped.

 > > 5. encoded in SOCKS username/password
 >
 > Here, I felt that the lack of backslashing equals signs was a bug in the
 spec and interpolated into a comment above `parseClientParameters`:
 > {{{
 > // "If a key or value value must contain [an equals sign or] a semicolon
 > // or a backslash, it is escaped with a backslash."
 > }}}
 I would tend to agree, but see #22088 for a possible way to update the
 specs to avoid escaping `=`.  Note that to make `tor` conform to the
 current spec (which requires `=` to be escaped), it might need to do
 additional parsing of the PT arguments (to split them into pairs of keys
 and values) beyond what it does now (treating them as a sequence of space-
 separated words each of which contains an `=` character).

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #12930 [Obfuscation/Pluggable transport]: Someone, somewhere needs to unescape pluggable transport "SMETHOD ARGS" arguments.

[Tor Bug Tracker & Wiki]

#12930: Someone, somewhere needs to unescape pluggable transport "SMETHOD ARGS"
arguments.
-+-
 Reporter:  yawning  |  Owner:  asn
 Type:  defect   | Status:  new
 Priority:  Medium   |  Milestone:
Component:  Obfuscation/Pluggable transport  |Version:
 Severity:  Normal   | Resolution:
 Keywords:  goptlib  |  Actual Points:
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+-

Comment (by dcf):

 Replying to [comment:10 catalyst]:
 > 1.  `SMETHOD ARGS`
 >  - comma-separated key=value pairs
 >  - `,` and `=` (but not `\` -- at least not consistently, e.g. goptlib)
 escaped by `\` ([https://gitweb.torproject.org/torspec.git/tree/pt-
 spec.txt#n566 pt-spec.txt#n566])
 >  - no provision for escaping whitespace

 Is there a place where goptlib doesn't escape `\`? If so it is probably a
 bug. `\` has to be escaped in order to make the escaping reversible, even
 though the spec doesn't explicitly call for it ("Equal signs and commas
 MUST be escaped with a backslash"); my comment in goptlib interpolates
 "[and backslashes]".

 The `encodeSmethodArgs` function has
 {{{
 // "Equal signs and commas [and backslashes] must be escaped with a
 backslash."
 escape := func(s string) string {
 return backslashEscape(s, []byte{'=', ','})
 }
 }}}
 but that doesn't mean `\` isn't itself escaped; `backslashEscape` escapes
 `\` ''in addition to'' the other bytes that are listed.

 > 5. encoded in SOCKS username/password
 >  - semicolon-separated key=value pairs
 >  - `;`, `=`, and `\` escaped by `\`
 ([https://gitweb.torproject.org/torspec.git/tree/pt-spec.txt#n638 pt-
 spec.txt#n638])
 >  - currently, `transports.c` doesn't escape `=`, contrary to spec
 ([https://gitweb.torproject.org/tor.git/tree/src/or/transports.c#n1668
 transports.c#n1668])
 >  - on the other hand,
 [https://gitweb.torproject.org/torspec.git/tree/proposals/180-pluggable-
 transport.txt#n157 180-pluggable-transport.txt#n157] doesn't specify
 escaping of `=` when sending to the transport's SOCKS proxy, so maybe this
 can remain unchanged

 Here, I felt that the lack of backslashing equals signs was a bug in the
 spec and interpolated into a comment above `parseClientParameters`:
 {{{
 // "If a key or value value must contain [an equals sign or] a semicolon
 // or a backslash, it is escaped with a backslash."
 }}}

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #12930 [Obfuscation/Pluggable transport]: Someone, somewhere needs to unescape pluggable transport "SMETHOD ARGS" arguments.

[Tor Bug Tracker & Wiki]

#12930: Someone, somewhere needs to unescape pluggable transport "SMETHOD ARGS"
arguments.
-+-
 Reporter:  yawning  |  Owner:  asn
 Type:  defect   | Status:  new
 Priority:  Medium   |  Milestone:
Component:  Obfuscation/Pluggable transport  |Version:
 Severity:  Normal   | Resolution:
 Keywords:  goptlib  |  Actual Points:
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+-

Comment (by catalyst):

 If there are to be any code changes, they should probably be in the
 BridgeDB implementation and maybe the pluggable transport client of `tor`.
 I like the idea of using something like Bourne shell variable syntax,
 e.g., `a=b=c` sets `$a` to the string  `b=c`. Variable (key) names may not
 contain `=` but values may, without needing quoting or escaping.  I know
 obfs4 worked around the `=` escaping problem by using unpadded base64, but
 could other transports need `=` in argument values in the future? (maybe
 as query parameters to URLs?)

 Here's how I understand the existing data flow:
 1.  `SMETHOD ARGS`
  - comma-separated key=value pairs
  - `,` and `=` (but not `\` -- at least not consistently, e.g. goptlib)
 escaped by `\` ([https://gitweb.torproject.org/torspec.git/tree/pt-
 spec.txt#n566 pt-spec.txt#n566])
  - no provision for escaping whitespace
 2. `transport` lines in `extra-info`
  - comma-separated key=value pairs
 ([https://gitweb.torproject.org/torspec.git/tree/bridgedb-spec.txt#n113
 bridgedb-spec.txt#n113])
  - no specification for escaping any characters
  - apparently copied unchanged from `SMETHOD ARGS`
 ([https://gitweb.torproject.org/tor.git/tree/src/or/transports.c#n1624
 transports.c#n1624]) apart from deleting the `ARGS:` prefix.
 3. BridgeDB (as output to users)
  - space-separated key=value pairs
 ([https://gitweb.torproject.org/torspec.git/tree/bridgedb-spec.txt#n350
 bridgedb-spec.txt#n350])
  - no specification for escaping any characters
 4. `Bridge` line in `torrc` or Tor Browser config dialog
  - space-separated key=value pairs
  - no specification for escaping any characters
 5. encoded in SOCKS username/password
  - semicolon-separated key=value pairs
  - `;`, `=`, and `\` escaped by `\`
 ([https://gitweb.torproject.org/torspec.git/tree/pt-spec.txt#n638 pt-
 spec.txt#n638])
  - currently, `transports.c` doesn't escape `=`, contrary to spec
 ([https://gitweb.torproject.org/tor.git/tree/src/or/transports.c#n1668
 transports.c#n1668])
  - on the other hand,
 [https://gitweb.torproject.org/torspec.git/tree/proposals/180-pluggable-
 transport.txt#n157 180-pluggable-transport.txt#n157] doesn't specify
 escaping of `=` when sending to the transport's SOCKS proxy, so maybe this
 can remain unchanged

 Proposed spec changes will go in a separate ticket.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #12930 [Obfuscation/Pluggable transport]: Someone, somewhere needs to unescape pluggable transport "SMETHOD ARGS" arguments.

[Tor Bug Tracker & Wiki]

#12930: Someone, somewhere needs to unescape pluggable transport "SMETHOD ARGS"
arguments.
-+-
 Reporter:  yawning  |  Owner:  asn
 Type:  defect   | Status:  new
 Priority:  Medium   |  Milestone:
Component:  Obfuscation/Pluggable transport  |Version:
 Severity:  Normal   | Resolution:
 Keywords:  goptlib  |  Actual Points:
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+-

Comment (by catalyst):

 Related ticket: #20341.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #12930 [Obfuscation/Pluggable transport]: Someone, somewhere needs to unescape pluggable transport "SMETHOD ARGS" arguments.

[Tor Bug Tracker & Wiki]

#12930: Someone, somewhere needs to unescape pluggable transport "SMETHOD ARGS"
arguments.
-+-
 Reporter:  yawning  |  Owner:  asn
 Type:  defect   | Status:  new
 Priority:  Medium   |  Milestone:
Component:  Obfuscation/Pluggable transport  |Version:
 Severity:  Normal   | Resolution:
 Keywords:  goptlib  |  Actual Points:
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+-
Changes (by catalyst):

 * cc: catalyst (added)


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #12930 [Obfuscation/Pluggable transport]: Someone, somewhere needs to unescape pluggable transport "SMETHOD ARGS" arguments.

[Tor Bug Tracker & Wiki]

#12930: Someone, somewhere needs to unescape pluggable transport "SMETHOD ARGS"
arguments.
-+-
 Reporter:  yawning  |  Owner:  asn
 Type:  defect   | Status:  new
 Priority:  Medium   |  Milestone:
Component:  Obfuscation/Pluggable transport  |Version:
 Severity:  Normal   | Resolution:
 Keywords:  goptlib  |  Actual Points:
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+-

Comment (by dcf):

 If it helps, here is how goptlib handles args (goptlib being the primary
 implementation of the PT protocol other than tor itself). It's mostly
 based on my own interpretation of what the spec says, but it at least has
 a lot of tests. The comments that quote pt-spec.txt are taken from an
 earlier version of the spec, probably
 [https://gitweb.torproject.org/torspec.git/tree/pt-
 spec.txt?id=4dcd7e94f17c072e771119ec90d7cbce4a4788a4 4dcd7e94f1] from July
 2014.

 [https://gitweb.torproject.org/pluggable-
 transports/goptlib.git/tree/args.go?id=0.7 args.go]: the main functions
 are `parseClientParameters`, `parseServerTransportOptions`, and
 `encodeSmethodArgs` (we haven't yet needed a function to encode client
 parameters). `encodeSmethodArgs` escapes only these three bytes: `\` `=`
 `,`. Other byte values such as `\n` and `\x00` are handled instead in
 pt.go.

 [https://gitweb.torproject.org/pluggable-
 transports/goptlib.git/tree/args_test.go?id=0.7 args_test.go]: is test
 code for the functions in args.go. Please let me know if you have
 additional test cases or if any of the existing ones seem wrong to you.

 [https://gitweb.torproject.org/pluggable-
 transports/goptlib.git/tree/pt.go?id=0.7 pt.go]: interacts a little bit
 with argument syntax in the `formatline` function, which is responsible
 for formatting stdout lines like `SMETHOD`. `formatline` panics (i.e.
 crashes) on any `\n`, `\x00`, or byte value greater than `\x7f` (see the
 `argIsSafe` function). Formerly, goptlib didn't panic but applied an
 additional backslash encoding to these bytes, which Yawning noted in
 comment:1 and has since been removed in favor of panicking.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #12930 [Obfuscation/Pluggable transport]: Someone, somewhere needs to unescape pluggable transport "SMETHOD ARGS" arguments.

[Tor Bug Tracker & Wiki]

#12930: Someone, somewhere needs to unescape pluggable transport "SMETHOD ARGS"
arguments.
-+-
 Reporter:  yawning  |  Owner:  asn
 Type:  defect   | Status:  new
 Priority:  Medium   |  Milestone:
Component:  Obfuscation/Pluggable transport  |Version:
 Severity:  Normal   | Resolution:
 Keywords:  goptlib  |  Actual Points:
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+-
Changes (by catalyst):

 * severity:   => Normal


Comment:

 There are multiple conflicting definitions of pluggable transport
 arguments that probably cannot be made consistent in a backward-compatible
 way.  https://gitweb.torproject.org/torspec.git/tree/pt-spec.txt#n368
 defines the general BNF syntax for a managed transport process-to-parent
 communication, which excludes NUL and NL.  The `SMETHOD` syntax at
 https://gitweb.torproject.org/torspec.git/tree/pt-spec.txt#n544 looks more
 ad-hoc, but it implies that the options are space-separated words.  The
 `ARGS` option is `k=v` pairs separated by commas, and only has provisions
 for escaping commas and equals signs (but not NUL, NL, SP, or backslash).

 Ultimately, the SMETHOD ARGS will end up in a `Bridge` config line,
 described at https://gitweb.torproject.org/torspec.git/tree/proposals/180
 -pluggable-transport.txt#n146.  This config line has space-separated `k=v`
 pairs.  The syntax has provisions for escaping backslash and semicolon but
 not spaces, equals signs, commas, newlines, or NUL characters.  The
 `tor(1)` manual page is out of date and doesn't reflect the prop180 config
 line syntax.  (This is bug #20341.)

 https://gitweb.torproject.org/torspec.git/tree/bridgedb-spec.txt#n93 says
 the arguments are comma-separated `k=v` pairs, but
 https://gitweb.torproject.org/torspec.git/tree/bridgedb-spec.txt#n338 then
 recommends displaying them as space-separated `k=v` pairs.  (This is
 consistent with the prop180 config line syntax.)

 After IRC discussion with arma and Yawning, it seems that the best
 solution may be to amend the specs to disallow certain characters from
 keys or values in transport args, such as NUL, NL, SP, backslash, equals
 sign, comma, and maybe semicolon.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs