Opened 5 years ago

Closed 2 years ago

Last modified 2 years ago

#12931 closed defect (fixed)

TOR_PT_SERVER_TRANSPORT_OPTIONS are not escaped according to pt-spec.txt

Reported by: yawning Owned by: nickm
Priority: Low Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: pluggable, transports, pt-spec, review-group-18
Cc: asn, catalyst, dcf Actual Points: 0
Parent ID: Points:
Reviewer: Sponsor: SponsorM

Description

pt-spec.txt:

      "TOR_PT_SERVER_TRANSPORT_OPTIONS" -- A semicolon-separated list
       of <key>:<value> pairs, where <key> is a transport name and
       <value> is a k=v string value with options that are to be passed
       to the transport. Colons, semicolons, equal signs and backslashes
       MUST be escaped with a backslash. TOR_PT_SERVER_TRANSPORT_OPTIONS
       is optional and might not be present in the environment of the
       proxy if no options are need to be passed to transports.

or/transports.c:get_transport_options_for_server_proxy()

      char *escaped_opts = tor_escape_str_for_pt_args(options, ":;\\");

Equal signs are not escaped. I'm not sure if anything will break with different behavior.

Child Tickets

Change History (19)

comment:1 Changed 5 years ago by nickm

Keywords: pt-spec added
Milestone: Tor: 0.2.6.x-final

I think the fix here is just to fix pt-spec?

comment:2 Changed 5 years ago by nickm

Keywords: 026-triaged-1 added

comment:3 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.???

Defer some items from 0.2.6. They are mostly worth doing, but not going to happen super-fast.

comment:4 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:5 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:6 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:7 Changed 2 years ago by nickm

Keywords: 026-triaged-1 removed

comment:8 Changed 2 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.1.x-final
Owner: set to nickm
Severity: Normal
Status: newassigned

comment:9 Changed 2 years ago by catalyst

Cc: catalyst added

comment:10 Changed 2 years ago by nickm

Actual Points: 0
Status: assignedneeds_review

Proposed patch as bug12931 in my torspec repository; needs review.

comment:11 Changed 2 years ago by nickm

Keywords: review-group-18 added

comment:12 Changed 2 years ago by catalyst

Status: needs_reviewmerge_ready

This looks good to me. I was worried we might paint ourselves into a corner with respect to #12930 and #22088, but your changes look compatible with several reasonable options.

comment:13 Changed 2 years ago by dcf

Cc: dcf added

This would make it impossible for a key or a k to contains an equals sign; I guess that's all right.

Does the same inconsistency exist with regard to client SOCKS arguments?

First the "<Key>=<Value>" formatted arguments MUST be escaped, such that all backslash, equal sign, and semicolon characters are escaped with a backslash.

What is an implementation supposed to do when it finds a backslash that precedes something other than colon, semicolon, or backslash? That is, colon, semicolon, and backslash MUST be backslash-escaped, but MAY other characters be backslash-escaped? Also: are equals signs forbidden in v, or can you have unescaped ones as in t:k=base64==?

For what it's worth, goptlib allows any character to be backslash-escaped (and for that reason allows = in k). There is a test for it; I'd like to know if this specification change would require any change in the expected output. Input:

t:k\:1=v;t:k\=2=v;t:k\;3=v;t:k\\4=v

Expected output:

{ "t": { "k:1": "v", "k=2": "v", "k;3": "v", "k\\4": "v" } }

comment:14 Changed 2 years ago by nickm

So, this change is just to make the spec match the code. And if I'm reading the code right, the only escaped values are \, ;, and :. We have never specified what happens if a different character is escaped.

= signs should not be forbidden in v; does the spec imply otherwise?

Last edited 2 years ago by nickm (previous) (diff)

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

Replying to nickm:

So, this change is just to make the spec match the code.

You're right. My questions are outside the scope of this ticket.

comment:16 Changed 2 years ago by nickm

okay. merging this one; do you have time to open a different ticket for the remaining issue and just copy-and paste what you said above?

comment:17 Changed 2 years ago by dcf

It's not important. I wanted to know if I needed to change any code in response to the spec change, and it looks like I don't have to.

comment:18 Changed 2 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

okay. thanks for the reply! Closing this ticket.

comment:19 Changed 2 years ago by catalyst

Sponsor: SponsorM
Note: See TracTickets for help on using tickets.