We should call SetDllDirectory("") in our NSIS installers as a precaution

RESOLVED FIXED in Firefox 19

Status

()

defect
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

({sec-moderate})

unspecified
mozilla19
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox18 affected, firefox19 fixed, firefox-esr10 wontfix, firefox-esr17 wontfix, b2g18 wontfix)

Details

(Whiteboard: [adv-main19-])

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

7 years ago
There is no known DLL injection call that would be fixed by calling SetDllDirectory(""), but it would be good practice to do so. 

SetDllDirectory("") helps only with LoadLibrary calls, so this isn't a blanket fix for all DLL injection attacks to the installer.  In particular it doesn't help with implicitly linked DLLs. 

Leaving as security-sensitive for now in case there is a problem I don't know about.

See also bug 792106
Assignee

Comment 1

7 years ago
Posted patch Path v1 (obsolete) — Splinter Review
Attachment #671835 - Flags: review?(robert.bugzilla)
(In reply to Brian R. Bondy [:bbondy] from comment #0)
> There is no known DLL injection call that would be fixed by calling
> SetDllDirectory(""), but it would be good practice to do so. 

The problem can come from the OS itself -- Some of the libraries will attempt to load other OS dlls to see if they're there, and have fallback code if they're not (e.g. Vista vs. Win7). This is, basically, where the dwmapi.dll problem comes from, and I'd lay odds that new stuff will be added in Win8 that will thereafter make Win7 and Vista users vulnerable to injections that are not seen today.
Keywords: sec-moderate
Assignee

Comment 3

7 years ago
Agreed, in these implicitly loaded cases though, SetDllDirectoryW(L"") will not help.
Comment on attachment 671835 [details] [diff] [review]
Path v1

Seems like you should add it to UninstallUnOnInitCommon as well.

There is other code that runs before the common macros and I wonder if it wouldn't be better to run it in the associated .oninit, etc.
Attachment #671835 - Flags: review?(robert.bugzilla)
Assignee

Comment 5

7 years ago
Posted patch Patch v2. (obsolete) — Splinter Review
I think we can just remove the common.nsh changes and let other products port this fix into their own nsi files.
Attachment #671835 - Attachment is obsolete: true
Attachment #675389 - Flags: review?(robert.bugzilla)
Comment on attachment 675389 [details] [diff] [review]
Patch v2.

(In reply to Brian R. Bondy [:bbondy] from comment #5)
> Created attachment 675389 [details] [diff] [review]
> Patch v2.
> 
> I think we can just remove the common.nsh changes and let other products
> port this fix into their own nsi files.
If there is any concern about code running before this call then that would be the better way to go and r=me if you want to go with that instead.
Attachment #675389 - Flags: review?(robert.bugzilla) → review+
Assignee

Comment 7

7 years ago
I'm not really concerned but I think that's the best way, so I'll go with the current patch minus common.nsh and r=you. Thanks!
Not a problem and I hope you have fun for a week without any stub craziness! :)
Assignee

Comment 9

7 years ago
Posted patch Patch v3Splinter Review
Same as last patch -common.nsh.
Carrying forward r+.

(In reply to Robert Strong [:rstrong] (vac 10/26-11/4) (do not email) from comment #8)
> Not a problem and I hope you have fun for a week without any stub craziness!
> :)

fingers crossed :)
Attachment #675389 - Attachment is obsolete: true
Attachment #675394 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/fb30db0fa0aa
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Blocks: 806814
This doesn't meet ESR criteria (sec-moderate) so marking wontfix on esr17.
Marking this wontfix for b2g as well as it is sec-moderate.
If needed,please nominate(tracking-b2g18) in case this would be more severe for b2g vs ESR to get this landed.
Whiteboard: [adv-main19-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.