2
\$\begingroup\$

I'm designing a web interface to a controller for my school's bell. Are there any security flaws in this code?

The source code is available here.

Front End:

bell-ringer.cgi:

#!/bin/bash

function unescape
{
    local s="$1"
    while [ "a$s" != "a" ]; do
        if [ "a${s:0:1}" == "a%" -a "a`echo "${s:1:2}" | sed '{s/\([0-9a-fA-F]\)/\1/gp; s/.*//}'`" == "a${s:1:2}" ]; then
            printf "\x${s:1:2}"
            s="${s:3}"
        else
            printf "%s" "${s:0:1}"
            s="${s:1}"
        fi
    done
}

function escape
{
    local s="$1"
    while [ "a$s" != "a" ]; do
        if [ "a`echo "${s:0:1}" | grep "[a-zA-Z0-9 ]"`" != "a" ]; then
            printf "%s" "${s:0:1}"
        else            
            printf "%s" "${s:0:1}" | hexdump -e '/1 "&#%02X;"'
        fi
        s="${s:1}"
    done
}

function output_content
{
    cat <<EOF
Content-Type: text/html; charset=UTF-8

<!DOCTYPE html>
<html>
<head>
<title>Administration Building Bell Ringer</title>
</head>
<body>
<div style="margin-top: 10%; margin-left: 30%; margin-right: 30%; background-color: #E5E2E0; text-align: center">
<span>Main Page | <a href="change-password.cgi">Set Password</a></span>
<h2>Bell Ringer</h2><br/>
${status_message}<p>The Bell Ringer is currently $(if [ "a$(ps -A | sed '{s/bell-ringer\.cgi//}' | grep "bell-ringer")" != "a" ]; then echo "running"; else echo "not running"; fi)</p><br/>
<form action="bell-ringer.cgi" method="POST">
Action:<br/>
<select name="action">
    <option value="" selected="">--</option>
    <option value="start">Start Bell Ringer</option>
    <option value="stop">Stop Bell Ringer</option>
</select><br/>
Ring Count (for starting): <input type="number" name="ring_count" value="0"/><br/>
Username: <input type="text" name="username" value="$username"/><br/>
Password: <input type="password" name="password"></input><br/>
<input type="submit" value="Ok"/>
</form>
</div>
</body>

EOF
}

if [ "${HTTPS:=off}" == "off" ]; then
    printf "Location: https://%s%s\n\n" "${HTTP_HOST}" "${REQUEST_URI}"
    exit 0
fi
if [ "${REQUEST_METHOD:=GET}" == "GET" ]; then
    output_content
    exit 0
fi
sed '{s/\([-_a-z0-9A-Z]*\)=\([^&=]*\)&*/\1=\2\n/gp; s/.*//}' | grep "..*" | (
    while read input_line; do
        var_name="`echo "$input_line" | sed '{s/\([^=]*\)=.*/\1/p; s/.*//}' | grep "..*"`"
        if [ "a$var_name" != "a" ]; then
            var_value="$(unescape "`echo "$input_line" | sed '{s/[^=]*=\(.*\)/\1/}'`")"
            if [ "a$var_name" == "ausername" ]; then
                username="$var_value"
            elif [ "a$var_name" == "apassword" ]; then
                password="$var_value"
            elif [ "a$var_name" == "aaction" ]; then
                action="$var_value"
            elif [ "a$var_name" == "aring_count" ]; then
                ring_count="$var_value"
            fi
        fi
    done
    if [ "a$ring_count" == "a0" ]; then
        ring_count=""
    fi
    status_message="`printf "%s\n" "$password" | (bell-ringer-administrate bell "$username" "$action" "$ring_count" 2>&1) | (while read s; do printf "%s<br/>" "$s"; done)`"
    if [ "a$status_message" != "a" ]; then
        status_message="<p>$status_message</p><br/>"
    fi
    sleep 0.5 
    output_content
)

change-password.cgi:

#!/bin/bash

function unescape
{
    local s="$1"
    while [ "a$s" != "a" ]; do
        if [ "a${s:0:1}" == "a%" -a "a`echo "${s:1:2}" | sed '{s/\([0-9a-fA-F]\)/\1/gp; s/.*//}'`" == "a${s:1:2}" ]; then
            printf "\x${s:1:2}"
            s="${s:3}"
        else
            printf "%s" "${s:0:1}"
            s="${s:1}"
        fi
    done
}

function escape
{
    local s="$1"
    while [ "a$s" != "a" ]; do
        if [ "a`echo "${s:0:1}" | grep "[a-zA-Z0-9 ]"`" != "a" ]; then
            printf "%s" "${s:0:1}"
        else            
            printf "%s" "${s:0:1}" | hexdump -e '/1 "&#%02X;"'
        fi
        s="${s:1}"
    done
}

function output_content
{
    cat <<EOF
Content-Type: text/html; charset=UTF-8

<!DOCTYPE html>
<html>
<head>
<title>Administration Building Bell Ringer - Change Password</title>
</head>
<body>
<div style="margin-top: 10%; margin-left: 30%; margin-right: 30%; background-color: #E5E2E0; text-align: center">
<span><a href="bell-ringer.cgi">Main Page</a> | Set Password</span>
<h2>Bell Ringer - Change Password</h2><br/>
${status_message}
<form action="change-password.cgi" method="POST">
Username: <input type="text" name="username" value="$username"/><br/>
Old Password: <input type="password" name="password"></input><br/>
New Password: <input type="password" name="password1"></input><br/>
Reenter New Password: <input type="password" name="password2"></input><br/>
<input type="submit" value="Change Password"/>
</form>
</div>
</body>

EOF
}

if [ "${HTTPS:=off}" == "off" ]; then
    printf "Location: https://%s%s\n\n" "${HTTP_HOST}" "${REQUEST_URI}"
    exit 0
fi
if [ "${REQUEST_METHOD:=GET}" == "GET" ]; then
    output_content
    exit 0
fi
sed '{s/\([-_a-z0-9A-Z]*\)=\([^&=]*\)&*/\1=\2\n/gp; s/.*//}' | grep "..*" | (
    while read input_line; do
        var_name="`echo "$input_line" | sed '{s/\([^=]*\)=.*/\1/p; s/.*//}' | grep "..*"`"
        if [ "a$var_name" != "a" ]; then
            var_value="$(unescape "`echo "$input_line" | sed '{s/[^=]*=\(.*\)/\1/}'`")"
            if [ "a$var_name" == "ausername" ]; then
                username="$var_value"
            elif [ "a$var_name" == "apassword" ]; then
                password="$var_value"
            elif [ "a$var_name" == "apassword1" ]; then
                password1="$var_value"
            elif [ "a$var_name" == "apassword2" ]; then
                password2="$var_value"
            fi
        fi
    done
    status_message="`printf "%s\n" "$password" "$password1" "$password2" | (bell-ringer-administrate passwd "$username" 2>&1) | (while read s; do printf "%s<br/>" "$s"; done)`"
    if [ "a$status_message" != "a" ]; then
        status_message="<p>$status_message</p><br/>"
    fi
    sleep 0.5
    output_content
)

Back End:

set-uid root bell-ringer-administrate:

#include <unistd.h>

int main(int argc, char **argv, char *const *envp)
{
    setuid(0);
    setgid(0);
    execve("/usr/local/bin/bell-ringer-administrate.sh", argv, envp);
    return 1;
}

bell-ringer-administrate.sh:

#!/bin/bash
function get_users_in_group
{
    local group_name="$1"
    grep "^$group_name:" < /etc/group | sed '{s/[^:]*:[^:]*:[^:]*:\([^:]*\)/\1/p; s/.*//}' | sed '{s/,/ /g}' | grep "."
}

if [ "a`whoami`" != "aroot" ]; then
    echo "must be run as root" 1>&2
    exit 1
fi
valid_users="`get_users_in_group "adm"`"
found=0
if [ "a$1" != "abell" -a "a$1" != "apasswd" ]; then
    echo "invalid action class" 1>&2
    exit 1
fi
action_class="$1"
for i in $valid_users; do
    if [ "a$i" == "a$2" -a "a$2" != "a" ]; then
        found=1
    fi
done
if [ $found == 0 ]; then
    echo "invalid user name or password" 1>&2
    exit 1
fi
if [ $action_class == bell ]; then
    if [ "a$3" != "astart" -a "a$3" != "astop" ]; then
        echo "invalid action" 1>&2
        exit 1
    fi
    arg=""
    if [ "a$4" != "a" -a "$3" == "start" ]; then
        arg="$(($4))"
    fi
    su "$2" -c "sudo -k; sudo -S -- bash -c \"(${3}-bell-ringer.sh $arg 2>&1); exit 0\"" 2> /dev/null || echo "invalid user name or password" 1>&2
elif [ $action_class == passwd ]; then
    read old_password 
    read new_password
    read new_password2
    invalid_character_set=':'
    if [ "a$new_password" == "a" ]; then
        echo "empty password" 1>&2
        exit 1
    elif [ "a`printf "%s\n" "$new_password" | grep "[$invalid_character_set]"`" != "a" ]; then
        echo "invalid character in password. invalid characters '$invalid_character_set'" 1>&2
        exit 1
    elif [ "a$new_password" != "a$new_password2" ]; then
        echo "new passwords don't match" 1>&2
        exit 1
    else
        printf "%s\n" "$old_password" "$2:$new_password" | su "$2" -c "sudo -k; sudo -S -- bash -c \"(chpasswd 2>&1 && echo password changed successfully); exit 0\"" 2> /dev/null || echo "invalid user name or password" 1>&2
    fi
else
    echo "invalid action class 2" 1>&2
    exit 1
fi

Daemon control:

start-bell-ringer.sh:

#!/bin/bash
if [ "a`whoami`" != "aroot" ]; then
    echo "must be root" 1>&2
    exit 1
fi
killall bell-ringer >& /dev/null
if [ "a$1" == "a" ]; then
    su bell-ringer -c "(bell-ringer -v &>> /home/bell-ringer/bell-ringer.log&)" 2> /dev/null
    echo "started bell ringer"
else
    su bell-ringer -c "(bell-ringer -vr $(($1)) &>> /home/bell-ringer/bell-ringer.log&)" 2> /dev/null
    echo "started bell ringer ringing $(($1)) times"
fi

stop-bell-ringer.sh:

#!/bin/bash
if [ "a`whoami`" != "aroot" ]; then
    echo "must be root" 1>&2
    exit 1
fi
killall bell-ringer >& /dev/null
echo "stopped bell ringer"
\$\endgroup\$
2
  • \$\begingroup\$ Are you looking for a code review only terms of security, or are you be interested in other improvements as well, including best practices, deprecated uses and coding style? \$\endgroup\$ Commented Sep 23, 2014 at 9:21
  • \$\begingroup\$ @janos mostly security, but others will help too \$\endgroup\$ Commented Sep 23, 2014 at 10:53

1 Answer 1

Reset to default
1
\$\begingroup\$

When you have to ask, if a "bell controller" has any security flaws, there is a serious problem with the design. And looking at this code, asking if it has security flaws is far from non-sense, it's a very much valid question.

This code is scary. It violates bad practices in security design in many levels:

  • A bell controller should not have security concerns by design
  • CGI scripts should not run as root
  • setuid programs are very dangerous, setuid root programs are extremely dangerous
  • What is a bell controller doing changing the password of user accounts on the server?
  • Anything that comes even close to working with user accounts on a server should be state of the art technology
    • bash is not designed for state of the art, bash is designed to be glue code
    • the code violates good practices of bash coding, most notably: archaic constructs, duplicated code, messy and overly complicated code

This web interface is begging to get hacked.

So what can you do?

  1. First of all, separate the bell controller functionality from user account management.
  2. Don't mess with real user accounts. If you want to manage access to the bell, implement local user management for the bell controller, that's completely distinct from the user accounts on the server.

Even if this is on a raspi, messing with user accounts this way is just scary.

\$\endgroup\$
2
  • \$\begingroup\$ I was specifically referring to the web interface code. I need to run the bell controller as something other than nobody so it has access to the gpio/parallel ports. I created a bell-ringer user that's the equivalent of nobody except that it can access the port hw. Are there any specific spots that it can be hacked? I used all the strange compare syntax so the strings won't accidentally be interpreted as options. same thing for printf "%s\n" arg \$\endgroup\$ Commented Sep 23, 2014 at 21:56
  • \$\begingroup\$ It would be better to have a dedicated user with access to the bell controller hardware, and run the web interface as that user so you don't need to sudo. Any modern web server can do this. Or you can do it without a proper web server, implementing a minimal HTTP protocol with a single-threaded simple server. Asking for specific hackable spots in this program is like asking for improvement ideas on a house built on a swamp. \$\endgroup\$ Commented Sep 24, 2014 at 6:44

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.