1406154 - Stack buffer overflow in nr_transport_addr_fmt_ifname_addr_string

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

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_*.

Comment 1

[Byron Campen [:bwc]]

Here's the list of bugs in webrtc networking fixed in 54, but not in 53:

https://bugzilla.mozilla.org/buglist.cgi?f1=cf_status_firefox53&list_id=13816163&o1=notequals&o2=equals&query_format=advanced&f2=cf_status_firefox54&v1=fixed&component=WebRTC%3A%20Networking&v2=fixed

Comment 2

[Byron Campen [:bwc]]

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.

Comment 3

[Byron Campen [:bwc]]

Not seeing any changes to the callsites in that period either.

Comment 4

[Byron Campen [:bwc]]

I wonder if something in our buildconfig changed in 54 that caused us to pick up a different implementation of inet_ntop?

Comment 5

[Daniel Veditz [:dveditz]]

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.

Comment 6

[Byron Campen [:bwc]]

So the majority of these seem to be happening when useragent_locale is zh-CN. Wonder what that's about.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

(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?

Comment 8

[Byron Campen [:bwc]]

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

Comment 9

[Byron Campen [:bwc]]

Here's a report with a better stack:

https://crash-stats.mozilla.com/report/index/dcc097f5-139b-458d-898e-dd5ba0171011

Comment 10

[Byron Campen [:bwc]]

Another similar stack:

https://crash-stats.mozilla.com/report/index/a019c07f-6233-47e5-a35f-a181a0171001

Comment 11

[Byron Campen [:bwc]]

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).

Comment 12

[Byron Campen [:bwc]]

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.

Comment 13

[Byron Campen [:bwc]]

Attachment: Allow more space for expanded IPV4-mapped IPV6 addresses

MozReview-Commit-ID: KMTpbkvA4N

Comment 14

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af54159debae2d516f481b81d4639b6f74343b68

Comment 15

[Byron Campen [:bwc]]

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.

Comment 16

[Nils Ohlmeier [:drno]]

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 17

[Nils Ohlmeier [:drno]]

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.

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8917928 [details] [diff] [review]
Allow more space for expanded IPV4-mapped IPV6 addresses

sec-approval+ for trunk.

Comment 19

[Byron Campen [:bwc]]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 21

[Byron Campen [:bwc]]

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.

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

I believe we should take this (very safe/belt-and-suspenders) change for beta

Comment 23

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b3c9f9d54ebd

Comment 25

[Nils Ohlmeier [:drno]]

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.

Comment 26

[Al Billings [:abillings - ex-MoCo]]

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).

Comment 27

[Byron Campen [:bwc]]

Yeah, let's watch what happens when 57 goes to release.