Probable security issue, leaking file descriptors into terminal process

[dunkelstern]

Issue Type: Bug

When running a process from the integrated terminal it seems some file-descriptors are inherited. For example my bash has some open file-descriptors to various asar and font files (and a lot of shared memory!).

To reproduce (on Linux) use the following command directly in the integrated Terminal: ls -l /proc/$$/fd

This lists all open files for the current process (the bash) from the /proc file system. If you use the -l switch it shows to which file the file-descriptor points too.

Example output (in summary because of long output)

$ ls -l /proc/$$/fd
total 0
lrwx------ 1 johannes johannes 64 Sep 17 12:10 0 -> /dev/pts/12
lrwx------ 1 johannes johannes 64 Sep 17 12:10 1 -> /dev/pts/12
lr-x------ 1 johannes johannes 64 Sep 17 12:10 10 -> /usr/share/code/pdf_viewer_resources.pak
lrwx------ 1 johannes johannes 64 Sep 17 12:10 100 -> '/dev/shm/.org.chromium.Chromium.HWn72A (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 101 -> '/dev/shm/.org.chromium.Chromium.xhltP6 (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 102 -> '/dev/shm/.org.chromium.Chromium.mGbMg5 (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 103 -> '/dev/shm/.org.chromium.Chromium.tgA7H3 (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 104 -> '/dev/shm/.org.chromium.Chromium.iQqruz (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 105 -> '/dev/shm/.org.chromium.Chromium.xo8u0x (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 106 -> '/dev/shm/.org.chromium.Chromium.VwDTi2 (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 107 -> '/dev/shm/.org.chromium.Chromium.sptiBw (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 108 -> '/dev/shm/.org.chromium.Chromium.vRJj7S (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 109 -> '/dev/shm/.org.chromium.Chromium.8xnka2 (deleted)'
lr-x------ 1 johannes johannes 64 Sep 17 12:10 11 -> /usr/share/code/blink_image_resources_200_percent.pak
lrwx------ 1 johannes johannes 64 Sep 17 12:10 110 -> '/dev/shm/.org.chromium.Chromium.Md4G4x (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 112 -> '/dev/shm/.org.chromium.Chromium.b7N4Y3 (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 113 -> '/dev/shm/.org.chromium.Chromium.XvX6Cs (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 114 -> '/dev/shm/.org.chromium.Chromium.vdpNXz (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 117 -> '/dev/shm/.org.chromium.Chromium.AbehVB (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 119 -> '/dev/shm/.org.chromium.Chromium.JXv2T7 (deleted)'
lr-x------ 1 johannes johannes 64 Sep 17 12:10 12 -> /usr/share/code/content_resources_200_percent.pak
lrwx------ 1 johannes johannes 64 Sep 17 12:10 121 -> '/dev/shm/.org.chromium.Chromium.o5F609 (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 122 -> '/dev/shm/.org.chromium.Chromium.wRzQSD (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 123 -> '/dev/shm/.org.chromium.Chromium.Mr52wG (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 124 -> '/dev/shm/.org.chromium.Chromium.L6r02c (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 125 -> '/dev/shm/.org.chromium.Chromium.Kf0YyJ (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 126 -> '/dev/shm/.org.chromium.Chromium.t87X4f (deleted)'
lr-x------ 1 johannes johannes 64 Sep 17 12:10 127 -> /usr/share/fonts/truetype/ubuntu/UbuntuMono-B.ttf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 128 -> /usr/share/fonts/truetype/liberation2/LiberationMono-Bold.ttf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 129 -> /usr/share/fonts/truetype/ubuntu/UbuntuMono-B.ttf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 13 -> /usr/share/code/ui_resources_200_percent.pak
lrwx------ 1 johannes johannes 64 Sep 17 12:10 130 -> '/dev/shm/.org.chromium.Chromium.5vO06i (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 131 -> '/dev/shm/.org.chromium.Chromium.EU0YAM (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 132 -> '/dev/shm/.org.chromium.Chromium.Dq7802 (deleted)'
lr-x------ 1 johannes johannes 64 Sep 17 12:10 14 -> /usr/share/code/views_resources_200_percent.pak
lrwx------ 1 johannes johannes 64 Sep 17 12:10 17 -> 'socket:[609354]'
lr-x------ 1 johannes johannes 64 Sep 17 12:10 18 -> /usr/share/code/natives_blob.bin
lr-x------ 1 johannes johannes 64 Sep 17 12:10 19 -> /usr/share/code/snapshot_blob.bin
lrwx------ 1 johannes johannes 64 Sep 17 12:10 2 -> /dev/pts/12
lrwx------ 1 johannes johannes 64 Sep 17 12:10 24 -> 'anon_inode:[eventpoll]'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 255 -> /dev/pts/12
lrwx------ 1 johannes johannes 64 Sep 17 12:10 30 -> 'socket:[604449]'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 36 -> 'socket:[602066]'
lr-x------ 1 johannes johannes 64 Sep 17 12:10 37 -> /usr/share/code/resources/app/node_modules.asar
lr-x------ 1 johannes johannes 64 Sep 17 12:10 38 -> /usr/share/code/resources/electron.asar
lrwx------ 1 johannes johannes 64 Sep 17 12:10 39 -> '/dev/shm/.org.chromium.Chromium.yM9tCr (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 4 -> 'socket:[43671]'
lr-x------ 1 johannes johannes 64 Sep 17 12:10 40 -> /usr/share/fonts/truetype/ubuntu/Ubuntu-R.ttf
lrwx------ 1 johannes johannes 64 Sep 17 12:10 41 -> '/dev/shm/.org.chromium.Chromium.C94wW5 (deleted)'
lr-x------ 1 johannes johannes 64 Sep 17 12:10 42 -> /usr/share/fonts/truetype/freefont/FreeSerif.ttf
lrwx------ 1 johannes johannes 64 Sep 17 12:10 43 -> '/dev/shm/.org.chromium.Chromium.pTAaGy (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 44 -> '/dev/shm/.org.chromium.Chromium.CiV1s5 (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 45 -> '/dev/shm/.org.chromium.Chromium.PEZWfC (deleted)'
lr-x------ 1 johannes johannes 64 Sep 17 12:10 48 -> /usr/share/fonts/truetype/liberation2/LiberationSerif-Regular.ttf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 5 -> /usr/share/code/icudtl.dat
lrwx------ 1 johannes johannes 64 Sep 17 12:10 51 -> '/dev/shm/.org.chromium.Chromium.yOHgod (deleted)'
lr-x------ 1 johannes johannes 64 Sep 17 12:10 53 -> /usr/share/fonts/truetype/liberation2/LiberationMono-Regular.ttf
lrwx------ 1 johannes johannes 64 Sep 17 12:10 54 -> '/dev/shm/.org.chromium.Chromium.xUg3RS (deleted)'
lr-x------ 1 johannes johannes 64 Sep 17 12:10 55 -> /usr/share/fonts/truetype/droid/DroidSansFallbackFull.ttf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 57 -> /usr/share/fonts/opentype/noto/NotoSansCJK-Regular.ttc
lrwx------ 1 johannes johannes 64 Sep 17 12:10 58 -> '/dev/shm/.org.chromium.Chromium.0MbhSI (deleted)'
lr-x------ 1 johannes johannes 64 Sep 17 12:10 6 -> /usr/share/code/snapshot_blob.bin
lr-x------ 1 johannes johannes 64 Sep 17 12:10 60 -> /usr/share/fonts/truetype/ubuntu/UbuntuMono-RI.ttf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 61 -> /usr/share/fonts/truetype/liberation2/LiberationMono-Italic.ttf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 62 -> /usr/share/fonts/truetype/ubuntu/UbuntuMono-RI.ttf
lrwx------ 1 johannes johannes 64 Sep 17 12:10 63 -> '/dev/shm/.org.chromium.Chromium.WGxD5c (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 64 -> '/dev/shm/.org.chromium.Chromium.auH0iH (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 65 -> '/dev/shm/.org.chromium.Chromium.WW6CK2 (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 66 -> '/dev/shm/.org.chromium.Chromium.rFgowb (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 67 -> '/dev/shm/.org.chromium.Chromium.yPgMJF (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 68 -> '/dev/shm/.org.chromium.Chromium.LEEjo8 (deleted)'
lr-x------ 1 johannes johannes 64 Sep 17 12:10 7 -> /usr/share/code/natives_blob.bin
lr-x------ 1 johannes johannes 64 Sep 17 12:10 70 -> /usr/share/fonts/truetype/ubuntu/Ubuntu-RI.ttf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 72 -> /usr/share/fonts/truetype/liberation2/LiberationSans-Italic.ttf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 75 -> /usr/share/fonts/truetype/ubuntu/Ubuntu-RI.ttf
lrwx------ 1 johannes johannes 64 Sep 17 12:10 76 -> /dev/ptmx
lr-x------ 1 johannes johannes 64 Sep 17 12:10 78 -> /usr/share/fonts/truetype/liberation2/LiberationSans-Regular.ttf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 79 -> /usr/share/fonts/truetype/ubuntu/Ubuntu-R.ttf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 8 -> /usr/share/code/locales/en-US.pak
lr-x------ 1 johannes johannes 64 Sep 17 12:10 80 -> /usr/share/fonts/truetype/ubuntu/Ubuntu-B.ttf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 81 -> /usr/share/fonts/truetype/liberation2/LiberationSans-Bold.ttf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 82 -> /usr/share/fonts/truetype/ubuntu/Ubuntu-B.ttf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 83 -> /usr/share/fonts/truetype/oxygen/Oxygen-Sans.ttf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 84 -> /usr/share/fonts/truetype/liberation2/LiberationSerif-Regular.ttf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 85 -> /usr/share/fonts/opentype/font-awesome/FontAwesome.otf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 86 -> /usr/share/fonts/truetype/ubuntu/UbuntuMono-R.ttf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 87 -> /usr/share/fonts/truetype/liberation2/LiberationMono-Regular.ttf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 88 -> /usr/share/fonts/truetype/droid/DroidSansFallbackFull.ttf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 89 -> /usr/share/fonts/truetype/ubuntu/UbuntuMono-R.ttf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 9 -> /usr/share/code/content_shell.pak
lrwx------ 1 johannes johannes 64 Sep 17 12:10 90 -> '/dev/shm/.org.chromium.Chromium.9VuhX9 (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 92 -> '/dev/shm/.org.chromium.Chromium.pLcNaE (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 93 -> '/dev/shm/.org.chromium.Chromium.RU2r1N (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 94 -> '/dev/shm/.org.chromium.Chromium.JOHE9m (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 95 -> '/dev/shm/.org.chromium.Chromium.kLfShW (deleted)'
lrwx------ 1 johannes johannes 64 Sep 17 12:10 96 -> '/dev/shm/.org.chromium.Chromium.cV6mrv (deleted)'
lr-x------ 1 johannes johannes 64 Sep 17 12:10 97 -> /usr/share/fonts/truetype/liberation2/LiberationSans-Regular.ttf
lr-x------ 1 johannes johannes 64 Sep 17 12:10 98 -> /usr/share/fonts/truetype/freefont/FreeMono.ttf
lrwx------ 1 johannes johannes 64 Sep 17 12:10 99 -> '/dev/shm/.org.chromium.Chromium.fFzQBC (deleted)'

---

I can see no direct issue for now but it is possible to access files or shared memory this way and probably crash VScode this way. Additionally it is completely unexpected behaviour (i was stumped why my single-file python process had over 100 open file descriptors).

VS Code version: Code 1.27.1 (5944e81, 2018-09-06T09:21:47.222Z)
OS version: Linux x64 4.15.0-34-generic
Ubuntu 18.04

System Info

Item	Value
CPUs	Intel(R) Core(TM) i7-7700K CPU @ 4.20GHz (8 x 1487)
GPU Status	2d_canvas: enabled checker_imaging: disabled_off flash_3d: enabled flash_stage3d: enabled flash_stage3d_baseline: enabled gpu_compositing: enabled multiple_raster_threads: enabled_on native_gpu_memory_buffers: disabled_software rasterization: disabled_software video_decode: unavailable_software video_encode: unavailable_software webgl: enabled webgl2: enabled
Load (avg)	1, 1, 1
Memory (System)	31.34GB (15.56GB free)
Process Argv	/usr/share/code/code --unity-launch
Screen Reader	no
VM	0%

Extensions (26)

Extension	Author (truncated)	Version
EditorConfig	Edi	0.12.4
postgresql	JPT	0.0.2
better-toml	bun	0.3.2
npm-intellisense	chr	1.3.0
vscode-eslint	dba	1.6.0
vscode-dash	dee	1.10.0
jupyter	don	1.1.4
gitlens	eam	8.5.6
vscode-npm-script	eg2	0.3.5
vscode-npm	fkn	3.3.0
gc-excelviewer	Gra	2.1.26
todo-tree	Gru	0.0.86
node-module-intellisense	lei	1.5.0
restructuredtext	lex	78.0.0
rainbow-csv	mec	0.5.0
ecdc	mit	0.12.0
python	ms-	2018.8.0
cpptools	ms-	0.18.1
vsliveshare	ms-	0.3.666
vscode-versionlens	pfl	0.21.1
vscode-sort-json	ric	1.13.0
vscode-icons	rob	7.26.0
rust	rus	0.4.10
vscode-hexdump	sle	1.6.0
vscode-lldb	vad	0.8.9
debug	web	0.22.0

[Tyriar]

If I'm understanding you correctly, I'm not sure how this sort of thing could happen. A pty is forked here:

https://github.com/Microsoft/node-pty/blob/80fcc8ccf15c2a2f2649fdca9a48411ffdd24ad9/src/unix/pty.cc#L693

And the process is launched here:

https://github.com/Microsoft/node-pty/blob/80fcc8ccf15c2a2f2649fdca9a48411ffdd24ad9/src/unix/pty.cc#L413

You seem more familiar with what's going on so if you have any insights into how this could happen that would be helpful.

[dunkelstern]

That's the problem here, there are multiple ways to do that. The cleanest way would be that all opened files would be opened with the O_CLOEXEC flag set. That's not purely a function of the pty/terminal code but a more fundamental node/electron thing. (And sometimes you actually want to inherit those file descriptors, for example the pty itself or shared memory for the various child processes electron uses)

There are some "workarounds", one is to manually loop through all possible open filedescriptors (0 to getrlimit(RLIMIT_NOFILE, &rlim), and others, see for example https://stackoverflow.com/a/21952155) and set the flag with fcntl(fd, F_SETFD, FD_CLOEXEC); before calling the exec. This will absolutely slow execution of processes down as there's no list of open file descriptors (well, kind of, there is on linux but I am not sure if that holds for other OSs), you'll have to try them all and ignore errors.

I could probably implement the workaround and send a pull request if wanted, but i am not set up for building vscode completely here.

[Tyriar]

/cc @jerch

[jerch]

@Tyriar
Thats one of the issues I stumbled over when I worked the reimplementation of node-pty and the process tracker. I decided to force close left over fds before I ended up using the fork c helper through the childprocess API (libuv handles this correctly already by duping them with O_CLOEXEC).

Imho a serious bug in node-pty, it might even leak the fds to subprocesses (depends on the shell, imho zsh wipes them while zsh (Edit: bash) doesnt or vice versa, cant remember).

Only chance we have imho is looping through it. Id even vote for closing any fd > 2 before exec, since thats all a pty connection needs.

Imho there was even an old issue for it.

[dunkelstern]

Bash seems to leak them at least. Not using z-shell or something other here so I can't comment on this.

[jerch]

Edited the typo above.

@dunkelstern If you like to write a patch, node-pty is the place to go. (You dont have to build the full vscode for it).

[dunkelstern]

I am looking to send a patch on the weekend. So looping through all descriptors is fine in your opinion too?

[jerch]

@dunkelstern Not sure if it will break other things, as it would also close fds that are leaked by purpose (my process tracker relied on that, some debugger might also). I guess only a member of vscode can tell if they utilize this behavior to some degree.
Leaving it open I'd see as an security risk (the fds will survive a setuid call), maybe node-pty needs some kind of API to tell, which fds should stay open after the fork. Then everything else can be closed safely.

[dunkelstern]

Any idea whom/where to ask for something like that? I mean i could try out some things on my own (js, python, some native debuggers and the extensions I use come to mind) but of course i can not test everything on my own.

[jerch]

Testing all one by one is, well tedious, errorprone and might lead to false assumptions.
For vscode you could trace from the pty process caller upwards and see if there are some suspicious pipes/filehandles etc. placed (just strace for files, pipes and sockets - those are possible targets). The extension thing complicates this further, not sure if an extension can place such primitives into the vscode process that calls the pty.
But note, node-pty gets used in many other places too, not only vscode. Therefore I think an API extension in node-pty to announce left open fds is more failsafe and would give enough security.

[Tyriar]

I don't think this is an issue because the pty launched by node-pty by design is always launched at the same permission level as what it's launched from, therefore the pty can do anything of the parent process anyway. As an action item I'll make a note on the node-pty readme about what not to do microsoft/node-pty#230