4
\$\begingroup\$

I decided to go with a very simple CGI user registration program to pair with Apache's mod_auth_form wrapper around a private site. It was only just after I finished writing that I realized I'd written security-sensitive code in a language I'm not my best at.

Is this code secure from parameter injection attacks? I.e., if the attacker fiddles with the POST values, can they run arbitrary commands on the server as the web server user?

#!/bin/bash

echo "Content-type: text/html"
echo ''

cat <<EOT
<!doctype html>
<html>
        <head>
                <title>Registered</title>
                <link rel="icon" href="/favicon.png">
        </head>
        <body>
EOT

POST=$(cat)
if [ "$REQUEST_METHOD" != "POST" ] || [ ! "$POST" ]; then
        echo "<h1>Error</h1><p>Please go back and try again</p></body></html>";
        exit 1;
fi

function input {
        echo "$POST" | sed 's/^.*'"$1"'=\([^&]\+\)&.*$/\1/' | python -c "import sys, urllib.parse as p; print(p.unquote(sys.stdin.read()));"
}

USERNAME=$(input username)
PASSWORD=$(input password)
PASSWORD2=$(input password2)

if [ "$PASSWORD" != "$PASSWORD2" ]; then
        echo "<h1>Error</h1><p>Your passwords do not match.</p></body></html>";
        exit 1;
fi

if [ ${#PASSWORD} -lt 10 ]; then
        echo "<h1>Error</h1><p>Please pick a longer password.</p></body></html>";
        exit 1;
fi

echo "$PASSWORD" | htpasswd -i ../registrations "$USERNAME"

cat <<EOT
                <h1>You have registered</h1>
                <p>Now, please email me so I can enable your account.</p>
        </body>
</html>
EOT

exit 0

To clarify - I'm not concerned about the line of python - it urldecodes/unescapes the field from the POST data and is extremely battle-tested. I put that in to avoid bugs/security concerns over other solutions with fewer dependencies (many with scary-looking bash constructs).

The sed is also pretty straightforward - it pulls out the named parameter from the urlencoded POST data. & is escaped as %26 and so will never show up in the input. I wrote/modified the sed there and am confident enough that it'll behave as I expect (sed is more familiar to me than bash).

Essentially, please feel free to ignore the implementation of the input function and assume it properly extracts POST variables into bash variables. My concerns center entirely around whether that user supplied data (which can be essentially any sequence of bytes) in bash variables will break or escape from any of the places I use it.

\$\endgroup\$
3
  • 2
    \$\begingroup\$ Bash and sed and Python! You'll likely get some good code reviews, but don't expect an exhaustive security review (i.e. we might find issues, but we can't promise not to miss any). \$\endgroup\$
    – Toby Speight
    Commented Oct 16, 2018 at 8:06
  • \$\begingroup\$ Heh, that's fair :), though bash and sed are frequently together (the "shell programming environment"). Honestly, the python was thrown in there to avoid security holes and bugs with url decoding - the python function is heavily battle tested and doesn't worry me. \$\endgroup\$
    – Iiridayn
    Commented Oct 16, 2018 at 22:03
  • 1
    \$\begingroup\$ BTW, the "three languages" thing isn't a (negative) criticism - using the right tools for the task is a good thing! Oh, and it's a great first post! (sorry about the pun) \$\endgroup\$
    – Toby Speight
    Commented Oct 17, 2018 at 7:55

2 Answers 2

Reset to default
2
\$\begingroup\$

Safety

Is this code secure from parameter injection attacks?

I think it's safe:

  • Is any part of the user input executed (for example in eval, exec, or other)? -> no
  • Is any part of the user input passed to another command that may execute it? -> no. Although parts of the input are passed to echo, sed, python, they don't execute any part of that.
  • Is any part of the user input returned back in the response without sanitization -> no

A small tip: when you don't need variable interpolation in a here-document, then it's good to enclose the start symbol within "..." or '...', or to make sure variables don't get expanded accidentally, that is:

cat << "EOT"
...
EOT

This can eliminate some potential human error, leaking variable content unintentionally.

HTTP status code in responses

Based on a couple of local tests on an Apache web server, it seems to me the exit code of a CGI script is not used at all to determine the HTTP status code. A well-behaving web service should use appropriate HTTP status codes. You could achieve that by using Toby's helper function, adding a Status: header with appropriate code: 200 for success, 400 for user error, and so on.

Minor tips

In addition to what Toby already told you, there are a couple of redundant ; at some line ends.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you. I've updated write_html to accept a status code, and removed the redundant ;s. I suspect my root concern was that something like echo $PASSWORD | htpassword... could become echo foo; rm ./*; echo foo | htpassword... and interpreted as such by bash. \$\endgroup\$
    – Iiridayn
    Commented Oct 18, 2018 at 20:16
  • \$\begingroup\$ @Iiridayn If you had $PASSWORD | htpassword ... without echo in front, that would be a risk, falling into the case of my first bullet point, in "other" category. You're safe. So far :-) \$\endgroup\$
    – janos
    Commented Oct 19, 2018 at 5:09
3
\$\begingroup\$

It's a couple of decades (!) since I did any CGI, but I'll cast a quick eye over this. I hope other reviewers will pitch in and fill the gaps (or correct my errors).

It's an interesting choice to use the same TITLE for success and failure. It might be better to write a small function:

header()
{
cat <<EOT
Content-type: text/html; charset=us-ascii

<!doctype html>
<html>
        <head>
                <title>$*</title>
                <link rel="icon" href="/favicon.png">
        </head>
        <body>
EOT
}

Note also the form of function declaration - that's more portable than the function keyword, and to be preferred. We now use it like

if [ "$REQUEST_METHOD" != "POST" ] || [ ! "$POST" ]
then
    header 'Error'
    echo '<h1>Error</h1><p>Wrong HTTP method; please go back and try again</p></body></html>';
    exit 1;
fi

Also, for that exit, I'd exit 0 there, because the script has succeeded in its job (to report the status to the user). There's no error as far as the caller (the Web server) is concerned - everything is working correctly.

A further refinement: if we add another cat to the end of header(), it can write the message, too:

#renamed to reflect new purpose
write_html()
{
cat <<EOT
Content-type: text/html; charset=us-ascii

<!doctype html>
<html>
        <head>
                <title>$*</title>
                <link rel="icon" href="/favicon.png">
        </head>
        <body>
        <h1>$*</h1>
EOT
cat
}

if [ "$REQUEST_METHOD" != "POST" ] || [ ! "$POST" ]
then
    write_html 'Error' <<"EOT"
        <p>Wrong HTTP method; please go back and try again</p>
    </body></html>
EOT
fi

There seems to be a couple of errors in the sed command in input():

s/^.*$1=\([^&]\+\)&.*$/\1/

The most obvious is the requirement for & after the target parameter - this means we'll be unable to extract the last parameter in the line. The more subtle bug is that we can be confused if one parameter is a suffix of another, because we didn't anchor the beginning of $1 at all. Finally, if $1 isn't present in the string, we'll use the entire request as value; we can fix that by printing only if there's a substitution:

# untested
sed -n 's/^.*[?&]'"$1"'=\([^&]\+\).*$/\1/p'

Here, I rely on the greediness of + to take us to the next & or end of line.

Do have another look through the Python library to see if there's something there to help us unpack POST data - since we're using Python anyway for decoding characters, there's no penalty to adding code there.

Since we're using Bash, we could write sed <<<"$POST" instead of echo in that function.

BTW, good job getting the quoting right there!

From here onwards, nothing I haven't already mentioned.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you. I've added write_html for better formatting - I used $1, etc instead of $*. Also updated the function definitions. I've fixed an issue w/sed where it was ignoring blank supplied fields (\+ -> *). \$\endgroup\$
    – Iiridayn
    Commented Oct 18, 2018 at 20:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.