socks4 parse error #1892

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

Comments

Projects
None yet
3 participants
@Jackarain

Jackarain commented Sep 17, 2017

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

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

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

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

should be:

(((unsigned char)socksreq[2] << 8) | (unsigned char)socksreq[3])

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 17, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@Jackarain

Jackarain Sep 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 18, 2017

Member

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

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

socks: fix incorrect port number in SOCKS4 error message
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

This comment has been minimized.

Show comment
Hide comment
@jay

jay Sep 18, 2017

Member

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.

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 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.