TOR_PT_SERVER_TRANSPORT_OPTIONS are not escaped according to pt-spec.txt

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.

changed milestone to %Tor: 0.3.1.x-final

added actualpoints::0 component::core tor/tor milestone::Tor: 0.3.1.x-final owner::nickm pluggable priority::low pt-spec resolution::fixed review-group-18 severity::normal sponsor::M status::closed transports type::defect version::tor unspecified labels

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

Trac:
Keywords: pluggable transports deleted, pluggable transports pt-spec added
Milestone: N/A to Tor: 0.2.6.x-final

Trac:
Keywords: pluggable transports pt-spec deleted, pluggable transports pt-spec 026-triaged-1 added

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

Trac:
Milestone: Tor: 0.2.6.x-final to Tor: 0.2.???

Milestone renamed

Trac:
Milestone: Tor: 0.2.??? to Tor: 0.3.???

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

Trac:
Keywords: pluggable transports pt-spec 026-triaged-1 deleted, pluggable, pt-spec, tor-03-unspecified-201612, transports, 026-triaged-1 added
Milestone: Tor: 0.3.??? to Tor: unspecified

Remove an old triaging keyword.

Trac:
Keywords: tor-03-unspecified-201612 deleted, N/A added

Trac:
Keywords: 026-triaged-1 deleted, N/A added

Trac:
Milestone: Tor: unspecified to Tor: 0.3.1.x-final
Status: new to assigned
Reviewer: N/A to N/A
Owner: N/A to nickm
Sponsor: N/A to N/A
Severity: N/A to Normal

Trac:
Cc: asn to asn, catalyst

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

Trac:
Actualpoints: N/A to 0
Status: assigned to needs_review

Trac:
Keywords: N/A deleted, review-group-18 added

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

Trac:
Status: needs_review to merge_ready

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 "=" 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" } }

Trac:
Cc: asn, catalyst to asn, catalyst, dcf

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?

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.

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?

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.

okay. thanks for the reply! Closing this ticket.

Trac:
Resolution: N/A to fixed
Status: merge_ready to closed

Trac:
Sponsor: N/A to SponsorM

closed

mentioned in issue #22088 (moved)

moved to tpo/core/tor#12931 (closed)