28
\$\begingroup\$

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!

\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to CodeReview, maverick. Your code is currently not platform independent due to the use of srandom and random. If this is intended, please edit your post to include your target and host programming platform, as reviewers can add platform dependent notes. You might also add relevant tags. If you intended to create platform independent code, feel free to also edit your post to indicate this. \$\endgroup\$ – Zeta Sep 30 at 20:26
  • \$\begingroup\$ @Mast: this version does have some of the bug fixes pointed out in SO comments. (e.g. to_be_used is now initialized before the first strcat reads it. I would have used strcpy instead of zeroing that local on the stack and then using strcat. Or really I would have done what @Baldrickk suggests and not done any copying). But anyway, this is a repost with some of the bugs fixed. I guess still a crosspost but I was wondering why nobody was mentioning the things from the SO comments) \$\endgroup\$ – Peter Cordes Oct 2 at 14:49
  • 1
    \$\begingroup\$ I know you're doing it only for excercise, but see Why shouldn't we roll our own? on security stack. \$\endgroup\$ – Mołot Oct 3 at 8:54
  • \$\begingroup\$ @MooingDuck: I see a double-backslash \\ inside the double quoted string, so there's a backslash in the character literal. Or were you typing a space as special char? \$\endgroup\$ – Peter Cordes Oct 3 at 11:25
  • \$\begingroup\$ @PeterCordes: I swear I checked it three times before posting. But it doesn't seem to have been edited, so I must have lost my mind \$\endgroup\$ – Mooing Duck Oct 3 at 20:28
26
\$\begingroup\$

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

\$\endgroup\$
  • 7
    \$\begingroup\$ Thank you for the accept, but I suggest that you instead unaccept, upvote, and wait a while for other suggestions to come in for other users before you decide on an accepted answer. \$\endgroup\$ – Reinderien Sep 30 at 20:38
  • 2
    \$\begingroup\$ return terminates the function, so nothing after it will be run. But you're correct to identify that, in general, allocated memory should be freed. In this case, it would be the responsibility of main to do that. \$\endgroup\$ – Reinderien Sep 30 at 20:44
  • 4
    \$\begingroup\$ @PeterJennings Key and password generation are absolutely activities requiring cryptographic strength. Given that password generation requires a tiny amount of data and only needs to occur once, the marginal added cost and complexity are more than worth it - even if it brings password attacks from "infeasible" to "extremely infeasible". \$\endgroup\$ – Reinderien Oct 1 at 1:39
  • 5
    \$\begingroup\$ @PeterJennings The srandom function is initialized with the return from time. If we know the year the password is generated, there are fewer than 32 million possible passwords. You are also wrong about the "knowing the exact random algorithm won't help you crack the password" - there are many, many passwords that using random cannot generate even if the initial seed is adequate. \$\endgroup\$ – Martin Bonner supports Monica Oct 1 at 9:44
  • 2
    \$\begingroup\$ Using srandom(time(NULL)) is clearly the limiting bug; if used server-side, it opens the barn door for attackers to ask for a password reset and try just a handful of options before succeeding. Client or server-side, it means instantly pwning a new sign-up. A better source of entropy, even if you don't resort to OS/hardware sources, is clearly preferred, but using rand alone isn't a problem given the huge range of rand mapped to a tiny range of symbols, unless the lib implementation of rand is horribly broken. The point of a password generator is just reasonable entropy, not maximum. \$\endgroup\$ – SilverbackNet Oct 2 at 4:07
11
\$\begingroup\$

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 
\$\endgroup\$
  • 2
    \$\begingroup\$ Instead of commenting the description of a variable, why not use descriptive names? The compiler does not care if the varaible has a long name like alphanumerics_plus_special_characters or length_of_characters_to_be_used. \$\endgroup\$ – dustytrash Oct 1 at 15:39
  • 4
    \$\begingroup\$ @dustytrash It's a matter of personal preference to some extent. Long variable names are a double edged weapon. Yes they are descriptive, but they can become unwieldy and make lines of code excessively long and awkward to read. My take is to use fairly short but descriptive names and expand on their use in the comments to allay any doubt. As for comments, I'm old school. I've been writing C on and off for over 35 years (Kernighan and Ritchie was my text book) and have always adhered to the idea of commenting most important lines to aid the next guy who needs to edit the code. \$\endgroup\$ – Peter Jennings Oct 1 at 22:18
  • 2
    \$\begingroup\$ @dustytrash I admit that 'n' instead of 'length_of_characters_to_be_used' was a slip of the pen! It should, at least , have been a bit more descriptive. Another reason I've been liberal with the comments is to help the OP, an learner, understand the code. \$\endgroup\$ – Peter Jennings Oct 1 at 22:25
  • \$\begingroup\$ The suggested code leaks memory \$\endgroup\$ – Mooing Duck Oct 2 at 23:14
  • \$\begingroup\$ @MooingDuck I never said it was perfect. I like some variation on Ronald's idea of declaring the memory in the calling function (main) and passing a pointer. That should fix it. If this were some full blown password generator you would take user input for the password length, malloc the memory, call this function and free up the memory in the calling function once you have used the password, \$\endgroup\$ – Peter Jennings Oct 3 at 0:13
7
\$\begingroup\$

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);
}
\$\endgroup\$
  • 2
    \$\begingroup\$ That's an excellent point; creating a contract where one doesn't need to exist is a bad idea. Expect callers always forget and leak memory, or worse use the wrong kind of free, even if you tell them to call deallocate_password(). That said, for security reasons requiring a call to wipe the password from memory might be necessary. \$\endgroup\$ – SilverbackNet Oct 2 at 3:16
  • 1
    \$\begingroup\$ Do you mean "without" in the first line? \$\endgroup\$ – Baldrickk Oct 2 at 15:02
  • \$\begingroup\$ @Baldrickk Thanks \$\endgroup\$ – Roland Illig Oct 2 at 15:36
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Too bad C doesn't make it easy to put labels part way through a string so you could have the compiler calculate alphanumerics_len for you. That would be easy in assembly language: just put a label after those bytes, and another label at the end of the string, so you can do end1-start or end2-start instead of hardcoding either length. (You could have used an array so you could at least use sizeof for all_character_len though.) \$\endgroup\$ – Peter Cordes Oct 2 at 14:54
  • \$\begingroup\$ If you put the special chars first, you could use strchr(characters, 'a') to find a new start position (and a smart compiler could optimize that to pointer += constant). But choosing just a different length with the same start is most efficient. I guess you could strrchr at runtime if you wanted to avoid hardcoding lengths that match the string contents. Or memrchr if you use an array so total length is a compile-time constant. \$\endgroup\$ – Peter Cordes Oct 2 at 14:57
  • \$\begingroup\$ @PeterCordes you can use sizeof (see stackoverflow.com/a/5022113/4022608) but only to find the maximum length, so you would need to have a hard-coded value regardless... There is probably something a little more inventive that can do it, but would possibly impact readability, or memory use. I can think of a readable alternative using the preprocessor, but it would rely on the compiler optimising out the creation of unused arrays, which is a little dirty. \$\endgroup\$ – Baldrickk Oct 2 at 15:01
  • \$\begingroup\$ That's what I said: you can use sizeof for all_character_len but not alphanumerics_len. Good idea though; with macros you could concat everything for the actual definition, or just the alphanumeric part for sizeof("abcdefg...") as a compile-time-constant integer. Yes that works: godbolt.org/z/ikLz_3 I thought I was going to have to cast it to an anonymous array or something. (Just using a string literal as an operand to sizeof doesn't make it part of your program so it doesn't even need to be "optimized away") Oh, another answer on the Q you linked points that out. \$\endgroup\$ – Peter Cordes Oct 2 at 15:08
  • 1
    \$\begingroup\$ Yes, I'd put that in. Hard-coding a length requires updating 2 things if you ever add another special character, so it's highly frowned-upon and usually only justifiable for performance reasons. An answer that recommends hard-coding lengths / magic numbers isn't very good for coding-style. Since macros allow us to get just as efficient machine code without hard-coding lengths, we should use that. (And as a bonus, it's no worse to read than a runtime strcat + strlen). Your method (selecting a different length) is clearly more elegant and doesn't offend the reader's sense of efficiency. \$\endgroup\$ – Peter Cordes Oct 2 at 15:43

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.