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

• Rocketboy235: After taking a look at it. I guess I could have it so it first looks at the username first and checks to see if it even exists in the USERS file just in case as to not waste cpu cycles on generating a md5 hash if the username never existed in the USERS file at all. Also, I guess what I could also do is have it so it checks that a username and password were entered and not just one or the other to not waste time executing the rest of the script. Do you guys agree with these 2 points (at least for robustness/efficiency)?
• Zeta: Nope. That would be a side-channel attack. If the authentication fails faster than others we immediately know that the user doesn't exist and can continue with the next name.
• Rocketboy235: Hmm, good point. Did not think of that...
• Toby Speight: Tagged bash but with #!/bin/sh - do you intend this to be POSIX-compliant shell?
• Rocketboy235: @TobySpeight I guess I might have confused the terms between sh and bash. Didn't fully understood POSIX-compliant until I looked it up. I chose #!/bin/sh to be run on the DD-WRT router as #!/bin/bash would not work.

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.

• Rocketboy235: Thanks for all of the tips/suggestions Toby! I have went ahead and modified my script so it uses what you suggest. Only question I have is about when you said it may not be necessary to check for valid characters if we don't pass the username as a regular expression. Would it be possible for someone to hijack it when username is being called elsewhere in the script or actually maybe not since it's in quotes within the printf command. Thanks again.
• Toby Speight: Yes, it might still be a good idea to check that usernames correspond to your expectations, on the principle of Defence in Depth - it could prevent accidents when the script is modified and the username is used in a less-safe context. It's safe where it's used in the printf and grep commands in this answer - note that we always use %s with printf so that % is treated as a normal character.