Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http, x/net/proxy: Panic triggerable by SOCKS5 server #21333

Closed
special opened this issue Aug 7, 2017 · 10 comments

Comments

Projects
None yet
4 participants
@special
Copy link

commented Aug 7, 2017

x/net/proxy's SOCKS5 implementation has a logic error that allows the SOCKS server to trigger a client-side panic:

From https://github.com/golang/net/blob/master/proxy/socks5.go and reduced for clarity:

	bytesToDiscard := 0
	switch buf[3] {
	case socks5Domain:
		_, err := io.ReadFull(conn, buf[:1])
		bytesToDiscard = int(buf[0])
	}

	if cap(buf) < bytesToDiscard {
		buf = make([]byte, bytesToDiscard)
	} else {
		buf = buf[:bytesToDiscard]
	}
	if _, err := io.ReadFull(conn, buf); err != nil {
		return errors.New("proxy: failed to read address from SOCKS5 proxy at " + s.addr + ": " + err.Error())
	}

	// Also need to discard the port number
	if _, err := io.ReadFull(conn, buf[:2]); err != nil {

The length of buf is determined by the server here and may be less than 2.

@bradfitz bradfitz added the NeedsFix label Aug 7, 2017

@bradfitz bradfitz added this to the Go1.10 milestone Aug 7, 2017

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 7, 2017

/cc @mikioh

@mikioh

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2017

See https://go-review.googlesource.com/c/38278; it also fixes this issue.

@mikioh

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2017

Not sure why packages in x/net repo need to have Go1.10 label, though.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 8, 2017

@mikioh, it gets a Go1.x label if it's vendored into std.

@mikioh

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2017

@special,

I agree that it's a bug that io.ReadFull is not cancelable in the existing SOCKS client implementation. Although, I'm still not sure how the corrupted message from a SOCKS server makes a SOCKS client crash. Can you please explain the detail?

@bradfitz,

Ah, I forgot it.

@special

This comment has been minimized.

Copy link
Author

commented Aug 8, 2017

@mikioh

If the server sends socks5Domain and a length (bytesToDiscard) of < 2, buf is resized to that length. The slice buf[:2] when reading the port will then be invalid.

It looks like the code at https://go-review.googlesource.com/c/38278 does avoid this issue.

@mikioh

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2017

@special,

Thanks for the information. Looks like it's better to add a few test cases on corrupted TLV-style data into CL 38378.

@gopherbot

This comment has been minimized.

Copy link

commented Aug 9, 2017

Change https://golang.org/cl/38278 mentions this issue: internal/{socks,sockstest}: new packages

@gopherbot

This comment has been minimized.

Copy link

commented Aug 9, 2017

Change https://golang.org/cl/41031 mentions this issue: net/http: replace SOCKS client implementation

@mikioh mikioh changed the title x/net/proxy: Panic triggerable by SOCKS5 server net/http, x/net/proxy: Panic triggerable by SOCKS5 server Dec 1, 2017

@mikioh mikioh modified the milestones: Go1.10, Go1.11 Dec 1, 2017

@mikioh

This comment has been minimized.

Copy link
Contributor

commented Dec 1, 2017

@rsc marked CL 38278 and 41931 with Go 1.11.

gopherbot pushed a commit that referenced this issue Apr 6, 2018

net/http: replace SOCKS client implementation
This change replaces the vendored socks client implementation with the
bundle of golang.org/x/net/internal/socks package which contains fixes
for 19354 and 11682.

golang.org/x/net/internal/socks becomes socks_bundle.go.

At git rev 61147c4. (golang.org/cl/38278)

Updates #11682.
Updates #18508.
Updates #19354.
Fixes #19688.
Updates #21333.

Change-Id: I8cf6c3f5eb87c24685a7592be015729f84fbed77
Reviewed-on: https://go-review.googlesource.com/41031
Run-TryBot: Mikio Hara <mikioh.mikioh@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@golang golang locked and limited conversation to collaborators Apr 6, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.