1757038 - [Windows] PATH expansion occurs in every process

Status: NEW

(Core :: Widget: Win32, enhancement, P3)

Description

[Ray Kraesig [:rkraesig]]

(Followup from bug 1753910.)

The PATH environment variable is expanded at startup to avert a class of binary planting attacks (bug 642469). However, this is done in every process, and doesn't really need to be.

Approaches to remedying this include D139388 and D139432.

Comment 1

[Ray Kraesig [:rkraesig]]

The above two patches are both unsatisfying:

• The code in D139432 is self-contained, but its effects are not: it adds a new environment variable which isn't completely trustworthy. (It also does some C-style string twiddling, but that could be avoided.)
• The D139388 patch uses only existing facilities and has fewer lines of code; but it's also very difficult to test, and those existing facilities are somewhat hairy.

It's possible there's a better approach than either of these. But, given that:

• running SanitizeEnvironmentVariables twice will have no practical effect on almost any system,
• the original Microsoft CVE seems to have been fixed as of Win10 (or at least I couldn't reproduce it), and
• PATH-based DLL planting isn't considered a significant risk nowadays,

... I'm not sure this is worth addressing.

Comment 2

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1757038 - limit PATH expansion to launcher process r?tkikuchi

It probably isn't going to help to expand PATH multiple times: if the
first pass didn't get them all, there's likely some recursion afoot.

Expand PATH only in the topmost Firefox process in the process tree.
Add tests confirming this.

Comment 3

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1757038 - limit PATH expansion to launcher process r?tkikuchi

It probably isn't going to help to expand PATH multiple times: if the
first pass didn't get them all, there's probably some recursion afoot.

Expand PATH only in the launcher process (including when the launcher
process is just the browser).

Depends on D139072

Comment 4

[Ray Kraesig [:rkraesig]]

Removing myself as the assignee, since I don't foresee coming back to this anytime soon.