Bug 194483 - [PATCH] libfetch: Properly deal with multi-line proxy responses to CONNECT requests
Summary: [PATCH] libfetch: Properly deal with multi-line proxy responses to CONNECT re...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Dag-Erling Smørgrav
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2014-10-20 11:55 UTC by Fabian Keil
Modified: 2017-01-10 08:13 UTC (History)
2 users (show)

See Also:
des: mfc-stable9+
des: mfc-stable10+


Attachments
[PATCH] libfetch: Properly deal with multi-line proxy responses to CONNECT requests (1.51 KB, patch)
2014-10-20 11:55 UTC, Fabian Keil
no flags Details | Diff
Revised patch (1.78 KB, patch)
2015-10-14 15:54 UTC, Dag-Erling Smørgrav
no flags Details | Diff
Revised² patch (1.83 KB, patch)
2015-10-16 10:52 UTC, Dag-Erling Smørgrav
no flags Details | Diff
Correct while statement. (269 bytes, patch)
2016-10-04 11:09 UTC, Miłosz Kaniewski
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fabian Keil 2014-10-20 11:55:28 UTC
Created attachment 148498 [details]
[PATCH] libfetch: Properly deal with multi-line proxy responses to CONNECT requests

Currently http_connect() expects the proxy response for CONNECT requests
to consist of a single response line with no common HTTP headers and
otherwise leaves parts of the HTTP response unread.

OpenSSL does not appreciate that:

# Privoxy is being used as proxy
fk@r500 ~/papers $fetch https://www.usenix.org/system/files/conference/osdi14/osdi14-paper-yuan.pdf
34380892520:error:140770FC:SSL routines:SSL23_GET_SERVER_HELLO:unknown protocol:/usr/src/secure/lib/libssl/../../../crypto/openssl/ssl/s23_clnt.c:787:
fetch: https://www.usenix.org/system/files/conference/osdi14/osdi14-paper-yuan.pdf: Authentication error

The attached patch fixes this.
Comment 1 Dag-Erling Smørgrav freebsd_committer 2015-10-14 15:54:33 UTC
Created attachment 162043 [details]
Revised patch

Hi Fabian,

First, my apologies for not picking this up earlier.  It was not assigned to me, and your cc: drowned in my inbox.

For the record, this bug is trivial to reproduce:

$ sudo pkg install privoxy
$ privoxy /dev/null
$ HTTP_PROXY=http://localhost:8118/ fetch -o/dev/null -v https://www.freebsd.org/ 

I have prepared a different patch that uses http_next_header() to iterate over the response instead of just reading until the buffer is empty (partly for cleanliness and partly because I was unable to convince myself that this would always work correctly).  Could you please confirm that it works for you?
Comment 2 Fabian Keil 2015-10-15 16:20:45 UTC
Your patch works for me as well, thanks.

While testing it I noticed that fetch's behaviour if the proxy
rejects the request isn't ideal, but this is unrelated to
your patch (and mine obviously did not address this either):

# fetch gets 403 from the proxy, fails as expected,
# but does not mention the cause.
fk@r500 ~ $fetch -v https://ads.example.org/
looking up 127.0.1.1
connecting to 127.0.1.1:8118
fetch: https://ads.example.org/: 
fk@r500 ~ $echo $?
1
Comment 3 Dag-Erling Smørgrav freebsd_committer 2015-10-16 10:52:53 UTC
Created attachment 162113 [details]
Revised² patch

That's because http_connect() is missing a call to http_seterr().  Try the new patch.
Comment 4 Fabian Keil 2015-10-16 11:19:20 UTC
Thanks for the quick update. Works for me:

fk@r500 ~ $fetch -v  https://ads.example.org/
looking up 127.0.1.1
connecting to 127.0.1.1:8118
fetch: https://ads.example.org/: Forbidden
Comment 5 commit-hook freebsd_committer 2015-10-16 12:21:48 UTC
A commit references this bug:

Author: des
Date: Fri Oct 16 12:21:44 UTC 2015
New revision: 289419
URL: https://svnweb.freebsd.org/changeset/base/289419

Log:
  Fix two bugs in HTTPS tunnelling:
   - If the proxy returns a non-200 result, set the error code accordingly
     so the caller / user gets a somewhat meaningful error message.
   - Consume and discard any HTTP response header following the result line.

  PR:		194483
  Tested by:	Fabian Keil <fk@fabiankeil.de>
  MFC after:	1 week

Changes:
  head/lib/libfetch/http.c
Comment 6 commit-hook freebsd_committer 2016-01-16 20:24:55 UTC
A commit references this bug:

Author: des
Date: Sat Jan 16 20:24:03 UTC 2016
New revision: 294194
URL: https://svnweb.freebsd.org/changeset/base/294194

Log:
  MFH (r280630): remove all traces of SSLv2 support
  MFH (r285141): remove unused variable
  MFH (r288217): correctly check return value from getaddrinfo(3)
  MFH (r289419): fix bugs in HTTPS tunnelling
  MFH (r289420): use fopen()'s "e" mode instead of fcntl for close-on-exec
  MFH (r291453, r291461): use .netrc for http servers and proxies
  MFH (r292330, r292332): reset bufpos to 0 after refilling in chunked mode

  PR:		194483 199801 193740 204771

Changes:
_U  stable/10/
  stable/10/lib/libfetch/common.c
  stable/10/lib/libfetch/fetch.3
  stable/10/lib/libfetch/file.c
  stable/10/lib/libfetch/http.c
  stable/10/usr.bin/fetch/fetch.1
  stable/10/usr.bin/fetch/fetch.c
Comment 7 commit-hook freebsd_committer 2016-01-26 07:44:41 UTC
A commit references this bug:

Author: des
Date: Tue Jan 26 07:44:27 UTC 2016
New revision: 294776
URL: https://svnweb.freebsd.org/changeset/base/294776

Log:
  MFH (r261233): cleanup
  MFH (r261234): increase buffer size
  MFH (r280630): remove all traces of SSLv2 support
  MFH (r285141): remove unused variable
  MFH (r288217): correctly check return value from getaddrinfo(3)
  MFH (r289419): fix bugs in HTTPS tunnelling
  MFH (r289420): use fopen()'s "e" mode instead of fcntl for close-on-exec
  MFH (r291453, r291461): use .netrc for http servers and proxies
  MFH (r292330, r292332): reset bufpos to 0 after refilling in chunked mode

  PR:		194483 199801 193740 204771

Changes:
_U  stable/9/
_U  stable/9/lib/
_U  stable/9/lib/libfetch/
  stable/9/lib/libfetch/common.c
  stable/9/lib/libfetch/fetch.3
  stable/9/lib/libfetch/file.c
  stable/9/lib/libfetch/http.c
_U  stable/9/usr.bin/
_U  stable/9/usr.bin/fetch/
  stable/9/usr.bin/fetch/fetch.1
  stable/9/usr.bin/fetch/fetch.c
Comment 8 Miłosz Kaniewski 2016-10-04 11:09:59 UTC
Created attachment 175412 [details]
Correct while statement.

Hi,

I think there is a typo in a while statement (see patch for details). For only one header it worked well but if there is more headers (in my case there were two of them) then problem described in bug description appeared again.
Comment 9 commit-hook freebsd_committer 2016-12-30 14:55:32 UTC
A commit references this bug:

Author: des
Date: Fri Dec 30 14:54:54 UTC 2016
New revision: 310823
URL: https://svnweb.freebsd.org/changeset/base/310823

Log:
  Fix inverted loop condition which broke multi-line responses to CONNECT.

  PR:		194483
  Submitted by:	Mi?osz Kaniewski <milosz.kaniewski@gmail.com>
  MFC after:	1 week

Changes:
  head/lib/libfetch/http.c
Comment 10 commit-hook freebsd_committer 2017-01-09 14:14:27 UTC
A commit references this bug:

Author: des
Date: Mon Jan  9 14:13:47 UTC 2017
New revision: 311786
URL: https://svnweb.freebsd.org/changeset/base/311786

Log:
  MFH (r310823): fix multi-line CONNECT responses

  PR:		194483

Changes:
_U  stable/11/
  stable/11/lib/libfetch/http.c
Comment 11 commit-hook freebsd_committer 2017-01-10 08:13:56 UTC
A commit references this bug:

Author: des
Date: Tue Jan 10 08:12:56 UTC 2017
New revision: 311864
URL: https://svnweb.freebsd.org/changeset/base/311864

Log:
  MFH (r301027): fix 307 / 308 redirects
  MFH (r310823): fix multi-line CONNECT responses

  PR:		112515 173451 194483 209546

Changes:
_U  stable/10/
  stable/10/lib/libfetch/http.c