Use mkdtemp for secure temp files #1912

0xch4z · 2018-09-04T16:15:09Z

Closes #1905.

This closely mirrors the official TypeScript extension's implementation, which does not persist the directory path for reuse between activations.

@ramya-rao-a, making the directory path persistent is pretty hacky and not the convention for any of the official extensions I've seen, so I've left it out (let me know your thoughts on this).

msftclas · 2018-09-04T16:15:21Z

All CLA requirements met.

ramya-rao-a

Thanks for the PR @Charliekenney23!

I would still insist on persisting the tmp dir path. Otherwise, we would end up creating new directories unnecessarily each time the user saves a file or runs code coverage.

ramya-rao-a · 2018-09-09T20:05:14Z

src/goBuild.ts

@@ -70,7 +70,7 @@ export function goBuild(fileUri: vscode.Uri, goConfig: vscode.WorkspaceConfigura
 	}

 	const buildEnv = Object.assign({}, getToolsEnvVars());
-	const tmpPath = path.normalize(path.join(os.tmpdir(), 'go-code-check.' + getUserNameHash()));
+	const tmpPath = getTempFile('go-code-check.' + getUserNameHash());


We can skip the use of user name hash here which was introduced to fix #1829
The current solution of using fs.mkdtempSync should fix #1829 as well.

ramya-rao-a · 2018-09-09T20:08:01Z

src/util.ts

+	let dir: string | undefined;
+	return (): string => {
+		if (!dir) {
+			dir = fs.mkdtempSync(os.tmpdir() + '/vscode-go');


Use path.sep to create the path here instead of /

0xch4z · 2018-09-10T15:08:07Z

@ramya-rao-a I've updated the code to persist the temp directory path between vscode sessions. Please let me know if there's anything else that needs to be updated!

ramya-rao-a · 2018-09-10T17:37:02Z

src/debugAdapter/goDebug.ts

@@ -12,6 +12,7 @@ import { basename, dirname, extname } from 'path';
 import { spawn, ChildProcess, execSync, spawnSync, execFile } from 'child_process';
 import { Client, RPCConnection } from 'json-rpc2';
 import { parseEnvFile, getBinPathWithPreferredGopath, resolveHomeDir, getInferredGopath, getCurrentGoWorkspaceFromGOPATH, envPath, fixDriveCasingInWindows } from '../goPath';
+import { TempFileProvider } from '../util';


The debug adapter runs in a separate process and does not have access to anything from the vscode module. Therefore a dependency here on util will fail as util imports from vscode.

The tmp dir is used in the debug adapter to create the trace files. Since this is not done by default and the user sets explicitly to debug an issue with the debugger, I believe we are ok with creating a new file under os.tmpdir()

ramya-rao-a · 2018-09-10T17:42:23Z

src/util.ts

+	private constructor() {}
+	private static _instance: TempFileProvider;
+
+	static get instance() {


Why not make this a static class? Then, we can skip the instance() method here.

Very good point. I'll make this a static class 😃

ramya-rao-a · 2018-09-10T17:43:13Z

src/util.ts

+		return TempFileProvider._instance;
+	}
+
+	private store: vscode.Memento;


let's call this the globalState so that when code reading we know exactly where the data is stored

ramya-rao-a · 2018-09-10T17:44:32Z

src/util.ts

+		this.store = store;
+	}
+
+	get dir(): string {


Since getFilePath is the only place this is used, can we merge this code into getFilePath?

0xch4z · 2018-09-10T18:02:20Z

I've updated the code with the suggested changes 😃

ramya-rao-a · 2018-09-10T18:07:00Z

src/goMain.ts

@@ -83,6 +83,7 @@ export function activate(ctx: vscode.ExtensionContext): void {
 			});
 		}
 		ctx.globalState.update('goroot', currentGoroot);
+		TempFileProvider.registerStore(ctx.globalState);


Move this to the very top. Just to make sure that the globalState is available in the TempFileProvider right from the start

ramya-rao-a · 2018-09-10T18:08:34Z

src/util.ts

+	 * returns path to temp file with name
+	 */
+	static getFilePath(name: string): string {
+		let tempDir = TempFileProvider.globalState.get<string>('tempDir');


One corner case is that getFilePath gets called when the globalState is not yet set. This may not happen now, but when the code that registers the store gets moved around in the future, it can happen.

I've added a comment to above TempFileProvider.registerStore must be called before this.

One work around could be creating a new temp dir if globalState is undefined (and storing it locally on the object). Thoughts?

antong · 2018-09-10T19:02:15Z

Many systems clean the tmp directory every boot or on other schedules or even use a RAM based file system that is not persisted to disk. Will the persisted tmpdir in this PR work correctly if the tmpdir disappears?

0xch4z · 2018-09-10T19:16:19Z

@antong yes. If you have a look here, I'm checking if the persisted directory exists and creating it if not present.

antong · 2018-09-10T21:19:00Z

Usually, mkdtemp and friends are used so that the library safely creates and picks a directory and then you use whatever dir you got. I haven't seen it used so that you recreate one with the same name you got earlier. It may be ok, but I'm not sure.

Now, you may call me overly paranoid, but what about the following attack scenario: Alice uses the vscode extension, which correctly makes a tmpdir /tmp/vscode.hMF460T1CB and it is persisted in vscode's config. Bob on the same machine monitors /tmp and sees Alice's tmpdir. It is inaccessible to him, so he can't do anything about it. However, Bob knows that /tmp is wiped on boot, and he crafts a cron job (a scheduled program execution) that as soon as possible (when the dir disappears) creates a directory with Alice's tmpdir name and sets the permissions to be accessible by Alice (e.g., world readable and writable) and performs the symlink attack I suggested earlier, e.g., ln -s /home/alice/thesis.tex /tmp/vscode.hMF460T1CB/go-code-cover. Alice then opens up vscode, the correctly named dir exists, so the vscode extension reuses it and her thesis.tex is overwritten.

Personally I'd err on the side of caution and not persist the tmpdir name, but recreate it every time and remove it when done with. If the concern is that junk may be left behind, we could remove all /tmp/vscode.* owned by the user at some convenient time, perhaps before creating a new tmpdir. If we want a persistent dir, then tmpdir might not be the right thing and we should put it under the users home dir or maybe the workspace dir.

Sorry for trying to figure out how to break things, I've my security goggles on this week :-)

0xch4z · 2018-09-10T22:48:12Z

So I think the only benefit of persisting the temp directory path is to limit VSCode from polluting the /tmp directory (like with microsoft/vscode#53423). While the scenario that you walked through would be extreme, I agree that creating a directory at launch and cleaning up in deactivate would solve the problem and without any extra risk in the first place. Thanks for weighing in.

Thoughts, @ramya-rao-a?

ramya-rao-a · 2018-09-11T00:35:32Z

Those are some great points @antong, thanks for bringing them up.

I have pushed a change to not re-use a persisted directory if it does not exist. That should take care of most of your concerns

@Charliekenney23 The comment is helpful, but I added a check regardless. Do take a look.

creating a directory at launch and cleaning up in deactivate would solve the problem and without any extra risk in the first place.

Yes, lets do this if @antong finds any other risk with the change I made :)

antong · 2018-09-11T19:47:00Z

What if the directory does exist and is writable, but is owned by somebody else (was autoremoved, but recreated by evil Bob)?

ramya-rao-a · 2018-09-11T21:25:57Z

good point, lets go with creating a tmpdir on extension activation (which happens for each VS Code session), recreating it if it doesnt exist during the session and removing it when extension is deactivated (which happens when VS Code window is closed)

ramya-rao-a · 2018-09-11T21:26:27Z

On a side note: @antong you are awesome

0xch4z · 2018-09-11T23:49:56Z

Agreed! I've just updated the code.

0xch4z · 2018-09-12T00:05:44Z

Just a note: There will be some edge cases where the tmp dir will not be deleted (for instance, when the user force quits vscode, the extension does not have time to call deactivate and delete the temp dir). Though, any stray temp dirs will not be reused.

In effort to minimize these stray directories, I'm only creating the temp directory as needed (when the TempFileProvider.getFilePath method is first called).

ramya-rao-a · 2018-09-13T04:12:47Z

@Charliekenney23 I've refactored the code a bit to avoid using classes as the rest of the util.ts file doesn't use any. This is just to be consistent with the coding style in the file and to make it easier for future maintenance. Can you take a look?

0xch4z · 2018-09-13T04:30:47Z

@ramya-rao-a LGTM!

use mkdtemp for secure temp files

c1a69f2

clean up

bf3174b

ramya-rao-a suggested changes Sep 9, 2018

View reviewed changes

0xch4z force-pushed the secure-tmp-files branch 2 times, most recently from ff41b84 to 1664a15 Compare September 10, 2018 17:28

ramya-rao-a suggested changes Sep 10, 2018

View reviewed changes

0xch4z force-pushed the secure-tmp-files branch from 1664a15 to ec11985 Compare September 10, 2018 18:00

ramya-rao-a suggested changes Sep 10, 2018

View reviewed changes

0xch4z force-pushed the secure-tmp-files branch 2 times, most recently from fc2cdfd to 10757de Compare September 10, 2018 18:21

persist temp dir

5472067

0xch4z force-pushed the secure-tmp-files branch from 10757de to 5472067 Compare September 10, 2018 18:22

Create new tmpdir if persisted one doesnt exist

f9ab388

ramya-rao-a approved these changes Sep 11, 2018

View reviewed changes

remove tmpdir on deactivate

007ac66

Avoid class in util to be consistent with the code style in the file

67853e8

ramya-rao-a merged commit 7885423 into microsoft:master Sep 13, 2018

ramya-rao-a mentioned this pull request Sep 24, 2018

Test Plan for Sept 2018 (0.6.90) #1939

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use mkdtemp for secure temp files #1912

Use mkdtemp for secure temp files #1912

0xch4z commented Sep 4, 2018

msftclas commented Sep 4, 2018 •

edited

Loading

ramya-rao-a left a comment

ramya-rao-a Sep 9, 2018

ramya-rao-a Sep 9, 2018

0xch4z commented Sep 10, 2018

ramya-rao-a Sep 10, 2018

0xch4z Sep 10, 2018

ramya-rao-a Sep 10, 2018

0xch4z Sep 10, 2018

ramya-rao-a Sep 10, 2018

ramya-rao-a Sep 10, 2018

0xch4z commented Sep 10, 2018

ramya-rao-a Sep 10, 2018

ramya-rao-a Sep 10, 2018

0xch4z Sep 10, 2018 •

edited

Loading

antong commented Sep 10, 2018

0xch4z commented Sep 10, 2018

antong commented Sep 10, 2018

0xch4z commented Sep 10, 2018

ramya-rao-a commented Sep 11, 2018

antong commented Sep 11, 2018

ramya-rao-a commented Sep 11, 2018

ramya-rao-a commented Sep 11, 2018

0xch4z commented Sep 11, 2018

0xch4z commented Sep 12, 2018

ramya-rao-a commented Sep 13, 2018

0xch4z commented Sep 13, 2018

Use mkdtemp for secure temp files #1912

Use mkdtemp for secure temp files #1912

Conversation

0xch4z commented Sep 4, 2018

msftclas commented Sep 4, 2018 • edited Loading

ramya-rao-a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xch4z commented Sep 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xch4z commented Sep 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xch4z Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

antong commented Sep 10, 2018

0xch4z commented Sep 10, 2018

antong commented Sep 10, 2018

0xch4z commented Sep 10, 2018

ramya-rao-a commented Sep 11, 2018

antong commented Sep 11, 2018

ramya-rao-a commented Sep 11, 2018

ramya-rao-a commented Sep 11, 2018

0xch4z commented Sep 11, 2018

0xch4z commented Sep 12, 2018

ramya-rao-a commented Sep 13, 2018

0xch4z commented Sep 13, 2018

msftclas commented Sep 4, 2018 •

edited

Loading

0xch4z Sep 10, 2018 •

edited

Loading