#34383: Accept request if GetHost() in mixed content blocking check fails ------------------------------------------+-------------------------------- Reporter: gk | Owner: tbb-team Type: defect | Status: new Priority: Medium | Milestone: Component: Applications/Tor Browser | Version: Severity: Normal | Keywords: | TorBrowserTeam202006 Actual Points: | Parent ID: Points: | Reviewer: Sponsor: | ------------------------------------------+-------------------------------- While reviewing acat's rebase work in #33533 I noticed the following block in our patch for #23247: {{{ if (!parentIsHttps) { + nsAutoCString parentHost; + rv = innerRequestingLocation->GetHost(parentHost); + if (NS_FAILED(rv)) { + NS_ERROR("requestingLocation->GetHost failed"); + *aDecision = REJECT_REQUEST; + return NS_OK; + } + + bool parentIsOnion = + StringEndsWith(parentHost, NS_LITERAL_CSTRING(".onion")); + if (!parentIsOnion) { + *aDecision = ACCEPT; + return NS_OK; + } + } }}} The corresponding part in Mozilla's code looks like [https://searchfox.org /mozilla-esr68/source/dom/security/nsMixedContentBlocker.cpp#748 this]: {{{ if (!parentIsHttps) { *aDecision = ACCEPT; return NS_OK; } }}} Mozilla does not even check the host but is accepting requests at this point outright while we reject them if `GetHost()` fails.
I wonder whether we should follow them here because I am a little worried that we might introduce a subtle bug by diverging from them. I guess the question is whether it can be the case that we have a .onion URL but the `GetHost()` call is failing. Mozilla does not care as there is no decision to be made depending on the *host* but the scheme alone in this check. However, we do care. Let's look at that from a different angle: the code block we added is essentially a check for a .onion domain: if we don't have one, accept the request otherwise pass it on for further checks. Let's look what we do elsewhere when checking for a .onion domain, e.g. in `IsPotentiallyTrustworthyOnion()`. [https://searchfox.org/mozilla- esr68/source/dom/security/nsMixedContentBlocker.cpp#401 There] we have: {{{ nsAutoCString host; nsresult rv = aURL->GetHost(host); NS_ENSURE_SUCCESS(rv, false); return StringEndsWith(host, NS_LITERAL_CSTRING(".onion")); }}} This means if the `GetHost()` check fails we say "no, that's not a .onion". Following that logic we would get to `parentIsOnion = false` in our code block in question and we should `ACCEPT` the request as well in case the `GetHost()` call fails. -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/34383> Tor Bug Tracker & Wiki <https://trac.torproject.org/> The Tor Project: anonymity online
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs