978177 - ASSERTION: nsTDependentString must wrap only null-terminated strings, in InterposeNtCreateFile() (via VideoCaptureDS::SetCameraOutput() / CreatePin())

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: callstack + vars

This may be a dup of sorts of Bug 124987, since the string appears properly zero-terminated here too, but this assert is getting in my way trying to debug gUM on windows so I wanted to point it out to see if there's anything we can do.

Given the location of the code, I decided to mark it a security bug until we're sure there's no problem, just in case. We can always remove sec that later.

STR:
1. Open URL.
2. Hit "Capture!" + OK

[10368] ###!!! ASSERTION: nsTDependentString must wrap only null-terminated stri
ngs. You are probably looking for nsTDependentSubstring.: 'mData[mLength] == 0',
 file c:\moz\mozilla-central\obj-i686-pc-mingw32-debug\dist\include\nsTString.h,
 line 397

See attached callstack + vars

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Hitting Continue appears to work.

Comment 2

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

regressed when this function was added via bug 902587 (landed in 29).  Note: the system library calling us here likely has a bug itself, which caused it to fail to terminate the GUID string.

The ObjectName is a PUNICODE_STRING, and per http://msdn.microsoft.com/en-us/library/windows/desktop/aa380518%28v=vs.85%29.aspx those do not have to be null-terminated, so we can't assign it to an nsDependentString.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

As bsmedberg pointed out on IRC, we should be using nsDependentSubstring instead.

Comment 4

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

aklotz: how likely do you think there's a real security risk here?  The nsDependentString is used only for  WinIOAutoObservation timer(IOInterposeObserver::OpCreateOrOpen, filename);

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: fix

This fix handles the two interposed functions that have UNICODE_STRING pathname parameters. All the other interposed functions use handles.

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Randell Jesup [:jesup] from comment #4)
> aklotz: how likely do you think there's a real security risk here?  The
> nsDependentString is used only for  WinIOAutoObservation
> timer(IOInterposeObserver::OpCreateOrOpen, filename);
That string is eventually fed into Telemetry but that feature is enabled only in Nightly.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d82096b00f8a

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8383859 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 902587
User impact if declined: Potential security issue if this feature is triggered
Testing completed (on m-c, etc.): Locally
Risk to taking this patch (and alternatives if risky): None, simple fix
String or IDL/UUID changes made by this patch: None

Comment 9

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

(In reply to Aaron Klotz [:aklotz] from comment #6)
> (In reply to Randell Jesup [:jesup] from comment #4)
> > aklotz: how likely do you think there's a real security risk here?  The
> > nsDependentString is used only for  WinIOAutoObservation
> > timer(IOInterposeObserver::OpCreateOrOpen, filename);
> That string is eventually fed into Telemetry but that feature is enabled
> only in Nightly.

So does this still qualify as a sec bug then?  If so, severity?
(testing the update now)

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Randell Jesup [:jesup] from comment #9)
> So does this still qualify as a sec bug then?  If so, severity?
> (testing the update now)
I think that it's better classified as a crasher than as a security bug.

Comment 11

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

My concern would be if the nsDependentString were fed to telemetry without null termination, it that could cause telemetry to read memory it's not supposed to, overrun a buffer, or send random memory to us (or crash).

If this is only done on nightly, that's far less important.  And if it's an error but nothing bad will happen, then it probably doesn't need to be a sec bug.  Of course figuring this out might be more trouble than it's worth. :-)

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d82096b00f8a

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/01934b65d701