Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This code's sole purpose is to create a very random password, using letters and numbers.

I think that it fulfills its purpose, but I wonder, could it be done better? I'm not talking about efficiency, but security.

In your eyes, how secure is this method for generating passwords, and if it is barely secure at all, what better practices are there for this sort of thing?

import random  as r
import sys
import os
from subprocess import call
alpha_lower = "abcdefghijklmnopqrstuvwxyz"
alpha_upper = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
call('color a', shell=True)
class MutateSeed(object):
    def __init__(self, seed, alpha_lower, alpha_upper):
        self.seed = seed
        self.mutated_seed = []
        self.alphabet_lower = list(alpha_lower)
        self.alphabet_upper = list(alpha_upper)
    def mutate_seed(self):
        for x in range(self.seed):
            a = r.randint(0, 9)
            b = r.randint(0, 9)
            c = r.randint(0, 9)
            d = r.randint(0, 25)
            e = r.randint(0, 25)
            f = r.randint(0, 25)

            con = r.randint(1, 6)
            if con == 1:
                mut = a
            elif con == 2:
                mut = b
            elif con == 3:
                mut = c     
            elif con == 4:
                lower_or_higher = r.randint(0, 1)
                if lower_or_higher == 0:
                    mut = self.alphabet_lower[d]
                elif lower_or_higher == 1:
                    mut = self.alphabet_upper[d]
            elif con == 5:
                lower_or_higher = r.randint(0, 1)
                if lower_or_higher == 0:
                    mut = self.alphabet_lower[e]
                elif lower_or_higher == 1:
                    mut = self.alphabet_upper[e]
            elif con == 6:
                lower_or_higher = r.randint(0, 1)
                if lower_or_higher == 0:
                    mut = self.alphabet_lower[f]
                elif lower_or_higher == 1:
                    mut = self.alphabet_upper[f]

            self.mutated_seed.append(mut)

def Display():  
    os.system("cls")    
    user_input = int(raw_input("Length of password: "))
    mutate_seed = MutateSeed(user_input, alpha_lower, alpha_upper)
    mutate_seed.mutate_seed()
    display_pass = "".join(str(x) for x in mutate_seed.mutated_seed)
    f = open('PasswordFile.txt', 'a+')  
    f.write(display_pass)
    f.write("\n")
    print "\n" "Your Password: "+display_pass+ "\n"
    raw_input("Press Enter to continue: ")
    pass
while True:
    Display()
share|improve this question
    
Depends in large part on the quality of the PRNG random implements, i imagine. –  cHao May 10 at 22:34
    
Yes, exactly, I was hoping someone knew what that was... –  FrigidDev May 10 at 22:37
    
The most common weakness with things like this is lack of entropy in the initial seed. If (for example) you start with a 32-bit integer for the initial seed, somebody can simply cycle through all possible inputs. –  Jerry Coffin May 10 at 22:37
    
So maybe I should write something that makes the algorithm change every time the program is run, like have the main program run a "generate_new_process" script once complete? –  FrigidDev May 10 at 22:51
1  
Just take a look at the documentation: "Warning: The pseudo-random generators of this module should not be used for security purposes. Use os.urandom() or SystemRandom if you require a cryptographically secure pseudo-random number generator." –  CodesInChaos May 11 at 12:49

2 Answers 2

up vote 11 down vote accepted

Copy&pasting code multiple times does not make it more secure. If we clean up your mutate_seed method, we'd end up with this:

def mutate_seed(self):
    for x in range(self.seed):
        number_or_letter = r.randint(0, 1)
        if number_or_letter:
            self.mutated_seed.append(r.randint(0, 9))
        else:
            lower_or_higher = r.randint(0, 1)
            if lower_or_higher:
                self.mutated_seed.append(r.choice(self.alphabet_lower))
            else:
                self.mutated_seed.append(r.choice(self.alphabet_upper))

Quick Notes on the changes:

  • I don't request random values which I don't end up using. Throwing away values from the random number generator does not make those numbers that you use more secure.

  • I don't use single-letter variable names or abbreviations like con or mut that obscure the meaning.

  • random.choice is so simple and nice – consider using it.

  • This solution features far less duplicate code. Did you calculate r.ranint(1, 6) as an analogy for six-sided dice?

Now the code is cleaned up so that we can do some more substantial reviewing.

There is no use in separating between numbers, lowercase letters, and uppercase letters. They all should be in the same character pool so that each character has the same probability of occuring at each position in the password. With your current methods, a digit is much more likely to occur than any given letter, simply because there are less digits than letters.

So we should have something like

lower = [chr(x) for x in range(ord('a'), ord('z') + 1)] # I can't be bothered to type
upper = [chr(x) for x in range(ord('A'), ord('Z') + 1)] #   out all letters manually
digits = [str(x) for x in range(0, 9)]
character_pool = lower + upper + digits
...
password = [random.choice(character_pool) for _ in range(0, length)]

The interface of your class is horrible. We have to create an instance, then call the mutate_seed method, then access an instance variable, then must join that array to a string until we can finally display it. Consider packing all of this functionality into a single, convenient function instead:

def make_password(length, *collections_of_characters):
    # join all character collections into a single set
    characters = set()
    for collection in collections_of_characters:
        characters.update(str(c) for c in collection)
    characters = list(characters)

    password = [random.choice(characters) for _ in range(0, length)]
    return "".join(password)

Then using this is as simple as

password = make_password(10,
                         "0123456789",
                         "abcdefghijklmnopqrstuvwxyz", 
                         "ABCDEFGHIJKLMNOPQRSTUVWXYZ")
print(password)

If you need a certain set of characters more often, put those into a list:

alphanumeric_chars = ["0123456789",
                      "abcdefghijklmnopqrstuvwxyz", 
                      "ABCDEFGHIJKLMNOPQRSTUVWXYZ"]
password = make_password(10, *alphanumeric_chars)
print(password)

or create a wrapper function:

def make_alphanumeric_password(length):
    return make_password(length,
                         "0123456789",
                         "abcdefghijklmnopqrstuvwxyz", 
                         "ABCDEFGHIJKLMNOPQRSTUVWXYZ")
print(make_alphanumeric_password(10))

Not all abstractions map cleanly to classes.

Your code uses the word “seed” quite often. A seed is the initial state of a random number generator. When set to the same seed each time, the random number generator will produce the same sequence of numbers. In your class, “seed” seems to mean “length” instead. Use clear, obvious names for your variables rather than using technical lingo if you aren't sure what it means – although a quick trip to Wikipedia would have cleared that up.

You issue a few Windows-only system commands such as cls. Unless this is central to the working of your application, don't manipulate the console, such as setting the color or clearing the screen. Users can do this themselves if they want to. Removing these commands also makes your code usable on other operating systems like OS X or Linux.

You are using the random library as a random number generator. The numbers produced by this library aren't really random, they are just pseudo-random. This is usually not an issue as long as a really random seed is chosen, but because the number sequence is derived algorithmically, it should not be used for really really important stuff. Most passwords are only important and not really really important, so this should be fine ;-) but it's good to remember that real randomness is something different.

That is one aspect where the security of your program is not optimal, but good enough. Writing the password to a file is however absolutely irresponsible. When the password is only displayed on the command line, we can assume that it will only be immediately visible until the screen is cleared or other content has caused it to scroll out of sight. It will be held in memory for a while longer, but at least a rebooting of the system will remove any trace of this. A plain text file however is much more persistent (it stays there after a reboot). It is also much easier for other users to access that password. Don't store secrets at all, or if you have to store them, secure them properly.

You should consider writing your Python code so that it is usable both as a module, and a script. For this, we first write all definitions that should be usable as a module, and at the end add an if __name__ == '__main__' section, which will only be executed if the file was invoked as a script. Now we can both do:

# in some other program:
import make_password as mp
random_hex = mp.make_password(10, "0123456789abcdef")

or

python make_password.py

to use the functionality of your code. Designing for code reuse is a generally good idea.

share|improve this answer
    
Wow, that is really comprehensive and useful, and is exactly what I was looking for, thank you. –  FrigidDev May 10 at 23:12
    
Oh, and random.choice is AWESOME, thank you! –  FrigidDev May 10 at 23:33
    
I didn't even know it existed... :) –  FrigidDev May 10 at 23:33
    
@FrigidDev Re: random.choice – neither did I know about it until I re-read the random docs to find out whether the randint boundaries were inclusive or exclusive. 200_success seems to have a similar find in his answer with the string.ascii_lowercase constant which I didn't know about either. Reviewing code means everyone involved learns :) –  amon May 10 at 23:55
1  
@eBusiness “good enough” depends on what we're trying to achieve, and what we're defending against. A secret does not have to be generated via a cryptography-grade RNG to enhance your security. I use a similar Perl script to generate my passwords, which increases my security as it limits password reuse across web sites. If a hash of one of my passwords is leaked and cracked, other accounts remain secure. In theory RNG weaknesses could be used to accelerate cracking, but that doesn't bother me. I'm not defending against advanced persistent threats, just against run-of-the-mill breaches. –  amon May 11 at 16:40

Expected behaviour / User experience

  • Logging passwords to a file in unhashed, unencrypted form is bad practice. Doing so without telling the user is a sin.

  • Using cls to clear the screen is non-portable.

  • There is no clean way to exit the infinite loop in your program. I think that the following conditions should all trigger a clean exit:

    • ControlC (keyboard interrupt)
    • ControlD or ControlZ (end of file)
    • Entering 0 or a negative number for the password length
    • Entering a non-numeric value for the password length

The code

  • MutateSeed is a weird name for the class; mutate_seed is a weird name for the method. There is no seeding going on. Furthermore, there is no point in the "mutation" (appending more characters to a previously generated password).

  • There's no point in making a class. There is also no point in accepting parameters alpha_lower and alpha_upper. A simple generate_password(length) function would be just as good.

  • The mutated_seed array consists of some one-digit integers and some one-character strings. It would be better to make them all the same data type, i.e. all one-character strings. Better yet, why not make it more convenient for the caller and just return a string?

  • The set of all characters is available as string.ascii_lowercase and string.ascii_uppercase.

  • You are working too hard. For each character, you want to pick a digit with 50% probability, and a letter with 50% probability. Instead of deciding on uppercase/lowercase, then deciding on a letter, just pick from among 52 letters.

  • By making digits appear with 50% probability, you've made digits overrepresented. You would actually get better security if you choose from among all characters with equal probability, i.e. digits with probability \$\dfrac{10}{10 + 26 + 26}\$, uppercase with probability \$\dfrac{26}{10 + 26 + 26}\$, and lowercase with probability \$\dfrac{26}{10 + 26 + 26}\$.

Suggested implementation

Mostly preserving the original behaviour…

from random import choice
import os
import platform
import string

def clear_screen():
    os.system(['clear', 'cls'][platform.system() == 'Windows'])

def generate_password(length):
    CHAR_POOLS = [string.digits, string.ascii_letters]
    return ''.join(choice(choice(CHAR_POOLS)) for _ in range(length))

try:
    while True:
        clear_screen()
        password_length = int(raw_input("Length of password: "))
        if password_length <= 0:
            break
        password = generate_password(password_length)
        print "\nYour password: %s\n" % (password)
        raw_input("Press Enter to continue: ")
except (ValueError, KeyboardInterrupt, EOFError) as exit_on_invalid_input:
    print '',
share|improve this answer
    
A good review, two points though: (1) If CHAR_POOLS were defined as string.digits + string.ascii_letters rather than a list, you wouldn't need to call choice twice, making the code a little easier to read. (2) I disagree that there is no point in accepting the alphabet as a parameter. If CHAR_POOLS could be passed as a parameter, the method would be much more versatile, for instance allowing the caller to request a password containing symbols as well (as in my recent review of another password-generating question). –  codesparkle May 11 at 2:32
    
@codesparkle I call choice() twice to preserve the probability distribution of the original code. Yes, parameterizing the character repertoire would make it more flexible. In that case, it should be an optional rather than a required parameter. –  200_success May 11 at 2:37

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.