Opened 14 months ago

Closed 10 months ago

Last modified 10 months ago

#32034 closed defect (invalid)

tor reads PT protocol messages from stderr

Reported by: dcf Owned by: neel
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version: Tor: 0.4.2.2-alpha
Severity: Normal Keywords: tor-pt
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Create a fake server transport plugin that writes PT protocol strings to stderr, not stdout:

#!/bin/sh
(
echo "VERSION 1"
echo "SMETHOD testpt 127.0.0.1:9999"
echo "SMETHODS DONE"
) 2>&1

Verify that it writes nothing to stdout:

$ ./testpt.stderr.sh >/dev/null | wc
      0       0       0

Run tor with this torrc:

PublishServerDescriptor 0
AssumeReachable
SOCKSPort 0

ORPort auto
ServerTransportPlugin testpt exec ./testpt.stderr.sh
Bridge testpt 127.0.0.1:9999

A log line shows that tor has interpreted PT protocol messages from stderr, when it should only be looking for them on stdout.

Oct 10 16:21:11.000 [notice] Registered server transport 'testpt' at '127.0.0.1:9999'

I think this is a bug. pt-spec says "All Pluggable Transport Proxies communicate to the parent process via writing NL-terminated lines to stdout." It does not say that tor should inspect stderr and interpret any lines that happen to be a well-formed PT protocol message that it finds there, as if they had been sent on stdout.

The only mention of stderr in the spec is in the context of LOG and STATUS, and that is a fairly recent addition with #28179/#28181. I always thought that the intention was to copy anything seen on stderr to tor's log, as if it had been part of a LOG message on stdout. The mention of stderr in relation to STATUS is something I missed during review of that part of the spec, because I don't think it makes sense there.

What I expected to see was this, treating verbatim stderr lines as things to be logged:

[notice] Unknown line received by managed proxy (VERSION 1)
[notice] Unknown line received by managed proxy (SMETHOD testpt 127.0.0.1:9999)
[notice] Unknown line received by managed proxy (SMETHODS DONE)

Otherwise, tor is essentially holding transport plugins to a contract that is unstated in the spec: transport plugins cannot safely write anything to stderr at all, because there is always a danger that a future version of tor will recognize it as a PT message and interpret it, instead of just logging it.

Child Tickets

Change History (9)

comment:1 Changed 14 months ago by nickm

Keywords: tor-pt added
Milestone: Tor: 0.4.3.x-final

comment:2 Changed 11 months ago by neel

Cc: neel added
Owner: set to neel
Status: newassigned

comment:3 Changed 11 months ago by neel

Cc: neel removed

comment:4 Changed 11 months ago by neel

Status: assignednew

comment:5 Changed 10 months ago by ahf

I think the example script have a bug: The echo's in the subshell writes to standard out and you redirect all output from the subshell from standard error to standard out, where instead it should be redirect all output from the subshell's standard out to standard error, so 1>&2 instead of 2>&1.

The example with wc also shows this, as the redirect to /dev/null should still show the three lines to standard error:

user@tor-dev:~$ ./test-fake-pt.sh > /dev/null | wc -l
0
VERSION 1
SMETHOD testpt 127.0.0.1:9999
SMETHODS DONE

Tor's PT standard error handler does not send the standard error messages into the PT state machine handler and logs the errors with the warning log level. I do think that's a bit higher than what we want for this kind of event, so I'm submitting a patch in a moment that lowers the log level for PT standard error messages from warning to debug.

comment:6 Changed 10 months ago by ahf

PR to lower the log level from warn to debug in https://github.com/torproject/tor/pull/1676 - let's see what CI says.

comment:7 in reply to:  5 ; Changed 10 months ago by dcf

Replying to ahf:

I think the example script have a bug: The echo's in the subshell writes to standard out and you redirect all output from the subshell from standard error to standard out, where instead it should be redirect all output from the subshell's standard out to standard error, so 1>&2 instead of 2>&1.

You're absolutely right. It was my mistake. I apologize for the false report.

Tor's PT standard error handler does not send the standard error messages into the PT state machine handler and logs the errors with the warning log level. I do think that's a bit higher than what we want for this kind of event, so I'm submitting a patch in a moment that lowers the log level for PT standard error messages from warning to debug.

Could we rather close this ticket immediately and do the log level in a separate ticket?

comment:8 in reply to:  7 Changed 10 months ago by ahf

Resolution: invalid
Status: newclosed

Replying to dcf:

Could we rather close this ticket immediately and do the log level in a separate ticket?

Let's do that. I'll create a new ticket for the lowering of the log level in a moment.

comment:9 Changed 10 months ago by ahf

Created #33005 for this.

Note: See TracTickets for help on using tickets.