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

socks: fix expected length of SOCKS5 reply #5527

Closed
wants to merge 3 commits into from
Closed

socks: fix expected length of SOCKS5 reply #5527

wants to merge 3 commits into from

Conversation

Cherish98
Copy link
Contributor

Commit 4a4b63d forgot to initialize the expected SOCKS5 reply length, and neither did it set it to the correct value of 10 when the reply ATYP is X'01'. This results in erroneously expecting more bytes when the request length is greater than the reply length (e.g., when remotely resolving the hostname).

@Cherish98
socks: fix expected length of SOCKS5 reply
70f9514 
Commit 4a4b63d forgot to initialize the expected SOCKS5 reply length, and neither did it set it to the correct value of 10 when the reply ATYP is X'01'. This results in erroneously expecting more bytes when the request length is greater than the reply length (e.g., when remotely resolving the hostname).
@Cherish98
Copy link
Contributor Author

Alternatively, set the length when socksreq[3] == 1 here:

curl/lib/socks.c

Lines 928 to 937 in a00668d

/* Calculate real packet size */
if(socksreq[3] == 3) {
/* domain name */
int addrlen = (int) socksreq[4];
len = 5 + addrlen + 2;
}
else if(socksreq[3] == 4) {
/* IPv6 */
len = 4 + 16 + 2;
}

Otherwise, len is left with the length of the SOCKS5 request. When the request ATYP is domain name and reply ATYP is V4 address, it will wait for more bytes here:

curl/lib/socks.c

Lines 944 to 952 in a00668d

if(len > 10) {
sx->outstanding = len - 10; /* get the rest */
sx->outp = &socksreq[10];
sxstate(conn, CONNECT_REQ_READ_MORE);
}
else {
sxstate(conn, CONNECT_DONE);
break;
}

@bagder
Copy link
Member

bagder commented Jun 5, 2020

Alternatively, set the length when socksreq[3] == 1 here:

I think would prefer that for greater redability, and then do a final elseand return error if that byte is not one of the known values instead of just silently continuing...

@bagder bagder self-assigned this Jun 5, 2020
@Cherish98
Copy link
Contributor Author

Alternatively, set the length when socksreq[3] == 1 here:

I think would prefer that for greater redability, and then do a final elseand return error if that byte is not one of the known values instead of just silently continuing...

On a second thought, I agree that would be the correct fix. Because the original patch in the pull request set len across sock state boundaries, and thus wrong (zero) values can sometimes be used here.

@bagder
Copy link
Member

bagder commented Jun 5, 2020

Yes, very good point!

@Cherish98
socks: fix expected length of SOCKS5 reply
af1a5f5 
Commit 4a4b63d forgot to set the expected SOCKS5 reply length when the reply ATYP is X'01'. This resulted in erroneously expecting more bytes when the request length is greater than the reply length (e.g., when remotely resolving the hostname).
@Cherish98
Merge pull request #1 from Cherish98/fix-socks5-reply-len-v2
9cdd1b5 
socks: fix expected length of SOCKS5 reply
@Cherish98
Copy link
Contributor Author

I have drafted a v2 patch in af1a5f5.

@bagder bagder closed this in e980cbb Jun 5, 2020
@bagder
Copy link
Member

bagder commented Jun 5, 2020

Thanks!

@xlfjn xlfjn mentioned this pull request Aug 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bagder bagder bagder approved these changes

Assignees

@bagder bagder

Labels
connecting & proxies
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Cherish98 @bagder