596798 - win_rand.c (among others) uses unsafe _snwprintf

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[J. Paul Reed [:preed]]

Attachment: Use conforming version of snprintf

While I was working on updater.cpp, a reviewer noticed that it has a #define that switches NS_tsnprintf() to the platform's specific string-printf function.

On windows, this currently gets defined to _snwprintf.

Ostensibly, snprintf was used because it guarantees that the string will be null-terminated.

However, on Win32, this isn't always the case.

According to http://msdn.microsoft.com/en-us/library/2ts7cx93%28VS.80%29.aspx:

* If len < count, then len characters are stored in buffer, a null-terminator is appended, and len is returned.
* If len = count, then len characters are stored in buffer, no null-terminator is appended, and len is returned.
* If len > count, then count characters are stored in buffer, no null-terminator is appended, and a negative value is returned.

updater.cpp doesn't check any of the return values of calls to NS_tsprintf(), so while both glibc and BSD's implementation of snprintf do null-terminate the string, Win32's does not.

Attached is a patch we used to address this issue in updater.cpp.

It should be noted that a mxr search for _snprintf and _snwprintf around the tree show numerous cases of use without checking return values. While the uses of snwprintf() in updater.cpp mostly come from manipulation of file paths, and  those come from a manifest in the mar file, which is verified via hash, it probably isn't exploitable (and even if it were, if you can pwn the mar, you win anyway).

But, I'm not sure about the various other uses in the tree.

There's also a comment in
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/common/windows/string_utils-inl.h#52 that discusses MSVC's swprintf vs. snprintf, but based on tests I did with an executable built using Mozilla's build system (so all the various flags get defined as they would for the app), its claim that snprintf behaves in the same way that BSD and glibc do is incorrect; the MSDN documentation seems to be correct.

Since the impact of the other uses of snwprintf() is unknown, I'm marking this security sensitive, but cc'ing people who I consulted with on this bug.

I also set the milestone to be 1.9.2 builds, since those are currently within their support life, but this bug seems to date back to initial versions of, at least, updater.

Comment 1

[Robert Strong (they/them - no direct email)]

I've been meaning to go through updater.cpp and review it for unsafe calls. In the case of _snwprintf are you referring to it not gauranteeing null termination?

Comment 2

[J. Paul Reed [:preed]]

(In reply to comment #1)
> I've been meaning to go through updater.cpp and review it for unsafe calls. In
> the case of _snwprintf are you referring to it not gauranteeing null
> termination?

Yes, on Win32, its _snprintf implementations (the non-_s ones) don't guarantee to null-terminate on long paths.

Comment 3

[J. Paul Reed [:preed]]

Spitzer pointed out to me, BTW, that while this bug report is in regards to updater.cpp, mxr shows 46 uses of _snwprintf() across 12 files, which may (or may not) be exploitable in a more serious way:

http://mxr.mozilla.org/mozilla2.0/search?string=_snwprintf%28&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla2.0

In the case of updater.cpp, it's mostly a reliability/crash issue, but for instance, it's also used in NSS, bits of XPCOM, and some of the xulrunner runtime.

Comment 4

[Robert Strong (they/them - no direct email)]

I suspect that in the vast majority of uses it will never crash or be exploitable. I'm in no way implying this shouldn't be fixed and as a matter of fact planned on doing so for the updater code.

The other uses should have bugs filed against their components.

Comment 5

[Benjamin Smedberg]

If this bug is only about updater.cpp it can be opened: there is no untrusted data handled by that file.

Comment 6

[Robert Strong (they/them - no direct email)]

(In reply to comment #5)
> If this bug is only about updater.cpp it can be opened: there is no untrusted
> data handled by that file.
Do you mean closed instead of opened?

Comment 7

[Robert Strong (they/them - no direct email)]

(In reply to comment #6)
> (In reply to comment #5)
> > If this bug is only about updater.cpp it can be opened: there is no untrusted
> > data handled by that file.
> Do you mean closed instead of opened?
he meant opened as in remove security sensitive.

So, if this bug is for anything besides updater.cpp it should remain closed for now and I'd prefer it if this bug was moved to a more appropriate component and a new bug that is open filed specifically for updater.cpp. If this bug is just for updater.cpp then it should just be opened.

Comment 8

[J. Paul Reed [:preed]]

(In reply to comment #5)
> If this bug is only about updater.cpp it can be opened: there is no untrusted
> data handled by that file.

This bug was indeed intended to be about updater.cpp, but as I've mentioned twice, _snwprintf is used in other places in the tree that I didn't look into; its usage there could be more problematic. I don't really know.

I agree that from what I can see, updater.cpp would only trip on this bug in pathological installation cases (installed many sub-directories deep) or if the mar accidentally (since it's trusted) broke it, which would likely be caught in QA.

Having said that, I imagine it would be unfortunate to have to do a "chemspill" release because there wasn't a chance to at least skim through the usage of that function in NSS, XPCOM, and various places in the XRE because this bug got opened, and an unfriendly decided to take a closer look and actually find something interesting.

Of course, I defer to those dealing in such currencies daily regarding whether to make a tracking bug for all known-unsafe uses of s*printf on Win32 and make this dependent on it, clone this bug for all the other components, or just open 'er up.

Comment 9

[Robert Strong (they/them - no direct email)]

So, as bsmedberg stated and I agree with this isn't a security sensitive issue in regards to updater.cpp.I agree it would be unfortunate if there were a chemspill due to this which is why I'd like this filed in a more appropriate component so those in the know of the code could evaluate if it is a potential security issue vs. the current people involved in this bug that don't know the other code referenced.I'll go ahead an open one a separate bug for updater.cpp as time permits.Moving over to NSS so people familiar with the NSS code can evaluate the usage of _snwprintf at:http://mxr.mozilla.org/mozilla2.0/source/security/nss/lib/freebl/win_rand.c#153andhttp://mxr.mozilla.org/mozilla2.0/source/security/nss/lib/freebl/win_rand.c#165

Comment 10

[Mook (songbird dead account)]

The set of interesting functions to check is probably:
http://mxr.mozilla.org/mozilla-central/search?string=printf&filter=\b_[a-z]*n[tw]%3Fprintf
That is: starts with underscore, has 'n', and may be t or w.  Unlike the BSD/glibc implementation, the MSVC version has a leading underscore.  A quick skim didn't seem too scary either, though.

Comment 11

[Wan-Teh Chang]

(In reply to comment #9)
Thank you for the bug report.  I didn't know _snwprintf
behaves differently from snwprintf of Unix.

Can we simulate the _TRUNCATE behavior of _snwprintf_s
by blindly doing
    buffer[count - 1] = L'\0';
after the _snwprintf calls?

NSS may need to support older versions of Visual C++, which
is why I'm looking for a solution that doesn't require
_snwprintf_s.

Comment 12

[Robert Strong (they/them - no direct email)]

I think it would be safer to subtract 1 from the count param... something like this should work

int len = _snwprintf(szFileName, sizeof(szFileName)/sizeof(szFileName[0]) - 1,
                     L"%s\\*.*", szSysDir);
if (len == -1)
  return;

szFileName[len + 1] = L'\0';

Comment 13

[Robert Strong (they/them - no direct email)]

I wrote this while way too sleepy and messed up... sorry about that.

(In reply to comment #12)
> I think it would be safer to subtract 1 from the count param... something like
> this should work
> 
> int len = _snwprintf(szFileName, sizeof(szFileName)/sizeof(szFileName[0]) - 1,
>                      L"%s\\*.*", szSysDir);
> if (len == -1)
>   return;
> 
> szFileName[len + 1] = L'\0';
the last line should be
szFileName[len] = L'\0';

Comment 14

[Daniel Veditz [:dveditz]]

Once we fix this in the updater it may call attention to the user of this function. We should clean up the tree all at once.

Comment 15

[Robert Strong (they/them - no direct email)]

The updater has been fixed in bug 597630.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Proposed patch v1 for win_rand.c

This seems like the simplest solution.

Comment 18

[Wan-Teh Chang]

Comment on attachment 482598 [details] [diff] [review]
Proposed patch v1 for win_rand.c

r=wtc.

>+    szFileName[_MAX_PATH - 1] = 0;

Nit: please use L'\0' instead of 0.

Comment 20

[Wan-Teh Chang]

This would be a good patch for NSS 3.12.x.

Bob, is it too late for NSS 3.12.9?

Comment 21

[Wan-Teh Chang]

Attachment: Proposed patch v2 for win_rand.c, by Nelson Bolyard

Patch checked in on the NSS trunk (NSS 3.13) and
NSS_3_12_BRANCH (NSS 3.12.9).

Checking in win_rand.c;
/cvsroot/mozilla/security/nss/lib/freebl/win_rand.c,v  <--  win_rand.c
new revision: 1.27; previous revision: 1.26
done

Checking in win_rand.c;
/cvsroot/mozilla/security/nss/lib/freebl/win_rand.c,v  <--  win_rand.c
new revision: 1.26.4.1; previous revision: 1.26
done

Comment 22

[LegNeato]

Deferring to a future point release as we have run out of time. If this absolutely needs to be fixed and can land today or tomorrow, please send a note to [e-mail]

Comment 23

[Kai Engert (:KaiE:)]

I guess this needs to be landed into Mozilla stable branches as part of landing the next NSS release into Mozilla - which is NSS 3.12.10 - which is not yet released.

The tracker for landing 3.12.0 is bug 642148

Comment 24

[Daniel Veditz [:dveditz]]

NSS 3.12.9 was shipped with 1.9.2.17 and 1.9.1.19