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.

Sign up for GitHub

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

Jump to bottom

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

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

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

special opened this issue Aug 7, 2017 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.11

Comments

@special
Copy link

special 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 The path to resolution is known, but the work has not been done. label Aug 7, 2017
@bradfitz bradfitz added this to the Go1.10 milestone Aug 7, 2017
@bradfitz
Copy link
Contributor

bradfitz commented Aug 7, 2017

/cc @mikioh

@mikioh
Copy link
Contributor

mikioh commented Aug 8, 2017

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

@mikioh
Copy link
Contributor

mikioh commented Aug 8, 2017

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

@bradfitz
Copy link
Contributor

bradfitz commented Aug 8, 2017

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

@mikioh
Copy link
Contributor

mikioh 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
Copy link
Author

special 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
Copy link
Contributor

mikioh 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
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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
Copy link
Contributor

mikioh 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
@cixtor @bradfitz
net/http: replace SOCKS client implementation
4acb305 
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.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
4 participants
@bradfitz @special @mikioh @gopherbot