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

VBS Shell.Application and PowerShell Start-Process do not escape ampersands internally #2419

Closed
jorangreef opened this issue Aug 13, 2019 · 10 comments
Assignees
Labels
Needs-Tag-Fix Doesn't match tag requirements

Comments

@jorangreef
Copy link

Environment

Windows build number: Microsoft Windows [Version 10.0.15063]

Steps to reproduce

  1. Create a directory named q&a.

  2. Within this directory create a bat file named elevate.bat with the following contents:

echo hello
timeout /t 30
  1. Again, within this directory create a vbs file named elevate.vbs with the following contents:
Set objShell = CreateObject("Shell.Application")
objShell.ShellExecute "elevate.bat", "", "", "runas", 1

Then double-click elevate.vbs.

Expected behavior

Windows prompts for administrator privileges and then the batch script executes and outputs "hello".

Actual behavior

Windows prompts for administrator privileges but the batch script never executes, because Windows does not escape the ampersand in the current working directory correctly.

The interesting thing here is we are relying on the current working directory to pass the full path to the elevate.bat script. This leaves all escaping to Windows.

If the full path to these files has no ampersand, i.e. qa instead of q&a, everything will work.

But, if the full path includes an ampersand, Windows itself won't escape the ampersand when ShellExecute expands the path to elevate.bat.

This can also be reproduced using a PowerShell Start-Process script, making me think it's something common to both.

We found this while working through jorangreef/sudo-prompt#97.

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Aug 13, 2019
@jorangreef
Copy link
Author

@bitcrazed, please would you take a quick look at this. It's easy to reproduce.

@bitcrazed bitcrazed self-assigned this Sep 9, 2019
@bitcrazed bitcrazed removed Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 9, 2019
@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Sep 9, 2019
@DHowett-MSFT
Copy link
Contributor

Please don't do that. conhost is not involved in CreateProcess, Shell.Application or Start-Process. You're just asking for trouble.

@bitcrazed
Copy link
Contributor

@jorangreef Appreciate you filing the issue. It would, at first glance, seem like it's the command-line's job to launch these processes, but it is not!

PowerShell's Start-Process uses ShellExecute or CreateProcess (on Windows), and open/xdgOpen on Mac/Linux via Process.Start.

Trying to find someone involved in CreateProcess et al. to help out. Stay tuned.

@jorangreef
Copy link
Author

Thanks @bitcrazed, appreciate you looking into this and getting people involved.

@jorangreef
Copy link
Author

jorangreef commented Sep 15, 2020

Just an update for everyone following this:

We're now thinking to work around this simply by copying cmd.exe to the same directory as the script we want to execute.

It's an ugly (but beautiful) hack...

It shouldn't be necessary... but it is.

@bitcrazed thanks for your help with this, as a last-ditch effort, if there's anyone you can rope in on this it would be much appreciated.

@DHowett
Copy link
Member

DHowett commented Sep 15, 2020

I am not certain how copying cmd.exe from Windows (which will have the same issues as the ones in-box) will help. Can you explain that a bit more?

@zvin
Copy link

zvin commented Mar 19, 2021

@DHowett @bitcrazed
Imagine you need to run C:\Users\Alice & Bob's Computer\script.bat.
If you copy cmd.exe from C:\Windows\System32\ to C:\Users\Alice & Bob's Computer and cd into C:\Users\Alice & Bob's Computer, you can run ./cmd.exe -ArgumentList "/C" "script.bat".
You no longer need to escape the directory name.

@zadjii-msft
Copy link
Member

I dunno why Rich thought to bring this one to the Terminal repo, I'm pretty sure we have no influence over how VBS escapes its arguments. Also, the original repo that was looking into this looks like it's archived? https://github.com/jorangreef/sudo-prompt. If people are really still interested, I could try digging up the current maintainer of VBS, if there still is one.

I suppose it repros even without VBS, with

start-process -FilePath .\foo.bat -verb runas

(in D:\dev\tmp\q&a),

and that prompted to spawn "c:\WINDOWS\System32\cmd.exe" /C "D:\dev\tmp\q&a\foo.bat"

but yea that seems like somewhere in ShellExecute, that command isn't getting escaped correctly. I suspect part of the reason is due to the elevated cmd.exe always launching in C:\Windows\System32, not the q&a directory where elevate.bat lives.

I'd REALLY recommend not copying cmd.exe around the filesystem. That sounds like a ripe opportunity for some malicious actor who's trying to hijack your script to insert their malicious pwn-you.exe as the cmd.exe where you thought you just copied the one from system32.

@zvin
Copy link

zvin commented Nov 23, 2021

I don't work on this anymore but why closing it if the issue is not solved ?

@jorangreef
Copy link
Author

jorangreef commented Nov 23, 2021

I would agree with @zvin that this shouldn't be closed, at least not until it's handed off correctly to the appropriate team.

Especially, since this is probably a latent security issue in how Windows does escaping of the ampersand. I would guess it's also deeper than both VBS and PowerShell since it's common to both.

Hopefully, someone can find a team for this to belong to :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Tag-Fix Doesn't match tag requirements
Projects
None yet
Development

No branches or pull requests

6 participants