OpenVPN Login Bash Shell script for DD-WRT router

The name of the pictureThe name of the pictureThe name of the pictureClash Royale CLAN TAG#URR8PPP





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;







up vote
4
down vote

favorite












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






share|improve this question





















  • 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)?
    – Rocketboy235
    Mar 24 at 4:19







  • 2




    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.
    – Zeta
    Mar 24 at 7:03











  • Hmm, good point. Did not think of that...
    – Rocketboy235
    Mar 24 at 14:09










  • Tagged bash but with #!/bin/sh - do you intend this to be POSIX-compliant shell?
    – Toby Speight
    Mar 26 at 8:54










  • @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.
    – Rocketboy235
    Mar 26 at 23:32
















up vote
4
down vote

favorite












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






share|improve this question





















  • 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)?
    – Rocketboy235
    Mar 24 at 4:19







  • 2




    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.
    – Zeta
    Mar 24 at 7:03











  • Hmm, good point. Did not think of that...
    – Rocketboy235
    Mar 24 at 14:09










  • Tagged bash but with #!/bin/sh - do you intend this to be POSIX-compliant shell?
    – Toby Speight
    Mar 26 at 8:54










  • @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.
    – Rocketboy235
    Mar 26 at 23:32












up vote
4
down vote

favorite









up vote
4
down vote

favorite











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






share|improve this 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








share|improve this question












share|improve this question




share|improve this question








edited Mar 27 at 7:11









Toby Speight

17.6k13490




17.6k13490









asked Mar 24 at 4:06









Rocketboy235

233




233











  • 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)?
    – Rocketboy235
    Mar 24 at 4:19







  • 2




    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.
    – Zeta
    Mar 24 at 7:03











  • Hmm, good point. Did not think of that...
    – Rocketboy235
    Mar 24 at 14:09










  • Tagged bash but with #!/bin/sh - do you intend this to be POSIX-compliant shell?
    – Toby Speight
    Mar 26 at 8:54










  • @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.
    – Rocketboy235
    Mar 26 at 23:32
















  • 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)?
    – Rocketboy235
    Mar 24 at 4:19







  • 2




    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.
    – Zeta
    Mar 24 at 7:03











  • Hmm, good point. Did not think of that...
    – Rocketboy235
    Mar 24 at 14:09










  • Tagged bash but with #!/bin/sh - do you intend this to be POSIX-compliant shell?
    – Toby Speight
    Mar 26 at 8:54










  • @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.
    – Rocketboy235
    Mar 26 at 23:32















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)?
– Rocketboy235
Mar 24 at 4:19





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)?
– Rocketboy235
Mar 24 at 4:19





2




2




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.
– Zeta
Mar 24 at 7:03





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.
– Zeta
Mar 24 at 7:03













Hmm, good point. Did not think of that...
– Rocketboy235
Mar 24 at 14:09




Hmm, good point. Did not think of that...
– Rocketboy235
Mar 24 at 14:09












Tagged bash but with #!/bin/sh - do you intend this to be POSIX-compliant shell?
– Toby Speight
Mar 26 at 8:54




Tagged bash but with #!/bin/sh - do you intend this to be POSIX-compliant shell?
– Toby Speight
Mar 26 at 8:54












@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.
– Rocketboy235
Mar 26 at 23:32




@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.
– Rocketboy235
Mar 26 at 23:32










1 Answer
1






active

oldest

votes

















up vote
1
down vote



accepted










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"

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.






share|improve this answer























  • 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.
    – Rocketboy235
    Mar 30 at 4:40






  • 1




    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.
    – Toby Speight
    Apr 2 at 10:40










Your Answer




StackExchange.ifUsing("editor", function ()
return StackExchange.using("mathjaxEditing", function ()
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
);
);
, "mathjax-editing");

StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");

StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "196"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);

else
createEditor();

);

function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
convertImagesToLinks: false,
noModals: false,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);



);








 

draft saved


draft discarded


















StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f190349%2fopenvpn-login-bash-shell-script-for-dd-wrt-router%23new-answer', 'question_page');

);

Post as a guest






























1 Answer
1






active

oldest

votes








1 Answer
1






active

oldest

votes









active

oldest

votes






active

oldest

votes








up vote
1
down vote



accepted










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"

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.






share|improve this answer























  • 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.
    – Rocketboy235
    Mar 30 at 4:40






  • 1




    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.
    – Toby Speight
    Apr 2 at 10:40














up vote
1
down vote



accepted










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"

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.






share|improve this answer























  • 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.
    – Rocketboy235
    Mar 30 at 4:40






  • 1




    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.
    – Toby Speight
    Apr 2 at 10:40












up vote
1
down vote



accepted







up vote
1
down vote



accepted






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"

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.






share|improve this answer















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"

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.







share|improve this answer















share|improve this answer



share|improve this answer








edited Mar 26 at 10:14


























answered Mar 26 at 9:51









Toby Speight

17.6k13490




17.6k13490











  • 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.
    – Rocketboy235
    Mar 30 at 4:40






  • 1




    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.
    – Toby Speight
    Apr 2 at 10:40
















  • 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.
    – Rocketboy235
    Mar 30 at 4:40






  • 1




    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.
    – Toby Speight
    Apr 2 at 10:40















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.
– Rocketboy235
Mar 30 at 4:40




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.
– Rocketboy235
Mar 30 at 4:40




1




1




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.
– Toby Speight
Apr 2 at 10:40




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.
– Toby Speight
Apr 2 at 10:40












 

draft saved


draft discarded


























 


draft saved


draft discarded














StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f190349%2fopenvpn-login-bash-shell-script-for-dd-wrt-router%23new-answer', 'question_page');

);

Post as a guest













































































Popular posts from this blog

Greedy Best First Search implementation in Rust

Function to Return a JSON Like Objects Using VBA Collections and Arrays

C++11 CLH Lock Implementation