1760674 - (CVE-2022-29916) Utilizing CSS variables caused a browser behavior that leaks the information on visited links

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[mat.sionkowski]

Attachment: files.zip

The video shows it the best: https://drive.google.com/file/d/1StFD0aHblp5iREsysaWbnytvCyj0DnX1/view

If you create multiple links like this:
<a href="https://facebook.com" id="facebook">facebook</a> <br>

with the style like this:
#facebook
{
--visited-facebook: url('facebook-visited.gif');
background-image: var(--visited-facebook);
}

Using the CSS variables will cause the browser to request the background image twice.
If there are multiple links like this, each one with a certain background image - the browser will do one full run of requesting the images, then it will start the second full run of requesting images.

But the behavior completely changes when the same website is refreshed. Somehow caching influences the order of requesting background images.
After refreshing - the browser still performs two runs of requesting images. The first one consists of all images in the right order. But the second run is one of two:
a) unvisited links, and then - at the end - the visited links
or
b) only visited links - but still - at the end.

So no matter what - the background images of visited links get requested right at the end.

So I added additional links at the beginning and at the end to play a role of markers. Their background images contain a "MARKER" keyword.

I opened the list of links in the iframe, and added javascript that reloads this inframe after a second to simulate a refresh.

This way a server creates logs for all requested images.
I created the script that finds the last entry of the MARKER image in the logs. This way we know that each log entry that happens after that is the entry created by visited links.
Script "get_history.sh" shows which images got requested after the MARKER ( meaning that they come from visited links).

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Huh, that's a bit odd, we don't call TriggerImageLoads for visited styles, afaics, and we should always restyle all links (which should trigger the second request in this case) regardless of whether they were visited.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

The other issue is, I guess, that we don't cache the error response, so we do the request every time we restyle.

Comment 3

[mat.sionkowski]

I don't want to pretend that I know anything about what is going on under the hood, but I think you mentioned a method "Link::VisitedQueryFinished" that forces the style change no matter what. But maybe this method isn't even called?

Look at the below code from BaseHistory::RegisterVisitedCallback:

=======================================================================
  // If this link has already been queried and we should notify, do so now.
  switch (links->mStatus) {
    case VisitedStatus::Unknown:
      break;
    case VisitedStatus::Unvisited:
      if (!StaticPrefs::layout_css_notify_of_unvisited()) {
        break;
      }
      [[fallthrough]];
    case VisitedStatus::Visited:
      aLink->VisitedQueryFinished(links->mStatus == VisitedStatus::Visited);
      break;
=====================================================================

It looks like some check is performed even before the "VisitedQueryFinished" is being run. So if the browser assumes the link is unvisited on this stage - it will not go deeper. Hence - will not reload styles.

Comment 4

[Andrew McCreight [:mccr8]]

(I edited the previous comment to fix the markup.)

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Yeah, but layout.css.notify-of-unvisited should be true, so we should get to VisitedQueryFinished, afaict.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1760674. r=mak

The PoC uses two interesting facts to observe the restyle timing:

• That CSS variables create new CSS values.
• That we don't cache error image responses.

However, that arguably is not the root cause, since we should restyle
both visited and unvisited links. The root cause is what causes the
timing to be different.

This patch fixes that. If we stop tracking visited links, the next time
a link with the same URL is added to the document we will trigger an
extra query, which wouldn't happen for known-unvisited links.

This is really long-standing behavior, but I should've fixed this as
part of bug 1591717, arguably...

This isn't observable, except in the order of the query results (since
without this patch only non-visited links resolve immediately in
RegisterVisitedCallback instead of VisitedQueryFinished), so not sure
how easy it is to really test this, ideas welcome.

Comment 7

[Tom Ritter [:tjr]]

Adding Tor; they'll want this I think. (Unless it's safe enough to uplift to ESR for them.)

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed: https://hg.mozilla.org/integration/autoland/rev/ddf7a48e3bf19b0b4e8c5d34df1e7d31bbee65c5

Backed out for causing mochitest failures in Link.cpp:
https://hg.mozilla.org/integration/autoland/rev/7d3600925e24a1c8cf634968d0afa43e41e00d1d

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry&revision=ddf7a48e3bf19b0b4e8c5d34df1e7d31bbee65c5
Failure log: https://treeherder.mozilla.org/logviewer?job_id=372672112&repo=autoland

Assertion failure: mState == State::Unvisited (Why would we want to know our visited state otherwise?), at /builds/worker/checkouts/gecko/dom/base/Link.cpp:64

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

r=mak
https://hg.mozilla.org/integration/autoland/rev/ad9c69e7ecf4d55e9bc649e09b7e4f1cc077c21c
https://hg.mozilla.org/mozilla-central/rev/ad9c69e7ecf4

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9269209 [details]
Bug 1760674. r=mak

Beta/Release Uplift Approval Request

• User impact if declined: Potential history leak.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: comment 0 has STR
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Pretty simple patch.
• String changes made/needed: none

Comment 11

[Donal Meehan [:dmeehan]]

:emilio we are finalizing the last RC build.
If you want to modify this to approval-mozilla-release, we could consider this as a dot release ride along?

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9269209 [details]
Bug 1760674. r=mak

Beta/Release Uplift Approval Request

• User impact if declined:
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String changes made/needed:

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Tom, just confirming that you think it's worth uplifting. Should I try to uplift to ESR too? I think it should apply cleanly.

Comment 14

[Tom Ritter [:tjr]]

It would be great to get this into a release and ESR (especially for Tor) but we shouldn't do anything special/out of the ordinary to take it this late in the cycle.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9269209 [details]
Bug 1760674. r=mak

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Privacy issue with trivial-ish fix.
• User impact if declined: History leak.
• Fix Landed on Version: 100
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Patch is very simple and well understood.

Comment 16

[Alexandru Trif, Desktop Test Engineering [:atrif]]

We started investigation and verification of this bug, we will come back with a comment with our results when the investigation/verification is completed.

Comment 17

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Hello! Reproduced the issue with Firefox 100.0a1 (2022-03-21) on Windows 10x64 and Ubuntu 20.04. After refreshing the page the visited links appear again after the END MARKER inside the web console and appear as visited when running the provided script from comment 0.

With Firefox 100.0a1 (2022-04-03) on Windows 10x64, macOS 11.6, and Ubuntu 20 there are no links displayed inside the Firefox web console after END MARKER, but after some refreshes, random links are displayed as visited when running the attached script. Screen recording here and here. Is this expected or am I doing something wrong here when refreshing?

Also, I noticed that sometimes after refresh not all links are loaded twice inside the web console. As can be seen inside the recording here the BEGIN MARKER is displayed and only three links are displayed after and not all links from the test webpage which is ending with END MARKER. Is this expected?

Thank you in advance!

Comment 18

[Emilio Cobos Álvarez (:emilio)]

If the script isn't able to reliably detect which links are visited, that means that the fix is working.

Comment 19

[Alexandru Trif, Desktop Test Engineering [:atrif]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #18)

If the script isn't able to reliably detect which links are visited, that means that the fix is working.

Thank you, Emilio! The script is only showing random links as visited after refreshing the page and not the exact ones that were visited. Marking this as verified with Firefox 100.0a1 (2022-04-03) per comment 17 ad comment 18.

Comment 20

[Daniel Veditz [:dveditz]]

re-rating as sec-high to match previous examples of history leaking

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 22

[Donal Meehan [:dmeehan]]

Comment on attachment 9269209 [details]
Bug 1760674. r=mak

Rejecting release uplift request, see Comment 14

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9269209 [details]
Bug 1760674. r=mak

The consensus is that bug 1762387 isn't a blocker to uplifting this fix. Approved for 91.9esr.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/6b9f8bda69c2

Comment 25

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Verified fixed with 91.9.0esr (20220426172435) on Windows 10x64, macOS 11.6 and Ubuntu 20. The script is only showing random links as visited after refreshing the page and not the exact ones that were visited.

Comment 26

[Frederik Braun [:freddy]]

Attachment: advisory.txt

Comment 27

[mat.sionkowski]

This bug/ticket has been fixed and public for a while so I don't see any issues in covering it publicly by me, but just to keep you informed - I covered it in my Youtube video: https://www.youtube.com/watch?v=0upamEvT-_w

Thank you again for the whole journey!

Comment 28

[Frederik Braun [:freddy]]

Thank you for posting the video!