The testcase crashes Firefox ASAN on Linux with SUMMARY: "AddressSanitizer: heap-buffer-overflow /builds/worker/checkouts/gecko/gfx/cairo/libpixman/src/pixman-sse2.c:4919:13 in sse2_composite_over_x888_8_8888 with READ of size 4 at 0x61000074d2f4"

When I change testcase JS code from String.fromCodePoint(10040); into String.fromCodePoint(10100); the ASAN shadow bytes point will change from [04] into [fa].

The code in pixman-sse2.c:4919 (that ASAN show) is from commit Bug 1689998 - Update pixman sources to release 0.40.0 as below:

memcpy(&m, mask++, sizeof(uint32_t));

Reproduced on:

• Firefox ASAN build on last commit ca22dc040c3a6f9257efdaedec848bb48334f8bd (HEAD -> master, origin/master, origin/HEAD) (Sat Mar 6 05:40:39 2021 +0000) on Arch Linux
• Firefox ASAN from fuzzfetch with Build ID 20210305214118 on Arch Linux

Steps to reproduce:

1. Open Firefox ASAN with recent build
2. Visit attached testcase.html
3. When print dialog show, select Destination "Save to PDF" (default on my desktop) or "Print to File" (on Firefox ESR Linux)
4. Click "Save" or "Print"
5. Browser crashed

I can also reproduce this using Firefox ASAN with Build ID 20210308094833 on Windows 10.

sec-moderate because the user interaction required means an attack would need some social-engineering aspect.

Does this crash during actual printing, too? I don't know if we take a different path for saving vs printing (without knowing, I suspect both generate postscript?)

(In reply to Daniel Veditz [:dveditz] from comment #4)

sec-moderate because the user interaction required means an attack would need some social-engineering aspect.

Does this crash during actual printing, too? I don't know if we take a different path for saving vs printing (without knowing, I suspect both generate postscript?)

Thanks Dan, the crash also occur when print destination is other than "Save to PDF" (e.g. Microsoft Print to PDF, Fax)

This is a bug in upstream pixman; I will file a PR there, but we may want to go ahead and fix it in the mozilla tree without waiting for upstream to deal with it.

The important changes here are a handful of places where we replace

        memcpy(&m, mask++, sizeof(uint32_t));

or similar code with

        uint8_t m = *mask++;

because we're only supposed to be reading a single byte from *mask,
and accessing a 32-bit value may read out of bounds (besides that
it reads values we don't actually want; whether this matters would
depend exactly how the value in m is subsequently used).

I've also changed a bunch of other places to use this same pattern
(a local 8-bit variable) when reading individual bytes from the mask;
the code was inconsistent about this, sometimes casting the byte to
a uint32_t instead. This makes no actual difference, it just seemed
better to use a consistent pattern throughout the file.

For Firefox, this is a regression from bug 1689998, because that's where we took the pixman update that includes the bug.

It seems unlikely to be seriously exploitable, as the flaw just means we may look at a few bytes outside the intended area of a mask during a compositing operation. In the worst case, this could cause a segfault if it touches a bad address; more likely just a slightly wrong pixel value at the edge of a masked image.

[Tracking Requested - why for this release]: it would be good if we could avoid shipping a security regression

Comment on attachment 9209432 [details]
Bug 1696844 - Fix mask access in pixman sse2 code. r=jrmuizel

Beta/Release Uplift Approval Request

• User impact if declined: Possible crash when printing (reproducible as an assertion in ASAN builds; less likely to crash a non-ASAN build, but probably possible).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• 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): Trivial fix to avoid reading a multi-byte block (potentially causing a bounds violation) when only a single-byte value is wanted.
• String changes made/needed:

Are we assuming this will be fixed upstream by the time we update the vendored copy, or are we somehow keeping track of this change so we don't regress in the future?

(In reply to Julien Cristau [:jcristau] from comment #12)

Are we assuming this will be fixed upstream by the time we update the vendored copy, or are we somehow keeping track of this change so we don't regress in the future?

I assume it will be fixed before we're likely to re-vendor; the patch has been sent upstream, and if necessary I believe Jeff has contacts that should be able to facilitate getting it integrated.

Also, adding the testcase to our tree means that if we were to regress it that test would start failing.

Fix mask access in pixman sse2 code. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/887b81d71378c7c206804784d81368478e910c24
Add testcase. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/5418c19e3c4636fa1f358a0178c57f869c496295
https://hg.mozilla.org/mozilla-central/rev/887b81d71378
https://hg.mozilla.org/mozilla-central/rev/5418c19e3c46

Comment on attachment 9209432 [details]
Bug 1696844 - Fix mask access in pixman sse2 code. r=jrmuizel

approved for 87 rc2

https://hg.mozilla.org/releases/mozilla-release/rev/b6f13967f7bd
https://hg.mozilla.org/releases/mozilla-release/rev/b4465d6606e9