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

fix(electron-updater): escape quotes instead of throwing an error #5116

Merged
merged 1 commit into from
Jul 27, 2020
Merged

Conversation

kevinlinv
Copy link
Contributor

Descriptions

This dead150 commit was added as a security measure to avoid command injection when we use downloaded file path that contains quotes when running powershell command.

e.g: Get-AuthenticodeSignature 'C:\\path\\my-bad-';calc;'filename.exe' will start a calculator app.

Banning the quotes does stop the command injection, however it also introduce unwanted behavior when performing an update, an example, when the user's path contains a quote, update will never be installed. See: #4889

This PR

Fixes #4889 by escaping the quotation as per windows powershell documentation.

e.g.

// Unescaped
`Get-AuthenticodeSignature 'C:\\Users\\MyName's\\AppData\\Local\\MyApp.exe'`

// Escaped
`Get-AuthenticodeSignature 'C:\\Users\\MyName''s\\AppData\\Local\\MyApp.exe'`

Also when there is a command injection

// Unescaped
`Get-AuthenticodeSignature 'C:\\Users\\MyName';calc;'s\\AppData\\Local\\MyApp.exe'`

// Escaped
`Get-AuthenticodeSignature 'C:\\Users\\MyName'';calc;''s\\AppData\\Local\\MyApp.exe'`

The calculator app will not be opened.

Comment on lines -30 to -32
if (hasQuotes(destinationFile) || (packageFile != null && hasQuotes(packageFile))) {
throw newError(`destinationFile or packageFile contains illegal chars`, "ERR_UPDATER_ILLEGAL_FILE_NAME")
}
Copy link
Contributor Author

@kevinlinv kevinlinv Jul 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am moving the check to the verifier, since I think that is the more appropriate location to do this!

@kevinlinv
Copy link
Contributor Author

@develar could you help take a look at this PR please 🙏

We have a subset of user that can no longer automatically update their app because their username contains a ' character.

@kevinlinv
Copy link
Contributor Author

Ping @develar

@develar develar merged commit e2cc9f9 into electron-userland:master Jul 27, 2020
@kevinlinv
Copy link
Contributor Author

Thanks for merging 👍

@sandeep1995
Copy link
Contributor

Thank you very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quotes in updater path
3 participants