format string mismatch in CreateDXVAManagerEvent::Run()

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

({csectype-wildptr, sec-moderate})

Trunk
mozilla51
All
Windows
csectype-wildptr, sec-moderate
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox48 wontfix, firefox49 fixed, firefox50 fixed, firefox51 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main49-])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8782125 [details] [diff] [review]
avoid passing nsCString objects to %s format string codes

I took the liberty of fixing up (some of) the nsACString stuff floating around
as well.
Attachment #8782125 - Flags: review?(gsquelart)
(Assignee)

Updated

2 years ago
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.
Keywords: csectype-wildptr, sec-moderate
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+
(Assignee)

Comment 4

2 years ago
(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.)
(Assignee)

Comment 6

2 years ago
It looks like this code was introduced in bug 1273691 and bug 1271525.
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox50: --- → affected
Group: core-security → media-core-security
https://hg.mozilla.org/mozilla-central/rev/7d3bc3efebe0
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → wontfix
status-firefox51: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Comment 8

2 years ago
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.