Closed Bug 1744525 Opened 1 year ago Closed 1 year ago

Crash [@ <missing>]

Categories

(Core :: Graphics: CanvasWebGL, defect, P1)

x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
100 Branch
Tracking Status
firefox-esr91 99+ fixed
firefox98 --- wontfix
firefox99 + fixed
firefox100 + verified

People

(Reporter: jkratzer, Assigned: jgilbert)

References

(Blocks 1 open bug)

Details

(Keywords: crash, sec-high, testcase, Whiteboard: [bugmon:bisected,confirmed][fuzzblocker][sec-survey][adv-main99+r][adv-esr91.8+r])

Crash Data

Attachments

(5 files, 1 obsolete file)

Testcase found while fuzzing mozilla-central rev 422c7594bf65 (built with: --enable-address-sanitizer --enable-fuzzing).

This testcase only reproduces on linux with xvfb. As this is the standard method for running our fuzz tests, bugs like these have a significant impact on overall fuzzing performance. Please prioritize accordingly.

Testcase can be reproduced using the following commands:

$ pip install fuzzfetch grizzly-framework
$ python -m fuzzfetch --build 422c7594bf65 --asan --fuzzing -n firefox
$ python -m grizzly.replay ./firefox/firefox testcase.html --xvfb
[@ <missing>]

    =================================================================
    ==703078==ERROR: AddressSanitizer: SEGV on unknown address 0x7fb06a395320 (pc 0x7fb16aeac299 bp 0x7fff739b75d0 sp 0x7fff739b72e0 T0)
    ==703078==The signal is caused by a READ memory access.
        #0 0x7fb16aeac299  (<unknown module>)
        #1 0x7fb1800c57ab  (/usr/lib/x86_64-linux-gnu/dri/swrast_dri.so+0x7857ab)
    
    AddressSanitizer can not provide additional info.
    SUMMARY: AddressSanitizer: SEGV (<unknown module>) 
    ==703078==ABORTING
Attached file Testcase

Bugmon Analysis
Verified bug as reproducible on mozilla-central 20211206092340-422c7594bf65.
Failed to bisect testcase (Testcase reproduces on start build!):

Start: e090b0e10f5de9c444d145caeff7b166fbc17736 (20201207092823)
End: 422c7594bf65d1e930d52165356b0098402ff6fe (20211206092340)
BuildFlags: BuildFlags(asan=True, tsan=False, debug=False, fuzzing=True, coverage=False, valgrind=False, no_opt=False, fuzzilli=False)

Whiteboard: [bugmon:confirm][fuzzblocker] → [bugmon:bisected,confirmed][fuzzblocker]

The severity field is not set for this bug.
:jgilbert, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(jgilbert)

A Pernosco session is available here: https://pernos.co/debug/XwfK8603nlCqyIRumBWajg/index.html

Group: gfx-core-security
Flags: in-testsuite?
Keywords: crash

The main suspicious line from the testcase:

drawArrays(context.TRIANGLES, 145346482, 21017)
Flags: needinfo?(jgilbert)
Severity: -- → S4
Priority: -- → P2

Kelsey, this bug is currently blocking our libfuzzer xpcshell fuzzing efforts. Could you please prioritize accordingly? If there's anything I can do to help, please let me know.

Flags: needinfo?(jgilbert)
Flags: needinfo?(jgilbert)
Priority: P2 → P1

Looking at the Pernosco session I think this might be due to stack-corruption, Kelsey do you agree?

Bug 1755448 might also be a duplicate or related (note the stack-less SEGV report on that bug).

bug 1749292 might also be similar.

Keywords: sec-high

This just showed up in the latest security report. Maybe we can find a webgl owner here.

Flags: needinfo?(bhood)
Assignee: nobody → jgilbert
Flags: needinfo?(bhood)

I think our fake-attrib0 glBufferData allocation is failing, and it doesn't look like we're checking it.

Elsewhere, we have RBAB, but on Mesa, we disabled it in bug 1429963.
We need try harder to re-enable it, I think.

See Also: → 1673468

(RBAB should cause this to fail more safely, even if we forget to check an allocation)

Can you try again with this patch? (Or should I have CI make builds for you?)

Flags: needinfo?(jkratzer)
Severity: S4 → S3

I can do it right now.

Flags: needinfo?(jkratzer)

(In reply to Kelsey Gilbert [:jgilbert] (previously Jeff) from comment #12)

Elsewhere, we have RBAB, but on Mesa, we disabled it in bug 1429963.
We need try harder to re-enable it, I think.

Actually this doesn't disable it, it just tells WebGL to act like it's not present, and do extra work to do validation ourselves, which is fine, probably. (though hurts perf)

I am still able to trigger the crash with the attached test case with the patch applied.

I can get a Pernosco session but I will need to rebuilt with optimization disabled. Let me know if you'd like me to do so.

I think I did it:

[2022-03-14 07:31:51] Result: [@ <missing>] (fe8e32ce:bbbd22ac)
[2022-03-14 07:31:51] Result successfully reproduced

Yes, I'm working on minimizing the testcase.
Right now it looks very internal to mesa.
Current theory is that while our gecko math is proof against overflows, mesa isn't handling this large offset (~140M) very well.
Perhaps mesa is expanding a vec2 to a vec4, and then having i32 overflow issues?

Attachment #9267482 - Attachment is obsolete: true

Looks like it is indeed just the attrib that's needed. Removing the attrib, or causing it to go unused (and thus eliminated by the compiler) prevents the issue.

      let offset = 145346482; // bad
      offset = 0x8000 * 0x10000 / (4*4); // 134217728, bad
      offset -= 0x100; // good
      context.drawArrays(context.TRIANGLES, offset, 3)

Hypothesis:

  • vec2 is padded to vec4, 16 bytes
  • mesa has an i32 overflow if offset*16 >= i32_max

Comment on attachment 9268157 [details]
Bug 1744525 - Prevent too-high vert-count draws on Mesa.

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: I think it would be hard. This is an out-of-bounds read seg-fault.
    This is more of a sec-vector than exploitable on its own.
    I believe Mesa allocates a too-small buffer in this case, and fails when it tries to read off the end. Alternatively, it might instead get addressing math wrong, and be reading from a negative offset, in which case it would be a user-controlled-offset read.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
  • Which older supported branches are affected by this flaw?: all
  • If not all supported branches, which bug introduced the flaw?: None
  • Do you have backports for the affected branches?: No
  • If not, how different, hard to create, and risky will they be?: Easy and not risky.
  • How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions. Existing CI testing should be sufficient.
Attachment #9268157 - Flags: sec-approval?

Comment on attachment 9268157 [details]
Bug 1744525 - Prevent too-high vert-count draws on Mesa.

Approved to land and uplift. For Advisory purposes, please comment if this is restricted to any particular operating system.

Attachment #9268157 - Flags: sec-approval? → sec-approval+

This is restricted to Linux machines running Mesa as a GPU driver. (most Linux systems)

Group: gfx-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 100 Branch

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Bugmon Analysis
Verified bug as fixed on rev mozilla-central 20220322051941-fb31185e427d.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Status: RESOLVED → VERIFIED
Keywords: bugmon

Regarding uplifts, this conflicts with bug 1759942 as-landed. We'll need a rebased patch for Beta/ESR along with approval requests.

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.

Flags: needinfo?(jgilbert)
Whiteboard: [bugmon:bisected,confirmed][fuzzblocker] → [bugmon:bisected,confirmed][fuzzblocker][sec-survey]

Comment on attachment 9269105 [details]
Bug 1744525 - (beta) Prevent too-high vert-count draws on Mesa.

Beta/Release Uplift Approval Request

  • User impact if declined: sec-high on Linux+Mesa
  • Is this code covered by automated tests?: No
  • 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): At worst this should over-reject, but it should be safe.
  • String changes made/needed: none

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined: sec-high on Linux+Mesa
  • Fix Landed on Version: 100
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): At worst this should over-reject, but it should be safe.
Flags: needinfo?(jgilbert)
Attachment #9269105 - Flags: approval-mozilla-esr91?
Attachment #9269105 - Flags: approval-mozilla-beta?

Comment on attachment 9269105 [details]
Bug 1744525 - (beta) Prevent too-high vert-count draws on Mesa.

Approved for 99.0b8. Thanks.

Attachment #9269105 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Prevent too-high vert-count draws on Mesa. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/releases/mozilla-beta/rev/d608a830c35d

Comment on attachment 9269105 [details]
Bug 1744525 - (beta) Prevent too-high vert-count draws on Mesa.

Approved for 91.8esr, thanks for the timely rebase.

Attachment #9269105 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+

Follow-up bustage fix for ESR91 (IsMesa() didn't exist until 93):
https://hg.mozilla.org/releases/mozilla-esr91/rev/6e352c94beadeef5e328d83dad715baefb147f85

Whiteboard: [bugmon:bisected,confirmed][fuzzblocker][sec-survey] → [bugmon:bisected,confirmed][fuzzblocker][sec-survey][adv-main99+r]
Whiteboard: [bugmon:bisected,confirmed][fuzzblocker][sec-survey][adv-main99+r] → [bugmon:bisected,confirmed][fuzzblocker][sec-survey][adv-main99+r][adv-esr91.8+r]

:jgilbert, since this bug contains a bisection range, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Flags: needinfo?(jgilbert)

Comment 3 says "Failed to bisect testcase".

Flags: needinfo?(jgilbert)

I filed an autonag issue for that.

Group: core-security-release
You need to log in before you can comment on or make changes to this bug.