Closed Bug 1179550 Opened 5 years ago Closed 5 years ago

Potential OOB reads due to use of strncpy in StackWalk.cpp

Categories

(Core :: mozglue, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox40 --- unaffected
firefox41 --- fixed
firefox42 --- fixed
firefox-esr38 --- unaffected

People

(Reporter: erahm, Assigned: BenWa)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: [CID 1307848])

Attachments

(1 file)

The strings copied in MozDescribeCodeAddress and Demangle w/ strncpy [1][2][3] are potentially not null terminated. This can lead to future OOB reads.

This appears to be a regression from bug 1172216 where |PL_strncpyz| was swapped back out for |strncpy| (most likely b/c mozglue can't use NSPR).

[1] https://hg.mozilla.org/mozilla-central/annotate/ca0fd580a9ce/mozglue/misc/StackWalk.cpp#l770
[2] https://hg.mozilla.org/mozilla-central/annotate/ca0fd580a9ce/mozglue/misc/StackWalk.cpp#l1037
[3] https://hg.mozilla.org/mozilla-central/annotate/ca0fd580a9ce/mozglue/misc/StackWalk.cpp#l852
Flags: needinfo?(bgirard)
Spoke with :erahm. The difference between |PL_strncpyz| and |strncpy| is that the former has a stronger guarantee for the return value:

82  * [...] The destination string is always terminated with a '\0',
83  * unlike the traditional libc implementation.

We should be safe if we do an unconditional:
buff[len - 1] = '\0'

Eric warns that |PL_strncpyz| is faster:
erahm: it's also more efficient on bsd-ish systems where strncpy will zero out the whole buffer
erahm: I profiled at one point and it was an issue, I think either deadlock detector or DMD was passing in a 4k buffer, it was ridiculous

We wont optimize prematurely for now but if it shows up here in profiles later we will fix it.
Attached patch patchSplinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=935f55fae0b5
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Flags: needinfo?(bgirard)
Attachment #8629062 - Flags: review?(erahm)
Comment on attachment 8629062 [details] [diff] [review]
patch

Review of attachment 8629062 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8629062 - Flags: review?(erahm) → review+
https://hg.mozilla.org/mozilla-central/rev/01e460df7b65
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Is this worth taking on 41?
Flags: needinfo?(bgirard)
Target Milestone: --- → mozilla42
Comment on attachment 8629062 [details] [diff] [review]
patch

I think we should, it's low risk. But also I think there's just debugging code that triggers this code so it's not high impact either.

Approval Request Comment
[Feature/regressing bug #]: bug 1172216
[User impact if declined]: Probably none, the stackwalking is only used during debugging features
[Describe test coverage new/current, TreeHerder]: Has been on central for awhile now.
[Risks and why]: Fairly low, we're forcing the buffers to be null terminated.
[String/UUID change made/needed]: None
Flags: needinfo?(bgirard)
Attachment #8629062 - Flags: approval-mozilla-aurora?
Comment on attachment 8629062 [details] [diff] [review]
patch

Fx41 is on Beta now.
Attachment #8629062 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8629062 [details] [diff] [review]
patch

This fix has been in Nightly and Aurora for over 2 months. Seems safe to uplift to Beta41.
Attachment #8629062 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.