Opened 5 months ago

Last modified 5 months ago

#34383 new defect

Accept request if GetHost() in mixed content blocking check fails

Reported by: gk Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam202006
Cc: acat, mcs, brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by gk)

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

Child Tickets

Change History (1)

comment:1 Changed 5 months ago by gk

Description: modified (diff)
Note: See TracTickets for help on using tickets.