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

Jump to bottom

socks4 parse error #1892

Closed
Jackarain opened this issue Sep 17, 2017 · 4 comments
Closed

socks4 parse error #1892

Jackarain opened this issue Sep 17, 2017 · 4 comments
Labels
connecting & proxies

Comments

@Jackarain
Copy link

Jackarain commented Sep 17, 2017 •
edited

curl/lib/socks.c

Line 309 in c866608

(((unsigned char)socksreq[8] << 8) | (unsigned char)socksreq[9]),

curl/lib/socks.c

Line 319 in c866608

(((unsigned char)socksreq[8] << 8) | (unsigned char)socksreq[9]),

curl/lib/socks.c

Line 329 in c866608

(((unsigned char)socksreq[8] << 8) | (unsigned char)socksreq[9]),

curl/lib/socks.c

Line 338 in c866608

(((unsigned char)socksreq[8] << 8) | (unsigned char)socksreq[9]),

should be:

(((unsigned char)socksreq[2] << 8) | (unsigned char)socksreq[3])
@bagder
Copy link
Member

bagder commented Sep 17, 2017

Sorry but that doesn't make sense, the sockreq[4] is already shown as the first byte of the IP address.

The short documentation in the comment above would however imply that index 2 and 3 are the ones that hold the port number.

@Jackarain
Copy link
Author 

 *     +----+----+----+----+----+----+----+----+
 *     | VN | CD | DSTPORT |      DSTIP        |
 *     +----+----+----+----+----+----+----+----+
 * # of bytes:  1    1      2              4

@bagder
Copy link
Member

bagder commented Sep 18, 2017

That's the comment I referred to. It mentions DSTPORT on index 2 and 3.

jay added a commit that referenced this issue Sep 18, 2017
@jay
socks: fix incorrect port number in SOCKS4 error message
6d43664 
Prior to this change it appears the SOCKS5 port parsing was erroneously
used for the SOCKS4 error message, and as a result an incorrect port
would be shown in the error message.

Bug: #1892
Reported-by: Jackarain@users.noreply.github.com
@jay
Copy link
Member

jay commented Sep 18, 2017

Thanks, landed in 6d43664. I worked on those lines last but I didn't catch it. It's been like that since it was added in 2006, see a15d107. My guess is it was copied over from some SOCKS5 error message parsing since in a SOCKS5 response the port comes after the ip address.

@jay jay closed this as completed Sep 18, 2017
@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
connecting & proxies
Milestone
No milestone
Development

No branches or pull requests
3 participants
@bagder @Jackarain @jay