Closed Bug 353040 Opened 17 years ago Closed 14 years ago

Wrong handling of SOCKS v5 CONNECT Replies from socks server.


(Core :: Networking, defect)

Not set





(Reporter: diegobilli, Assigned: michal)



(2 files, 3 obsolete files)

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

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
2. Setup a TCP proxy which delays TCP data (byte by byte) on and connect it to the SOCKS server.
3. Start firefox and connect it to 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.
Component: General → Networking
Product: Firefox → Core
QA Contact: general → networking
Version: unspecified → 1.8 Branch
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 

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

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.
Assignee: nobody → michal
Ever confirmed: true
Attached patch fix (obsolete) — Splinter Review
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.
Attachment #320913 - Flags: review?(cbiesinger)
Attached patch fix with proper timeout handling (obsolete) — Splinter Review
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.
Attachment #320915 - Flags: review?(cbiesinger)
I noticed that with network.proxy.socks_remote_dns=true, loading of 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.
Biesi: Do you have time to review this patch in the near future or should we try to find another reviewer ?
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?
Attachment #320913 - Attachment is obsolete: true
Attachment #320915 - Attachment is obsolete: true
Attachment #369560 - Flags: review?(cbiesinger)
Attachment #369560 - Flags: superreview+
Attachment #369560 - Flags: review?(cbiesinger)
Attachment #369560 - Flags: review+
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.
Keywords: checkin-needed
Comment on attachment 370025 [details] [diff] [review]
patch v4 - SendData -> pr_Send
[Checkin: Comment 11]
Attachment #370025 - Attachment description: patch v4 - SendData -> pr_Send → patch v4 - SendData -> pr_Send [Checkin: Comment 11]
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Version: 1.8 Branch → Trunk
You need to log in before you can comment on or make changes to this bug.