1802834 - Issue when trying to install nomachine windows client from downloads dialog (should use ShellExecuteByExplorer for downloads after isolating DLLs to their own subdirectory)

Status: NEW

(Firefox :: File Handling, defect, P3)

Description

[Ilgaz Öcal]

Attachment: nomachineerror.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:109.0) Gecko/20100101 Firefox/109.0

Steps to reproduce:

1. Navigate to https://www.nomachine.com
2. Click "Download Now" which will download an .exe file
3. Double click the downloaded file from "Downloads" window/dialog to run it

Actual results:

Several library (dll) errors displayed with "OK" prompt

Expected results:

Show installer dialogue of NoMachine

Comment 1

[Ilgaz Öcal]

No third-party antivirus installed.
It may be only happening on Windows 11.
Also reported to https://webcompat.com/issues/114776
Forum post covering the issue with a response from nomachine staff. https://forums.nomachine.com/topic/errors-during-installation-on-windows-11

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Firefox::Downloads Panel' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 3

[Negritas Sergiu, Desktop QA]

Hello! I have managed to reproduce the issue with firefox 109.0a1(2022-12-02), 107.0.1 and and 108.0b9. with windows 10 x64. I will mark this issue as NEW and update the corresponding flags in order to get our developers involved.

Have a nice day!

Comment 4

[Adam Pankow]

(In reply to Ilgaz Öcal from comment #1)

No third-party antivirus installed.
It may be only happening on Windows 11.
Also reported to https://webcompat.com/issues/114776
Forum post covering the issue with a response from nomachine staff. https://forums.nomachine.com/topic/errors-during-installation-on-windows-11

Will add that I encountered this today too, but on a Windows 10 install. No third-party AV either, only Windows Defender.

Comment 5

[:Gijs (he/him)]

We explicitly ask explorer to open the file. If it's helpful for the NoMachine devs, our code is obviously open source and can also be searched/explored on the web without making a local build etc. I think the relevant code is https://searchfox.org/mozilla-central/rev/2d24d893669ad0fe8d76b0427b25369d35fcc19b/uriloader/exthandler/win/nsMIMEInfoWin.cpp#69-76 , which calls this method: https://searchfox.org/mozilla-central/rev/2d24d893669ad0fe8d76b0427b25369d35fcc19b/widget/windows/ShellHeaderOnlyUtils.h#48 . If opening via explorer fails, we fall back to doing it ourselves - and in that case I could believe that somehow sandboxing on Firefox would break something (see also the comments on the first file around why we try to open via explorer in the first place).

Molly, I don't suppose you're in a position to try to repro/diagnose what could be going wrong in the interaction with this installer and ShellExecuteByExplorer ?

Comment 6

[Molly Howell (she/her) (no longer active)]

Well. It isn't that ShellExecuteByExplorer is failing, it's that we aren't trying it. If I force calling ShellExecuteByExplorer then the installation proceeds normally. But we explicitly disabled using it for downloaded executables back in bug 1605308, because of various issues you can read about there and in its linked bugs.

Gijs, how would we feel about removing the .exe check, and always using the technique of starting with ShellExecuteByExplorer and falling back to ShellExecuteEx if that fails? Personally, unless I'm missing something, I'm not sure the issues that led us to make that change are more serious than things like this one here.

Comment 7

[:Gijs (he/him)]

(In reply to Molly Howell (she/her) [:mhowell] from comment #6)

Well. It isn't that ShellExecuteByExplorer is failing, it's that we aren't trying it.

Well, that's embarrassing... I'd forgotten about bug 1605308.

If I force calling ShellExecuteByExplorer then the installation proceeds normally. But we explicitly disabled using it for downloaded executables back in bug 1605308, because of various issues you can read about there and in its linked bugs.

Gijs, how would we feel about removing the .exe check, and always using the technique of starting with ShellExecuteByExplorer and falling back to ShellExecuteEx if that fails? Personally, unless I'm missing something, I'm not sure the issues that led us to make that change are more serious than things like this one here.

I'm not sure. AIUI the potential of downloading a dll file that then gets inadvertently loaded by a (potentially benign) executable that is then downloaded and executed later is still there, right? I guess you're saying it's better than breaking other, legit executables 100% of the time? (Needinfo to confirm)

I would tend to agree but I'd like a second opinion (hi, Freddy!).

I'm also wondering if there are other sandbox / mitigation policies that we already (or in the future intend to) employ in Firefox itself that we don't necessarily want to impose on apps started from the downloads dir and that would bolster the case to revert the change from bug 1605308. Chris, can you comment on this?

If there's some uncertainty on how common this kind of breakage is (certainly I personally would not want to predict numbers - is it specific to something the nomachine installer is doing? How common is that thing, whatever it is?), I wonder if there's some way we could collect telemetry - e.g. by collecting count of 0 vs non-0 return codes from ShellExecute through telemetry?

Comment 8

[Masatoshi Kimura [:emk]]

Falling back to ShellExecuteByExplorer will defeat the purpose of bug 1605308. If you consider introducing a fallback, please just backout bug 1605308 and live with DLL hijacking attacks.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Since downloading .dll files is not that common, we may download them into a folder with the same name (ex. testlibrary_dll/testlibrary.dll). This should still prevent the hijacking attack, unless the user is using another browser to download the dll, or a dll was already downloaded in the past, but those are edge cases. Another edge case is a site or ftp giving you separate links for exe and libraries, rather than some kind of package.

Comment 10

[Molly Howell (she/her) (no longer active)]

That kind of DLL hijacking is vanishingly rare as an actual attack, to the point that I'd be willing to call it theoretical, and it can be prevented by legitimate apps themselves (as our installer does, and most other installers do as well). It's hard for me to see mitigating that on our end as being worth breaking legitimate applications.

Comment 11

[Frederik Braun [:freddy]]

Is this the first installer we are breaking in the 3 years since we developed that patch?
Do we have any confidence over the amount of bugs prevented versus installers broken?
We still require a prompt before allowing a DLL to be placed in the download directory, right?

On the one hand, I can see us taking care of our users and help prevent dll hijacking attacks.
On the other hand, the bug we are trying to prevent here are bugs in third-party software of which we have limited knowledge.

If we would allow DLL downloads automatically and without a prompt, I would feel uncomfortable removing our fix from bug 1605308 and would recommend looking for other mitigations. e.g.:

• block list known system DLLs. This would need maintenance and moving in lockstep with Windows releases and is still going to be brittle.
• if DLL lookup is just in the CWD? Then we could switch the working directory to a temporary sub directory

Comment 12

[Ilgaz Öcal]

I have informed the software vendor about this issue and bug report and they have written a 'trouble report' describing the issue and the workaround. On that article they have written:

"The described behaviour is strictly related to how Firefox handles the executable, in particular we found that it sets the 'PreferSystem32Images' flag before running the .exe application. This makes the system link to the system libssl instead of the NoMachine libssl library sported by the package. As a consequence the 'Entry point not found' message is triggered."

https://kb.nomachine.com/TR12T10715

Comment 13

[Molly Howell (she/her) (no longer active)]

(In reply to Frederik Braun [:freddy] from comment #11)

Is this the first installer we are breaking in the 3 years since we developed that patch?

As far as I know, it's the first that's been reported to us. That certainly does not mean it's the first that we've broken.

On the one hand, I can see us taking care of our users and help prevent dll hijacking attacks.
On the other hand, the bug we are trying to prevent here are bugs in third-party software of which we have limited knowledge.

Well, I think it's debatable whether the third-party software really is exhibiting a bug here; I don't see how anything they're doing is incorrect. It's our responsibility as the creator of the execution environment to make sure that we avoid violating any assumptions that might reasonably be made about that environment (or sometimes even assumptions being unreasonably made; we are a web browser after all).

• if DLL lookup is just in the CWD? Then we could switch the working directory to a temporary sub directory

I like this idea, I think this is definitely something we could do.

Comment 14

[Masatoshi Kimura [:emk]]

(In reply to Molly Howell (she/her) [:mhowell] from comment #13)

• if DLL lookup is just in the CWD? Then we could switch the working directory to a temporary sub directory

I like this idea, I think this is definitely something we could do.

Unfortunately, DLL lookup directory is appdir (where exe is located), not CWD. It is not easy to remove it from DLL search path.

Comment 15

[Chris Martin [:cmartin]]

I'm also wondering if there are other sandbox / mitigation policies that we already (or in the future intend to) employ in Firefox itself that we don't necessarily want to impose on apps started from the downloads dir and that would bolster the case to revert the change from bug 1605308. Chris, can you comment on this?

Off the top of my head, I can't think of anything coming down the pipeline that would treat the Downloads folder differently. This question might be a better fit for Freddy anyhow, as I think his team has a better idea of what high-level security features we might implement in the future.

As for solving this issue, I have a proposal that combines some of the ideas others have already mentioned above - Please feel free to poke holes or offer up suggestions for improvement!

Let's combine two of the things suggested above: We switch back to using ShellExecuteByExplorer to launch programs out of the download folder (IE get rid of this special treatment for Downloads) AND we move DLL downloads into their own subfolder in the Download directory (Bikeshed alert: maybe we just call it Dlls?).

This has the added benefit that it protects our users if they open up the Downloads folder itself and double-click the executable, which the current solution doesn't prevent. In this sense, it's actually better that we use ShellExecuteByExplorer, since it's the same mechanism they would be using, and ideally we should protect our users even when they're not using Firefox if the potentially-dangerous situation is one that we allowed to happen.

(It also means that we don't have to rely on folks that write executables to ensure that they disallow adjacent DLL loading, which a lot of program authors would never think of)

It is mildly inconvenient to the user, but as was said above -- How often are people really downloading DLLs? And if they are, presumably they either: 1. Know what they are doing, so the concept of a subdirectory isn't going to throw them off, or 2. Don't know what they're doing, at which point they probably should not be doing anything with DLLs.

Curious to hear people's thoughts on this idea...

Comment 16

[Molly Howell (she/her) (no longer active)]

I'll +1 cmartin's suggestion. Certainly there's ample room for bikeshedding about the subdirectory name (perhaps we should leave some kind of trace of where that directory came from, just as one example), but if we can get past that, it seems to me like this would be an effective compromise.

(In reply to Chris Martin [:cmartin] from comment #15)

It is mildly inconvenient to the user, but as was said above -- How often are people really downloading DLLs?

Anecdotally I can't remember the last time I downloaded just a DLL on its own, and I think I'd be fairly high up the list of people you'd expect to be doing that more than most.

Comment 17

[Ray Kraesig [:rkraesig]]

I suspect most people who download DLLs do so because they get an "MSTHINGY.dll was not found" error, Google it, and are immediately led to one of a dozen websites where they will download a DLL which is at least very plausibly full of malware. There's probably not much we can do at that point.

(A Dlls folder would at least prevent the loading of accidentally- or unknowingly-downloaded DLLs, which is a risk I don't think we have many other mitigations against.)

Comment 18

[Molly Howell (she/her) (no longer active)]

Plus, I actually went looking for a few of those sites when this bug was sent to me last week, and the ones I found don't send you the .dll file directly, it's always zipped. So there really isn't anything we can do about that case, because we can't prevent the user from extracting the thing.

Comment 19

[stefan.rystedt]

Rather then having an special way of executing downloaded executable in the download folder. Would it not be possible to scan the download folder for DLLs when a user tries to run an executable from the download folder. If any DLLs is found send an notice to the user with an dialog about the potential security risk with having downloaded DLLs in that folder and let the user chose to proceed or cancel.
This way the user gets enlighten and can chose to remove the DLLs and be safe no matter how he chose to execute the downloaded executable.

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to stefan.rystedt from comment #19)

Would it not be possible to scan the download folder for DLLs when a user tries to run an executable from the download folder.

AFaik it's expensive, you must walk the whole directory every time, extract each file extension and compare, there's no "search-for-file-with-extension" API.

Comment 21

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #20)

(In reply to stefan.rystedt from comment #19)

Would it not be possible to scan the download folder for DLLs when a user tries to run an executable from the download folder.

AFaik it's expensive, you must walk the whole directory every time, extract each file extension and compare, there's no "search-for-file-with-extension" API.

(also you risk ending up with TOCTOU issues around e.g. incomplete DLL downloads at the time the user runs the executable)

Comment 22

[Chris Martin [:cmartin]]

(In reply to stefan.rystedt from comment #19)

Rather then having an special way of executing downloaded executable in the download folder. Would it not be possible to scan the download folder for DLLs when a user tries to run an executable from the download folder. If any DLLs is found send an notice to the user with an dialog about the potential security risk with having downloaded DLLs in that folder and let the user chose to proceed or cancel.
This way the user gets enlighten and can chose to remove the DLLs and be safe no matter how he chose to execute the downloaded executable.

I'm curious to hear your thoughts about my suggestion in Comment #15. It also doesn't require a special way of executing downloaded executables, but it doesn't require any extra work at launch time, and it has the secondary advantage of protecting the user even when they just launch from Windows Explorer instead of Firefox. Thoughts?

Comment 23

[Frederik Braun [:freddy]]

(In reply to Chris Martin [:cmartin] from comment #15)

I'm also wondering if there are other sandbox / mitigation policies that we already (or in the future intend to) employ in Firefox itself that we don't necessarily want to impose on apps started from the downloads dir and that would bolster the case to revert the change from bug 1605308. Chris, can you comment on this?

Off the top of my head, I can't think of anything coming down the pipeline that would treat the Downloads folder differently. This question might be a better fit for Freddy anyhow, as I think his team has a better idea of what high-level security features we might implement in the future.

Nothing that I know of, no.

As for solving this issue, I have a proposal that combines some of the ideas others have already mentioned above - Please feel free to poke holes or offer up suggestions for improvement!

Let's combine two of the things suggested above: We switch back to using ShellExecuteByExplorer to launch programs out of the download folder (IE get rid of this special treatment for Downloads) AND we move DLL downloads into their own subfolder in the Download directory (Bikeshed alert: maybe we just call it Dlls?).

+1 sounds convincing.

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:spohl, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 25

[Stephen A Pohl [:spohl]]

Moving to the same component as bug 1605308 and setting it as regressor.

Comment 26

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1605308

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:Gijs, could you have a look please?

For more information, please visit auto_nag documentation.