[tbb-bugs] #34383 [Applications/Tor Browser]: Accept request if GetHost() in mixed content blocking check fails

[Tor Bug Tracker & Wiki]

#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                    |     Resolution:
 Keywords:  TorBrowserTeam202006      |  Actual Points:
Parent ID:                            |         Points:
 Reviewer:                            |        Sponsor:
--------------------------------------+--------------------------
Description changed by gk:

Old description:

> 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.

New description:

 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#comment:1>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online