Removed snapUpdate.sh and replaced with inline command#65579
Removed snapUpdate.sh and replaced with inline command#65579joaomoreno merged 1 commit intomicrosoft:masterfrom Kedstar99:snapFix
Conversation
+Simplifies the build process. +Avoids creating a new shell that simply spawns another shell environment. +Simplifies the update mechanism by removing the need to track the existence of snapUpdate.sh. +Keeps the logic for snapUpdate.sh close to updateService.snap.ts
joaomoreno
added
install-update
|
Yeah let's take this! Thanks! 🍻 |
|
Hello @joaomoreno and @Tyriar , Would it be possible for you to take a look at my branch Kedstar99@663eca8 I am happy to discuss further if you want on the commit thread. Kind regards, |
|
What does it fix? |
|
https://portal.msrc.microsoft.com/en-US/security-guidance/advisory/CVE-2019-0728 According to this CVE, if an attacker is able to define environment variables, they may be able to cause execution of their code by getting users to just open the project. This was deemed serious enough to get a CVE. I am curious if this same issue can potentially extend to here. In any case, on a Linux non-snap install, if an attacker is able to set the environment variables for SNAP_REVISION, SNAP and SNAP_NAME, they may be able to get execution through doQuitAndInstall. In my new patch I removed the snap related code from the LinuxUpdate file. If the user has any of the snap variables set, they would be directed to use the SnapUpdate service instead. As such it's either dead code or a vulnerability (assuming a user would be able to change the environment variables for another running process). I also removed the use of the environment variable when executing spawn. Instead, I replaced it with a call to explicitly the current running binary rather than whatever is set as SNAP_NAME. Arguably, if an attacker has control of the environment variables, they have in theory already broken the system because they could just change the path variables to point to their own binary location and get execution whenever vscode calls lsof or cd. I would argue doing it through SNAP would be more subtle compared to modifying lsof, cd. In any case, with the code in my patch, I can be more confident that the above issue with the SNAP variables doesn't occur. |
Isn't that the executable of the older snap version? |
|
By calling path.basename(process.argv[0]), I should get the same effect as SNAP_NAME. It just returns the name of the current running binary, either code-insiders, vscode etc... |
|
Shall I make a PR for this I guess? |
Oh right!
Yeah please go ahead. |
3 participants
This patch removes the need for snapUpdate.sh. This has the following benefits:
+Simplifies the build process by avoiding setting and moving the file.
+Avoids creating a new shell that simply spawns another shell environment.
+Simplifies the update mechanism by removing the need to track the existence of snapUpdate.sh.
+Keeps the logic for snapUpdate.sh close to updateService.snap.ts
I have tested the code changes to spawn within a Node environment in vscode console and it correctly spawns another instance of vscode.
If this patch isn't wanted, there are still some changes that may be suggested. For example, currently updateService.linux.ts is calling snapUpdate.sh but the default Linux builds aren't being built with the script. Similarly, the comment for executing snapUpdate.sh isn't correct.
Running the command 'grep -nr --exclude-dir=".git" 'snapUpdate.sh' .' produces no results so I assume I have removed all references.