November 2013 - tor-commits - lists.torproject.org

[goptlib/master] Use constant-time compare for auth cookie header.
by dcf＠torproject.org 10 Nov '13

10 Nov '13

commit c00891640650bc553f7c4e7eb12ba43cce23f273 Author: David Fifield <david(a)bamsoftware.com> Date: Sat Nov 9 17:08:58 2013 -0800 Use constant-time compare for auth cookie header. Why not. --- pt.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pt.go b/pt.go index d64f169..a8bd001 100644 --- a/pt.go +++ b/pt.go @@ -338,7 +338,7 @@ func readAuthCookie(f io.Reader) ([]byte, error) { } header := buf[0:32] cookie := buf[32:64] - if !bytes.Equal(header, authCookieHeader) { + if subtle.ConstantTimeCompare(header, authCookieHeader) != 1 { return nil, errors.New(fmt.Sprintf("missing auth cookie header")) }

1 0

[goptlib/master] Use generic io.Reader and io.Writer.
by dcf＠torproject.org 10 Nov '13

10 Nov '13

commit cec49fb391cf08c4b5cb957009aecc189255c0a2 Author: David Fifield <david(a)bamsoftware.com> Date: Sat Nov 9 17:20:27 2013 -0800 Use generic io.Reader and io.Writer. Where we don't actually depend on it being a real TCP connection. --- pt.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/pt.go b/pt.go index a8bd001..d967c31 100644 --- a/pt.go +++ b/pt.go @@ -431,7 +431,7 @@ func computeClientHash(info *ServerInfo, clientNonce, serverNonce []byte) []byte return h.Sum([]byte{}) } -func extOrPortAuthenticate(s *net.TCPConn, info *ServerInfo) error { +func extOrPortAuthenticate(s io.ReadWriter, info *ServerInfo) error { r := bufio.NewReader(s) // Read auth types. 217-ext-orport-auth.txt section 4.1. @@ -519,7 +519,7 @@ const ( extOrCmdDeny = 0x1001 ) -func extOrPortWriteCommand(s *net.TCPConn, cmd uint16, body []byte) error { +func extOrPortWriteCommand(s io.Writer, cmd uint16, body []byte) error { var buf bytes.Buffer if len(body) > 65535 { return errors.New("command exceeds maximum length of 65535") @@ -546,22 +546,22 @@ func extOrPortWriteCommand(s *net.TCPConn, cmd uint16, body []byte) error { // Send a USERADDR command on s. See section 3.1.2.1 of // 196-transport-control-ports.txt. -func extOrPortSendUserAddr(s *net.TCPConn, conn net.Conn) error { +func extOrPortSendUserAddr(s io.Writer, conn net.Conn) error { return extOrPortWriteCommand(s, extOrCmdUserAddr, []byte(conn.RemoteAddr().String())) } // Send a TRANSPORT command on s. See section 3.1.2.2 of // 196-transport-control-ports.txt. -func extOrPortSendTransport(s *net.TCPConn, methodName string) error { +func extOrPortSendTransport(s io.Writer, methodName string) error { return extOrPortWriteCommand(s, extOrCmdTransport, []byte(methodName)) } // Send a DONE command on s. See section 3.1 of 196-transport-control-ports.txt. -func extOrPortSendDone(s *net.TCPConn) error { +func extOrPortSendDone(s io.Writer) error { return extOrPortWriteCommand(s, extOrCmdDone, []byte{}) } -func extOrPortRecvCommand(s *net.TCPConn) (cmd uint16, body []byte, err error) { +func extOrPortRecvCommand(s io.Reader) (cmd uint16, body []byte, err error) { var bodyLen uint16 data := make([]byte, 4) @@ -590,7 +590,7 @@ func extOrPortRecvCommand(s *net.TCPConn) (cmd uint16, body []byte, err error) { // Send USERADDR and TRANSPORT commands followed by a DONE command. Wait for an // OKAY or DENY response command from the server. Returns nil if and only if // OKAY is received. -func extOrPortSetup(s *net.TCPConn, conn net.Conn, methodName string) error { +func extOrPortSetup(s io.ReadWriter, conn net.Conn, methodName string) error { var err error err = extOrPortSendUserAddr(s, conn)

1 0

[goptlib/master] ClientSetup and ServerSetup don't exit anymore.
by dcf＠torproject.org 10 Nov '13

10 Nov '13

commit e74981b9acf761bd7891e36ca505bbd4a5b79b30 Author: David Fifield <david(a)bamsoftware.com> Date: Sat Nov 9 17:22:28 2013 -0800 ClientSetup and ServerSetup don't exit anymore. --- pt.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pt.go b/pt.go index d967c31..4e8170a 100644 --- a/pt.go +++ b/pt.go @@ -219,8 +219,8 @@ type ClientInfo struct { } // Check the client pluggable transports environments, emitting an error message -// and exiting the program if any error is encountered. Returns a subset of -// methodNames requested by Tor. +// and returning a non-nil error if any error is encountered. Returns a subset +// of methodNames requested by Tor. func ClientSetup(methodNames []string) (ClientInfo, error) { var info ClientInfo @@ -368,9 +368,9 @@ type ServerInfo struct { } // Check the server pluggable transports environments, emitting an error message -// and exiting the program if any error is encountered. Resolves the various -// requested bind addresses, the server ORPort and extended ORPort, and reads -// the auth cookie file. Returns a ServerInfo struct. +// and returning a non-nil error if any error is encountered. Resolves the +// various requested bind addresses, the server ORPort and extended ORPort, and +// reads the auth cookie file. Returns a ServerInfo struct. func ServerSetup(methodNames []string) (ServerInfo, error) { var info ServerInfo

1 0

[goptlib/master] Show the error message if there is one.
by dcf＠torproject.org 10 Nov '13

10 Nov '13

commit 3ca51ff88f1fd5fad714e261b37baaf6bb4cdb6b Author: David Fifield <david(a)bamsoftware.com> Date: Sat Nov 9 17:40:11 2013 -0800 Show the error message if there is one. --- pt_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pt_test.go b/pt_test.go index 62ec5f6..21cb5cb 100644 --- a/pt_test.go +++ b/pt_test.go @@ -75,7 +75,7 @@ func TestGetManagedTransportVer(t *testing.T) { os.Setenv("TOR_PT_MANAGED_TRANSPORT_VER", test.input) output, err := getManagedTransportVer() if err != nil { - t.Errorf("%q unexpectedly returned an error", test.input) + t.Errorf("%q unexpectedly returned an error: %s", test.input, err) } if output != test.expected { t.Errorf("%q → %q (expected %q)", test.input, output, test.expected)

1 0

[goptlib/master] ClientSetup returns a struct.
by dcf＠torproject.org 10 Nov '13

10 Nov '13

commit 90fc5487cdbee5bf70fe24c808c6d7b969155793 Author: David Fifield <david(a)bamsoftware.com> Date: Sat Nov 9 17:23:26 2013 -0800 ClientSetup returns a struct. --- pt.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pt.go b/pt.go index 4e8170a..8c9c8d9 100644 --- a/pt.go +++ b/pt.go @@ -219,8 +219,8 @@ type ClientInfo struct { } // Check the client pluggable transports environments, emitting an error message -// and returning a non-nil error if any error is encountered. Returns a subset -// of methodNames requested by Tor. +// and returning a non-nil error if any error is encountered. Returns a +// ClientInfo struct. func ClientSetup(methodNames []string) (ClientInfo, error) { var info ClientInfo

1 0

[goptlib/master] import syntax.
by dcf＠torproject.org 10 Nov '13

10 Nov '13

commit c855df8c678205d534bccb7874632a9ac8cd3a27 Author: David Fifield <david(a)bamsoftware.com> Date: Sat Nov 9 17:34:24 2013 -0800 import syntax. --- pt_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pt_test.go b/pt_test.go index 44cfa54..62ec5f6 100644 --- a/pt_test.go +++ b/pt_test.go @@ -1,8 +1,10 @@ package pt -import "bytes" -import "os" -import "testing" +import ( + "bytes" + "os" + "testing" +) func stringIsSafe(s string) bool { for _, c := range []byte(s) {

1 0

[goptlib/master] Add tests for getClientTransports.
by dcf＠torproject.org 10 Nov '13

10 Nov '13

commit eb036ed1a7fc9925d6734fcf2f7a72f0cb1c1f77 Author: David Fifield <david(a)bamsoftware.com> Date: Sat Nov 9 18:03:43 2013 -0800 Add tests for getClientTransports. --- pt_test.go | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/pt_test.go b/pt_test.go index 21cb5cb..793635b 100644 --- a/pt_test.go +++ b/pt_test.go @@ -3,6 +3,7 @@ package pt import ( "bytes" "os" + "sort" "testing" ) @@ -83,6 +84,92 @@ func TestGetManagedTransportVer(t *testing.T) { } } +// return true iff the two slices contain the same elements, possibly in a +// different order. +func stringSetsEqual(a, b []string) bool { + ac := make([]string, len(a)) + bc := make([]string, len(b)) + sort.Strings(ac) + sort.Strings(bc) + if len(ac) != len(bc) { + return false + } + for i := 0; i < len(ac); i++ { + if ac[i] != bc[i] { + return false + } + } + return true +} + +func TestGetClientTransports(t *testing.T) { + tests := [...]struct { + envvar string + methodNames []string + expected []string + }{ + { + "*", + []string{}, + []string{}, + }, + { + "*", + []string{"alpha", "beta", "gamma"}, + []string{"alpha", "beta", "gamma"}, + }, + { + "alpha,beta,gamma", + []string{"alpha", "beta", "gamma"}, + []string{"alpha", "beta", "gamma"}, + }, + { + "alpha,beta", + []string{"alpha", "beta", "gamma"}, + []string{"alpha", "beta"}, + }, + { + "alpha", + []string{"alpha", "beta", "gamma"}, + []string{"alpha"}, + }, + { + "alpha,beta", + []string{"alpha", "beta", "alpha"}, + []string{"alpha", "beta"}, + }, + // my reading of pt-spec.txt says that "*" has special meaning + // only when it is the entirety of the environment variable. + { + "alpha,*,gamma", + []string{"alpha", "beta", "gamma"}, + []string{"alpha", "gamma"}, + }, + { + "alpha", + []string{"beta"}, + []string{}, + }, + } + + os.Clearenv() + _, err := getClientTransports([]string{"alpha", "beta", "gamma"}) + if err == nil { + t.Errorf("empty environment unexpectedly succeeded") + } + + for _, test := range tests { + os.Setenv("TOR_PT_CLIENT_TRANSPORTS", test.envvar) + output, err := getClientTransports(test.methodNames) + if err != nil { + t.Errorf("%q unexpectedly returned an error: %s", test.envvar, err) + } + if !stringSetsEqual(output, test.expected) { + t.Errorf("%q %q → %q (expected %q)", test.envvar, test.methodNames, output, test.expected) + } + } +} + func TestReadAuthCookie(t *testing.T) { badTests := [...][]byte{ []byte(""),

1 0

[goptlib/master] Rename resolveBindAddr to resolveAddr.
by dcf＠torproject.org 10 Nov '13

10 Nov '13

commit ff9223db55e42b88bdd105b23db325f1265fca2a Author: David Fifield <david(a)bamsoftware.com> Date: Sat Nov 9 18:28:22 2013 -0800 Rename resolveBindAddr to resolveAddr. Better to reserver "BindAddr" for the BindAddr struct. --- pt.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pt.go b/pt.go index 8c9c8d9..45efc04 100644 --- a/pt.go +++ b/pt.go @@ -246,20 +246,20 @@ type BindAddr struct { } // Resolve an address string into a net.TCPAddr. -func resolveBindAddr(bindAddr string) (*net.TCPAddr, error) { - addr, err := net.ResolveTCPAddr("tcp", bindAddr) +func resolveAddr(addrStr string) (*net.TCPAddr, error) { + addr, err := net.ResolveTCPAddr("tcp", addrStr) if err == nil { return addr, nil } // Before the fixing of bug #7011, tor doesn't put brackets around IPv6 // addresses. Split after the last colon, assuming it is a port // separator, and try adding the brackets. - parts := strings.Split(bindAddr, ":") + parts := strings.Split(addrStr, ":") if len(parts) <= 2 { return nil, err } - bindAddr = "[" + strings.Join(parts[:len(parts)-1], ":") + "]:" + parts[len(parts)-1] - return net.ResolveTCPAddr("tcp", bindAddr) + addrStr = "[" + strings.Join(parts[:len(parts)-1], ":") + "]:" + parts[len(parts)-1] + return net.ResolveTCPAddr("tcp", addrStr) } // Return a new slice, the members of which are those members of addrs having a @@ -298,7 +298,7 @@ func getServerBindAddrs(methodNames []string) ([]BindAddr, error) { return nil, EnvError(fmt.Sprintf("TOR_PT_SERVER_BINDADDR: %q: doesn't contain \"-\"", spec)) } bindAddr.MethodName = parts[0] - addr, err := resolveBindAddr(parts[1]) + addr, err := resolveAddr(parts[1]) if err != nil { return nil, EnvError(fmt.Sprintf("TOR_PT_SERVER_BINDADDR: %q: %s", spec, err.Error())) }

1 0

[goptlib/master] Add tests for resolveAddr.
by dcf＠torproject.org 10 Nov '13

10 Nov '13

commit 0f8fbff83c68e6b7c7db96a39b994ae288309bae Author: David Fifield <david(a)bamsoftware.com> Date: Sat Nov 9 18:33:42 2013 -0800 Add tests for resolveAddr. --- pt_test.go | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/pt_test.go b/pt_test.go index 793635b..01dd0ad 100644 --- a/pt_test.go +++ b/pt_test.go @@ -2,6 +2,7 @@ package pt import ( "bytes" + "net" "os" "sort" "testing" @@ -170,6 +171,48 @@ func TestGetClientTransports(t *testing.T) { } } +func TestResolveAddr(t *testing.T) { + badTests := [...]string{ + "", + "1.2.3.4", + "1.2.3.4:", + "9999", + ":9999", + "[1:2::3:4]", + "[1:2::3:4]:", + "[1::2::3:4]", + "1:2::3:4::9999", + "1:2:3:4::9999", + "localhost:9999", + "[localhost]:9999", + } + goodTests := [...]struct { + input string + expected net.TCPAddr + }{ + {"1.2.3.4:9999", net.TCPAddr{IP: net.ParseIP("1.2.3.4"), Port: 9999}}, + {"[1:2::3:4]:9999", net.TCPAddr{IP: net.ParseIP("1:2::3:4"), Port: 9999}}, + {"1:2::3:4:9999", net.TCPAddr{IP: net.ParseIP("1:2::3:4"), Port: 9999}}, + } + + for _, input := range badTests { + output, err := resolveAddr(input) + if err == nil { + t.Errorf("%q unexpectedly succeeded: %q", input, output) + } + } + + for _, test := range goodTests { + output, err := resolveAddr(test.input) + if err != nil { + t.Errorf("%q unexpectedly returned an error: %s", test.input, err) + } + if !output.IP.Equal(test.expected.IP) || output.Port != test.expected.Port { + t.Errorf("%q → %q (expected %q)", test.input, output, test.expected) + } + } +} + func TestReadAuthCookie(t *testing.T) { badTests := [...][]byte{ []byte(""),

1 0

[goptlib/master] Make resolveAddr more strict.
by dcf＠torproject.org 10 Nov '13

10 Nov '13

commit 43b5924d68041cf73f473b56c275e5983ee40014 Author: David Fifield <david(a)bamsoftware.com> Date: Sat Nov 9 18:34:05 2013 -0800 Make resolveAddr more strict. Go's net.ResolveTCPAddr allows an empty host or port part, returning for them nil and 0 respectively. It will also resolve names, which I don't think we need so we should avoid it for now. These are the test failures that this change fixes: --- FAIL: TestResolveAddr (0.00 seconds) pt_test.go:201: "" unexpectedly succeeded: "<nil>:0" pt_test.go:201: "1.2.3.4:" unexpectedly succeeded: "1.2.3.4:0" pt_test.go:201: ":9999" unexpectedly succeeded: "<nil>:9999" pt_test.go:201: "[1:2::3:4]:" unexpectedly succeeded: "[1:2::3:4]:0" pt_test.go:201: "localhost:9999" unexpectedly succeeded: "127.0.0.1:9999" pt_test.go:201: "[localhost]:9999" unexpectedly succeeded: "127.0.0.1:9999" --- pt.go | 43 +++++++++++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/pt.go b/pt.go index 45efc04..308238c 100644 --- a/pt.go +++ b/pt.go @@ -245,21 +245,40 @@ type BindAddr struct { Addr *net.TCPAddr } -// Resolve an address string into a net.TCPAddr. +// Resolve an address string into a net.TCPAddr. We are a bit more strict than +// net.ResolveTCPAddr; we don't allow an empty host or port, and the host part +// must be a literal IP address. func resolveAddr(addrStr string) (*net.TCPAddr, error) { - addr, err := net.ResolveTCPAddr("tcp", addrStr) - if err == nil { - return addr, nil - } - // Before the fixing of bug #7011, tor doesn't put brackets around IPv6 - // addresses. Split after the last colon, assuming it is a port - // separator, and try adding the brackets. - parts := strings.Split(addrStr, ":") - if len(parts) <= 2 { + ipStr, portStr, err := net.SplitHostPort(addrStr) + if err != nil { + // Before the fixing of bug #7011, tor doesn't put brackets around IPv6 + // addresses. Split after the last colon, assuming it is a port + // separator, and try adding the brackets. + parts := strings.Split(addrStr, ":") + if len(parts) <= 2 { + return nil, err + } + addrStr := "[" + strings.Join(parts[:len(parts)-1], ":") + "]:" + parts[len(parts)-1] + ipStr, portStr, err = net.SplitHostPort(addrStr) + } + if err != nil { + return nil, err + } + if ipStr == "" { + return nil, net.InvalidAddrError(fmt.Sprintf("address string %q lacks a host part", addrStr)) + } + if portStr == "" { + return nil, net.InvalidAddrError(fmt.Sprintf("address string %q lacks a port part", addrStr)) + } + ip, err := net.ResolveIPAddr("ip", ipStr) + if err != nil { + return nil, err + } + port, err := net.LookupPort("tcp", portStr) + if err != nil { return nil, err } - addrStr = "[" + strings.Join(parts[:len(parts)-1], ":") + "]:" + parts[len(parts)-1] - return net.ResolveTCPAddr("tcp", addrStr) + return &net.TCPAddr{IP: ip.IP, Port: port, Zone: ip.Zone}, nil } // Return a new slice, the members of which are those members of addrs having a

1 0