Krdytkyu

I have implemented a program in C to crack passwords by generating all possible combinations of words ([A-Z][a-z]) up to the length of 5. While the program works, I would like to receive comments on the efficiency of the algorithm and other design decisions that would improve the code. The exercise is part of the course CS50 by Harvard.

I timed the program using unix's time and the time the program took to print all the combinations was

**real**    14m39.433s; 

**user**    0m10.040s;

**sys**     0m36.356s. 

CS50.h is a library developed for the course as training wheels for students. String (char*) and Bool are types defined in this library.

#define _XOPEN_SOURCE

#include <stdio.h>

#include <string.h>

#include <unistd.h>

#include <cs50.h>



int main(int argc, char *argv)

{

    if(argc != 2)

    {

        printf("Usage: ./crack hashn");

        return 1;

    }



    string alphabet = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";

    char password[6] = "";

    string hash = argv[1];

    char salt[3];

    memcpy(salt, hash, 2);

    salt[2] = '';



    bool flag = false;

    int alphabet_len = 52;



    for(int i = 0; i < alphabet_len; i++)

    {

        password[0] = alphabet[i];

        password[1] = password[2] = password[3] = password[4] = '';

        if(!strcmp(hash, crypt(password, salt)))

        {

            flag = true;

            break;

        }

        for(int j = 0; j < alphabet_len; j++)

        {

            password[1] = alphabet[j];

            password[2] = password[3] = password[4] = '';

            if(!strcmp(hash, crypt(password, salt)))

            {

                flag = true;

                break;

            }

            for(int k = 0; k < alphabet_len; k++)

            {

                password[2] = alphabet[k];

                password[3] = password[4] = '';

                if(!strcmp(hash, crypt(password, salt)))

                {

                    flag = true;

                    break;

                }

                for(int l = 0; l < alphabet_len; l++)

                {

                    password[3] = alphabet[l];

                    password[4] = '';

                    if(!strcmp(hash, crypt(password, salt)))

                    {

                        flag = true;

                        break;

                    }

                    for(int m = 0; m < alphabet_len; m++)

                    {

                        password[4] = alphabet[m];

                        if(!strcmp(hash, crypt(password, salt)))

                        {

                            flag = true;

                            break;

                        }

                    }

                    if(flag)

                        break;

                }

                if(flag)

                    break;

            }

            if(flag)

                break;

        }

        if(flag)

            break;

    }



    if(flag)

        printf("Password: %sn", password);

    else

        printf("Password not foundn");

}

cs50.h?

This seems less like a set of "training wheels" and more like a bicycle for fish. It's potentially confusing, opaque, and doesn't seem all that useful. If I were you, I'd be learning how to code in real C - using char*, and bool from stdbool.h.

Don't store things that should be computed

Your string alphabet shouldn't exist. Just iterate a char between a-z and A-Z. Characters can be incremented the same way that integers can.

Input validation

It seems like you expect hash to be two characters long, but you don't check that. You should be checking it with strlen; then you can issue memcpy without later setting a null terminator, as it'll be null-terminated already.

DRY

Don't repeat yourself. This is the most important aspect of the program that needs improvement. This block:

                for(int m = 0; m < alphabet_len; m++)

                {

                    password[4] = alphabet[m];

                    if(!strcmp(hash, crypt(password, salt)))

                    {

                        flag = true;

                        break;

                    }

                }

                if(flag)

                    break;

is repeated nearly verbatim five times. There are many different ways to condense this. The easiest is probably a recursive function that calls itself with an increasing depth integer. This may actually decrease the performance of the application, but that's up to you to test. There are also ways to rewrite this loop to have state so that neither copy-and-paste nor recursion are necessary; you'll probably want to compare such a method against a recursive method to see which is more performant and clean.

regarding:

    printf("Usage: ./crack hashn");

1. Error messages should be output to stderr, not stdout.


2. an executable can be renamed, so 'crack' is not a good thing to use.

Suggest:

    fprintf( stderr, "USAGE: %s hashn", argv[0] );

Note: argv[0] always contains the executable name