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

RESOLVED FIXED in Firefox 29

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jib, Assigned: aklotz)

Tracking

Trunk
mozilla30
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(firefox28 unaffected, firefox29 fixed, firefox30 fixed, firefox-esr24 unaffected)

Details

()

Attachments

(2 attachments)

Posted file 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
Hitting Continue appears to work.
Summary: ASSERTION: nsTDependentString must wrap only null-terminated strings, in VideoCaptureDS::SetCameraOutput() / CreatePin() → ASSERTION: nsTDependentString must wrap only null-terminated strings, in InterposeNtCreateFile() (via VideoCaptureDS::SetCameraOutput() / CreatePin())
Assignee: nobody → aklotz
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.
As bsmedberg pointed out on IRC, we should be using nsDependentSubstring instead.
Component: WebRTC → General
aklotz: how likely do you think there's a real security risk here?  The nsDependentString is used only for  WinIOAutoObservation timer(IOInterposeObserver::OpCreateOrOpen, filename);
Posted patch fixSplinter Review
This fix handles the two interposed functions that have UNICODE_STRING pathname parameters. All the other interposed functions use handles.
Attachment #8383859 - Flags: review?(benjamin)
(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.
Flags: needinfo?(aklotz)
Attachment #8383859 - Flags: review?(benjamin) → review+
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
Attachment #8383859 - Flags: approval-mozilla-aurora?
(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)
(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.
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. :-)
https://hg.mozilla.org/mozilla-central/rev/d82096b00f8a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Attachment #8383859 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Group: core-security
You need to log in before you can comment on or make changes to this bug.