Opened 23 months ago

Last modified 6 weeks ago

#22088 new defect

pluggable transport specs need to be more consistent about quoting

Reported by: catalyst Owned by:
Priority: Medium Milestone:
Component: Obfuscation/Pluggable transport Version:
Severity: Normal Keywords: tor-spec, pt
Cc: asn, dcf, serene, arlolra, isis Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

There's some inconsistency among the specs (and code doesn't necessarily match the specs either) about how pluggable transports quote or escape special characters in transport arguments. See #12930 for additional background.

Proposal:

  • Explicitly disallow whitespace (or control characters for that matter) in keys or values of PT arguments. (No PT does this now that I know of, and people with Unix-ish backgrounds are likely to avoid using whitespace in this context anyway.)
  • Explicitly disallow = and \ in keys of PT arguments. (I'm assuming PT designers have more flexibility in choosing keys than value encodings, but if this poses a problem for someone please speak up.)
  • Allow but discourage = in values of PT arguments. (If you encode something in base64 or base32, try to truncate the trailing padding.)
  • Allow but discourage \ in values of PT arguments.
  • Require \ to be escaped by \ (in addition to escaping ,, which is already required) in SMETHOD ARGS and transport lines of extra-info documents. (Almost everywhere else I've seen that uses \ for escaping also requires that \ itself be escaped, and it's closer to what people already expect. goptlib already implemented this despite it not being specified in pt-spec.txt)
  • Do not require = to be escaped by \ in SMETHOD ARGS and transport lines of extra-info documents.
  • Do not require any PT argument characters to be escaped in BridgeDB output or Bridge lines in torrc. (Any \ characters stand for themselves. This requires the fewest changes to existing tor code.)

Child Tickets

Change History (8)

comment:1 Changed 23 months ago by catalyst

Cc: asn dcf serene arlolra added

Add some PT people as CC.

comment:2 Changed 23 months ago by catalyst

Component: Core Tor/TorObfuscation/Pluggable transport
Owner: set to asn

comment:3 Changed 23 months ago by asn

Hello.

So what are the total code changes that would need to happen after these proposed spec changes?

Also, what's the point of "Allow but discourage"?

comment:4 in reply to:  3 Changed 23 months ago by catalyst

Replying to asn:

So what are the total code changes that would need to happen after these proposed spec changes?

BridgeDB would need to transform PT arguments from extra-info escaping to Bridge line escaping. (I think BridgeDB might currently handle escaped , in extra-info incorrectly, but that might not be a problem in practice.)

Existing and future PTs would need to accept unescaped = in their SOCKS client argument values (the ones encoded in the SOCKS authentication fields).

Also, what's the point of "Allow but discourage"?

The point of allowing but discouraging = is because some transports (meek?) could have URLs in their transport arguments. If those URLs might contain query string parameters, these proposed spec changes would allow = in those query parameters to appear without escaping. I guess there are fewer likely uses of \ in query strings, so maybe we should just forbid \ in values as well.

comment:5 Changed 22 months ago by isis

Cc: isis added

comment:6 Changed 21 months ago by catalyst

#12931 is a partial implementation of these proposals.

comment:7 Changed 7 weeks ago by teor

Owner: asn deleted
Status: newassigned

asn does not need to own any obfuscation tickets any more. Default owners are trouble.

comment:8 Changed 6 weeks ago by cohosh

Status: assignednew

tickets were assigned to asn, setting them as unassigned (new) again.

Note: See TracTickets for help on using tickets.