My first random password generator

Question

I'm making a simple program that generates a random password of some length with or without special characters, just for the sake of learning the C language. Finally I've got this working very well based on the outputs below:

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


char *generate_random_password(int password_lenght, int has_special_characters)
{
    const char *letters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
    const char *digits = "0123456789";
    const char *special_characters = "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~";

    char *random_password = malloc(sizeof(char) * (password_lenght+1));

    srandom(time(NULL));

    if(has_special_characters)
    {
        char to_be_used[95] = "\0";

        strcat(to_be_used, letters);
        strcat(to_be_used, digits);
        strcat(to_be_used, special_characters);

        for(int i = 0; i < password_lenght; i++)
        {
            const int random_index = random() % strlen(to_be_used);
            const char random_character = to_be_used[random_index];

            random_password[i] = random_character;
        }
    }
    else
    {
        char to_be_used[63] = "\0";

        strcat(to_be_used, letters);
        strcat(to_be_used, digits);

        for(int i = 0; i < password_lenght; i++)
        {
            const int random_index = random() % strlen(to_be_used);
            const char random_character = to_be_used[random_index];

            random_password[i] = random_character;
        }
    }

        return random_password;

        free(random_password);
}


int main(void)
{
    printf("%s\n", generate_random_password(17, 1));
    printf("%s\n", generate_random_password(17, 0));

    return 0;
}

The output is:

|ZzN>^5}8:i-P8197

vPrbfzBEGzmSdaPPP

It's working!

But I'm completely in doubt about these strings, pointers, char arrays, etc. I have no idea if this is written "the right way" or how it could be better. I'm concerned if I allocated the right amount for each string/char array, and if it can break or crash in some future.

PS: I'm new at C programming, that's why I don't know much about pointers and memory management yet.

If can anyone give me some feedback about it I will be very grateful!

Answer 1

Typo

lenght is spelled length.

Magic numbers

What does 95 signify? You'll want to put this in a named #define or a const.

Allocation failure

After calling malloc, always check that you've been given a non-null pointer. Allocation failure does happen in real life.

Indentation

You'll want to run this through an autoformatter, because your if block has wonky indentation and needs more columns to the right.

Inaccessible statement

    return random_password;

    free(random_password);

This free will never be called; delete it.

Random

The larger conceptual problem with this program is that it uses a very cryptographically weak pseudorandom number generator. This is a large and fairly complex topic, so you'll need to do some reading, but asking for random data from an entropy-controlled system source will already be better than using C rand.

That aside: you aren't calling rand, you're calling random:

The random() function uses a nonlinear additive feedback random number generator employing a default table of size 31 long integers to return successive pseudo-random numbers in the range from 0 to RAND_MAX. The period of this random number generator is very large, approximately 16 * ((2^31) - 1).

It's probably not appropriate for cryptographic purposes. Have a read through this:

https://stackoverflow.com/questions/822323/how-to-generate-a-random-int-in-c/39475626#39475626

Answer 2

Whilst your code works, there are a number of simplifications that you might try.

1. As Reinderien says, get rid of "magic" numbers

2. Having done that, declare a single string containing all 95 characters with the special ones last. This does away with all the strcat code.

3. It's good practice to declare has_special_characters as type bool. You will have to include <stdbool.h>.

4. You can then test it to set an integer variable, modulus_divider, to the correct const or #define value as in 1).

5. You can then take the modulus of the random number with modulus_divider That way you don't need to keep using strlen(to_be_used) and you only need one generating loop.

6. You don't really need all the intermediate variables in your for loop. Assuming you have set up char_set as the full 94 character array as in 2), your entire for loop could become:

   for(int i = 0; i < password_lenght; i++)
   {
       random_password[i] = char_set[random() % modulus_divider];
   }
   

---

Later

I'm not claiming this is perfect, but here is my version. I don't currently have a C compiler installed but this does compile and run with the online compiler at onlinegdb.com

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

char *generate_random_password(const int password_length, bool has_special_characters)
{
    const int alphamerics = 64; /* length of alphameric character set */
    const int alphamerics_plus = 94; /* length of alphameric character set plus special chatacters */
    const char character_set[] = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"; /* full character set */
    int n =  has_special_characters ? alphamerics_plus : alphamerics; /* set the length of character_set to be used */

    char *random_password = malloc(sizeof(char) * (password_length + 1)); /* allocate memory for password */

    srandom(time(NULL)); /* initialise random number generator, srandom(clock()); works just as well*/
    for (int i = 0; i < password_length; i++)
    {
        random_password[i] = character_set[random() % n]; /* get a character from character_set indexed by the remainder when random() os divided by n */
    }
    random_password[password_length] = '\0'; /* append terminating null to string */

    return random_password;
}

int main(void)
{
    printf("%s\n", generate_random_password(17, true));
    printf("%s\n", generate_random_password(17, false));

    return 0;
}

Typical output is

W$Mg-tT?oTwa~EF$S
xGLMrqJBS6IB96xvp 

Answer 3

You could get away without all the complicated memory allocation if you simply require that the calling code passes you the memory for the password. It could look like this:

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

void generate_password(char *password, size_t password_size) {
    for (size_t i = 0; i < password_size - 1; i++) {
        password[i] = alphabet[rnd_int(alphabet_len)];
    }
    password[password_size - 1] = '\0';
}

The char * means "a pointer to a character". In C, a pointer to a character can also mean "a pointer to a character and some memory beyond". This is commonly used to refer to strings. The char * then points to the first character of the string, and the string continues until it reaches the character '\0', which is binary 0. Not to be confused with the character '0', which is the digit zero.

Of course, the variables alphabet and alphabet_len are undeclared in the above code. Same as the rnd_int function that generates a random number from the range [0, n).

The code would be called like this:

int main(void) {
    char password[80];

    generate_password(password, sizeof password);
    fprintf("password: %s\n", password);
}

Answer 4

It's been touched on (e.g. fixed in Peter's example) but not explicitly stated by anyone - but to me, the most obvious issue is in the duplication of code.

You have the following if statement:

if(has_special_characters)
{
  //codeblock 1
}
else
{
  //codeblock 2
}

where codeblock 1 and codeblock 2 are almost exactly identical. In fact it seems that the only difference is that you have this line in codeblock 1:

strcat(to_be_used, special_characters);

You can completely remove the duplication of code and wrap only that line in an if block.

Although, I'd also suggest using Peter's second point, and not using strcat at all. You can put all the characters into one string from the start and use the if to determine the range which you will cover:

//adjacent strings are concatenated by the compiler
const char* characters = "abcdefghijklmnopqrstuvwxyz"
                         "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
                         "0123456789"
                         "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~";
const unsigned int alphanumerics_len = 62;
const unsigned int all_character_len = 96;

int char_range_max;
if (has_special_characters)
{
  char_range_max = all_character_len;
} 
else
{
  char_range_max = alphanumerics_len;
}

//...intermediate code

const int random_index = random() % char_range_max;

//...more code

We can then improve upon this further by having the compiler handle the string lengths for us with a little pre-processor use to prevent anything needing repeating:

#define AN "abcdefghijklmnopqrstuvwxyz"\
           "ABCDEFGHIJKLMNOPQRSTUVWXYZ"\
           "0123456789"
#define SP "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
const int alphanumerics_len = sizeof (AN);
const int all_character_len = sizeof (AN SP);
const char* characters = AN SP;

I'd personally also prefer to replace the verbose if-block with the more concise:

const int char_range_max = has_special_characters ? all_character_len : alphanumerics_len;

which also has the advantage that it can be defined as const too.