1653371 - (CVE-2020-15667) heap overflow in libmar

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect, P1)

Description

[crixer]

Attachment: testcase.mar.zip

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/84.0.4147.89 Safari/537.36

Steps to reproduce:

Hi, i just found heap overflow.
attacker can trigger a heap overflow using a maliciously crafted mar file.
However, this vulnerability is difficult to exploit.

this is asan result:
vm@ubuntu:~/Desktop$ ./mar -T ./TEST.mar
Signature block found with 0 signatures
1 additional block found:

• Product Information Block:
  • MAR channel name: firefox-mozilla-central
  • Product version: 38.0a1

SIZE MODE NAME
UndefinedBehaviorSanitizer:DEADLYSIGNAL
==2740700==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x7fead7b12000 (pc 0x7fead7ab97ce bp 0x000080000000 sp 0x7fffba0b4eb8 T2740700)
==2740700==The signal is caused by a READ memory access.
#0 0x7fead7ab97ce /build/glibc-YYA7BZ/glibc-2.31/string/../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:499
#1 0x428efb in mar_insert_item /home/vm/firefox-source/modules/libmar/src/mar_read.c:42:3
#2 0x428efb in mar_consume_index /home/vm/firefox-source/modules/libmar/src/mar_read.c:109:10
#3 0x428efb in mar_read_index /home/vm/firefox-source/modules/libmar/src/mar_read.c:149:29
#4 0x42922c in mar_enum_items /home/vm/firefox-source/modules/libmar/src/mar_read.c:574:9
#5 0x426300 in mar_test /home/vm/firefox-source/modules/libmar/tool/mar.c:120:3
#6 0x426300 in main /home/vm/firefox-source/modules/libmar/tool/mar.c:311:14
#7 0x7fead79520b2 in __libc_start_main /build/glibc-YYA7BZ/glibc-2.31/csu/../csu/libc-start.c:308:16
#8 0x4045fd in _start (/home/vm/Desktop/mar+0x4045fd)

UndefinedBehaviorSanitizer can not provide additional info.
SUMMARY: UndefinedBehaviorSanitizer: SEGV /build/glibc-YYA7BZ/glibc-2.31/string/../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:499
==2740700==ABORTING

bug is here:
https://searchfox.org/mozilla-central/source/modules/libmar/src/mar_read.c#42

namelen can be 0x7fffffff then namelen will be 0xffffffff80000000 when execute memcpy function.

Comment 1

[crixer]

Attachment: you can see namelen +1 that 0xffffffff80000000

Comment 2

[Molly Howell (she/her) (mostly inactive)]

Thanks for the report. I've verified that the segfault does occur as described. For some background, the Firefox updater as shipped requires that all MAR files have a valid signature, and it won't do much with files don't have one; we stop before even parsing their index. That means that the function affected here is only called after the signature has been checked, so it would be necessary to construct a malicious MAR with a valid Mozilla signature in order to deploy an exploit. We should still fix this bug, but that does make it quite minor.

Because it's not entirely obvious just from looking at the code, the problem is that on this line after the length has been incremented to make it 0x80000000, since it is a signed int it gets promoted to size_t for the memcpy call via a sign extension, which turns it into 0xffffffff80000000, and that causes memcpy to read and write off the end of our allocations.

Comment 3

[Molly Howell (she/her) (mostly inactive)]

I'm just gonna go ahead and patch this, it seems straightforward.

Comment 4

[Molly Howell (she/her) (mostly inactive)]

Attachment: Bug 1653371 - Don't used a signed type for a length parameter. r=bytesized

Comment 5

[Molly Howell (she/her) (mostly inactive)]

This bug doesn't have a sec rating yet, so I'll request approval before landing the patch even though I believe it to be minor, just in case.

Comment 6

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 9164469 [details]
Bug 1653371 - Don't used a signed type for a length parameter. r=bytesized

Security Approval Request

• How easily could an exploit be constructed based on the patch?: This is fixing a known class of bug, so anyone familiar with that could see what's going on, but using this bug to craft an effective exploit is another story, since it requires creating a specially crafted version of a file which is required to be signed, and also developing an exploit for the particular memory write bug involved here.
• 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?: Should all be trivial.
• How likely is this patch to cause regressions; how much testing does it need?: There shouldn't be any difference in behavior in normal update scenarios, only when working with specially crafted MAR's as in the PoC here.

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 9164469 [details]
Bug 1653371 - Don't used a signed type for a length parameter. r=bytesized

sec-approval+ (though not needed now)

Comment 8

[Molly Howell (she/her) (mostly inactive)]

https://hg.mozilla.org/integration/autoland/rev/b79b6cc78248eea7fda10bfb76aa273c19c9fa65

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/b79b6cc78248

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Doesn't sound like this is something we need to worry about backporting to ESR.

Comment 11

[Tom Ritter [:tjr]]

Attachment: advisory.txt