Opened 5 years ago

Closed 4 years ago

#14135 closed defect (fixed)

Incorrect SocksListener temporary error check

Reported by: adam-p Owned by: dcf
Priority: Low Milestone:
Component: Circumvention/Pluggable transport Version:
Severity: Keywords: goptlib
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

You can see the code in question -- in acceptLoop() -- here:
https://gitweb.torproject.org/pluggable-transports/meek.git/tree/meek-client/meek-client.go#n300

The existing code will treat an error that is not a net.Error as temporary, which is the reverse of how it should be. See the diff to fix it below.

Note that it is highly unlikely (impossible?) that the existing code would have caused an actual problem.

(The code in question is probably based on [the example](https://github.com/Yawning/goptlib/blob/master/socks.go#L76) in the goptlib code. I have also submitted a [pull request](https://github.com/Yawning/goptlib/pull/1) to fix that example.)

  • meek-client/meek-client.go

    diff --git a/meek-client/meek-client.go b/meek-client/meek-client.go
    index 280ab1b..bb6c0b1 100644
    a b func acceptLoop(ln *pt.SocksListener) error { 
    303303               conn, err := ln.AcceptSocks()
    304304               if err != nil {
    305305                       log.Printf("error in AcceptSocks: %s", err)
    306                        if e, ok := err.(net.Error); ok && !e.Temporary() {
    307                                return err
     306                       if e, ok := err.(net.Error); ok && e.Temporary() {
     307
     308                               continue
    308309                       }
    309                        continue
     310                       return err
    310311               }
    311312               go func() {
    312313                       err := handler(conn)

Child Tickets

Change History (8)

comment:1 Changed 5 years ago by dcf

Thanks for looking at this.

Currently, the code works like this:

non-net.Error:                try again
    net.Error, Temporary:     try again
    net.Error, non-Temporary: quit

The patch changes it to be this:

non-net.Error:                quit
    net.Error, Temporary:     try again
    net.Error, non-Temporary: quit

The problem is that at least once in the past, we wanted to try again on a non-net.Error. See this commit message:
https://gitweb.torproject.org/pluggable-transports/goptlib.git/commit/?id=3030f080eecf72b0e896236fca5fabd245c00bdb
Before that commit (and with the patch in this ticket), you can kill a client process by sending something unexpected in the SOCKS request, for example:

socat -v - SOCKS4A:127.0.0.1:1.1.1.1:2222,socksport=<meek-client port>,socksuser=bogus

The error you get is this, which comes from lower level goptlib code, is not a net.Error:

2015/01/09 12:40:59 no equals sign in "bogus"

We can agree that we shouldn't kill the process in this case. But there are a few ways we could do it. One way is to continue the way that it works now, and document that errors that are not net.Errors should be treated as Temporary. Another way is for AcceptSocks to intercept the error returned by readSocks4aConnect and, if it is not an net.Error, transform it into a Temporary net.Error. Failure to read/parse the connect message is arguably a network error.

I think I would prefer a patch to do the second thing. Basically everywhere in readSocks4aConnect that returns fmt.Errorf, return a Temporary net.Error instead. The errors from underlying network calls should be returned without alteration. We should make sure that errors in other functions are consistent, and also see what the standard library does in similar cases (i.e., how does it construct Temporary errors).

The upstream for goptlib is https://gitweb.torproject.org/pluggable-transports/goptlib.git/ BTW.

comment:2 Changed 5 years ago by adam-p

I see what you're saying, and I see that my original suggestion was insufficient.

I ended up implementing your "second thing" -- creating a temporary net.Error. Except... it's part of a larger Psiphon change... we needed both the original SOCKS4 support and the new SOCKS5 support that's in a goptlib branch, so I combined them (including their tests) in such a way that it'll branch on the SOCKS version byte sent by the client. You can see the changes here:

https://github.com/Psiphon-Inc/goptlib/pull/1

You'll see newTemporaryNetError(...) returned all over.

So... I could copy that change over to the Tor goptlib code base. Or... maybe you'd be interested in the whole PR I linked to? Is there any use to Tor+PT in simultaneously supporting SOCKS4+4a+5?

comment:3 in reply to:  2 Changed 5 years ago by yawning

Replying to adam-p:

So... I could copy that change over to the Tor goptlib code base. Or... maybe you'd be interested in the whole PR I linked to? Is there any use to Tor+PT in simultaneously supporting SOCKS4+4a+5?

Not at the moment, no. The pt spec assumes that PTs only support either SOCKS4(a) or SOCKS5 and not both simultaneously. I'm mostly indifferent here, and would be ok with such behavior being supported in the future, though doing so would require tor side code changes.

comment:4 Changed 4 years ago by yawning

Component: meekPluggable transport
Keywords: goptlib added
Status: newneeds_review

Since I was looking over the SOCKS code again in relation to #12535, I went and wrote something that also implements "second thing".

Similar approach taken to adam-p's branch, all errors in SocksListener.AcceptSocks() past the ln.Listener.Accept() call are returned as a net.Error that has a Temporary() that always returns true.

Either version of the check will continue to work, since ln.Listener.Accept()'s errors are returned untouched, and if non-temporary, will cause the loop to terminate, as they should. All other net.Errors will be re-wrapped since they are net.Errors generated from the new client connection, and have absolutely nothing to do with the listener.

A defer in SocksListener.AcceptSocks() used since it's cleaner to do it that way, saving the need to splatter the wrapper all over the rest of the socks code.

https://github.com/Yawning/goptlib/commit/e43b7d4ae09aaf10f0d544cf86da62bf1de3d74b

Note: My change depends on my #12535 branch, since modifying the SOCKS4a code is for people that are living in the past.

comment:5 Changed 4 years ago by dcf

What do you think about just not returning errors that happen post-Accept during SOCKS negotiation? Something like this:

  • socks.go

    a b func (ln *SocksListener) Accept() (net.Conn, error) { 
    126126//                     continue
    127127//             }
    128128//             go handleConn(conn)
    129129//     }
    130130func (ln *SocksListener) AcceptSocks() (*SocksConn, error) {
     131retry:
    131132       c, err := ln.Listener.Accept()
    132133       if err != nil {
    133134               return nil, err
    134135       }
    135136       conn := new(SocksConn)
    136137       conn.Conn = c
    137138       err = conn.SetDeadline(time.Now().Add(socksRequestTimeout))
    138139       if err != nil {
    139140               conn.Close()
    140                return nil, err
     141               goto retry
    141142       }
    142143       conn.Req, err = readSocks4aConnect(conn)
    143144       if err != nil {
    144145               conn.Close()
    145                return nil, err
     146               goto retry
    146147       }
    147148       err = conn.SetDeadline(time.Time{})
    148149       if err != nil {
    149150               conn.Close()
    150                return nil, err
     151               goto retry
    151152       }
    152153       return conn, nil
    153154}
    154155
    155156// Returns "socks4", suitable to be included in a call to Cmethod.

comment:6 in reply to:  5 Changed 4 years ago by dcf

Replying to dcf:

What do you think about just not returning errors that happen post-Accept during SOCKS negotiation? Something like this:

BTW I thought about a lot of options but part of my rationale is this logic in http.Serve:
https://github.com/golang/go/blob/77082481d48c8cd8ea93328f9ab962092fe0183f/src/net/http/server.go#L1782
It does an exponentially increasing delay on Temporary net.Error, and quits on any other kind of error. I'm thinking that, if there were an analogous situation using SocksListener, we would not want an invalid SOCKS handshake to result in this kind of delay handling, which appears to be aimed at situations of temporary network overload. So we can just keep such errors inside the library and never report them upward.

I think my original thinking was to report such errors upward so that they could be logged, but, eh, whatever. I like that, with this change, the Temporary checks match the ones in the standard library.

comment:7 Changed 4 years ago by dcf

I added tests (currently failing) for the behavior we want:
https://gitweb.torproject.org/pluggable-transports/goptlib.git/commit/?id=2a2a9b7cbff0dc9ff452a39a4199cec264e76dfe
That is, an error that happens after calling Accept must be either 1) not reported, or 2) a Temporary net.Error. The patch in comment:5 makes the test pass.

I know the SOCKS 4a code is old and busted, but if I let this patch depend on the one for SOCKS 5, then they are both going to get more delayed.

comment:8 in reply to:  5 Changed 4 years ago by dcf

Resolution: fixed
Status: needs_reviewclosed

Replying to dcf:

What do you think about just not returning errors that happen post-Accept during SOCKS negotiation? Something like this:

Merged this and updated demo code in 50b39b746c.

Note: See TracTickets for help on using tickets.