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

make parameter passing to shell scripts secure #7

Open
JensLincke opened this issue Apr 22, 2016 · 14 comments
Open

make parameter passing to shell scripts secure #7

JensLincke opened this issue Apr 22, 2016 · 14 comments

Comments

@JensLincke
Copy link
Contributor

Is there an accepted general way to pass parameters to the scripts securely? I just stripping our ' enough?

  var repository = req.headers["gitrepository"]
      var msg = req.headers["gitcommitmessage"].replace(/'/g,"")
      // # TODO...
      var cmd = 'cd ' + repository + "; git commit -m '"+ msg +"' -a "
@JensLincke
Copy link
Contributor Author

@timfel @fniephaus

@fniephaus
Copy link
Member

In this example, I'd make sure that repository can only be a valid path/directory (allowed chars are [a-z]/.~) and msg can only be a normal text without special characters. In the above code, I could set gitrepository to e.g. ~/; sudo whoami, I'm sure I could find something similar for gitcommitmessage.

@jgraichen
Copy link
Member

jgraichen commented Apr 22, 2016

Good programming languages usually have something like exec(exe, *args). A fork/exec call that is not spawning a shell but directly invoking the executable (git) and passing args as the usual argv array in main(char* argv).

As there is no shell involved the is not "command parsing", "string escaping" etc.

@jgraichen
Copy link
Member

@fniephaus
Copy link
Member

@jgraichen but that still would be exploitable if not used very carefully, right?

@jgraichen
Copy link
Member

No. Something like exec('git', 'commit', '-m', params["message"]) would always pass the param as the message string to git. That is only exploitable if git itself has a sanitizing bug.

@fniephaus
Copy link
Member

Passing the responsibility to the target command is probably a good idea here. And yes, it looks like exec has protection built in.

@JensLincke
Copy link
Contributor Author

but.. since the target programming is a shell script that we wrote... e.g. lively4sync than we have to deal with the security ourselves there again.... some of the commands are not just direct calls to the git api.

@fniephaus
Copy link
Member

Rewrite the shell script in JS? ;)

@JensLincke
Copy link
Contributor Author

if it would have been easy to write in JS I would not have needed a shell script ;-)

@JensLincke
Copy link
Contributor Author

JensLincke commented Apr 22, 2016

but for some git commands, I see that this is the most secure option...

@fniephaus
Copy link
Member

fniephaus commented Apr 22, 2016

let's have a look at this next monday!

@JensLincke
Copy link
Contributor Author

Tim sagt du sollst JETZT Wochenende machen....
Nicole ist doch da!

@fniephaus
Copy link
Member

fniephaus commented Apr 22, 2016

Tim ist d##f 😉 ...selber Wochenende!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants