Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#3683 closed defect (fixed)

Stream-isolation code does not handle NULs in SOCKS auth fields properly

Reported by: rransom Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: rransom Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The SOCKS server code goes to some trouble to handle any NULs embedded in SOCKS 4 authentication strings and SOCKS 5 usernames and passwords, and it records the lengths of those strings in the socks_request field of the edge_connection_t. Then the stream-isolation code uses strdup and strcmp on them as if they were NUL-terminated strings.

We should handle NULs embedded in those strings properly everywhere, and we should probably also use a comparison function that runs in data-independent time when comparing them.

Child Tickets

Change History (8)

comment:1 Changed 8 years ago by nickm

Status: newneeds_review

Yikes. Glad that never saw production.

Possible fix in branch bug3683 in my public repository.

Should-I-care questions: The memcmp is only data-independent under limited circumstances: if either input is NULL, or if their lengths vary, it returns faster than if they are both strings of the same length.

Also, I think that the use of uint8_t for usernamelen/socks_username_len might be wrong; socks4 authenticators are NUL-terminated IIRC, not length-extent?

comment:2 in reply to:  1 ; Changed 8 years ago by rransom

Replying to nickm:

Yikes. Glad that never saw production.

Possible fix in branch bug3683 in my public repository.

Should-I-care questions: The memcmp is only data-independent under limited circumstances: if either input is NULL, or if their lengths vary, it returns faster than if they are both strings of the same length.

There isn't much we can do about that except hash/HMAC the username and password immediately and only store and compare hashes, and that sounds like more trouble than it's currently worth.

Since we're treating the SOCKS authentication values as potentially sensitive, we should also (try to) zero them in socks_request_free.

Also, I think that the use of uint8_t for usernamelen/socks_username_len might be wrong; socks4 authenticators are NUL-terminated IIRC, not length-extent?

Yes. Fortunately, the integer overflow that produced in parse_socks seems to be relatively harmless.

comment:3 in reply to:  2 Changed 8 years ago by rransom

Replying to rransom:

Replying to nickm:

Possible fix in branch bug3683 in my public repository.

Some other pieces of Tor act as if an edge_connection_t might have socks_request set to NULL (or at least assert that it isn't). Your bug3683 branch doesn't.

Other than that, and the other issues I noted above, looks good.

Also, I think that the use of uint8_t for usernamelen/socks_username_len might be wrong; socks4 authenticators are NUL-terminated IIRC, not length-extent?

Yes. Fortunately, the integer overflow that produced in parse_socks seems to be relatively harmless.

Er, no. It would be very bad if someone used SOCKS4A with partially attacker-controlled authorization strings in the presence of that bug. That integer overflow seems to not lead to memory corruption within Tor, though.

comment:4 Changed 8 years ago by nickm

Okay, I think I fixed all of the above in branch bug3683 in my public repo.

(Except for asserting that socks_request is set. The rule, as far as I can figure, is that socks_request must be set for every CONN_TYPE_AP stream. Once I merge the patch that makes entry_connection_t exist, we can make the checks more unified, or just inline it.)

comment:5 in reply to:  4 Changed 8 years ago by rransom

Replying to nickm:

Okay, I think I fixed all of the above in branch bug3683 in my public repo.

Looks good!

(Except for asserting that socks_request is set. The rule, as far as I can figure, is that socks_request must be set for every CONN_TYPE_AP stream. Once I merge the patch that makes entry_connection_t exist, we can make the checks more unified, or just inline it.)

OK.

comment:6 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Thanks; merging!

comment:7 Changed 7 years ago by nickm

Keywords: tor-client added

comment:8 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.