353040 - Wrong handling of SOCKS v5 CONNECT Replies from socks server.

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Diego Billi]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.6) Gecko/20060728 Mozilla Firefox (Debian-1.5.dfsg+1.5.0.6-5)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.6) Gecko/20060728 Mozilla Firefox (Debian-1.5.dfsg+1.5.0.6-5)

When reading CONNECT replies from the SOCKS server, firefox doesn't handle the reply data in the right way. If server sends reply data separated in different segments with different delay, firefox start HTTP requests before reading the whole reply. I've discovered this problem while connecting to a slow socks server.
In my test i set up a socks server which send parts of SOCKS reply data in this way: VER+REP+RSV+ATYP...random delay...BND.ADDR+BND.PORT  (i use field names from the SOCKSv5 RFC).
Firefox starts HTTP request before receiving BND.ADDR+BND.PORT and thinks that BND.ADDR+BND.PORT are part of the HTTP server response.


Reproducible: Always

Steps to Reproduce:
1. Setup a SOCKS v5 server on 127.0.0.1:8081
2. Setup a TCP proxy which delays TCP data (byte by byte) on 127.0.0.1:8080 and connect it to the SOCKS server.
3. Start firefox and connect it to 127.0.0.1:8080 with SOCKS v5 connection.
4. Final scenario FIREFOX <----->delay_proxy:8080<------>SOCKS_server:8081

Actual Results:  
Firefox downloads HTTP data and thinks it is BIN data.

Comment 1

[Simon Jackson]

Firstly, a quick me too to say that this bug exists in the SOCKS 4 handling as well.

I believe the problem stems from calls such as     
response_len = PR_Recv(fd, response, response_len, 0, timeout);

to get a response.  I believe that PR_Recv behaves in the same manner as recv( in that it does not guarantee to read response_len bytes from the socket.  According to http://developer.mozilla.org/en/docs/PR_Recv 

PR_Recv blocks until some positive number of bytes are transferred, a timeout occurs, or an error occurs. No more than amount bytes will be transferred.

Thus the calls to fetch the responses as above should be replaced with something like:
int received=0;
while (received<response_len){
       int r = PR_Recv(fd, response+received, response_len-received, 0, timeout);
       if (r<=0) ...
       received+= r
}

with ... being the current reaction to a failed read

Comment 2

[Michal Novotny [:michal]]

Attachment: tcp proxy written in bash using socat that relays data byte by byte

The most difficult thing on this bug is to reproduce it. I couldn't find any proxy that can do "byte by byte" relaying. In the end I found out that it can be done with socat. In case of SOCK5 following command was sufficient for me:

socat -b 1 \
  TCP4-LISTEN:8080,reuseaddr,fork,nodelay,sndbuf=1,sndbuf-late=1 \
  TCP4:127.0.0.1:1080,nodelay,sndbuf=1,sndbuf-late=1

Relayed data is sent so that each byte is in separate packet but without any delay. This didn't work for SOCK4 (multiple packets were received before PR_Recv finished) so I created set of bash scripts that put delay between first 20 relayed bytes.

Comment 3

[Michal Novotny [:michal]]

Attachment: fix

Problem is in PR_Recv() usage as described in comment #1. I had to change slightly handling of SOCKS5 connect reply, because it doesn't have fixed length. I'm pretty sure that old code couldn't work with SOCKS server that returns FQDN in the reply, because it didn't read whole reply but always only 22 bytes.

Comment 4

[Michal Novotny [:michal]]

Attachment: fix with proper timeout handling

This is modified version of previous patch that handles timeout IMHO in correct way. Timeout passed to ConnectSOCKS5() and ConnectSOCKS4() limits total duration of all operations and not each separately. But I'm not sure if it is worth checking in because this shouldn't be serious problem when proxy is in local network.

Comment 5

[Bart Van Assche]

I noticed that with network.proxy.socks_remote_dns=true, loading of http://teletekst.een.be/tt_een.php over SOCKSv5 fails systematically in Firefox 3.0.5. Firefox displays the HTML source of the mentioned URL with the letter 'P' prepended, which is the last byte of the SOCKSv5 response sent back by the SOCKSv5 server. I verified with Wireshark that the behavior of the SOCKS server was correct.

Comment 6

[Matthias Versen [:Matti]]

Biesi: Do you have time to review this patch in the near future or should we try to find another reviewer ?

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

OK, I reviewed bug 459524's patch. if there's some timeout handling or something from this patch that you want to get checked in, could you update the patch?

Comment 8

[Michal Novotny [:michal]]

Attachment: patch v3 - updated patch that fixes timeout handling

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 369560 [details] [diff] [review]
patch v3 - updated patch that fixes timeout handling

r+sr=biesi, but please rename SendData for consistency with pr_RecvAll. Maybe pr_Send.

Comment 10

[Michal Novotny [:michal]]

Attachment: patch v4 - SendData -> pr_Send [Checkin: Comment 11]

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 370025 [details] [diff] [review]
patch v4 - SendData -> pr_Send
[Checkin: Comment 11]


http://hg.mozilla.org/mozilla-central/rev/3147fc122cf5