views:

185

answers:

3

I have created a little password generation script. I'm curious to what improvements can be made for it except input error handling, usage information etc. It's the core functionality I'm interested in seeing improvements upon.

This is what it does (and what I like it to do):

  1. Keep it easy to change which Lowercase characters (L), Uppercase characters (U), Numbers (N) and Symbols (S) that are used in passwords.
  2. I'd like it to find a new password of legnth 10 for me in max two seconds.
  3. It should take a variable length of the password string as an argument.
  4. Only a password containing at least one L, U, N and S should be accepted.

Here is the code:

#!/bin/bash

PASSWORDLENGTH=$1
RNDSOURCE=/dev/urandom
L="acdefghjkmnpqrtuvwxy"
U="ABDEFGHJLQRTY"
N="012345679"
S="\-/\\)?=+.%#"

until [ $(echo $password | grep [$L] | grep [$U] | grep [$N] | grep -c [$S] ) == 1 ]; do
    password=$(cat $RNDSOURCE | tr -cd "$L$U$N$S" | head -c $PASSWORDLENGTH)
    echo In progress: $password # It's simply for debug purposes, ignore it
done
echo Final password: $password

My questions are:

  • Is there a nicer way of checking if the password is acceptable than the way I'm doing it?
  • What about the actual password generation?
  • Any coding style improvements? (The short variable names are temporary. Though I'm using uppercase names for "constants" [I know there formally are none] and lowercase for variables. Do you like it?)

Let's vote on the most improved version. :-)

For me it was just an exercise mostly for fun and as a learning experience, albeit I will start using it instead of the generation from KeepassX which I'm using now. It will be interesting to see which improvements and suggestions will come from more experienced Bashistas (I made that word up).


I created a little basic script to measure performance: (In case someone thinks it's fun)

#!/bin/bash

SAMPLES=100
SCALE=3

echo -e "PL\tMax\tMin\tAvg"
for p in $(seq 4 50); do
    bcstr=""; max=-98765; min=98765
    for s in $(seq 1 $SAMPLES); do
     gt=$(\time -f %e ./genpassw.sh $p 2>&1 1>/dev/null)
     bcstr="$gt + $bcstr"
     max=$(echo "if($max < $gt ) $gt else $max" | bc)
     min=$(echo "if($min > $gt ) $gt else $min" | bc)
    done
    bcstr="scale=$SCALE;($bcstr 0)/$SAMPLES"
    avg=$(echo $bcstr | bc)
    echo -e "$p\t$max\t$min\t$avg"
done
A: 

you could just use uuidgen or pwgen to generate your random passwords, maybe later shuffling some letters around or something of the sort

dsm
Maybe, but it doesn't use the exact symbols I used in my own script which is a must for me (as I took half an hour to come up with them).
DeletedAccount
there is also an application called pwgen that generates passwords randomly
dsm
I might look into that too. Primarly I'm looking for comments on if this script could have been written in a more elegant or efficient way. Using a third party application could be a good thing, but I actually think my script is as secure as anything `pwgen` etc comes up with.
DeletedAccount
A: 

We had some discussion about some of these issues before

http://stackoverflow.com/questions/55556/password-generation-best-practice

Aidan
Well I'm looking for code imporements to my current script. Though that discussion is interesting, but my primary goal here is a "better" implementation of this script. I want to improve my coding style and looking at how someone else might solve it is always a good learning experience.
DeletedAccount
+1  A: 

You're throwing away a bunch of randomness in your input stream. Keep those bytes around and translate them into your character set. Replace the password=... statement in your loop with the following:

ALL="$L$U$N$S"
password=$(tr "\000-\377" "$ALL$ALL$ALL$ALL$ALL" < $RNDSOURCE | head -c $PASSWORDLENGTH)

The repetition of $ALL is to ensure that there are >=255 characters in the "map to" set.

I also removed the gratuitous use of cat.

(Edited to clarify that what appears above is not intended to replace the full script, just the inner loop.)

Edit: Here's a much faster strategy that doesn't call out to external programs:

#!/bin/bash

PASSWORDLENGTH=$1
RNDSOURCE=/dev/urandom
L="acdefghjkmnpqrtuvwxy"
U="ABDEFGHJLQRTY"
N="012345679"
# (Use this with tr.)
#S='\-/\\)?=+.%#'
# (Use this for bash.)
S='-/\)?=+.%#'

ALL="$L$U$N$S"

# This function echoes a random index into it's argument.
function rndindex() { echo $(($RANDOM % ${#1})); }

# Make sure the password contains at least one of each class.
password="${L:$(rndindex $L):1}${U:$(rndindex $U):1}${N:$(rndindex $N):1}${S:$(rndindex $S):1}"

# Add random other characters to the password until it is the desired length.
while [[ ${#password} -lt $PASSWORDLENGTH ]]
do
  password=$password${ALL:$(rndindex $ALL):1}
done

# Now shuffle it.
chars=$password
password=""
while [[ ${#password} -lt $PASSWORDLENGTH ]]
do
  n=$(rndindex $chars)
  ch=${chars:$n:1}
  password="$password$ch"
  if [[ $n == $(( ${#chars} - 1 )) ]]; then
      chars="${chars:0:$n}"
  elif [[ $n == 0 ]]; then
      chars="${chars:1}"
  else
      chars="${chars:0:$n}${chars:$((n+1))}"
  fi
done
echo $password

Timing tests show this runs 5-20x faster than the original script, and the time is more predictable from one run to the next.

bstpierre
While it's *highly* likely that at least one character from each group will be represented, it's not guaranteed using your method.
Dennis Williamson
My intent was simply to replace the inner part of the loop. Sorry that wasn't clear. I believe the grep in the until clause will guarantee all classes are represented.
bstpierre
At length 45 and above (not practical in real life) my script seems faster than yours. I guess it's the break even point for all those calls to external programs. I found it a bit interesting. :-) Apart from that your timing tests seem accurate. I like how you're not using any external programs. I'll have another go at this soon and learn from your way of doing it! Thanks for your reply!
DeletedAccount
@bstpierre: Your script should run in linear time proportional to `$PASSWORDLENGTH` or O(n) and is thus predictable and consistent. @Kent: Your script runs in time proportional to the reciprocal of `$PASSWORDLENGTH` (the probability that a search for the inclusion of required characters succeeds improves as the length increases). However, it is possible that even for a long password many passes might need to be run, so the time is not predictable. I'm not sure what the O notation would be for this. The break even point is more related to the probability and less so to the external calls.
Dennis Williamson
@Dennis: Yes my brute force way is slow due to probability for passwords of around length <10. After that however it's still slow, but at this point there aren't many loops as the passwords are acceptable to begin with (see http://pastebin.com/f41083780 for some test data). At this point I would argue it's due to the external calls. And that's what I meant at approximately length 45, where my way is faster in practice (but as I pointed out, such long passwords are of no practical value [for me]). Sorry for not being clear in my comment above. :-)
DeletedAccount
I would guess complexity for a random operation is O(1) for best case and O(infinte) as the worst case. The average case is more interesting, and harder to calculate.
DeletedAccount