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

Status: RESOLVED FIXED

(Firefox :: Installer, defect)

Description

[Brian R. Bondy [:bbondy]]

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

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Path v1

Comment 2

[Daniel Veditz [:dveditz]]

(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.

Comment 3

[Brian R. Bondy [:bbondy]]

Agreed, in these implicitly loaded cases though, SetDllDirectoryW(L"") will not help.

Comment 4

[Robert Strong (they/them - no direct email)]

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.

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2.

I think we can just remove the common.nsh changes and let other products port this fix into their own nsi files.

Comment 6

[Robert Strong (they/them - no direct email)]

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.

Comment 7

[Brian R. Bondy [:bbondy]]

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!

Comment 8

[Robert Strong (they/them - no direct email)]

Not a problem and I hope you have fun for a week without any stub craziness! :)

Comment 9

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3

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 :)

Comment 10

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/fb30db0fa0aa

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

This doesn't meet ESR criteria (sec-moderate) so marking wontfix on esr17.

Comment 13

[bhavana bajaj [:bajaj]]

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.