Fix how 'Location' is parsed from header text by jeremyn · Pull Request #6878 · EFForg/https-everywhere

jeremyn · 2016-09-14T22:25:38Z

This fixes the test problem identified in pull request #6871 , where this code apparently (?) never finds the Location header, for example:

(Pdb) self._headerRe
regex.Regex('(?P<name>\\S+?): (?P<value>.*?)\\r\\n', flags=regex.V0)
(Pdb) headerStr
'HTTP/1.1 301 Moved Permanently\nServer: Apache\nDate: Tue Jun  1 12:48:03 PDT 1999 PDT\nReferer: http://itunes.com/\nLocation: http://www.apple.com/itunes/?cid=OAS-US-DOMAINS-itunes.com\nContent-type: text/html\nContent-length: 311\n\n'
(Pdb) headers = self._headerRe.findall(headerStr)
(Pdb) headers
[]
(Pdb)

Note that if in fact location has been incorrectly False-y for a while, then it's possible the code immediately after the raise HTTPFetcherError line has not been run recently or ever, and this fix may reveal new bugs in it.

Pinging @fuglede @Hainish @J0WI for review.

Hainish · 2016-09-15T23:53:26Z

test/rules/src/https_everywhere_checker/http_client.py

-				if not location:
+				location = None
+				for piece in headerStr.split('\n'):
+					if piece.startswith('Location:'):


We should be careful here. Although the Location header is usually capitalized in this way, it is not always. See https://www.shodan.io/search?query=location%3A

You're right, thanks. Also RFC 7230 says header field names are case-insensitive. This should be easy to fix.

Hainish

Otherwise, LGTM

Hainish · 2016-09-16T00:41:54Z

test/rules/src/https_everywhere_checker/http_client.py

+				location = None
+				for piece in headerStr.lower().split('\n'):
+					if piece.startswith('location:'):
+						location = ''.join(piece.split(': ')[1:])


For consistency, you might want to make the argument for the startswith call the same as the argument for the split call. This way, if the web server returns an improperly formatted header (lacking the space) it won't cause an error or unintended behavior.

Good point again, I'll change it. Actually though it would be properly formatted. From the same RFC (bold mine):

Each header field consists of a case-insensitive field name followed by a colon (":"), optional leading whitespace, the field value, and optional trailing whitespace.

Ah, interesting - I've never seen headers lacking the leading whitespace. In that case you might want to change just line 383 to be:

location = ''.join(piece.split(':')[1:]).strip()

''.join instead of ':'.join destroys the :. This wasn't noticeable with the much less likely :<space>, but very noticeable with : as in http://. (I made that mistake too.)

Hainish · 2016-09-16T00:42:54Z

test/rules/src/https_everywhere_checker/http_client.py

 				raise HTTPFetcherError("Pycurl fetch failed for '%s'" % newUrl)
 			elif httpCode in (301, 302, 303, 307):
-				# Parse out the headers and extract location, case-insensitively.
-				# If there are multiple location headers, pick the last one.


I'm curious, why remove the above comments?

I distrust comments in general. The code here is six lines long, it doesn't need two lines of comments.

Fair enough. I differ in opinion on comments, but I don't think it matters in this case.

Hainish · 2016-09-16T01:20:30Z

test/rules/src/https_everywhere_checker/http_client.py

+				location = None
+				for piece in headerStr.lower().split('\n'):
+					if piece.startswith('location:'):
+						location = ':'.join(piece.split(':')[1:]).strip()


I just spotted the source of another possible problem. Here, the location will always be a lowercased version of the URL, due to line 381. URLs should be treated as case-sensitive, though.

URLs in general are case-sensitive (with the exception of machine names). There may be URLs, or parts of URLs, where case doesn't matter, but identifying these may not be easy. Users should always consider that URLs are case-sensitive.

Ref: https://www.w3.org/TR/WD-html40-970708/htmlweb.html

Right again, in fact that comes up in the https://itunes.com example because the Location there is http://www.apple.com/itunes/?cid=OAS-US-DOMAINS-itunes.com . I'll fix it ~~tomorrow~~ before tomorrow.

Hainish · 2016-09-16T21:19:34Z

Great, lgtm

fuglede · 2016-09-17T11:13:47Z

Does this mean that this is actually a bug in curl in general?

jeremyn · 2016-09-17T14:45:08Z

No, I think the bug was entirely on our side because we failed to parse out the Location header. What would the curl bug be?

fuglede · 2016-09-17T15:48:14Z

I was just thinking about #6871 (comment).

jeremyn · 2016-09-17T15:50:54Z

Oh. Of course @J0WI has to confirm but I assume they meant that they saw Location with curl and so confirmed that the bug was in our stuff.

fuglede · 2016-09-17T15:59:04Z

Ah, could be. I'm just getting errors altogether;

$ curl -I https://www.itunes.com
curl: (51) SSL: no alternative certificate subject name matches target host name 'www.itunes.com'
$ curl -I https://itunes.com
curl: (56) SSL read: error:00000000:lib(0):func(0):reason(0), errno 104

jeremyn · 2016-09-17T16:20:09Z

https://www.itunes.com has an invalid certificate, that's what #6871 is trying to fix.

I get something similar with curl -I https://itunes.com, no idea what that means. curl -v https://itunes.com shows the response headers though, including Location.

fuglede · 2016-09-17T17:56:20Z

Looks like it's an iTunes thing: curl/curl#1020 (comment).

jay · 2016-09-17T18:26:50Z

I don't know if this helps but when I did a GET on it through Fiddler debugging proxy the server failed a compliance check:

X-HTTPPROTOCOL-VIOLATION: [ProtocolViolation] The Server did not return properly formatted HTTP Headers. HTTP headers should be terminated with CRLFCRLF. These were terminated with LFLF.

Hainish · 2016-09-19T21:49:46Z

@jeremyn I get no Location header with curl -v

(.venv)user@personal:~$ curl -I https://www.itunes.com
curl: (51) SSL: no alternative certificate subject name matches target host name 'www.itunes.com'
(.venv)user@personal:~$ curl -v https://www.itunes.com
* Rebuilt URL to: https://www.itunes.com/
* Hostname was NOT found in DNS cache
*   Trying 17.172.224.35...
* Connected to www.itunes.com (17.172.224.35) port 443 (#0)
* successfully set certificate verify locations:
*   CAfile: none
  CApath: /etc/ssl/certs
* SSLv3, TLS handshake, Client hello (1):
* SSLv3, TLS handshake, Server hello (2):
* SSLv3, TLS handshake, CERT (11):
* SSLv3, TLS handshake, Server finished (14):
* SSLv3, TLS handshake, Client key exchange (16):
* SSLv3, TLS change cipher, Client hello (1):
* SSLv3, TLS handshake, Finished (20):
* SSLv3, TLS change cipher, Client hello (1):
* SSLv3, TLS handshake, Finished (20):
* SSL connection using TLSv1.2 / AES128-GCM-SHA256
* Server certificate:
*    subject: 1.3.6.1.4.1.311.60.2.1.3=US; 1.3.6.1.4.1.311.60.2.1.2=California; businessCategory=Private Organization; serialNumber=C0806592; C=US; postalCode=95014; ST=California; L=Cupertino; street=1 Infinite Loop; O=Apple Inc.; OU=Internet Services; CN=itunes.com
*    start date: 2015-08-19 00:00:00 GMT
*    expire date: 2017-08-19 23:59:59 GMT
*    subjectAltName does not match www.itunes.com
* SSL: no alternative certificate subject name matches target host name 'www.itunes.com'
* Closing connection 0
* SSLv3, TLS alert, Client hello (1):
curl: (51) SSL: no alternative certificate subject name matches target host name 'www.itunes.com'

FYI, the error indicates that this host is using SAN to indicate which FQDNs the certificate is valid for, but the names listed in the x509 (itunes.com) didn't match the host name you specified (www.itunes.com). You can verify that with this command:

echo 1 | openssl s_client -connect www.itunes.com:443 | openssl x509 -noout -text

Looking at the x509v3 Subject Alternative Name field, we only get itunes.com and not www.itunes.com

jeremyn · 2016-09-19T22:52:13Z

@Hainish https://itunes.com is what redirects and has the Location header, not https://www.itunes.com .

Hainish · 2016-09-20T00:26:11Z

Ah. Yeah in that case the SSL error see is strange. For other https hosts that redirect, curl -I doesn't give me that error:

(.venv)user@personal:~$ curl -I https://eff.org
HTTP/1.1 301 Moved Permanently
Server: nginx
Date: Tue, 20 Sep 2016 00:25:37 GMT
Content-Type: text/html
Content-Length: 178
Connection: keep-alive
Location: https://www.eff.org/
Strict-Transport-Security: max-age=31536000; includeSubdomains
X-Content-Type-Options: nosniff

Fix how 'Location' is parsed from header text

6b85c29

jeremyn mentioned this pull request Sep 14, 2016

Fix ITunes.com.xml HTTPS Mismatch #6871

Merged

jeremyn closed this Sep 15, 2016

jeremyn reopened this Sep 15, 2016

Hainish reviewed Sep 15, 2016

View reviewed changes

Hainish suggested changes Sep 15, 2016

View reviewed changes

Lowercase before checking for location header

52df52e

Hainish suggested changes Sep 16, 2016

View reviewed changes

Improve location header parsing

0b2a3bc

Hainish reviewed Sep 16, 2016

View reviewed changes

Use another approach to get location header value

5377170

Hainish approved these changes Sep 16, 2016

View reviewed changes

Hainish merged commit 3d78fee into EFForg:master Sep 16, 2016

jeremyn deleted the fix-location-header-parsing branch September 17, 2016 14:44

fuglede mentioned this pull request Sep 17, 2016

Accessing https://itunes.com using -I produces no output curl/curl#1020

Closed

Conversation

jeremyn commented Sep 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hainish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hainish Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeremyn Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hainish commented Sep 16, 2016

Uh oh!

fuglede commented Sep 17, 2016

Uh oh!

jeremyn commented Sep 17, 2016

Uh oh!

fuglede commented Sep 17, 2016 via email

Uh oh!

jeremyn commented Sep 17, 2016

Uh oh!

fuglede commented Sep 17, 2016

Uh oh!

jeremyn commented Sep 17, 2016

Uh oh!

fuglede commented Sep 17, 2016

Uh oh!

jay commented Sep 17, 2016

Uh oh!

Hainish commented Sep 19, 2016

Uh oh!

jeremyn commented Sep 19, 2016

Uh oh!

Hainish commented Sep 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Hainish Sep 16, 2016 •

edited

Loading

jeremyn Sep 16, 2016 •

edited

Loading