23
\$\begingroup\$

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();
}
\$\endgroup\$
  • 5
    \$\begingroup\$ This is a very small subset of Unicode passwords. \$\endgroup\$ – Neil Dec 9 '19 at 22:07
  • 10
    \$\begingroup\$ Don’t actually use passwords generated with this for anything, by the way – beyond the bias Edward mentioned, rand() is weak, predictable, and reversible on most platforms. \$\endgroup\$ – Ry- Dec 10 '19 at 7:10
  • 8
    \$\begingroup\$ You should treat all characters equally, otherwise you reduce the randomness of the distribution. With your current approach, a given numeric character is more than twice as likely as any non-numeric character P("1") = 1/4 * 1/10 = 1/40, P("A") = 1/4 * 1/26 = 1/104 \$\endgroup\$ – DaveMongoose Dec 10 '19 at 9:42
  • 3
    \$\begingroup\$ Variable name letterr for capital letters seems lazy \$\endgroup\$ – Loko Dec 11 '19 at 15:14
  • 2
    \$\begingroup\$ This assignment doesn't require that something from each group be used, so why even have groups? I.e. use "1234567890abc…xyzABC…XYZ!@#$%^&*(){}[]:<>?,./", and most of the other code will collapse into one small loop. — Or better yet, assuming ASCII, just pick a random integer between 33 and 126 and use that as the character and you don't even need a list of characters. \$\endgroup\$ – Ray Butterworth Dec 13 '19 at 1:34
37
\$\begingroup\$

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');
}
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ I like most of your suggestions. But I think the biggest problem in OP's sample is use of a poor random generator to make a "random" password. I take issue with the Use a better random number generator section recommend to replace rand() with rand() / 10. Point to getrandom() or RtlGenRandom() or even /dev/urandom. The treatment of uniformity in your code sample is a good consideration. On some platforms, you can use the arc4random_uniform() function to generate numbers in uniform range without rolling your own. (Unfortunately, it's not in Linux libc.) \$\endgroup\$ – Conrad Meyer Dec 12 '19 at 18:02
  • \$\begingroup\$ @ConradMeyer: I chose to approach the problem with portability in mind, hence rand_lim uses only standard library functions. As for the larger problem, I'd go further and say that this entire password generation approach is deeply flawed. \$\endgroup\$ – Edward Dec 12 '19 at 18:17
  • 1
    \$\begingroup\$ The C standard doesn't require implementations to provide a real random number generator, but any real implementation that you'd be writing a program like this for does. Portability is generally a good aspiration, but doesn't mean we should be providing dangerous examples of "corrected" password generator code that uses non-random inputs. People really do copy/paste code off of stackoverflow, and those people are the same ones who are going to think rand/random are random. Anyway, that's just my opinion. :-) \$\endgroup\$ – Conrad Meyer Dec 13 '19 at 19:38
  • \$\begingroup\$ Did you intend "don't use scanf because it's unreliable, better use atoi" to be a joke, or was it an accident? \$\endgroup\$ – Roland Illig Jan 28 at 7:52
13
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
10
\$\begingroup\$

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.)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I agree! I think this is the biggest problem. rand() looks like a random number interface, but it very much isn't. It's not even a good fast simulation generator. Be careful not to confuse the range of rand() with entropy, though. Your rand has a 31-bit range but 0 bits of entropy. OP would be well served to learn about getrandom() or getentropy() if supported by their development platform, or simply opening /dev/urandom if not. (Or pick any of the secure CSPRNG APIs on Windows 10+ — they're all backed by the same, good algorithm.) \$\endgroup\$ – Conrad Meyer Dec 12 '19 at 17:47
  • \$\begingroup\$ RAND_MAX is independent of the entropy of the random number generator. Just adding srand_bytes(const char *, size_t) to the API could improve the entropy without touching RAND_MAX. This means your argument is flawed. RAND_MAX only describes the entropy of a single call to rand(), and for that, an entropy of 15 bits is enough. \$\endgroup\$ – Roland Illig Jan 28 at 7:57
  • \$\begingroup\$ @RolandIllig: Yes, I agree that RAND_MAX isn't really relevant to the entropy of the generated passwords. \$\endgroup\$ – Thomas Padron-McCarthy Jan 28 at 8:01
7
\$\begingroup\$

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]);
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Why stop? struct { char *const str; size_t n; } table[] = { { numbers, sizeof numbers - 1 }, { symbols, sizeof symbols - 1 }, ... }, *t; for(i = 0; i < n; i++) t = table + rand() % (sizeof table / sizeof *table), password[i] = t->str(rand() % t->n); \$\endgroup\$ – Neil Dec 9 '19 at 22:07
6
\$\begingroup\$

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))]);
}

| improve this answer | |
\$\endgroup\$
  • 43
    \$\begingroup\$ I was taught that variable declarations should pretty much always be right at the top of a function. I disagree; variables should not be declared until they are initialized, and in the smallest possible scope, to avoid accidentally "leaking" values from commonly used variable names like i and n, and to increase the chance you will get a compiler error or warning if you make a mistake. "Always put your variables at the top of a function" is a holdover from C89 when all variables had to be declared at the top of a function. Today, it's bad advice. \$\endgroup\$ – trentcl Dec 10 '19 at 12:58
  • 14
    \$\begingroup\$ Also, while I would also use malloc here, it's not more correct than using a VLA (variable-length array). Your teacher may have had a fit but that's just their opinion, not gospel truth. What is gospel truth is that void main() is non-standard. \$\endgroup\$ – trentcl Dec 10 '19 at 13:02
  • 11
    \$\begingroup\$ @StefanCole "I was taught that variable declarations should pretty much always be right at the top of a function." The days of C89 are long past us. \$\endgroup\$ – Alexander - Reinstate Monica Dec 10 '19 at 22:07
  • 7
    \$\begingroup\$ rand() % 21 I think this causes a modulo bias, because RAND_MAX isn't perfectly divisible by 21. \$\endgroup\$ – Alexander - Reinstate Monica Dec 10 '19 at 22:08
  • 5
    \$\begingroup\$ C89 requires variables to be defined at beginning of scope blocks, which is wider than just the start of functions. \$\endgroup\$ – borrible Dec 11 '19 at 11:17
3
\$\begingroup\$
  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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Why not static const char numbers[] = "1234567890"; inside the function? \$\endgroup\$ – Martin Bonner supports Monica Dec 11 '19 at 17:43
  • \$\begingroup\$ @Martin: That is fine either inside or outside the function. The point was to have it compile/load time initialized instead of run time initialized. I did say "make it global, or declare it static". One can of course do both, though the sematics of static at the global scope are different. \$\endgroup\$ – David G. Dec 11 '19 at 19:38
  • \$\begingroup\$ 4) is perfectly fine as long as you simply remove the unprintable characters and then repeat until you've got a long enough string. This is one of the standard ways how to get unbiased modulo N random numbers. The JDK does it this way (loop until you get a number smaller than the largest number that cannot be mapped unbiased into the different buckets). \$\endgroup\$ – Voo Dec 12 '19 at 17:21
  • \$\begingroup\$ I'd suggest avoiding any of the non-cryptographic unseeded libc PRNGs, including rand48. Just skip directly to getrandom(), or if you're on an older platform, /dev/urandom, or on Windows (especially 10+), there are good CSPRNG APIs available there that I don't memorize the names for :-). \$\endgroup\$ – Conrad Meyer Dec 12 '19 at 17:51
2
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Hi Eric! I totally agree that use of rand is concerning here and agree with your suggested replacements. One language quibble, though: rand isn't "at best pseuodrandom." It's explicitly required to be a deterministic sequence generator by the C standard. \$\endgroup\$ – Conrad Meyer Dec 12 '19 at 17:55
  • \$\begingroup\$ Good point. My comment "at best pseudorandom" referred to the real concern that the rand() implementation could itself be defective. Don't laugh! -- some nice examples are given here. \$\endgroup\$ – Eric Koski Dec 12 '19 at 20:57
  • \$\begingroup\$ Ah, sure. "Nine, nine, nine, nine..." \$\endgroup\$ – Conrad Meyer Dec 13 '19 at 19:30
1
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ It's a good point, but I did actually mention that in my answer. \$\endgroup\$ – Edward Dec 11 '19 at 17:11
  • \$\begingroup\$ you are right I over read it. \$\endgroup\$ – Sandro4912 Dec 11 '19 at 17:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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