1667872 - Buffer overrun in GetWindowsFolder

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[(Away)]

The SHGetSpecialFolderPathW API returns a BOOL value but we are treating it as HRESULT: https://searchfox.org/mozilla-central/rev/f27594d62e7f1d57626889255ce6a3071d67209f/xpcom/io/SpecialSystemDirectory.cpp#72

Unfortunately, they are both just aliases for some integer type, so the compiler doesn't complain about the mixup. More unfortunately, the two types have the opposite semantics for whether zero means success.

If the call fails, we'll mistakenly continue execution, and the uninitialized path will give a garbage value of len from wcslen(path). Then writing the trailing slash to path[len] can overrun the buffer.

This appears to have originally landed in CVS in 2003 and survived several refactorings: https://github.com/mozilla/gecko-dev/commit/34cd7b0fdbb012410f261f0d85aa71f8179497b3#diff-11b97e1fdc3d3142074da11d7be9a42aR192

It seems unlikely that the overrun would happen in practice, because any null char in the uninitialized garbage of path would make wcslen give an acceptable result. Things would only go wrong if we re-used a 256-char area of stack that didn't have a single null. That doesn't seem likely in shipping builds, but in debug builds it happens 100% reliably due to our use of -ftrivial-auto-var-init=pattern to fill stack buffers with 0xAA: https://searchfox.org/mozilla-central/rev/f27594d62e7f1d57626889255ce6a3071d67209f/build/moz.configure/toolchain.configure#1643

Comment 1

[(Away)]

Attachment: Fix GetWindowsFolder storage

Comment 2

[Andrew McCreight [:mccr8]]

I guess I'll mark this sec-moderate if you don't think it is an issue in practice. Thanks for filing the bug and working on it.

Comment 3

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

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

Comment 4

[Julien Cristau [:jcristau]]

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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Given comment 2, it doesn't sound like this is something we need to worry about uplifting to ESR.