1874489 - (CVE-2024-3859) Read beyond bounds in OpenTypeSTAT::Parse()

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[mozillabugs]

OpenTypeSTAT::Parse() (gfx\ots\src\stat.cc) can read beyond bounds if a font header contains specially-crafted values for designAxisSize, designAxisCount, and designAxisOffset. The bug can be reached only on 32-bit platforms, and it appears to be latent -- Searchfox indicates that OpenTypeSTAT::Parse() is not called anywhere, though it is built into the Firefox executable.

The bug is on lines 62-68 (FIREFOX_121_0_RELEASE):

 31: bool OpenTypeSTAT::Parse(const uint8_t* data, size_t length) {
 32:   Buffer table(data, length);
 33:   if (!table.ReadU16(&this->majorVersion) ||
 34:       !table.ReadU16(&this->minorVersion) ||
 35:       !table.ReadU16(&this->designAxisSize) ||
 36:       !table.ReadU16(&this->designAxisCount) ||
 37:       !table.ReadU32(&this->designAxesOffset) ||
 38:       !table.ReadU16(&this->axisValueCount) ||
 39:       !table.ReadU32(&this->offsetToAxisValueOffsets) ||
 40:       !(this->minorVersion < 1 || table.ReadU16(&this->elidedFallbackNameID))) {
 41:     return Drop("Failed to read table header");
 42:   }
 ...
 51:   if (this->designAxisSize < sizeof(AxisRecord)) {
 52:     return Drop("Invalid designAxisSize");
 53:   }
 54: 
 55:   size_t headerEnd = table.offset();
 56: 
 57:   if (this->designAxisCount == 0) {
 58:     if (this->designAxesOffset != 0) {
 59:       Warning("Unexpected non-zero designAxesOffset");
 60:       this->designAxesOffset = 0;
 61:     }
 62:   } else {
 63:     if (this->designAxesOffset < headerEnd ||
 64:         size_t(this->designAxesOffset) +
 65:           size_t(this->designAxisCount) * size_t(this->designAxisSize) > length) {
 66:       return Drop("Invalid designAxesOffset");
 67:     }
 68:   }

The issue is that the calculation on lines 64-5 can overflow on 32-bit builds, because in those builds, size_t is 32 bits. So, for example, if designAxesOffset is 0xffffffff, designAxisCount is 16, and designAxisSize is 8, the calculation yields 0x7f in 32 bits. This would then pass the length check on line 65. Once that occurs, line 71 sets table.offset_ to 0xffffffff. The table.Readxxx() functions on lines 74-6 then read from buffer_ + 0xffffffff, then wrap back to 0...:

 69: 
 70:   for (size_t i = 0; i < this->designAxisCount; i++) {
 71:     table.set_offset(this->designAxesOffset + i * this->designAxisSize);
 72:     this->designAxes.emplace_back();
 73:     auto& axis = this->designAxes[i];
 74:     if (!table.ReadU32(&axis.axisTag) ||
 75:         !table.ReadU16(&axis.axisNameID) ||
 76:         !table.ReadU16(&axis.axisOrdering)) {
 77:       return Drop("Failed to read design axis");
 78:     }
 79:     if (!CheckTag(axis.axisTag)) {
 80:       return Drop("Bad design axis tag");
 81:     }
 82:     if (!ValidateNameId(axis.axisNameID)) {
 83:       return true;
 84:     }
 85:   }

because the ReadUxxx() functions' checks also overflow, e.g., ReadU32() (gfx/ots/src/ots.h) does:

 135: bool ReadU32(uint32_t *value) {
 136:   if (offset_ + 4 > length_) {
 137:     return OTS_FAILURE();
 138:   }
 139:   std::memcpy(value, buffer_ + offset_, sizeof(uint32_t));
 140:   *value = ots_ntohl(*value);
 141:   offset_ += 4;
 142:   return true;
 143: }

where offset_ isa size_t, so lines 136 and 141 overflow from 0xffffffff to 3 on the first call to ReadU32(), passing the check on lines 136-38 and corrupting the value of offset on line 141.

There appears also to be a similar bug involving offsetToAxisValueOffsets in the same code.

Comment 1

[Andrew McCreight [:mccr8]]

FWIW, code coverage data on SearchFox says we are running at least some of the code in OpenTypeSTAT::Parse.

Comment 2

[mozillabugs]

(In reply to Andrew McCreight [:mccr8] from comment #1)

FWIW, code coverage data on SearchFox says we are running at least some of the code in OpenTypeSTAT::Parse.

This is interesting, as SearchFox search says it's not called, yet it gets built into the executable, so something must reference it (the linker should remove unreferenced code). Aha! It gets called when visiting...wait for it....google.com . So this isn't latent.

Comment 3

[mozillabugs]

(In reply to mozillabugs from comment #2)

(In reply to Andrew McCreight [:mccr8] from comment #1)

FWIW, code coverage data on SearchFox says we are running at least some of the code in OpenTypeSTAT::Parse.

This is interesting, as SearchFox search says it's not called, yet it gets built into the executable, so something must reference it (the linker should remove unreferenced code). Aha! It gets called when visiting...wait for it....google.com . So this isn't latent. The caller in this case is Font::ParseTable() in gfx\ots\src\ots.cc, so SearchFox is missing a live one.

Comment 4

[mozillabugs]

(In reply to mozillabugs from comment #3)

(In reply to mozillabugs from comment #2)

(In reply to Andrew McCreight [:mccr8] from comment #1)

FWIW, code coverage data on SearchFox says we are running at least some of the code in OpenTypeSTAT::Parse.

This is interesting, as SearchFox search says it's not called, yet it gets built into the executable, so something must reference it (the linker should remove unreferenced code). Aha! It gets called when visiting...wait for it....google.com . So this isn't latent. The caller in this case is Font::ParseTable() in gfx\ots\src\ots.cc, so SearchFox is missing a live one. I'll try to get a POC working soon.

Sorry about the multiple replies; I clicked the wrong icon.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Has this been reported to Chrome?

Comment 6

[mozillabugs]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)

Has this been reported to Chrome?

No. Does the Chrome team own OTS? Would Mozilla like me to report it there while this bug is pending here?

Comment 7

[Jeff Muizelaar [:jrmuizel]]

I'm not sure it's exact ownership, but it's originally from Google. I think reporting it there probably makes sense.

Comment 8

[mozillabugs]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)

I'm not sure it's exact ownership, but it's originally from Google. I think reporting it there probably makes sense.

I'll report it to the Chromium team. It seems to be widely-used, but I'm not sure who (if anyone) is actually the official maintainer at this point. https://bugzilla.mozilla.org/show_bug.cgi?id=1341088 seems to indicate that https://github.com/khaledhosny/ots was/is(?) the official repository for it, but the "security" tab there has no content, so reporting it there doesn't seem safe.

Comment 9

[Jonathan Kew [:jfkthame]]

Khaled is (and has always been, IIRC) the primary maintainer, though others (including myself) sometimes contribute. But I'm not sure what his preference would be for reporting potentially sensitive issues.

I do have an email address for him, if you'd like me to reach out directly. Though in this case, I'd be inclined to simply open a PR to use 64-bit arithmetic in the problematic calculation, and it'd probably get merged pretty quickly.

Comment 10

[mozillabugs]

If (as seems likely) this bug turns out to be exploitable, I'd like to minimize the number of projects that get 0-dayed. Does Mozilla have a policy on this issue?

Comment 11

[mozillabugs]

The data that can be forced to be read using this mechanism could be unowned heap if the STAT table in the font file is compressed. If the STAT table is uncompressed, the data is from a preceding part of the font file, (See GetTableData() in gfx/ots/src/ots.cc for details)

Here's a POC that demonstrates the read beyond bounds. Use it thusly:

1. Build FF for a 32-bit architecture (I used Win32).
2. Copy ffbug_2218.htm to a webserver in <root>/ffbug_2218/ffbug_2218.htm .
3. Copy Montserrat-VariableFont_wght-hacked.ttf to <root>/resources/fonts/Montserrat/Montserrat-VariableFont_wght-hacked.ttf (or put it somewhere else and modify the path in ffbug_2218.htm appropriately.
4. Start FF and attach a debugger to it.
5. Set a BP on OpenTypeSTAT::Parse().
6. Load ffbug_2218.htm .
7. When the BP fires, step lines 33-42.
8. Examine this->designAxesOffset and this->offsetToAxisValueOffsets . Notice that they're 0xfffffffc and 0xfffffffe, respectively.
9. Step lines 62-8 and notice that the check doesn't detect the invalid value for this->designAxesOffset .
10. Step into line 71 and notice that it sets table.offset_ to 0xfffffffc.
11. Step into line 74 and notice that ReadU32()'s check overflows, and that the memcpy() reads from buffer+0xfffffffc , which wraps to buffer-4 (where buffer is the lowest valid address here).
12. Set execution to line 91 to demonstrate the second, similar read beyond bounds.
13. Notice that the check on lines 96-102 doesn't detect the broken value of this->offsetToAxisValueOffsets.
14. Step the first iteration of the loop beginning on line 104 and notice that line 107 reads from buffer-2 instead of from buffer.

There are also some other overflow-vulnerable computations in this file, such as line 110. Really this code should be using CheckedInt or something like it.

Comment 12

[mozillabugs]

Attachment: ffbug_2218.htm

Comment 13

[mozillabugs]

Attachment: Montserrat-VariableFont_wght-hacked.ttf

Comment 14

[mozillabugs]

Reported to Google at https://bugs.chromium.org/p/chromium/issues/detail?id=1518885 .

Comment 15

[Jonathan Kew [:jfkthame]]

cc'ing Dominik, who has received the corresponding report to chromium.

Comment 16

[Bob Hood [:bhood]]

Jonathan, could you please assign a Severity to this?

Also, are we dependent on movement upstream for this one, or is there anything we can do to mitigate it?

Comment 17

[Jonathan Kew [:jfkthame]]

The potential consequences of the out-of-bounds reads would be either (a) crash (so this could be a DoS vector), or (b) allowing an invalid STAT table to pass validation, so we pass it through to the rasterizer. In theory that should be harmless, as the rasterizer should handle it safely, but we prefer not to rely on that (especially as we can't audit/fix issues there might be in Core Text or DirectWrite) which is why we validate the resources before using them.

So while this is clearly a valid issue, I don't think it's higher than S3 at this point, though if there were actually an exploitable STAT-based bug in CT or DW, that could make it more significant.

I'd be fine with patching this locally, if we want to fix it in gecko ahead of upstream.

Comment 18

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

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:jfkthame, could you consider increasing the severity of this security bug?

For more information, please visit BugBot documentation.

Comment 19

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1874489 - patch 1 - Don't check STAT designAxisSize if the designAxisCount is zero. r=#gfx-reviewers

This is just https://github.com/khaledhosny/ots/pull/277 from upstream, not really part
of this issue but touching the same code, so simplest to include it here.

Comment 20

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1874489 - patch 2 - Avoid potential arithmetic overflow during Buffer read operations. r=#gfx-reviewers

Comment 21

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1874489 - patch 3 - More careful range checks in STAT parsing. r=#gfx-reviewers

Comment 22

[Jonathan Kew [:jfkthame]]

Once we take these patches locally, I'll also open a PR to land them upstream in OTS.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dfd86e34e53f
patch 1 - Don't check STAT designAxisSize if the designAxisCount is zero. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/eed1acdcdfbd
patch 2 - Avoid potential arithmetic overflow during Buffer read operations. r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/df5fdbb9e3ec
patch 3 - More careful range checks in STAT parsing. r=gfx-reviewers,lsalzman

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/dfd86e34e53f
https://hg.mozilla.org/mozilla-central/rev/eed1acdcdfbd
https://hg.mozilla.org/mozilla-central/rev/df5fdbb9e3ec

Comment 25

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

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

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

For more information, please visit BugBot documentation.

Comment 26

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1874489 - patch 1 - Don't check STAT designAxisSize if the designAxisCount is zero.

This is just https://github.com/khaledhosny/ots/pull/277 from upstream, not really part
of this issue but touching the same code, so simplest to include it here.

Original Revision: https://phabricator.services.mozilla.com/D204916

Comment 27

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1874489 - patch 2 - Avoid potential arithmetic overflow during Buffer read operations.

Original Revision: https://phabricator.services.mozilla.com/D204917

Comment 28

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1874489 - patch 3 - More careful range checks in STAT parsing.

Original Revision: https://phabricator.services.mozilla.com/D204918

Comment 29

[Phabricator Automation]

Uplift Approval Request

• Explanation of risk level: just makes bounds-checking arithmetic more careful to avoid potential overflow
• User impact if declined: potential for out-of-bounds accesses during font validation; could result in crash, unclear whether more serious consequences
• Code covered by automated testing: no
• Is Android affected?: yes
• Needs manual QE test: no
• String changes made/needed: none
• Risk associated with taking this patch: low risk
• Fix verified in Nightly: no
• Steps to reproduce for manual QE testing: n/a

Comment 30

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/3f194d132c92
https://hg.mozilla.org/releases/mozilla-beta/rev/cae7c94e1e11
https://hg.mozilla.org/releases/mozilla-beta/rev/3d3e1580db15

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Is this something we're going to want on ESR also?

Comment 32

[Jonathan Kew [:jfkthame]]

I'd suggest we take it, yes. I'll do the moz-phab uplift thing...

Comment 33

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1874489 - patch 1 - Don't check STAT designAxisSize if the designAxisCount is zero.

This is just https://github.com/khaledhosny/ots/pull/277 from upstream, not really part
of this issue but touching the same code, so simplest to include it here.

Original Revision: https://phabricator.services.mozilla.com/D204916

Comment 34

[Jonathan Kew [:jfkthame]]

Huh, looks like that uplift only did the first patch, which doesn't actually address the issue.... I'll try again.

Comment 35

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1874489 - patch 2 - Avoid potential arithmetic overflow during Buffer read operations.

Original Revision: https://phabricator.services.mozilla.com/D204917

Comment 36

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1874489 - patch 3 - More careful range checks in STAT parsing.

Original Revision: https://phabricator.services.mozilla.com/D204918

Comment 37

[Phabricator Automation]

esr115 Uplift Approval Request

• User impact if declined: potential for out-of-bounds accesses during font validation
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: low
• Explanation of risk level: just makes bounds-checking arithmetic more careful to avoid potential overflow
• String changes made/needed: none
• Is Android affected?: yes

Comment 38

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/59182ff3f684
https://hg.mozilla.org/releases/mozilla-esr115/rev/e28125870322
https://hg.mozilla.org/releases/mozilla-esr115/rev/2cb4e79a2ac0

Comment 39

[Daniel Veditz [:dveditz]]

Attachment: advisory.txt