Random password generator in C

Question

This is a simple password generator. What do you think about it? I am learning C for a while at school and at home. This just has a mix of symbols, lowercase, uppercase and numbers, with a configurable length.

#include <stdio.h>
#include <stdlib.h>
#include <time.h>

int main()
{
    int i = 0;
    int n = 0;
    int randomizer = 0;
    srand((unsigned int)(time(NULL)));
    char numbers [] = "1234567890";
    char letter [] = "abcdefghijklmnoqprstuvwyzx";
    char letterr [] = "ABCDEFGHIJKLMNOQPRSTUYWVZX";
    char symbols [] = "!@#$%^&*(){}[]:<>?,./";
    printf("\nHow long password:");
    scanf("%d", &n);
    char password[n];
    randomizer = rand() % 4;
    for (i=0;i<n;i++)
    {
        if(randomizer == 1)
        {
            password[i] = numbers[rand() % 10];
            randomizer = rand() % 4;
            printf("%c", password[i]);
        }
        else if (randomizer == 2)
        {
            password[i] = symbols[rand() % 26];
            randomizer = rand() % 4;
            printf("%c", password[i]);
        }
        else if (randomizer == 3)
        {
            password[i] = letterr[rand() % 26];
            randomizer = rand() % 4;
            printf("%c", password[i]);
        }
        else
        {
            password[i] = letter[rand() % 21];
            randomizer = rand() % 4;
            printf("%c", password[i]);
        }
    }
    return main();
}

Answer 1

I see some things that I think could help you improve your code.

Decompose your program into functions

All of the logic here is in main in one rather long and dense chunk of code. It would be better to decompose this into separate functions.

Check return values for errors

The call to scanf can fail. You must check the return values to make sure they haven't or your program may crash (or worse) when given malformed input or due to low system resources. Rigorous error handling is the difference between mostly working versus bug-free software. You should strive for the latter.

Use more whitespace to enhance readability of the code

Instead of crowding things together like this:

for (i=0;i<n;i++)

most people find it more easily readable if you use more space:

for (i=0; i < n; i++)

Eliminate "magic numbers"

Instead of hard-coding the constants 26 and 4 in the code, it would be better to use a #define or const and name them.

Avoid scanf if you can

There are so many well known problems with scanf that you're usually better off avoiding it.

Don't recursively call main

The main() function can be called recursively in C, but it's a bad idea. You could blow up your stack and there's really no good reason to do that here. Just use a loop. See https://stackoverflow.com/questions/4238179/calling-main-in-main-in-c for details.

Use a better random number generator

You are currently using

password[i] = numbers[rand() % 10];

There are a number of problems with this approach. This will generate lower numbers more often than higher ones -- it's not a uniform distribution. Another problem is that the low order bits of the random number generator are not particularly random, so neither is the result. On my machine, there's a slight but measurable bias toward 0 with that. See this answer for details, but I'd recommend changing that to the rand_lim in that link and also duplicated below.

Results

Here's an alternative that uses all of these ideas. It also gets a length from the command line so there's no need for a prompt or scanf. It eliminates the need for counting characters and uses the better random number generator mentioned above:

#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#include <string.h>

int rand_lim(int limit) {
/* return a random number between 0 and limit inclusive.
 */

    int divisor = RAND_MAX/(limit+1);
    int retval;

    do { 
        retval = rand() / divisor;
    } while (retval > limit);

    return retval;
}

char picker(const char *charset) {
    return charset[rand_lim(strlen(charset)-1)];
}

int main(int argc, char *argv[])
{
    if (argc != 2) {
        puts("Usage: pwgen len");
        return 1;
    }
    int len = atoi(argv[1]);
    if (len <= 0) {
       puts("Length must be a positive non-zero integer"); 
       return 2;
    }
    const char* groups[] = {
        "1234567890",  // numbers
        "abcdefghijklmnoqprstuvwyzx",  // lowercase
        "ABCDEFGHIJKLMNOQPRSTUYWVZX",  // uppercase
        "!@#$%^&*(){}[]:<>?,./",    // symbols
    };
    const size_t numGroups = sizeof(groups)/sizeof(groups[0]);
    srand((unsigned int)(time(NULL)));

    // only proceed if we got a number
    for ( ; len; --len) {
        unsigned group = rand_lim(numGroups-1);
        putchar(picker(groups[group]));
    }
    putchar('\n');
}

Answer 2

There is a bug in this line:

password[i] = symbols[rand() % 26];

symbols is only 21 characters long, so this line triggers undefined behavior when rand() % 26 is greater than 21 (and when rand() % 26 is exactly 21, it puts a null byte in the password). You meant for this 26 to go with letter instead, and for the 21 to go with symbols.

This would be less likely to happen if you avoid using "magic" numbers. The error is harder to make (or at least, easier to spot) if you write this instead:

password[i] = symbols[rand() % (sizeof symbols - 1)]

Of course, you can also #define a macro (as Edward's answer also suggests), but I would still define it using sizeof:

#define NUM_SYMBOLS (sizeof symbols - 1)

Using sizeof to calculate the size, rather than using a literal value like 26, ensures that you will not forget to update the count if the size of the array changes.

Answer 3

I don't know if this is just an exercise to learn about working with strings and numbers, or if you ever intend to use this to generate real passwords. But if you do, this line can be a serious problem:

srand((unsigned int)(time(NULL)));

Since you are using the current time, in seconds, to seed the pseudo-random number generator, you have severly limited the number of possible passwords. If I know which day you ran this program to generate a password, there are only 86400 possible passwords I need to test. That's an entropy of 16 bits. If I can guess the hour, you are down to 12 bits. And if I know the exact time, perhaps because your mail with encrypted text has it in the headers, I know your password.

(RAND_MAX is only guaranteed by the standard to be at least 32767, which would correspond to 15 bits of entropy, but on my machine it is 2147483647, which is 31 bits of entropy.)

Answer 4

Take this line from every branch and put it at the end of the loop:

printf("%c", password[i]);

Take this from every branch and put it at the start of the loop (and delete it from before the loop):

randomizer = rand() % 4;

As Schwern observes, you can also turn the if chain into a switch/case, which is a bit faster and easier to read.

This makes the loop look like:

for (i=0;i<n;i++)
{
    randomizer = rand() % 4;
    switch (randomizer) {
    case 1:
        password[i] = numbers[rand() % 10];
        break;
    case 2:
        password[i] = symbols[rand() % 26];
        break;
    case 3:
        password[i] = letterr[rand() % 26];
        break;
    default:
        password[i] = letter[rand() % 21];
        break;
    }
    printf("%c", password[i]);
}

Answer 5

This is a good start!
You do have a couple of bugs though. Firstly, these two lines:

        password[i] = symbols[rand() % 26];

and

        password[i] = letter[rand() % 21];

seem to have their numbers mixed up - swap the 21 and 26 here.

Secondly, this line is bad practice. It may work, but is not the 'right' way to do this. My coding teacher would have had a hissy-fit if they'd seen this!

char password[n];

I was taught that variable declarations should pretty much always be right at the top of a function. The 'right' way to deal with this is to simply declare a char pointer, and then use malloc to allocate the correct amount of memory to the string when you know it's size, and don't forget to free the memory when it's no longer being used.

char *password = NULL;

printf("\nHow long password:");
scanf("%d", &n);

password = (char *)malloc((n + 1) * sizeof(char));  /* String length +1 for NULL terminator */

...

free(password);

Thirdly, sanitize your inputs - always assume your end user is the world's biggest idiot, and when asked to enter How long password:, may just enter twelve as their answer, or even just cabbage. A simple loop can do this, and a minor change to your prompt can help reduce invalid answers that crash your program:

    do
    {
      printf("\nHow long password (8-32):");
      scanf("%d", &n);
    } while ((n < 8) || (n > 32));

Fourthly, I would personally use switch/case rather than if/else if/else, but that is just a personal preference in this case.

Finally, I can only agree with everything in Edward's response.

Here is your code with the changes I'd make:

#include <stdio.h>
#include <stdlib.h>
#include <time.h>

#define ALPHA_SIZE 26
#define NUM_SIZE   10
#define SYM_SIZE   21

void main()
{
    int i, n;
    char *password = NULL;

    srand((unsigned int)(time(NULL)));

    do
    {
      printf("\nHow long password (8-32):");
      scanf("%d", &n);
    } while ((n < 8) || (n > 32));

    password = (char *)malloc((n + 1) * (sizeof(char)));
    printf("%s", makePassword(password, n));

    free(password);
}

char *makePassword(char *pwd, int pwd_length)
{
    int i, randomizer;
    char numbers[] = "1234567890";
    char letter[]  = "abcdefghijklmnoqprstuvwyzx";
    char letterr[] = "ABCDEFGHIJKLMNOQPRSTUYWVZX";
    char symbols[] = "!@#$%^&*(){}[]:<>?,./";

    for (i=0; i<pwd_length; i++)
    {
        randomizer = rand() % 4;

        switch(randomizer)
        {
            case 0: pwd[i] = getPwdChar(numbers, NUM_SIZE);
            break;

            case 1: pwd[i] = getPwdChar(letter, ALPHA_SIZE);
            break;

            case 2: pwd[i] = getPwdChar(letterr, ALPHA_SIZE);
            break;

            case 3: pwd[i] = getPwdChar(symbols, SYM_SIZE);
            break;

            default: break;
        }
    }
    password[pwd_length] = NULL;

    return (pwd);
}

char getPwdChar(char *charlist, int len) 
{
  return (charlist[(rand() / (RAND_MAX / len))]);
}

Answer 6

1. Don't initialize dynamic character arrays with a character string. I.e. don't, for an (implicitly) auto variable, write:

   char numbers [] = "1234567890";
   

   As written, the array is copied from a constant to an automatic variable at run time. Instead write:

   char *numbers = "1234567890";
   

   Or better still:

   char const *const numbers = "1234567890";
   

   Alternatively, move it outside the function to make it global, or declare it static (in which case the array is initialized at compile and load time). Thus, it is also reasonable to write something like:

   static char const numbers[] = "1234567890";
   

2. Don't use rand() and srand(). They really are just to old and decrepit. At the very least, use rand48() and srand48(). Better still, find a cryptographic grade random number generator and use that. This goes along with a previously mentioned "don't use scanf()". (There are a few other functions you shouldn't touch, like gets().)

3. If you try to develop this more fully, you may start getting extra restrictions, like don't use both "0" and "O", or both "1" and "l". These two are about eliminating confusing characters. Another might be to change the symbol set, as many sites use restricted symbol sets. (And I note you already eliminated ", ', `, and ; yourself.) Thus, I suggest dynamically building a single list of acceptable characters, and using a dynamic length for said list.

4. Personally, I've always like generating passwords by generating cryptographic-grade random characters (i.e. 8 bit samples) and then discarding unacceptable (unprintable) characters. I'm sure there are reasons why this isn't good.

Answer 7

Previous commenters have alluded to the fact that the rand() function provided by programming language libraries is at best pseudorandom and incapable of providing secure passwords. It would be reasonable to use the /dev/random device (https://en.wikipedia.org/wiki//dev/random) on unix-like systems, or the Microsoft library CryptGenRandom on Windows systems (https://en.wikipedia.org/wiki/CryptGenRandom -- lighter-weight alternative here: https://blogs.msdn.microsoft.com/michael_howard/2005/01/14/cryptographically-secure-random-number-on-windows-without-using-cryptoapi/) as sources of randomness, as these use observed external events (e.g., from device drivers) to generate a random number pool rather than using a pseudorandom generator whose outputs can be predicted.

Answer 8

Coming back to the already mentioned recursively main logic.

If you run the program logic to often with that you can generate a stack overflow and run out of memory.

You can use a forever loop instead:

int main()
{
    for(;;) {
    // youre code here
    }
    return 0;
}

Another advantage of changing to for(;;) not mentioned yet: This way you can also refactor some stuff out before the for(;;) like the constants.