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

Probable security issue, leaking file descriptors into terminal process #58814

Closed
dunkelstern opened this issue Sep 17, 2018 · 11 comments
Closed
Assignees
Labels
*as-designed Described behavior is as designed info-needed Issue requires more information from poster linux Issues with VS Code on Linux

Comments

@dunkelstern
Copy link

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
Copy link
Member

Tyriar commented Sep 21, 2018

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.

@Tyriar Tyriar added info-needed Issue requires more information from poster linux Issues with VS Code on Linux labels Sep 21, 2018
@dunkelstern
Copy link
Author

dunkelstern commented Sep 24, 2018

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
Copy link
Member

Tyriar commented Sep 24, 2018

/cc @jerch

@jerch
Copy link

jerch commented Sep 24, 2018

@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
Copy link
Author

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

@jerch
Copy link

jerch commented Sep 25, 2018

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
Copy link
Author

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

@jerch
Copy link

jerch commented Sep 26, 2018

@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
Copy link
Author

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
Copy link

jerch commented Sep 27, 2018

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
Copy link
Member

Tyriar commented Sep 28, 2018

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

@Tyriar Tyriar closed this as completed Sep 28, 2018
@Tyriar Tyriar added the *as-designed Described behavior is as designed label Sep 28, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
*as-designed Described behavior is as designed info-needed Issue requires more information from poster linux Issues with VS Code on Linux
Projects
None yet
Development

No branches or pull requests

3 participants