Wrong handling of SOCKS v5 CONNECT Replies from socks server.

RESOLVED FIXED in mozilla1.9.2a1

Status

()

--
major
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: diegobilli, Assigned: michal.novotny)

Tracking

Trunk
mozilla1.9.2a1
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
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.
Component: General → Networking
Product: Firefox → Core
QA Contact: general → networking
Version: unspecified → 1.8 Branch

Comment 1

12 years ago
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
(Assignee)

Comment 2

11 years ago
Created attachment 320910 [details]
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.
Assignee: nobody → michal
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 3

11 years ago
Created attachment 320913 [details] [diff] [review]
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.
Attachment #320913 - Flags: review?(cbiesinger)
(Assignee)

Comment 4

11 years ago
Created attachment 320915 [details] [diff] [review]
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.
Attachment #320915 - Flags: review?(cbiesinger)

Comment 5

10 years ago
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.
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?
(Assignee)

Updated

10 years ago
Attachment #320913 - Attachment is obsolete: true
(Assignee)

Comment 8

10 years ago
Created attachment 369560 [details] [diff] [review]
patch v3 - updated patch that fixes timeout handling
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.
(Assignee)

Comment 10

10 years ago
Created attachment 370025 [details] [diff] [review]
patch v4 - SendData -> pr_Send
[Checkin: Comment 11]
Attachment #369560 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Comment on attachment 370025 [details] [diff] [review]
patch v4 - SendData -> pr_Send
[Checkin: Comment 11]


http://hg.mozilla.org/mozilla-central/rev/3147fc122cf5
Attachment #370025 - Attachment description: patch v4 - SendData -> pr_Send → patch v4 - SendData -> pr_Send [Checkin: Comment 11]
Status: ASSIGNED → RESOLVED
Last Resolved: 10 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.