Closed Bug 1296078 Opened 3 years ago Closed 3 years ago

format string mismatch in CreateDXVAManagerEvent::Run()

Categories

(Core :: Audio/Video, defect)

All
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox48 --- wontfix
firefox49 --- fixed
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

(Keywords: csectype-wildptr, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main49-])

Attachments

(1 file)

CreateDXVAManagerEvent::Run has two instances of code that looks like:

    const nsACString& blacklistedDLL = FindD3D9BlacklistedDLL();
    if (!blacklistedDLL.IsEmpty()) {
      mFailureReason.AppendPrintf("D3D9 blacklisted with DLL %s",
                                  blacklistedDLL);
    } else {
      ...
    }

Notice that we're passing the nsACString& to the printf call, rather than the char* contained therein.  clang-cl complains about this, with dire warnings about bad things happening at runtime.  I don't know what MSVC does with this code, but it's clearly wrong.

Filing as a security bug because format string mismatches can be bad, but I'm not sure how bad this one is.
I took the liberty of fixing up (some of) the nsACString stuff floating around
as well.
Attachment #8782125 - Flags: review?(gsquelart)
Assignee: nobody → nfroyd
This kind of type confusion is potentially very bad if content could control the object, but I think this is not a huge deal because it is a preference string.
Comment on attachment 8782125 [details] [diff] [review]
avoid passing nsCString objects to %s format string codes

Review of attachment 8782125 [details] [diff] [review]:
-----------------------------------------------------------------

r+, thank you for the patch.

Why aren't these printf-style methods not checked through gcc/clang attributes? (Happy to have a look in a separate bug.)
Attachment #8782125 - Flags: review?(gsquelart) → review+
(In reply to Gerald Squelart [:gerald] from comment #3)
> Why aren't these printf-style methods not checked through gcc/clang
> attributes? (Happy to have a look in a separate bug.)

I think this is because this code is Windows-only, and we don't compile with gcc/clang on Windows.  I believe MSVC 2015 comes with some sort of format string checking...we should probably turn that on in a separate bug.
(In reply to Nathan Froyd [:froydnj] from comment #4)
> (In reply to Gerald Squelart [:gerald] from comment #3)
> > Why aren't these printf-style methods not checked through gcc/clang
> > attributes? (Happy to have a look in a separate bug.)
> 
> I think this is because this code is Windows-only, and we don't compile with
> gcc/clang on Windows.  I believe MSVC 2015 comes with some sort of format
> string checking...we should probably turn that on in a separate bug.

Oops, of course it's Windows-only code, sorry. But it seems there are indeed such checks in MSVC too.

I've opened bug 1296122 to investigate further. (With no direct link with this secure bug, to protect the guilty.)
It looks like this code was introduced in bug 1273691 and bug 1271525.
Group: core-security → media-core-security
https://hg.mozilla.org/mozilla-central/rev/7d3bc3efebe0
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment on attachment 8782125 [details] [diff] [review]
avoid passing nsCString objects to %s format string codes

Approval Request Comment
[Feature/regressing bug #]: bug 1273691 and bug 1271525
[User impact if declined]: possible weird crashes on windows; it's not clear to me what MSVC would try to do with the badly-written code.
[Describe test coverage new/current, TreeHerder]: we don't have tests for this code, AFAIK, but we do set the relevant prefs in modules/libpref/init/all.js, so we will have people hitting these codepaths.
[Risks and why]: I think this a low risk patch; it converts something that's clearly weird into something that we know works correctly.
[String/UUID change made/needed]: none.
Attachment #8782125 - Flags: approval-mozilla-beta?
Attachment #8782125 - Flags: approval-mozilla-aurora?
Comment on attachment 8782125 [details] [diff] [review]
avoid passing nsCString objects to %s format string codes

Sec bug fix, makes sense, let's uplift this for beta 7.
Attachment #8782125 - Flags: approval-mozilla-beta?
Attachment #8782125 - Flags: approval-mozilla-beta+
Attachment #8782125 - Flags: approval-mozilla-aurora?
Attachment #8782125 - Flags: approval-mozilla-aurora+
Group: media-core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main49-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.