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

sclang: SequenceableCollection:unixCmd respects PATH #3501

Merged
merged 1 commit into from Feb 9, 2018

Conversation

Projects
None yet
2 participants
@snappizz
Copy link
Member

snappizz commented Feb 6, 2018

fix #2317

This commit changes a call to execve to execvpe in SequenceableCollection:unixCmd, so that PATH is now respected.

This only affects *nix systems. SequenceableCollection:unixCmd is not currently supported on Windows, or so I hear.

test case:

["which", "scsynth"].unixCmd

once this method is working on Windows, i suggest that we refactor all calls to String:unixCmd to use this method. it is much better to call actual command line arguments as an array rather than formatting shell strings, which is vulnerable to injections.

@snappizz snappizz added this to the 3.9.2 milestone Feb 6, 2018

@brianlheim

This comment has been minimized.

Copy link
Member

brianlheim commented Feb 6, 2018

execvpe isn't available on macos, it's a GNU extension.

IIUC you could just change this to execvp and remove all use of environ from the this file. According to https://linux.die.net/man/3/execvp:

The other functions [not e-postfixed] take the environment for the new process image from the external variable environ in the calling process.

Since nothing is actually being added to environ in this call, using it again as an argument has no effect.

@snappizz snappizz force-pushed the snappizz:topic/unixcmd-path branch from 68c3218 to 1e2617a Feb 6, 2018

sclang: SequenceableCollection:unixCmd respects PATH
This commit changes a call to execve to execvp in
SequenceableCollection:unixCmd, so that PATH is now respected.

@snappizz snappizz force-pushed the snappizz:topic/unixcmd-path branch from 1e2617a to ed4be13 Feb 6, 2018

@brianlheim
Copy link
Member

brianlheim left a comment

can confirm this works on macos

@brianlheim

This comment has been minimized.

Copy link
Member

brianlheim commented Feb 8, 2018

it is much better to call actual command line arguments as an array rather than formatting shell strings, which is vulnerable to injections.

not sure why this is any less vulnerable - couldn't one of the args be something like "`rm -rf ~/*; 0`"?

@snappizz

This comment has been minimized.

Copy link
Member

snappizz commented Feb 8, 2018

seeing is believing :)

"ls `echo ~`".unixCmd // ls ~
["ls", "`echo ~`"].unixCmd  // literally tries to ls a directory called "`echo ~`"

the exec* family runs at a lower level than shell

@brianlheim

This comment has been minimized.

Copy link
Member

brianlheim commented Feb 8, 2018

ahhh, ok. thanks for the explanation!

@brianlheim brianlheim merged commit 24a4451 into supercollider:3.9 Feb 9, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment