2061161 - BTP: client bounce detection timer can finalize a record before finalHost is updated, exempting the wrong site

Status: NEW

(Core :: Privacy: Anti-Tracking, defect, P2)

Description

[Emma Zühlcke [:emz]]

Found while auditing the BTP navigation state machine for bug 2060310. Pre-existing, and spec-conformant - see below.

The problem. mClientBounceDetectionTimeout is armed when response headers arrive, but finalHost is only written once the document finishes loading. If a destination takes longer than the timer (clientBounceDetectionTimerPeriodMS, default 10s) to finish loading, the record is finalized while finalHost still names the previous hop - and RecordStatefulBounces exempts the final host.

Impact. Two effects, both reproduced:

1. False positive: a slow-loading destination is classified as a bounce tracker while the user is sitting on it.
2. Bypass: a tracker redirects through a throwaway domain which commits but never finishes loading - one pending subresource is enough. The throwaway absorbs the classification while the stateful redirector stays exempt as finalHost.

A third arm, not reproduced: the timer is cancelled at every navigation start and only OnResponseReceived re-arms it, so a cancelled load, an external protocol handoff or a non-http(s) destination leaves the record live with no finalization scheduled.

Reproduced. I'll attach a test case. A -> tracker (stateful client bounce) -> slow destination classifies the destination and exempts the tracker; the identical bounce to a destination which finishes loading gives the exact inverse, which isolates load completion as the cause.

Gecko follows the spec. The Bounce Tracking Mitigations draft arms the timeout in process response received step 4, writes the final host only in process document load step 5, cancels the pending finalization in process navigation start step 2 with nothing re-arming it, and exempts the final host in record bounces step 6.2 with no guard for a null one. Gecko matches all four.

The spec contradicts itself, though: the timer period is defined as "how long to wait for a client redirect after a navigation ends", but the algorithm arms it at response received, which is before the navigation ends.

We should raise this as a spec issue and update the implementation in Gecko.

Chromium already diverges this way. BtmBounceDetector::DidFinishNavigation re-arms at navigation finish rather than at response headers, unconditionally and above the HasCommitted() check, so it also covers the third arm. Same 10s default.

• https://source.chromium.org/chromium/chromium/src/+/main:content/browser/btm/btm_bounce_detector.cc;l=927;drc=e849c0cd9dc0260637fb105eb5364e64b4971f9d

Fix directions. Cheapest that covers both reproduced arms: resolve the committed top level site when the timer fires, reusing the helper bug 2060310 adds. Alternatively re-arm at navigation completion as Chromium does, which also covers the third arm. Writing finalHost at commit would be cleanest but BTP has no commit-time signal today.

Code:

• timer armed in OnResponseReceived: https://searchfox.org/firefox-main/rev/202150dcdade5798ca858b843b51b20112b4d061/toolkit/components/antitracking/bouncetrackingprotection/BounceTrackingState.cpp#789-816
• finalHost written in OnDocumentLoaded: https://searchfox.org/firefox-main/rev/202150dcdade5798ca858b843b51b20112b4d061/toolkit/components/antitracking/bouncetrackingprotection/BounceTrackingState.cpp#861-871
• timer cancelled in OnStartNavigation: https://searchfox.org/firefox-main/rev/202150dcdade5798ca858b843b51b20112b4d061/toolkit/components/antitracking/bouncetrackingprotection/BounceTrackingState.cpp#666-671
• bounce set filled from the channel URI: https://searchfox.org/firefox-main/rev/202150dcdade5798ca858b843b51b20112b4d061/toolkit/components/antitracking/bouncetrackingprotection/BounceTrackingState.cpp#819-823
• RecordStatefulBounces exemptions: https://searchfox.org/firefox-main/rev/202150dcdade5798ca858b843b51b20112b4d061/toolkit/components/antitracking/bouncetrackingprotection/BounceTrackingProtection.cpp#335-362

Comment 1

[Emma Zühlcke [:emz]]

Attachment: WIP: Bug 2061161 - Add a test case for the client bounce detection timer finalizing before finalHost is updated

The intended outcome is asserted with todo(), so the test reports a known
failure until the bug is fixed and then an unexpected pass.