Closed Bug 1406154 Opened 2 years ago Closed 2 years ago

Stack buffer overflow in nr_transport_addr_fmt_ifname_addr_string

Categories

(Core :: WebRTC: Networking, defect, P1, critical)

54 Branch
x86
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- wontfix
firefox57 --- fixed
firefox58 + fixed

People

(Reporter: bwc, Assigned: bwc)

Details

(Keywords: crash, csectype-bounds, sec-critical, Whiteboard: [adv-main57-][post-critsmash-triage])

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-0fb3f4a6-ff46-42a8-a95c-fe61e0171005.
=============================================================

This is a stack overflow bug, but the stack doesn't make a whole lot of sense to me. Might be that we're overflowing |buffer|. Seems to have been introduced in 54. By far the most common crash in nr_*.
So I've looked through these bugs, and I'm not seeing anything that looks like it might affect this function or its callers. Doing some more digging.
Not seeing any changes to the callsites in that period either.
I wonder if something in our buildconfig changed in 54 that caused us to pick up a different implementation of inet_ntop?
Group: media-core-security
I don't see any crashes in 57 beta, while most (but not all) of the 56 betas had a couple. Maybe fixed? morphed?

The two strncpy("[error]") calls are technically wrong to use "len" rather than the size of buffer, but that's not going to crash (fixed string shorter than buffer). No legit version of inet_ntop() will overflow buffer given those args, and if it's not honoring the buffer length we'd see crashes all over. The crashes have no call stacks to speak of so it could be stack corruption elsewhere that's confusing the crash reporter.

You're going to have to dig into the mini-dumps on this one.
Group: core-security
Summary: Crash in nr_transport_addr_fmt_ifname_addr_string → Stack buffer overflow in nr_transport_addr_fmt_ifname_addr_string
Rank: 9
Priority: -- → P1
So the majority of these seem to be happening when useragent_locale is zh-CN. Wonder what that's about.
(In reply to Byron Campen [:bwc] from comment #6)
> So the majority of these seem to be happening when useragent_locale is
> zh-CN. Wonder what that's about.

longer strings in UTF8?
This crash report has a comment that the crash always happens on a specific site, but I can't see a URL anywhere in the crash data.

https://crash-stats.mozilla.com/report/index/d83ea82d-e6b3-4f46-825f-218300170929
I wonder if part of the problem is snprintf. None of these crashes happen on Windows 10, which has a standards-complaint snprintf that always writes a null-terminator. Prior to windows 10, snprintf would only write a null-terminator if doing so would not truncate the output (ie; if the destination buffer was exactly the same size as the output to be written, there would be no null terminator). If we have an implementation of inet_ntop that relies on snprintf, it could be flawed too. Although I can't see us writing enough output to trigger this problem, unless inet_ntop is doing something weird like writing an IPV4 mapped address in expanded form (eg; 0000:0000:0000:0000:0000:ffff:192.168.100.100).
Let me try putting together a patch that increases the buffer size to accommodate a fully-expanded IPV4-mapped IPV6 address, as well as some belt-and-suspenders null terminators.
Assignee: nobody → docfaraday
MozReview-Commit-ID: KMTpbkvA4N
Attachment #8917928 - Flags: review?(drno)
Comment on attachment 8917928 [details] [diff] [review]
Allow more space for expanded IPV4-mapped IPV6 addresses

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

   Very hard, if I've guessed correctly at the cause of the problem; it would require the user to have a very specific networking setup to trigger the bug, and if I'm correct this is only a read overflow that goes into a string buffer that is output in logging only.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

   Yeah, more or less. But our exposure to vulnerability is low if the comments are correct.

Which older supported branches are affected by this flaw?

   All.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

   They should not be difficult. I want to see if the crash rate is affected before uplifting, though.

How likely is this patch to cause regressions; how much testing does it need?

   Pretty unlikely, we're just lengthening some buffers and adding some paranoid null-terminators.
Attachment #8917928 - Flags: sec-approval?
Since this all China only I bet the problem is somewhere around these users naming their network interfaces with some obscure UTF8 http://searchfox.org/mozilla-central/rev/a984558fa2bbde6492d3fb918496fc0b0835b2ce/media/mtransport/third_party/nICEr/src/net/transport_addr.c#117
We had problems with that before. I don't see/know exactly what is going wrong here exactly, but wanted to mention it.
Comment on attachment 8917928 [details] [diff] [review]
Allow more space for expanded IPV4-mapped IPV6 addresses

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

LGTM.

Can't harm to make these changes. Maybe we think a little bit more about my comment regarding the interface names before landing this.
Attachment #8917928 - Flags: review?(drno) → review+
Comment on attachment 8917928 [details] [diff] [review]
Allow more space for expanded IPV4-mapped IPV6 addresses

sec-approval+ for trunk.
Attachment #8917928 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c8d4905c2bdbb3cfa0db5e07a3cd6ba4eb23fdd
Bug 1406154: Ensure that we avoid truncating the interface description strings in a couple of corner cases. r+drno
https://hg.mozilla.org/mozilla-central/rev/5c8d4905c2bd

Please request Beta approval on this when you get a chance. I've confirmed that it grafts cleanly.
Flags: needinfo?(docfaraday)
Target Milestone: --- → mozilla58
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Comment on attachment 8917928 [details] [diff] [review]
Allow more space for expanded IPV4-mapped IPV6 addresses

Approval Request Comment
[Feature/Bug causing the regression]:
   Not totally sure, crashes started in 54.

[User impact if declined]:
   Rare crashes with unusual network setups.

[Is this code covered by automated tests?]:
   The code, yes, but not this specific bug.

[Has the fix been verified in Nightly?]:
   It seems to not cause any harm at least.

[Needs manual test from QE? If yes, steps to reproduce]: 
   No.

[List of other uplifts needed for the feature/fix]:
   None.

[Is the change risky?]:
   Not really.

[Why is the change risky/not risky?]:
   We're just taking some additional precautions wrt buffer length and null-termination.

[String changes made/needed]:
   None.
Flags: needinfo?(docfaraday)
Attachment #8917928 - Flags: approval-mozilla-beta?
I believe we should take this (very safe/belt-and-suspenders) change for beta
Comment on attachment 8917928 [details] [diff] [review]
Allow more space for expanded IPV4-mapped IPV6 addresses

Even if the volume is tiny, it is a sec critical issue, so taking it.
Attachment #8917928 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: media-core-security → core-security-release
Whiteboard: [adv-main57+]
I'm not 100% convinced that the patch we landed really fixed the issue. If I'm not mistaken there were no crashes on 57 or 58 before this patch landed, but always only in 56 Beta or Release (or before that). I would suggest that we wait until 57 is a couple of weeks into release to check the crash reports then, before opening up the report.

@bwc: what do you think?

@abillings: I don't want to mess with your sec flags directly.
Flags: needinfo?(docfaraday)
Flags: needinfo?(abillings)
This was just going to go into the general roll up of memory corruption and crashing bugs we fixed in 57. If we're not sure it is fixed, I can not mention it in the notes (well, in the bugzilla list on that item).
Flags: needinfo?(abillings)
Whiteboard: [adv-main57+] → [adv-main57-]
Yeah, let's watch what happens when 57 goes to release.
Flags: needinfo?(docfaraday)
Flags: qe-verify-
Whiteboard: [adv-main57-] → [adv-main57-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.