Opened 7 years ago

Closed 4 months ago

Last modified 4 months ago

#1667 closed enhancement (implemented)

Give a more appropriate "I'm not an HTTP proxy" message when we get an HTTP request on the control port

Reported by: nickm Owned by: nickm
Priority: Very Low Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-client, review-group-19
Cc: neenaoffline@… Actual Points:
Parent ID: Points: 0.2
Reviewer: dgoulet Sponsor:

Description

Right now if you accidentally configure your applications to use the control port as an HTTP proxy or a SOCKS proxy, the applications don't get any useful feedback, and the log messages aren't too helpful. They might tell you that _something_ weird happened, but they won't say what.

When possible, then, the control port should detect HTTP and SOCKS requests. It should respond to HTTP request with a "Tor isn't an HTTP Proxy" message to the HTTP request-maker, and should in both cases log the fact that the control port was used as the wrong kind of proxy.

Child Tickets

Change History (29)

comment:1 Changed 7 years ago by arma

Keywords: easy added

comment:2 Changed 7 years ago by nickm

Milestone: Tor: unspecified

comment:3 Changed 7 years ago by cypherpunks

Resolution: fixed
Status: newclosed

comment:4 Changed 7 years ago by rransom

Resolution: fixed
Status: closedreopened

Still present in 0.2.3.0-alpha-dev (git-32796bbe823909b2).

comment:5 Changed 6 years ago by neena

Cc: neenaoffline@… added
Status: reopenedneeds_review

Now checks for GET, POST and CONNECT requests on the control port
and responds/logs appropriately.

My Git branch with the fix:
http://repo.or.cz/w/tor/neena.git/shortlog/refs/heads/fix-1667

comment:6 Changed 6 years ago by nickm

Status: needs_reviewneeds_revision

I think we want to send back an HTTP response, not a raw string, right?

We don't want to do this unless the GET/POST/CONNECT/DELETE/whatever comes as the first thing on the control port; I think this patch might make a GET/POST/CONNECT get treated as HTTP at any point on the control port.

Connection_mark_and_flush() seems right here; we don't want to close the connection until it is done flushing.

We usually write multiline strings as "a string on one line\n"

"followed by a string on another line\n".

Let's try to get something like this into 0.2.4.x?

comment:7 Changed 6 years ago by neena

Milestone: Tor: unspecified

Pushed a commit with the suggested modifications.
The same link should work.

I'm not sure if I like what the message looks like right now.
I could do with some help with that.

comment:8 Changed 6 years ago by neena

Milestone: Tor: unspecified

comment:9 Changed 6 years ago by neena

Status: needs_revisionneeds_review

comment:10 Changed 6 years ago by rransom

Status: needs_reviewneeds_revision

TBB no longer includes an HTTP proxy.

According to the message which Tor's SOCKSPort emits when someone attempts to use it as an HTTP proxy/server, you need to send a larger response in order to ensure that Internet Explorer will display it.

Now the control port will silently eat commands with certain names. It should send a reply.

This still might send an HTTP-like response even if the HTTP-request-like line isn't the first thing send on a connection.

strcasecmp should be the wrong function. Perhaps you want strcasecmpstart?

comment:11 in reply to:  10 ; Changed 6 years ago by neena

Status: needs_revisionneeds_review

Replying to rransom:

According to the message which Tor's SOCKSPort emits when someone attempts to use it as an HTTP proxy/server, you need to send a larger response in order to ensure that Internet Explorer will display it.

Looked at it. It helped me understand a lot of things.
Also, used the same message, with a few modifications.

Now the control port will silently eat commands with certain names. It should send a reply.

This still might send an HTTP-like response even if the HTTP-request-like line isn't the first thing send on a connection.

I misunderstood something the last time. Should be right now.

strcasecmp should be the wrong function. Perhaps you want strcasecmpstart?

Used a switch-case, like the "Tor is not an HTTP Proxy" code instead.

comment:12 in reply to:  11 ; Changed 6 years ago by rransom

Replying to neena:

Replying to rransom:

strcasecmp should be the wrong function. Perhaps you want strcasecmpstart?

Used a switch-case, like the "Tor is not an HTTP Proxy" code instead.

This would be fine except that almost every controller we care about starts its control connections by sending “PROTOCOLINFO\r\n”. You really do need to use something like strcasecmpstart (defined in src/common/util.h and .c; hopefully I'm spelling its name correctly) to compare the first chunk of data with “GET ”, “POST ”, etc.. (The SOCKSPort code only looks at the first byte because the first byte sent on a SOCKS connection must be the SOCKS version number.)

Other than that, looks good!

comment:13 in reply to:  12 Changed 6 years ago by neena

Owner: set to neena
Status: needs_reviewaccepted

Replying to rransom:

This would be fine except that almost every controller we care about starts its control connections by sending “PROTOCOLINFO\r\n”. You really do need to use something like strcasecmpstart (defined in src/common/util.h and .c; hopefully I'm spelling its name correctly) to compare the first chunk of data with “GET ”, “POST ”, etc.. (The SOCKSPort code only looks at the first byte because the first byte sent on a SOCKS connection must be the SOCKS version number.)

Made the change. I had (incorrectly) assumed that TC connections begin with an AUTHENTICATE command. So, I decided to use the switch-case in the "not an HTTP Proxy" code instead of strcasecmpstart.

comment:14 Changed 6 years ago by neena

Status: acceptedneeds_review

comment:15 Changed 6 years ago by nickm

Looks good to me too. Copying your branch into my public repo for archival. We should merge this once 0.2.4.x is open.

comment:16 Changed 6 years ago by nickm

Status: needs_reviewneeds_revision

Hm. Actually, the strcmpstart() stuff in the last couple of commits isn't right yet. strcmpstart can fail if it gets something other than a NUL-terminated string, and neither buf->data nor the output of inspect_evbuffer() is guaranteed to be NUL-terminated. Also, if you want N characters from inspect_evbuffer, you need to tell it to give you N characters: right now, it is only guaranteed to give one character.

comment:17 in reply to:  16 Changed 6 years ago by neena

Status: needs_revisionneeds_review

Replying to nickm:

Hm. Actually, the strcmpstart() stuff in the last couple of commits isn't right yet. strcmpstart can fail if it gets something other than a NUL-terminated string, and neither buf->data nor the output of inspect_evbuffer() is guaranteed to be NUL-terminated. Also, if you want N characters from inspect_evbuffer, you need to tell it to give you N characters: right now, it is only guaranteed to give one character.

Fixed. I think.

comment:18 Changed 5 years ago by nickm

Keywords: tor-client added

comment:19 Changed 5 years ago by nickm

Component: Tor ClientTor

comment:20 Changed 21 months ago by nickm

Points: small
Severity: Normal

comment:21 Changed 5 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.2.x-final

comment:22 Changed 4 months ago by nickm

Keywords: review-group-18 added

comment:23 Changed 4 months ago by nickm

Owner: changed from neena to nickm
Status: needs_reviewassigned

comment:24 Changed 4 months ago by nickm

Keywords: easy review-group-18 removed
Status: assignedneeds_review

I've rebased and squashed neena's original branch, and made a couple of fixes, in branch neena-fix-1667 in my public repository. But now I need another review. :)

comment:25 Changed 4 months ago by nickm

Keywords: review-group-19 added

comment:26 Changed 4 months ago by dgoulet

The fix looks good but tbh I would have put that pile of html in a static const char above the function or something. Else, it increase the size of this function and kind of clobbers the indentation. No strong opinion, just esthetic.

comment:27 Changed 4 months ago by dgoulet

Points: small0.2
Reviewer: dgoulet
Status: needs_reviewneeds_revision

oups... putting in needs_revision but if you are not convinced, that's fine, merge it :).

comment:28 Changed 4 months ago by nickm

Resolution: implemented
Status: needs_revisionclosed

I've extracted both the "not an HTTP Proxy" messages (this one and the one in buffers.c). Now merging!

comment:29 Changed 4 months ago by nickm

I also merged a85ee62e74905af0c685d378be9dfc8ae1935af5 to make the strings static.

Note: See TracTickets for help on using tickets.