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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Irvan Kurniawan (:sourc7)]

Attachment: 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

Comment 1

[Irvan Kurniawan (:sourc7)]

Attachment: asan-doublefree.txt

Comment 2

[Irvan Kurniawan (:sourc7)]

Attachment: asan-freenotmalloced.txt

Comment 3

[Daniel Veditz [:dveditz]]

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?

Comment 4

[Jason Kratzer [:jkratzer]]

This may be related to bug 1812932.

Comment 5

[Timothy Nikkel (:tnikkel)]

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

Comment 6

[Irvan Kurniawan (:sourc7)]

(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.

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1819244. Cherry pic webp commit fix. r?aosmond

https://github.com/webmproject/libwebp/commit/a486d800b60d0af4cc0836bf7ed8f21e12974129

Comment 8

[Irvan Kurniawan (:sourc7)]

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

Comment 9

[Timothy Nikkel (:tnikkel)]

(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)

Comment 10

[Irvan Kurniawan (:sourc7)]

(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 11

[Timothy Nikkel (:tnikkel)]

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

Comment 12

[Irvan Kurniawan (:sourc7)]

(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.

Comment 13

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

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

For more information, please visit auto_nag documentation.

Comment 14

[Timothy Nikkel (:tnikkel)]

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

Comment 15

[Tom Ritter [:tjr]]

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...

Comment 16

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

Cherry pic webp commit fix. r=aosmond
https://hg.mozilla.org/integration/autoland/rev/816dc376678b84e16593143eea566cc2bd283b67
https://hg.mozilla.org/mozilla-central/rev/816dc376678b

Comment 17

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

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.

Comment 18

[Timothy Nikkel (: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

Comment 19

[Dianna Smith [:diannaS]]

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

Approved for 112.0b3 and 102.10esr

Comment 20

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/53b805c752ff

Comment 21

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/44a5d7e8d576

Comment 22

[Irvan Kurniawan (:sourc7)]

Attachment: testcase-fillmemoryint8.html

(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

Comment 23

[Irvan Kurniawan (:sourc7)]

Attachment: testcase-fillmemoryint16.html

Comment 24

[Tom Ritter [:tjr]]

Attachment: advisory.txt