Closed Bug 1819244 (CVE-2023-1999) Opened 1 year ago Closed 1 year ago

Double-free and attempting free on address which was not malloc()-ed on Firefox libwebp WebPEncodeRGBA

Categories

(Core :: Graphics: ImageLib, defect)

defect

Tracking

()

RESOLVED FIXED
113 Branch
Tracking Status
firefox-esr102 112+ fixed
firefox111 --- wontfix
firefox112 + fixed
firefox113 + fixed

People

(Reporter: sourc7, Assigned: tnikkel)

References

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main112+][adv-esr102.10+])

Attachments

(7 files)

Attached file testcase.html

When visiting testcase.html on Firefox ASan 32-bit, the AddresSanitizer print output attempting double-free or attempting free on address which was not malloc()-ed: the stack trace show its occur on media/libwebp library.

From my analysis on ASan stack trace it occurs on libwebp alpha_enc.c ApplyFiltersAndEncode function loop, at first loop the ok=1 it execute VP8BitWriterOutput(&best.bw) to free best.bw pointer then the code assign best = trial pointer. It then execute else branch that execute VP8BitWriterWipeOut(&trial.bw) that freed the trial.bw pointer.

On the second loop ok variable return 0 because AllocateTransformBuffer function using malloc but return NULL due to OOM, that cause ok to return 0, inside that loop it execute else branch that execute VP8BitWriterWipeOut(&trial.bw) that freed the trial.bw pointer.

Then when loop is done and ok variable is still 0, it take else branch that execute VP8BitWriterWipeOut(&best.bw) which freed up the best.bw pointer, however the pointer is still assigned to trial.bw that already freed up on the second loop which lead to AddressSanitizer: attempting double-free

I've filed this security issue on https://bugs.chromium.org/p/webp Issue 603. They are currently working on the patch. I'll post update when they change the issue status to Fixed.

Steps to reproduce:

  1. Compile Firefox 32-bit with AddressSanitizer
  2. Visit attached testcase.html
  3. Firefox tab crashed with AddressSanitizer: attempting double-free or AddressSanitizer: attempting free on address which was not malloc()-ed
Flags: sec-bounty?
Attached file asan-doublefree.txt
Group: firefox-core-security → gfx-core-security
Component: Security → Graphics: ImageLib
Product: Firefox → Core

Is there any opportunity for memory to be reallocated during this loop between the two frees? I know the behavior is undefined and that's bad, but it also doesn't look like there's much opportunity to exploit this. What did the webp folks say about it?

Flags: needinfo?(susah.yak)

This may be related to bug 1812932.

See Also: → 1812932

The 32 bit part must be required to reproduce as my 64 bit asan build does not seem to reproduce.

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

Is there any opportunity for memory to be reallocated during this loop between the two frees? I know the behavior is undefined and that's bad, but it also doesn't look like there's much opportunity to exploit this. What did the webp folks say about it?

After further investigation it turns out there are 2 issues on the code, the first is double-free which cause VP8BitWriterWipeOut(&best.bw) re-free previously freed buffer, on Firefox it mostly hit assertion MOZ_RELEASE_ASSERT((mapelm->bits & ((size_t)0x01U)) != 0) (Double-free?).

The second issue is attempting free on address which was not malloc()-ed on VP8BitWriterWipeOut(&trial.bw); it happen on below code:

for (filter = WEBP_FILTER_NONE; ok && try_map; ++filter, try_map >>= 1) {
      if (try_map & 1) {
         --> FilterTrial trial;
        ok = EncodeAlphaInternal(alpha, width, height, method, filter,
                                 reduce_levels, effort_level, filtered_alpha,
                                 --> &trial);
        if (ok && trial.score < best.score) {
          VP8BitWriterWipeOut(&best.bw);
          best = trial;
        } else {
         --> VP8BitWriterWipeOut(&trial.bw);
        }

The &trial.bw is not malloc()-ed because inside EncodeAlphaInternal function on EncodeLossless the VP8LEncodeStream set WebPEncodingSetError(picture, VP8_ENC_ERROR_OUT_OF_MEMORY); due to malloc return NULL, causing to EncodeAlphaInternal code to take else branch that cause &trial.bw to not set:

static int EncodeAlphaInternal
...
    } else {
      VP8LBitWriterWipeOut(&tmp_bw);
      return 0;
    }

On Firefox ESR 32-bit official build when modify the testcase it able to hit EXCEPTION_ACCESS_VIOLATION_READ on various address i.e 0x0e487008, 0x5f900000, 0x6fa00000 (common), 0x80800000 (common), it also able to hit interesting address i.e 0x41041049 (but hard to reproduce again).

And EXCEPTION_ACCESS_VIOLATION_WRITE on various address i.e 0x01f12ad0, 0x099709a6, 0x099809a7, and 0x10101020.

When I compile Firefox 32-bit I also able to hit the same address, but when trying on Firefox 32-bit official build the je_free mostly hit 0x00000000 which is strange, probably need to tweak the testcase further.

The good news is the libwebp patch has added memset(&result->bw, 0, sizeof(result->bw)); on the else branch which fixed both issues.

Flags: needinfo?(susah.yak)
Summary: Double-free or attempting free on address which was not malloc()-ed on Firefox libwebp WebPEncodeRGBA → Double-free and attempting free on address which was not malloc()-ed on Firefox libwebp WebPEncodeRGBA
Assignee: nobody → tnikkel
Status: NEW → ASSIGNED

After further testing I able to control the EXCEPTION_ACCESS_VIOLATION_WRITE address, the method is filling memory 0x80800000 or 0x80808080 using new Int8Array with .fill(55) the crash address will set to 0x37373747:

eax = 0x37373747	ebp = 0x0c7aee1c	ebx = 0x80800000
ecx = 0x37373743	edi = 0x80808080	edx = 0x050e7000
eflags = 0x00010206	eip = 0x77384435	esi = 0x00008080
esp = 0x0c7aee18
OS|Windows NT|10.0.22621
Crash|EXCEPTION_ACCESS_VIOLATION_WRITE|0x37373747|9

Then when changing to .fill(98) the address will be changed to 0x62626272:

eax = 0x62626272	ebp = 0x0c32f33c	ebx = 0x80800000
ecx = 0x6262626e	edi = 0x80808080	edx = 0x008f9000
eflags = 0x00010202	eip = 0x77384435	esi = 0x00008080
esp = 0x0c32f338
OS|Windows NT|10.0.22621
Crash|EXCEPTION_ACCESS_VIOLATION_WRITE|0x62626272|8

(In reply to Irvan Kurniawan (:sourc7) from comment #8)

After further testing I able to control the EXCEPTION_ACCESS_VIOLATION_WRITE address, the method is filling memory 0x80800000 or 0x80808080 using new Int8Array with .fill(55) the crash address will set to 0x37373747:

To be clear, this is all fixed by the same webp patch as you linked in comment 6? (Thank you for that link btw)

(In reply to Timothy Nikkel (:tnikkel) from comment #9)

(In reply to Irvan Kurniawan (:sourc7) from comment #8)

After further testing I able to control the EXCEPTION_ACCESS_VIOLATION_WRITE address, the method is filling memory 0x80800000 or 0x80808080 using new Int8Array with .fill(55) the crash address will set to 0x37373747:

To be clear, this is all fixed by the same webp patch as you linked in comment 6? (Thank you for that link btw)

Yes it is solved by same webp patch, when debugging above crash on WinDbg, the crash stack is same as second issue which on VP8BitWriterWipeOut(&trial.bw); the trial.bw.buf_ address points to 0x80808080 which is filled with 0x62626262.

Comment on attachment 9321531 [details]
Bug 1819244. Cherry pic webp commit fix. r?aosmond

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: patch gives an attacker the idea that there is some memory issues here but doesn't really tell them how to go about exploiting it
  • 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?: probably all
  • If not all supported branches, which bug introduced the flaw?: None
  • Do you have backports for the affected branches?: Yes
  • If not, how different, hard to create, and risky will they be?: easy
  • How likely is this patch to cause regressions; how much testing does it need?: not likely
  • Is Android affected?: Yes
Attachment #9321531 - Flags: sec-approval?

(In reply to Irvan Kurniawan (:sourc7) from comment #6)

When I compile Firefox 32-bit I also able to hit the same address, but when trying on Firefox 32-bit official build the je_free mostly hit 0x00000000 which is strange, probably need to tweak the testcase further.

After testing for a while on Firefox 32-bit official build, it also able to hit the same address (non-null pointer address) and controlled AV_WRITE address.

Keywords: sec-high

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(aosmond)

S2 because the bots will nag me if I set it any lower.

Severity: -- → S2
Flags: needinfo?(aosmond)

Comment on attachment 9321531 [details]
Bug 1819244. Cherry pic webp commit fix. r?aosmond

Approved to land and uplift.

Should get Updatebot doing smarter stuff...

Attachment #9321531 - Flags: sec-approval? → sec-approval+
Group: gfx-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 113 Branch

The patch landed in nightly and beta is affected.
:tnikkel, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox112 to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(tnikkel)

Comment on attachment 9321531 [details]
Bug 1819244. Cherry pic webp commit fix. r?aosmond

Beta/Release Uplift Approval Request

  • User impact if declined: sec bug
  • 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): simple sec fix in library we import
  • String changes made/needed: none
  • Is Android affected?: Yes
Flags: needinfo?(tnikkel)
Attachment #9321531 - Flags: approval-mozilla-beta?
Attachment #9321531 - Flags: approval-mozilla-esr102?

Comment on attachment 9321531 [details]
Bug 1819244. Cherry pic webp commit fix. r?aosmond

Approved for 112.0b3 and 102.10esr

Attachment #9321531 - Flags: approval-mozilla-esr102?
Attachment #9321531 - Flags: approval-mozilla-esr102+
Attachment #9321531 - Flags: approval-mozilla-beta?
Attachment #9321531 - Flags: approval-mozilla-beta+

(In reply to Irvan Kurniawan (:sourc7) from comment #8)

After further testing I able to control the EXCEPTION_ACCESS_VIOLATION_WRITE address, the method is filling memory 0x80800000 or 0x80808080 using new Int8Array with .fill(55) the crash address will set to 0x37373747:

eax = 0x37373747	ebp = 0x0c7aee1c	ebx = 0x80800000
ecx = 0x37373743	edi = 0x80808080	edx = 0x050e7000
eflags = 0x00010206	eip = 0x77384435	esi = 0x00008080
esp = 0x0c7aee18
OS|Windows NT|10.0.22621
Crash|EXCEPTION_ACCESS_VIOLATION_WRITE|0x37373747|9

Then when changing to .fill(98) the address will be changed to 0x62626272:

eax = 0x62626272	ebp = 0x0c32f33c	ebx = 0x80800000
ecx = 0x6262626e	edi = 0x80808080	edx = 0x008f9000
eflags = 0x00010202	eip = 0x77384435	esi = 0x00008080
esp = 0x0c32f338
OS|Windows NT|10.0.22621
Crash|EXCEPTION_ACCESS_VIOLATION_WRITE|0x62626272|8

Here I attach the testcase that able to ACCESS_VIOLATION_WRITE to 0x62626272 it use int8Array.fill(98) to fill memory on address 0x80808080 with 0x62626262. Then when I change int8Array.fill(98) to Int16Array.fill(10357) it able to ACCESS_VIOLATION_READ to 0x75287530 reproduced on Firefox ESR 102.9.0esr (32-bit).

When changing .fill(number) to another it also able to hit EXCEPTION_INVALID_HANDLE and EXCEPTION_GUARD_PAGE

Flags: sec-bounty? → sec-bounty+
Flags: qe-verify-
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage]
Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main112+]
Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main112+] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main112+][adv-esr102.10+]
Attached file advisory.txt
Group: core-security-release
Alias: CVE-2023-1999
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: