OpenVPN Login Bash Shell script for DD-WRT router

Question

This script is made to run on DD-WRT as I am running an OpenVPN server on it just to securely connect to my home network when I'm on the go. Basically, the input ($1) that goes into this script is a .tmp file generated by OpenVPN when some user wishes to log into the server. The .tmp file consists of 2 lines where the 1st line is the username and the 2nd line is the password.

In this day and age, I'm afraid that potential hackers could inject unintended malicious arguments into the username or password which could exploit my script with the intention to take control over the DD-WRT router (in a way similar to SQL-Injection). So far I have made some huge improvements to the script but I'm not sure if I am finished yet.

#!/bin/sh

#This script was made with OpenVPN via-file in mind

#Location of the Approved Username/Password File
USERS="/somefolder/users"

#Check to see if username and password in the OpenVPN file has any special characters line by line
#Terminate script if special characters are used
while IFS= read -r line
do
    case "$line" in *[!-_a-zA-Z0-9]*) exit 1 ;; esac
done < "$1"

Username=`awk 'NR==1' "$1"`
Password=`awk 'NR==2' "$1"`

        HASHPASS=`echo -n "$Username$Password" | md5sum | sed s'/\  -//'`
        i=0
        while [ $i -lt 10 ]; do
                HASHPASS=`echo -n $HASHPASS$HASHPASS | md5sum | sed s'/\  -//'`
                i=`expr $i + 1`
        done

    if grep -q "$Username:$HASHPASS" $USERS; then
        echo "User Authenticated."
        exit 0
    fi

echo "Login credentials failed."
exit 1

Answer 1

Be paranoid about $PATH

It's a good idea to start this script with

PATH=/usr/bin:/bin

Also consider using full paths for programs.

Get the shell to check some errors

set -e -u

Use lower-case for variables

Upper-case is used for communicating well-known environment variables between programs. Prefer lower-case for our own internal shell variables, to avoid any conflicts.

Simplify the valid-character checking

The while-do loop could be a simple grep:

if /bin/grep -q '[^-_a-zA-Z0-9]' "$1"
then exit 1
fi

With set -e, that's simply

! /bin/grep -q '[^-_a-zA-Z0-9]' "$1"

To be honest, I'm not convinced this checking is needed, provided we don't pass the username as a regular expression - see "Match exactly" below.

Options to echo are not portable

Consider printf '%s' instead (or, in Bash, <<< redirection)

Consider for instead of while for counted loop

With Bash, we could use an arithmetic for loop. For standard shell, consider

for i in $(seq 10)
do
     hashpass=$(printf '%s%s' "$hashpass" "$hashpass" | \
                /usr/bin/md5sum | /usr/bin/cut -d' ' -f1)
done

You could instead write a filter function and just put that in the code 10 times (which might work slightly faster, as parts can work in parallel, and built-in read is better than starting a process to remove the filename part of the output):

function hashround() {
    local hash rest
    read hash rest
    printf '%s%s' "$hash" "$hash" | /usr/bin/md5sum
}

hashpass=$(printf '%s%s' "$Username" "$Password" | /usr/bin/md5sum \
           | hashround | hashround | hashround | hashround | hashround \
           | hashround | hashround | hashround | hashround | hashround \
           | /usr/bin/cut -d' ' -f1)

(We could even eliminate that final cut if we agree to use it as ${hashpass%% *} to remove the second field as a shell substitution.)

Match exactly

Instead of passing a regular expression to the final grep, use grep -F, and also match the entire line (-x):

if /bin/grep -Fxq "$Username:$hashpass" "$users"
then
    echo "User Authenticated." >&2
    exit 0
fi

I've also redirected the output to standard error stream, so it doesn't interfere with actual output.