3
\$\begingroup\$

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;
}
\$\endgroup\$
1
\$\begingroup\$
  • 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 *.

\$\endgroup\$
  • \$\begingroup\$ salt and hash probably ought to be const char* rather than char* - and that's something difficult to achieve using that string alias. \$\endgroup\$ – Toby Speight Sep 18 '18 at 13:55
1
\$\begingroup\$

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';
\$\endgroup\$
  • \$\begingroup\$ 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). \$\endgroup\$ – Toby Speight Sep 18 '18 at 13:54
  • \$\begingroup\$ 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. \$\endgroup\$ – SKi Sep 18 '18 at 15:24

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.