1861254 - Integer underflow in LocaleAtomFromPath()

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Andrew McCreight [:mccr8]]

I looked over the code reachable from ContentParent::RecvGetHyphDict(), and it looks like LocaleAtomFromPath() has an integer underflow on this line: locale.Truncate(locale.Length() - 4);. If the string sent by the child process is shorter than 4 characters, it'll underflow into a very large value. However, I think underflow is defined for unsigned integers, so this isn't undefined behavior, and Truncate() will just crash if you pass in a value larger than the length of the string. So I'm not sure it is worth doing anything about this, unless the IPC fuzzers stumble across it.

Comment 1

[Christian Holler (:decoder)]

I cannot see if this was hit in fuzzing because it is a MOZ_RELEASE_ASSERT which we ignore (if we hit one, we reset immediately, so there shouldn't be a big performance penalty associated). However, it might be worthwhile to confirm this isn't happening anywhere in production as it sounds like an easy parent crash and could have stability implications.

Comment 2

[Andrew McCreight [:mccr8]]

Ah, that makes sense. Yeah, a fix here would just bail earlier so it sounds like it wouldn't change anything from a fuzzing perspective.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

This code was added in bug 1487212. Jonathan mind taking a look? It should be a simple fix.

Comment 4

[Jonathan Kew [:jfkthame]]

This wouldn't ever arise in normal operation; to get a string shorter than 4 chars here, we'd have to have a compromised/broken content process sending the message. (Note also the MOZ_ASSERT just above. But of course that's not present in release builds. So we should add a check-and-bail-out to be on the safe side.)

The IPC fuzzer presumably has (debug) asserts enabled? In that case a too-short string would trigger the earlier MOZ_ASSERT, before reaching the underflow-affected Truncate().

Comment 5

[Christian Holler (:decoder)]

(In reply to Jonathan Kew [:jfkthame] from comment #4)

The IPC fuzzer presumably has (debug) asserts enabled? In that case a too-short string would trigger the earlier MOZ_ASSERT, before reaching the underflow-affected Truncate().

No, IPC Fuzzing runs without debug asserts right now (we would violate too many). We only catch diagnostic asserts.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1861254 - Safely bail out of LocaleAtomFromPath if passed a bad hyphenation-file path. r=#layout

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41529cdc8638
Safely bail out of LocaleAtomFromPath if passed a bad hyphenation-file path. r=layout-reviewers,emilio

Comment 8

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/41529cdc8638