Race Condition in Curl_proxyCONNECT (bug # 39) #1132

Activity

1. bagder

   added

   connecting & proxies

   FTP

   on Nov 18, 2016
2. bagder commented on Nov 18, 2016

   

   bagder

   on Nov 18, 2016

   Member

   More actions

   There is no known bugs numbered like that anymore so if you want something added to KNOWN_BUGS I suggest you spell it out or provide a pull request.

   But then I also don't really see the big value of having this mentioned there. Isn't it instead much better to work on actually fixing the issue you have? Can you provide an example that repeats the problem and possible show us VERBOSE output or other debugging info so that we can try to fix whatever you're experiencing?
3. copelnug commented on Nov 23, 2016

   

   copelnug

   on Nov 23, 2016

   Last edited by copelnug

   Author

   More actions

   Sadly, it's a race condition. In some cases, libcurl will read the FTP welcome message as being the HTTP proxy response body and thus will never receive the FTP welcome. That means that it is almost impossible to reproduce predicatively with a real HTTP proxy.

   So, I've made a (partial) fake HTTP proxy server (curl39.txt). This (C++) program simply accept one connection and send back in a single write:

   • The HTTP proxy connection message.
   • The FTP server welcome message.

   With this server and curl command lineline:
   curl --verbose ftp://localhost:4500/test.txt -o test.txt --proxytunnel --proxy http://localhost:8080 --max-time 5
   I get the following errors:
   * Connected to localhost (127.0.0.1) port 8080 (#0)
* Establish HTTP proxy tunnel to localhost:4500
> CONNECT localhost:4500 HTTP/1.1
> Host: localhost:4500
> User-Agent: curl/7.47.0
> Proxy-Connection: Keep-Alive
>
< HTTP/1.0 200 Connection Established
< Proxy-agent: Apache/2.4.18 (Win64) OpenSSL/1.0.2e
<
* Proxy CONNECT followed by 32 bytes of opaque data. Data ignored (known bug #39)
* Proxy replied OK to CONNECT request
* Operation timed out after 0 milliseconds with 0 out of 0 bytes received
* Closing connection 0
curl: (28) Proxy CONNECT followed by 32 bytes of opaque data. Data ignored (known bug #39)

   As we can see, CURL never did see the welcome message from the FTP server (because it is the 32 bytes of opaque data).
4. copelnug commented on Nov 23, 2016

   

   copelnug

   on Nov 23, 2016

   Author

   More actions

   I've tested the fix of https://curl.haxx.se/mail/lib-2015-04/0169.html and it does resolve the problem.

   Sadly, it also break three tests (206, 1060 and 1061). All of those tests have HTTP proxy that return data in the proxy response. From the discussion on the mailing list, it appear that behaviour is broken for a HTTP proxy.

   What should be the next step? Should I open a pull request and move the discussion there or would it be better to discuss impact/alternatives in this issue first?
5. bagder commented on Nov 23, 2016

   

   bagder

   on Nov 23, 2016

   Member

   More actions

   Please do submit a PR. Regarding the test failures, it'd be great if you fixed them as well while at it. The spec text to lean on comes from RFC7231 section 4.3.6:

      A server MUST NOT send any Transfer-Encoding or Content-Length header
      fields in a 2xx (Successful) response to CONNECT.  A client MUST
      ignore any Content-Length or Transfer-Encoding header fields received
      in a successful response to CONNECT.
   
   
6. bagder

   self-assigned this

   on Nov 29, 2016
7. bagder commented on Nov 30, 2016

   

   bagder

   on Nov 30, 2016

   Member

   More actions

   Here's my suggested patch for this problem: 0001-CONNECT-fix-race-condition.patch

   It now returns error on Transfer-Encoding: or Content-Length: headers in 2xx responses. It reads the responses byte-by-byte to avoid over-reading. It also removes the 16K total-response size limit from before and it is now instead just a 16K single-line limit.

   It actually also made me find and fix a bug that probably can happen with the old code if that would end up reading the response one byte at a time too (a rare scenario).

   This patch is a squashed version of three separate commits that I want to merge, but serves as easier to test. I've also updated a bunch of test cases that were broken (used wrong or no headers), and some that actually sent the unaligned data amounts - where Content-Length: and the body size disagree. Without those fixes applied, this test patch will cause a series of test problems.
8. bagder

   closed this as completedin 3ea3518on Dec 1, 2016
9. bagder commented on Dec 1, 2016

   

   bagder

   on Dec 1, 2016

   Member

   More actions

   I went ahead and merged. But please let me know if something doesn't work as it should!
10. copelnug commented on Dec 2, 2016

    

    copelnug

    on Dec 2, 2016

    Author

    More actions

    I've just tested it and the bug disappear with the correction.

    Thanks.
11. bagder commented on Dec 2, 2016

    

    bagder

    on Dec 2, 2016

    Member

    More actions

    Lovely, thanks for verifying. Now I feel more confident in the change!
12. lock

    locked as resolved and limited conversation to collaborators on May 7, 2018