Opened 4 weeks ago

Closed 2 weeks ago

Last modified 8 days ago

#28100 closed defect (fixed)

Tor shouldn't set Content-Type: application/octet-stream when compressing results

Reported by: Hello71 Owned by: Hello71
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version: Tor: 0.3.1.1-alpha
Severity: Normal Keywords: 033-backport-maybe-not, 034-backport-maybe-not, 035-backport-maybe-not
Cc: arma Actual Points:
Parent ID: Points:
Reviewer: teor Sponsor:

Description

arma complained on IRC about his browser trying to download descriptor files instead of displaying them in the browser.

I tracked this to tor replacing the Content-Type with application/octet-stream when compression is done.

At least in modern HTTP, the Content-Type should not change with compression. You can see this by using curl --compressed -v or -I on any modern web server. Example:

$ curl --compressed -I https://www.torproject.org
HTTP/1.1 200 OK
Date: Thu, 18 Oct 2018 01:48:00 GMT
Server: Apache
Content-Location: index.html.en
Vary: negotiate,Accept-Encoding
TCN: choice
X-Content-Type-Options: nosniff
X-Frame-Options: sameorigin
X-Xss-Protection: 1
Referrer-Policy: no-referrer
Strict-Transport-Security: max-age=15768000; preload
Content-Security-Policy: default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline';
Last-Modified: Wed, 17 Oct 2018 18:48:13 GMT
ETag: "3ca6-578711cc71540-gzip"
Accept-Ranges: bytes
Content-Encoding: gzip
Cache-Control: max-age=3600
Expires: Thu, 18 Oct 2018 02:48:00 GMT
Content-Length: 4049
Content-Type: text/html
Content-Language: en

I didn't really look hard for any specs. It's probably somewhere in some RFC, but as a practical matter, 100% of web servers do this and 100% of web clients expect this.

I'm pretty sure this won't break Tor, since I searched the code for "content-type" and didn't find any results actually checking the type, only setting it.

Child Tickets

Change History (11)

comment:1 Changed 4 weeks ago by teor

Keywords: 033-backport-maybe 034-backport-maybe 035-backport-maybe added
Milestone: Tor: 0.3.5.x-final
Reviewer: teor
Status: newneeds_revision
Version: Tor: 0.3.1.1-alpha

This code is in ​https://github.com/torproject/tor/pull/416 , but it needs to be split into its own pull request.

It's a simple bugfix on 0.3.1.1-alpha, so we might want to backport it.

comment:2 Changed 4 weeks ago by Hello71

Owner: set to Hello71
Status: needs_revisionassigned

comment:3 Changed 4 weeks ago by Hello71

Status: assignedneeds_revision

comment:4 Changed 4 weeks ago by Hello71

Status: needs_revisionneeds_review

comment:5 Changed 4 weeks ago by Hello71

I think I wanted to put this in #22233 due to the magical ".z" behavior. Upon further consideration though, I realized that real Tor clients don't care about what the Content-Type is anyways. I checked Onionoo source (I assume https://gitweb.torproject.org/onionoo.git/ is correct) and that doesn't check the Content-Type either, only sets it. So I think this is safe to merge with minimal testing.

comment:6 Changed 2 weeks ago by teor

Status: needs_reviewneeds_revision

I left a review on the pull request.

I also want to test this branch with chutney, because we don't have chutney CI yet.

comment:7 Changed 2 weeks ago by teor

Keywords: 033-backport-maybe-not 034-backport-maybe-not 035-backport-maybe-not added; 033-backport-maybe 034-backport-maybe 035-backport-maybe removed
Milestone: Tor: 0.3.5.x-finalTor: 0.3.6.x-final

This change passes chutney.

Stem does check content-type in its unit tests:
https://github.com/torproject/stem/blob/1c030e32539d1e10facbb2684c65103bca45b966/test/unit/descriptor/remote.py#L284
Since we run test-stem in CI as of #27913, I re-ran CI on this branch. It passes:
https://travis-ci.org/torproject/tor/jobs/448224335

I also check if this branch changed browser behaviour. I fetched 127.0.0.1/tor/server/authority[.z] with Safari:

  • authority on 0.3.6.0-alpha-dev shows the text of the descriptor in the browser
  • authority.z on 0.3.6.0-alpha-dev downloads it as a file
  • authority on this branch shows the text of the descriptor in the browser
  • authority.z on this branch shows the text of the descriptor in the browser
    • was transparent decompression the intended behaviour?

This change is a tor version distinguisher for directory authorities and directory mirrors (including bridges). But it isn't a version distinguisher for onion services, because they do not use compression. So I think it's ok to merge.

It might not be necessary to backport this branch: I don't think we need a backport so that people can use their browser windows to view particular tor URLs.

Once the comments from the review have been fixed, please put this ticket in merge_ready.

Edit: clarify backport status

Last edited 2 weeks ago by teor (previous) (diff)

comment:8 in reply to:  7 Changed 2 weeks ago by Hello71

Replying to teor:

I also check if this branch changed browser behaviour. I fetched 127.0.0.1/tor/server/authority[.z] with Safari:

  • authority on 0.3.6.0-alpha-dev shows the text of the descriptor in the browser
  • authority.z on 0.3.6.0-alpha-dev downloads it as a file
  • authority on this branch shows the text of the descriptor in the browser
  • authority.z on this branch shows the text of the descriptor in the browser
    • was transparent decompression the intended behaviour?

It is unintended, but too difficult to avoid given the current code structure. As long as it doesn't cause any issues, I don't see any reason to change it. Once 0.2.9 is EOL, we should stop sending .z requests, and then at some point after that, we should stop accepting .z requests, so then it really won't matter.

comment:9 Changed 2 weeks ago by teor

Status: needs_revisionmerge_ready

Thanks for the comment changes, we're good to merge to master.

It might not be necessary to backport this branch: I don't think we need a backport so that people can use their browser windows to view particular tor URLs.

comment:10 Changed 2 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

Squashed and merged.

comment:11 Changed 8 days ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

Note: See TracTickets for help on using tickets.