Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sudo -N should preserve the current directory #63

Open
gerardog opened this issue Mar 14, 2024 · 6 comments
Open

sudo -N should preserve the current directory #63

gerardog opened this issue Mar 14, 2024 · 6 comments
Labels
Issue-Bug Something isn't working

Comments

@gerardog
Copy link

gerardog commented Mar 14, 2024

Sudo for Windows version

1.0.0

Windows build number

10.0.26063.1

Other Software

No response

Steps to reproduce

According to my tests, the general expectation regarding sudo is that it preserves the current directory.

If you run sudo rm * or sudo del *.* the user would expect to affect files in the current directory.
If you do sudo notepad ./filename, the user expects that the path resolves relative to the current directory.

But currently:

  • when -N is used
  • or sudo is configured in forceNewWindow mode

... then the current folder is set to C:\WINDOWS\System32 instead

  • Except if sudo is run from an already elevated console. (then it preserves the current directory)

For example

C:\test> del /P *.*
C:\Test\test-file.txt, Delete (Y/N)? Y
Access Denied

C:\test> sudo del /P *.*
C:\Test\test-file.txt, Delete (Y/N)? N

C:\test> sudo sudo config --enable forceNewWindow
Sudo mode set to Force New Window mode

C:\test> sudo del /P *.*
# a new window appears with:

C:\Windows\System32\07409496-a423-4a3e-b620-2cfb01a9318d_HyperV-ComputeNetwork.dll, Delete (Y/N)?

See:
image

I know you mentioned this was by-design. IMO that design choice should be reconsidered. This is not just unpredictable and inconvenient... In these edge cases it can also be dangerous.

Expected Behavior

Sudo should preserve the current directory, regardless of being invoked from an elevated process, a non-elevated one, with or without -N, and with any configuration mode.

Actual Behavior

Sudo preserves the current directory depending on the -N argument, the config mode, or the elevation status of the caller.

@gerardog gerardog added Issue-Bug Something isn't working Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Mar 14, 2024
@joadoumie
Copy link
Collaborator

Alas, for security reasons, the default mode for sudo for windows has to avoid doing any RPC, which means it's going to basically just act as a shim for ShellExecute. You can avoid it by passing -D . to sudo (added in one of the more recent builds).

We're still iterating/ideating on ways to avoid this, but we don't have anything at the moment that also avoid RPC.

@joadoumie joadoumie removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Mar 20, 2024
@gerardog
Copy link
Author

gerardog commented Apr 8, 2024

Given that constraint, (and others, like SHELLEXECUTEINFO.lpDirectory being replaced to System32 when Verb=RunAs) I would then suggest you to elevate cmd /c cd %folder% & %command% to workaround those limitations.
(Or pushd which supports network paths)

@zadjii-msft
Copy link
Member

You know, we had actually considered that one too. In that case, we then fall into the case where the UAC will show "Command Prompt" as the application being elevated, rather than the actual target application. I believe that was an undesirable outcome, though I'll loop back on that with the security folks.

I too agree, that solution would work, but I'm (very mildly) worried that less well-informed users will be surprised that sudo foo.exe will ask them to run "Command Prompt" rather than "Foo". Then again, if you're less-well-informed, you shouldn't be using sudo.

@gerardog
Copy link
Author

For some reason, ShellExecuteEx with Verb=RunAs respects the starting directory if it is elevating dotnet apps but not for other types... (eg. sudo -N Pwsh.exe works but sudo -N PowerShell.exe doesn't). Any idea why that distinction is?

Is it possible that you guys alter ShellExecuteEx (in a backwards-compatible way) to respect the starting directory in every case?

... and, if the answer is no (or if sudo is running on older windows) then... I think users would find it more familiar to see a UAC popup for sudo.exe than an unexpected Command Prompt... So, maybe sudo -N foo could elevate itself, i.e: to elevate another sudo -D %path% foo. That way, the elevated sudo.exe won`t do any RPC since it is already elevated (will just CD and RUN)... And the UAC popup would show something familiar...

@zadjii-msft
Copy link
Member

(eg. sudo -N Pwsh.exe works but sudo -N PowerShell.exe doesn't). Any idea why that distinction is?

So, there's a horrifying thing about ShellExecute("runas", .... If the OS determines that the thing that's being launched lives in the Windows dir (system32 included), then it manually ignores the CWD, and always starts it in system32. Since powershell is in system32, and pwsh isn't, they act differently here:
image

We looked into this... a couple months back? The code that's responsible for doing that is deep. For my own reference (so I don't have to dig through Teams again), the code responsible starts with a big ol' comment:

    // For elevated EXEs in the Windows directory, make sure
    // the current directory cannot be overriden to an insecure
    // location.  ...

(deep in AiLaunchProcess)

.... and it looks like that code dates back to 2006, in Vista.

BUG: 1844796: LUA: Reserve "Windows binary" marking in the elevation UI for EXEs launched from known Windows locations

Looks like that was multiple bug databases ago, so I'm not sure I can get farther back into the details than that.

@Masamune3210
Copy link

Alas, for security reasons, the default mode for sudo for windows has to avoid doing any RPC, which means it's going to basically just act as a shim for ShellExecute. You can avoid it by passing -D . to sudo (added in one of the more recent builds).

We're still iterating/ideating on ways to avoid this, but we don't have anything at the moment that also avoid RPC.

Ok, but.......Linux has been doing it this way since almost the start. Why can't Windows?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants