Brute force passwords in C (CS50 exercise)

Question

This script attempts to crack passwords by going through all possible 'words', hashing them, and comparing the hash to the input.

It seems to work, but I don't know if I have written 'good' C. I'm looking for advice on what areas would be most important/beneficial to focus on (e.g. efficiency, design, readability, something else?) in the next thing I write and how to improve within those. I also don't feel that comfortable with low level concepts like memory and pointers and I'm not sure if I'm using them right.

I'm not really sure what I want though so please give me whatever advice you think I need.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <cs50.h>
#include <crypt.h>

char ALPHABET[] = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
int alphabetSize = 52;

int max_length = 4;

void check(char *guess, char *salt, char *hash)
{
    string hashedGuess = crypt(guess, salt);
    if (!strcmp(hash, hashedGuess))
    {
        printf("%s\n", guess);
        free(guess);
        exit(0);
    }
}

// recursively fill the buffer and check it each time
void brute_force(char *buf, int index, int length, string salt, string hash)
{
    for (int i = 0; i < alphabetSize; i++)
    {
        sprintf(buf + index, "%c", ALPHABET[i]);

        if (index < length)
        {
            brute_force(buf, index + 1, length, salt, hash);
        }
        else
        {
            check(buf, salt, hash);
        }
    }
}

char *crack(string hash, string salt)
{
    // create a buffer big enough to hold the longest possible password
    char *buf = (char *) malloc(max_length + 1);

    // increment guess length starting from 1
    for (int length = 0; length < max_length; length++)
    {
        brute_force(buf, 0, length, salt, hash);
    }
    return "";
}

bool valid_args(int argc, string argv[])
{
    if (argc == 2)
    {
        return true;
    }
    return false;
}

int main(int argc, string argv[])
{
    if (valid_args(argc, argv))
    {
        string hash = argv[1];

        char salt[3];
        strncpy(salt, hash, 2); // salt is first 2 characters of hash

        string password = crack(hash, salt);
    }
    else
    {
        printf("Invalid arguments\n");
        return 1;
    }
    return 0;
}

vnp · Accepted Answer · 2018-09-15 20:57:02Z

valid_args is a well-known anti-idiom:

    if (condition) {
        return true;
    }
    return false;

is a long way to say

    return condition;

crack always return an empty string "". It is as good as returning nothing.
check terminates the program on success. This seriously limits its utility: the business logic doesn't belong to such depth of the call tree. Consider returning a boolean instead.
I do not endorse recursion when an iterative approach suffices. Try to express a power set generation iteratively (hint: think of base-52 numbers).
From what we can see, it is impossible to tell whether crypt returns a static buffer, or allocates a new one for each call. Consider documenting that.
I happen to know that <cs50.h> defines string as an alias to char *. I don't think it is a good idea to begin with; in any case you should be consistent: brute_force passes salt and hash as string, while check receives them as char *.

salt and hash probably ought to be const char* rather than char* - and that's something difficult to achieve using that string alias. — Toby Speight
– Toby Speight, Commented Sep 18, 2018 at 13:55

SKi · Accepted Answer · 2018-09-17 11:48:16Z

Global variables:

If those global variables are not changed, make those constant.
If those are used only in the C file, you can make those static.
Don't set magic number 52 to alphabetSize, use sizeof when possible.
Don't mix camel-case (alphabetSize) and underscores (max_length).

Instead of this:

char ALPHABET[] = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
int alphabetSize = 52;
int max_length = 4;

You can do it this way:

static const char ALPHABET[] = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
static const int ALPHABET_SIZE = sizeof(ALPHABET) - 1;
static const int MAX_LENGTH = 4;

Function parameters:

If function is not going to change data behind pointer parameter, use const keyword with those parameters.
Instead of string, I propose to use char *.

Instead of this:

char *crack(string hash, string salt)

Use this:

static char *crack(const char *hash, const char *salt)

Efficiency:

I assume that most of the time is spend in the crypt() call. If not, then there is one place for (useless) micro-optimization: Change the following line:

sprintf(buf + index, "%c", ALPHABET[i]);

to:

buf[index] = ALPHABET[i];
buf[index+1] = '\0';

I agree with most of this, except the use of uppercase for things that are not macros. There's a real benefit to reserving ALL_CAPS names for preprocessor macros (which otherwise look like identifiers, but have different scope and parsing rules). — Toby Speight
– Toby Speight, Commented Sep 18, 2018 at 13:54
Maybe. I have used upper case for static constants and enums, for indicating constness. It is also usefull if you change #define constant to static const variable or enum value, or backward. — SKi
– SKi, Commented Sep 18, 2018 at 15:24

Stack Exchange Network

Brute force passwords in C (CS50 exercise)

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Brute force passwords in C (CS50 exercise)

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions