607832 - Ask Adobe to use SetDllDirectory instead of SetCurrentDirectory before loading their DLLs for the Shockwave plugin

Status: RESOLVED FIXED

(Plugins Graveyard :: Shockwave (Adobe), defect)

Description

[(no longer active)]

Our fix for bug 286382 broke the Shockwave plugin (bug 607482).  This happens because the plugin tries to load some DLLs from a different directory, and to make it work, it uses SetCurrentDirectory.

We should contact Adobe and ask them to switch to using SetDllDirectory instead, so that we don't have to hack around this in our code.

Comment 1

[(no longer active)]

(In reply to comment #0)
> (bug 607482)

Oops, this should be bug 603679.

Comment 2

[Daniel Veditz [:dveditz]]

Kev: do we have any contacts with the Shockwave (not Flash) team we can work with on this one?

Comment 3

[Kev Needham [:kev]]

I am sure we can get them through the Flash team. Reaching out.

Comment 4

[Jeromie Clark]

I've passed a copy of this bug along to the Shockwave Player team, and have asked them to provide a couple named contacts to CC on this bug.

Comment 5

[Shyama Praveena S]

Latest version of Shockwave Player is already having the changes to use SetDllDirectory. I guess this change is added somewhere in Jan-2011.
Shockwave Player also has backward compatibility player module to take care of older shockwave content. And this module is not yet updated with the changes. Do you want us to update this module also immediately?

Comment 6

[Benjamin Smedberg]

Ehsan, thoughts? Also, does this need to remain private?

Comment 7

[Shyama Praveena S]

https://bugzilla.mozilla.org/show_bug.cgi?id=603679

This was one of the related bug shared to us by Kev on November 09, 2010; probably related to this bug.

Comment 8

[(no longer active)]

(In reply to Shyama Praveena S from comment #5)
> Latest version of Shockwave Player is already having the changes to use
> SetDllDirectory. I guess this change is added somewhere in Jan-2011.
> Shockwave Player also has backward compatibility player module to take care
> of older shockwave content. And this module is not yet updated with the
> changes. Do you want us to update this module also immediately?

That would be great, yes.

One other issue is how we can differentiate between the versions of Shockwave Flash which have this version versus the ones that don't.  Our current hack implemented in bug 603679 is based on the name of the DLL file which seems not sufficient here...

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> Ehsan, thoughts? Also, does this need to remain private?

Yes, since it's possible to inject code in Firefox with a DLL named similarly to Shockwave's.

Comment 9

[Shyama Praveena S]

As per the bug https://bugzilla.mozilla.org/show_bug.cgi?id=286382
The discussion focuses on not to use LoadLibrary with partial paths and then rely on the OS search paths for the dll to get loaded. Instead compose the full path and pass it to LoadLibrary to load the intended dll.

Is this sufficient if Shockwave is updated to pass full/absolute path info to LoadLibrary calls?

Comment 10

[Benjamin Smedberg]

Yes, if you are using full paths then not setting the current directory won't have any effect and we can apply our SetDllDirectory patch.

Comment 11

[Shyama Praveena S]

We will do the suggested changes and target it for the next release of shockwave.

Comment 12

[Shyama Praveena S]

@Ehsan Akhgari:
To determine Shockwave plugin version the File Version field of the dll, np32dswXXXXXXXX.dll can be used. Depending on the type of install or the source from where the installation happens the dll sometimes gets a suffix added in XXXX field (usually the version number). It will be something like np32dsw_1169620.dll.
So something like "name of the dll begining with np32dsw" can be used to get the dll. Currently the latest release version is 12.0.0.112.

If the shockwave patch is removed there might be machines with older version of shockwave already installed and the plugin will fail to load. For this case, is it possible to add a check like, if the vulnerable version of Shockwave is found do not load the plugin and ask the user to update shockwave plugin to the latest version?

Comment 13

[(no longer active)]

(In reply to Shyama Praveena S from comment #12)
> @Ehsan Akhgari:
> To determine Shockwave plugin version the File Version field of the dll,
> np32dswXXXXXXXX.dll can be used. Depending on the type of install or the
> source from where the installation happens the dll sometimes gets a suffix
> added in XXXX field (usually the version number). It will be something like
> np32dsw_1169620.dll.
> So something like "name of the dll begining with np32dsw" can be used to get
> the dll. Currently the latest release version is 12.0.0.112.

Thanks!

> If the shockwave patch is removed there might be machines with older version
> of shockwave already installed and the plugin will fail to load. For this
> case, is it possible to add a check like, if the vulnerable version of
> Shockwave is found do not load the plugin and ask the user to update
> shockwave plugin to the latest version?

I don't think that we can remove the workaround code completely for now, but we will need to do the version check you mentioned above then this bug is fixed to disable the workaround for versions of Shockwave which have this problem fixed.

Comment 14

[Shyama Praveena S]

Thank you.
We are working on the changes to be done. As said before, targeting for the next release of shockwave.
I think once we are live with our changes we will confirm the new version where the fix is available. Post that you can go ahead with your changes.

For the version check (on shockwave) to apply your workaround the DLL path can be picked up from the registry which will have the accurate DLL path. Shockwave registers np32dsw DLL at [HKLM "SOFTWARE\MozillaPlugins\@adobe.com/ShockwavePlayer" "Path" "$INSTDIR\Director\np32dsw_$VVVVVV.dll"]

Comment 15

[Shyama Praveena S]

We have our next release of Shockwave version 12.0.2.122 on April 9th. And in this release we have added the necessary changes to use SetDllDirectory as discussed earlier.

You may use this version value for applying your patch fix for all older version of Shockwave alone.

Kindly let us know when you are applying the SetDllDirectory patch on Shockwave in webkit sources.

Comment 16

[(no longer active)]

Brian, do you have the bandwidth to write the patch here?  I'm afraid that I won't in the near future.  Thanks!

Comment 17

[Brian R. Bondy [:bbondy]]

Sure, I'll try to take it at some point during v23 living on Nightly.

Comment 18

[Brian R. Bondy [:bbondy]]

Hey Ehsan, 

Sorry I'm just getting around to this now, was hoping to get to it sooner.

Did you want this code reverted?

> int wmain(int argc, WCHAR **argv)
> {
> #ifndef XRE_DONT_PROTECT_DLL_LOAD
>   mozilla::SanitizeEnvironmentVariables();
> ***  SetDllDirectoryW(L"");
> #endif


This code gets executed for firefox, does it also for plugin container?
Do we only want to not call it under some conditions?
Don't we want to keep this code so that we don't accidentally load the wrong dll from the current path?
Maybe I just don't understand what needs to be done here.

From Comment 14 it looks like we want to only set the dll directory conditional on the value of the version in the np32dsw_1202122.dll file (path read from HKEY_LOCAL_MACHINE\SOFTWARE\Wow6432Node\MozillaPlugins\@adobe.com/ShockwavePlayer).

12.0.2.122 is the current version of shockwave I just downloaded and installed.

(In reply to Shyama Praveena S from comment #15)
> Kindly let us know when you are applying the SetDllDirectory patch on Shockwave in webkit sources.

I think you mean Gecko, or else maybe this message was meant for somewhere else.

Comment 19

[(no longer active)]

Look at the patch for bug 603679.  In that bug, we added conditional code to not call this SetDllDirectory("") in the plugin-container when loading Shockwave.  What we want to do here is to lift that restriction for newer Shockwave plugin versions (see the earlier discussion in this bug.)  Does that make sense?

Comment 20

[Brian R. Bondy [:bbondy]]

OK thanks that makes sense, as I re-read this bug I missed Comment 1 correcting the bug id.  

It seems like this code is already not being hit though because it searches for np32dsw.dll and the new DLL name is: np32dsw_1202122.dll

Should I change the code to always read from:
HKEY_LOCAL_MACHINE\SOFTWARE\Wow6432Node\MozillaPlugins\@adobe.com/ShockwavePlayer
Path value?
Or was the registry storing the path something that is newer?

Maybe there's nothing to do here though if older versions were always called np32dsw.dll exactly and newer versions are named after the version.

Comment 21

[Brian R. Bondy [:bbondy]]

As long as the convention that newer versions are named after their version holds, I think this is resolved/WFM.

Comment 22

[Brian R. Bondy [:bbondy]]

Please re-open if you disagree with Comment 21.

Comment 23

[Shyama Praveena S]

Somewhere in the middle of 11.6 version of Shockwave, naming of the DLL after its version number is introduced. So the correct way to apply the patch would be by referring to File version (or Product version) field of np32dsw_XXXXXXX.dll. The version value to compare is 12.0.2.122.

So, "code to not call this SetDllDirectory("")" restriction is currently lifted out for these bucket of older Shockwave versions. So there might be users experiencing broken shockwave content when the compatibility module of Shockwave is in use.

So, I prefer, in addition to comparing for np32dsw.dll we also need to handle np32dsw_XXXXXXX.dll cases for anything below 12.0.2 version number.


Looking at the patch https://bugzilla.mozilla.org/attachment.cgi?id=487435&action=diff , pluginFileName is already available, so the registry path of np32dsw that I have mentioned in previous comments here might be irrelevant for this patch.

Comment 24

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

Comment 25

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1d114cc60c7

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

Al can you explain "sec-want"? That doesn't seem to fit with our ESR landing criteria and it will be in ESR24 - why should we take this for the second-to-last esr17?

Comment 28

[Al Billings [:abillings - ex-MoCo]]

Hmm...sec-want is a "nice to have." This is really working around a shockwave issue. I'd only take it because of plugin issues, maybe. I do think that we could probably skip it for ESR17 though.

Comment 29

[Lukas Blakk [:lsblakk] use ?needinfo]

If we have it in ESR 24 then our users have the workaround - update to latest ESR once they are released.  Let's not blow our ESR uplift criteria for this one.