Closed Bug 596798 Opened 10 years ago Closed 9 years ago

win_rand.c (among others) uses unsafe _snwprintf

Categories

(NSS :: Libraries, defect, P2)

x86
Windows XP
defect

Tracking

(blocking1.9.2 .17+, status1.9.2 .17-fixed, blocking1.9.1 needed, status1.9.1 .19-fixed)

RESOLVED FIXED
3.12.9
Tracking Status
blocking1.9.2 --- .17+
status1.9.2 --- .17-fixed
blocking1.9.1 --- needed
status1.9.1 --- .19-fixed

People

(Reporter: preed, Assigned: nelson)

References

Details

(Whiteboard: [sg:audit?])

Attachments

(1 file, 1 obsolete file)

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.
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?
(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.
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.
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.
If this bug is only about updater.cpp it can be opened: there is no untrusted data handled by that file.
(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?
(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.
(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.
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
Assignee: nobody → nobody
Component: Application Update → Libraries
Product: Toolkit → NSS
QA Contact: application.update → libraries
Summary: updater.cpp (among others) uses unsafe _snwprintf → win_rand.c (among others) uses unsafe _snwprintf
Version: 1.9.2 Branch → trunk
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.
(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.
OS: Windows XP → All
OS: All → Windows XP
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';
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';
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.
Whiteboard: [sg:moderate?]
blocking1.9.1: --- → needed
blocking1.9.2: --- → needed
status2.0: --- → ?
The updater has been fixed in bug 597630.
Attached patch Proposed patch v1 for win_rand.c (obsolete) — Splinter Review
This seems like the simplest solution.
Assignee: nobody → nelson
Attachment #482598 - Flags: review?(wtc)
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.
Attachment #482598 - Flags: review?(wtc) → review+
Target Milestone: --- → 3.13
Whiteboard: [sg:moderate?] → [sg:audit?]
This would be a good patch for NSS 3.12.x.

Bob, is it too late for NSS 3.12.9?
Target Milestone: 3.13 → 3.12.10
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: 3.12.10 → 3.12.9
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
Attachment #482598 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
blocking1.9.1: needed → .18+
blocking1.9.2: needed → .15+
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 release-drivers@mozilla.org
blocking1.9.1: .19+ → .20+
blocking1.9.2: .17+ → .18+
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
blocking1.9.1: .20+ → needed
blocking1.9.2: .18+ → .19+
NSS 3.12.9 was shipped with 1.9.2.17 and 1.9.1.19
blocking1.9.2: .20+ → .17+
status2.0: ? → ---
Depends on: 618368
Group: core-security
You need to log in before you can comment on or make changes to this bug.