Opened 7 months ago

Closed 6 months ago

#33726 closed defect (fixed)

Fix patch for #23247: Communicating security expectations for .onion

Reported by: acat Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam202004R
Cc: Actual Points:
Parent ID: Points:
Reviewer: pospeselr Sponsor:

Description

While working on #33533 I realized that in the switch to ESR68 (#30429) the patch for #23247 was ported incorrectly. The original patch for ESR60 was 651e4ef7de3e and the mistake was introduced in revision https://github.com/acatarineu/tor-browser/commits/30429+6 (see comment in https://trac.torproject.org/projects/tor/ticket/30429#comment:26).

My understanding is that in the original patch, the block of if (isHttpScheme && IsPotentiallyTrustworthyOnion(innerContentLocation)) { was moved from https://github.com/acatarineu/tor-browser/commit/651e4ef7de3e#diff-b6c711bd6646bb39271394da3fc55d0cL754 to https://github.com/acatarineu/tor-browser/commit/651e4ef7de3e#diff-b6c711bd6646bb39271394da3fc55d0cR737 in order to allow mixed contents in workers for the .onion case (which would get disallowed otherwise).

However, in ESR68 there's IsPotentiallyTrustworthyOrigin with includes IsPotentiallyTrustworthyOnion. So, I think this block: https://github.com/acatarineu/tor-browser/commit/6301359f2742d070b1b4149d13c388e96b1b8080#diff-b6c711bd6646bb39271394da3fc55d0cL778 should not be removed, since it's not the same as the one that is added in https://github.com/acatarineu/tor-browser/commit/6301359f2742d070b1b4149d13c388e96b1b8080#diff-b6c711bd6646bb39271394da3fc55d0cR771.

I think this is not a security issue, the result of this bug is that we are not allowing cases that we should (all cases of IsPotentiallyTrustworthyOrigin that are not .onion).

Child Tickets

Change History (6)

comment:1 Changed 7 months ago by acat

Keywords: TorBrowserTeam202003R added; TorBrowserTeam202003 removed
Status: newneeds_review

comment:2 Changed 7 months ago by cypherpunks

In short: you need a 100% coverage by tests for that set of patches.

comment:3 Changed 7 months ago by pili

Keywords: TorBrowserTeam202004R added; TorBrowserTeam202003R removed

We are no longer in March

comment:4 Changed 7 months ago by sysrqb

Reviewer: pospeselr

comment:5 Changed 6 months ago by pospeselr

Status: needs_reviewmerge_ready

Looks good to me!

comment:6 in reply to:  5 Changed 6 months ago by boklm

Resolution: fixed
Status: merge_readyclosed

Replying to pospeselr:

Looks good to me!

I cherry-picked the patch to branch tor-browser-68.7.0esr-9.5-1 as commit dcae22191c42bdb1948a6e55c7c50e0ab97dbf70.

Note: See TracTickets for help on using tickets.