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

socks: fix expected length of SOCKS5 reply #5527

Closed
wants to merge 3 commits into from

Conversation

@Cherish98
Copy link
Contributor

@Cherish98 Cherish98 commented Jun 5, 2020

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

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

@Cherish98 Cherish98 commented Jun 5, 2020

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

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

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 bagder commented Jun 5, 2020

Yes, very good point!

Cherish98 added 2 commits Jun 5, 2020
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).
socks: fix expected length of SOCKS5 reply
@Cherish98
Copy link
Contributor Author

@Cherish98 Cherish98 commented Jun 5, 2020

I have drafted a v2 patch in af1a5f5.

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

@bagder bagder commented Jun 5, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.