Opened 4 years ago

Last modified 18 months ago

#15000 needs_revision defect

bring some sanity to quoted strings in the controller api

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.2.6.3-alpha
Severity: Normal Keywords: tor-client, needs-proposal, tor-control tor-spec
Cc: arthuredelstein, atagar, gk Actual Points:
Parent ID: Points: medium
Reviewer: Sponsor:

Description

#14999 is about a possible mass migration to doing QuotedString right... but we can at least fix the things introduced in #8405, since we *know* they won't break any users.

Child Tickets

Change History (24)

comment:1 Changed 4 years ago by nickm

(I hereby apologize for the non-momentous nature of ticket 15000.)

comment:2 Changed 4 years ago by nickm

Cc: arthuredelstein atagar added
Status: newneeds_review

Branch "bug15000" in my public repo may fix this.

comment:3 Changed 4 years ago by nickm

One thing that breaks with quotedstring in this case: NUL characters in the string. Does this matter? Do we even support this? The control.c implementation is kind of assuming that everything we send out over the control port can be represented as a nul-terminated string.

comment:4 Changed 4 years ago by arthuredelstein

\r and \n are also potentially problematic for control port clients that make simplifying assumptions about the response protocol. (I think I'm guilty of writing one. :P) Maybe tor should reject SOCKS username/password with any dangerous characters? BTW, is a hostile SOCKS client part of the threat model?

Last edited 4 years ago by arthuredelstein (previous) (diff)

comment:5 Changed 4 years ago by nickm

Hmmm. I wonder whether we handle newlines or NULs correctly in the QuotedStrings in our input either...?

comment:6 Changed 4 years ago by nickm

Keywords: needs-proposal added
Milestone: Tor: 0.2.6.x-finalTor: 0.2.7.x-final

After conversation with atagar yesterday, I think we think that we need a better format here: the handling of NULs, control chars, and newlines in QuotedString won't actually work for us on this.

comment:7 Changed 4 years ago by nickm

Status: needs_reviewneeds_revision
Summary: Strings introduced in #8405 should be proper QuotedStringsbring some sanity to quoted strings in the controller api

comment:8 Changed 4 years ago by nickm

#4600 was related

comment:9 Changed 4 years ago by nickm

Keywords: 027-triaged-1-out added

Marking triaged-out items from first round of 0.2.7 triage.

comment:10 Changed 3 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.8.x-final

comment:11 Changed 3 years ago by nickm

Keywords: 028-triaged added

comment:12 Changed 3 years ago by nickm

Points: medium

comment:13 Changed 3 years ago by nickm

Keywords: added; 028-triaged removed
Milestone: Tor: 0.2.8.x-finalTor: 0.2.???

comment:14 Changed 3 years ago by gk

Cc: gk added
Severity: Normal

comment:15 Changed 2 years ago by teor

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

Milestone renamed

comment:16 Changed 2 years ago by nickm

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

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

comment:17 Changed 19 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:18 Changed 19 months ago by nickm

Keywords: 027-triaged-in added

comment:19 Changed 19 months ago by nickm

Keywords: 027-triaged-in removed

comment:20 Changed 19 months ago by nickm

Keywords: 027-triaged-1-out removed

comment:21 Changed 19 months ago by dgoulet

Keywords: controller added

Unify controller keyword to "tor-control".

comment:22 Changed 19 months ago by dgoulet

Keywords: tor-control added; controller removed

Unify "controller" keyword to "tor-control".

comment:23 Changed 19 months ago by dgoulet

Keywords: tor-controller removed

Cleanup remaining "tor-controller" that already have "tor-control"

comment:24 Changed 18 months ago by nickm

Keywords: tor-spec added
Note: See TracTickets for help on using tickets.