6

I intend to use the least privileged user to do a given task. The issue is that it involve a kind of responsibility chain and translate as a sudo chain in my scripts, spreading the overall task in many places.

Sudoer file

"www-data"      ALL = (webadmin) NOPASSWD: /home/webadmin/scripts/git-deploy.sh
"webadmin"      ALL = (root) NOPASSWD: /root/scripts/copy-deploy ""

Details

To make the previous paragraph more tangible: Some request is sent to Apache. The worker is running as www-data Through mod_php, the script /home/webadmin/scripts/git-deploy.sh is launched using

<?php
exec('/'.escapeshellarg($gitRepo))

$gitRepo is feed by the following script :

    $bitBucketUrl=json_decode($request->getContent(),true);

    if(isset($bitBucketUrl['url'])){
        $bitBucketUrl=$bitBucketUrl['url'];
        if(!preg_match('/\.git$/',$bitBucketUrl)){
            $bitBucketUrl.='.git';
        }
    }else{
        $result='bad url';
    }

    if(preg_match('/^[email protected]:TEAM\/[a-zA-Z0-9\.-]+\.git$/',$bitBucketUrl)){
        exec(escapeshellcmd('sudo -u webadmin /home/webadmin/scripts/git-deploy.sh '.escapeshellarg($bitBucketUrl)),$results);
        $result=$results;
    }else{
        $result=$bitBucketUrl;
    }

Of course, the script /home/webadmin/scripts/git-deploy.sh check $1

#!/bin/bash
valid=^[email protected]:TEAM/[a-zA-Z0-9-]+\.git$
if [[ $1 =~ $valid  ]]; then
  name=${1#[email protected]:TEAM/}
  name=${name%.git}
  fullName=/home/webadmin/websites/$name
  if [[ -e $fullName ]] ; then
    echo "$fullName exists, will do a git pull instead"
    echo "cd $fullName && git pull"
  else
    echo "/usr/bin/git clone $1"
  fi
  if [[ -e $fullName/deploy/apache-conf/ ]]; then
    #sudo -u root /root/scripts/copy-deploy
  fi
fi

Note: I know that it is only echoing, this is the test version with dummy users.

Each script is only u+rwx by his owner (700).

Question

  1. Is spreading the scripts in various places playing against security? (Harder to grasp what is going on)
  2. Do you see something else wrong?
4
  • Feel free to retag. I was unable to find "sudo" and "bash" in tags. And "chain-of-responsability". Mar 26, 2014 at 16:37
  • exec('/'.escapeshellarg($gitRepo)) ... what does contains $gitREpo ? /home/webadmin/scripts/git-deploy.sh ? Mar 27, 2014 at 17:58
  • @philippelhardy, I updated the question accordingly. Mar 28, 2014 at 8:40
  • The derivation of $result looks confused to me. In particular you set $result='bad url', and then appear to overwrite it in both branches of the next conditional. Does $result ever end up being 'bad url'?
    – Simon
    May 22, 2014 at 22:26

1 Answer 1

3

First of all, what are you trying to do? You are explaining how you are doing it but not what is your ultimate goal. Consider revising your question to add more detail on this.

That said, for the rest of this answer I'll assume that you're doing some sort of proxying (?) to mirror GIT repositories. Which sounds like a problem already solved, e.g. using git hooks.

Nevertheless, addressing your specific questions in order:

Is spreading the scripts in various places playing against security ? (Harder to grasp what is going on)

Yes. At the moment you have the following "moving parts":

  1. A PHP script that parses what seems to be the repository URL sent via a HTTP request
  2. A shell script that calls other binaries (git)
  3. All wrapped in sudo
  4. Spread on many servers, as you said

An error in any of the above could result in a system compromise.

Furthermore, a problem might creep up at a later stage: what happens if you find a bug in one copy of the script and forget to replicate it on the other servers? What if git returns an obscure error you can't parse and you end up with a corruption of user data (which is then backed up corrupted)? How are errors reported? Is there a centralised log system and - more importantly - is there someone monitoring it?

Do you see something else wrong ?

Yes and no. Your PHP script checks if there's a 'url' key in the JSON input, and if there isn't any it sets a 'return' to 'bad value'. Then it continues. This is bad practice: not a bug per se at the moment, but a bug waiting to happen. Rather than continuing, you should stop the program workflow here and return an error.

For example, what happens if in a few months somebody else adds some code like:

/* previous code */
if(preg_match('/^[email protected]:TEAM\/[a-zA-Z0-9\.-]+\.git$/',$bitBucketUrl)){
    exec(escapeshellcmd('sudo -u webadmin /home/webadmin/scripts/git-deploy.sh '.escapeshellarg($bitBucketUrl)),$results);
    $result=$results;
}else{
    $result=$bitBucketUrl;
}

/* new code */
[...]
exec('sudo -u webadmin /home/webadmin/scripts/new-stuff.sh', $bitBucketURL);

The code I added has two bugs: can you spot them?

  • One might think that the script new-stuff.sh is executed only if bitBucketURL contains valid data. Instead, it is executed regardless of what happens beforehand as you don't bail out in the else branches.
  • There's no escapeshellarg

If it took you more than a few seconds to spot the bugs, imagine someone who never saw your code before.

I purposefully omitted the escape from my code example for a reason: you should sanitize your input once and for all and keep using it rather than having to call the escape family of functions every time you need to use the user's input. It's too easy to forget a call to escapeshellargs, especially if it requires more effort (=typing). Instead, try to make the correct use of your code the path of least resistance. For example,

$bitBucketUrl=json_decode($request->getContent(),true);

could become:

$unsafe_user_input=json_decode($request->getContent(),true);
$safe_user_input = escapeshellarg($bitBucketUrl);
[...]
res = do_stuff(safe_user_input);
if (check_error(res))
{
    stop_with_error_message(message);

This way it is obvious what unsafe_user_input and safe_user_input are.

There might be other problems with your approach; the idea of executing code with the least privilege is good, but it's over-engineered here and can have nasty side-effects.

1
  • Accepted as it is comprehensive and bring something new. THK! Jun 10, 2014 at 9:18

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .