Brute force MD5 password checker

Question

I'm trying to create a C# brute force MD5 password checker. If I run it with "test1" without MD5, it completes in 15 seconds. I would like to make it faster, but I'm a c# beginner, and don't know how to optimize code anymore.

Basically, instead of nested for loops, if using flags (of type boolean), so if you have 4 char long pass it goes like this:

_ _ _ _ -> try every letter and number at last place, then set last flag on true. If last flag == true then increment counter for third place.

private string bruteForce(int lenght)
    {
        MD5 md55 = System.Security.Cryptography.MD5.Create();
        char[] words =
        {
            'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'r', 'q', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z',
            'A','B','C','D','E','F','G','H','I','J','K','L','M','N','O','P','R','Q','S','T','U','V','W','X','Y','Z',
            '0','1','2','3','4','5','6','7','8','9'

        };

        bool[] konec = new bool[lenght];

        int flag = lenght;
        char[] pass = new char[flag];
        int[] signal = new int[lenght];
        for (int j = 0; j < lenght; j++)
        {
            signal[j] = -1;
            konec[j] = false;
        }
        flag--;
        string tmp = "";
        bool over=false;

        while (!over)
        {

            for (int k = 0; k < lenght; k++)
            {
                if (konec[k])
                {
                    if (k == 0) return "not found";
                    int t = ++signal[k - 1];
                    if (t == words.Length - 1) {konec[k - 1] = true;
                        signal[k - 1] = 0;

                        break;
                    }
                    pass[k - 1] = words[t];
                    konec[k] = false;

                }
            }
            for (int i = 0; i < words.Length; i++)
            {

                if (!konec[flag])
                {
                    pass[flag] = words[i];
                    tmp = new string(pass);
                    byte[] inputBytes = System.Text.Encoding.ASCII.GetBytes(tmp);
                    byte[] hash = md55.ComputeHash(inputBytes);

                    // step 2, convert byte array to hex string
                  string  emde= BitConverter.ToString(hash).Replace("-", "");
                  emde = emde.ToLower();
                  if (tmp == target)
                    {
                        MessageBox.Show(tmp + "    " + target);
                        over = true;
                        break;
                    }
                }



            }
          konec[flag] = true;
        }
        return tmp;
    }

This is call on button click:

private void Button_Click(object sender, RoutedEventArgs e)
    {
        int l = Convert.ToInt32(stevilo.Text);
       // lenght = 1;
       // string a = "";

        if (md5)
        {

            if (brute)
            {
                Stopwatch time = new Stopwatch();
                time.Start();


                target = zgoscevalna.Text;
                for (int i = 1; i < l+1; i++)
                {
                    int _i = i;
                     Thread t = new Thread(() =>
                    {
                        string a = bruteForce(_i);
                        if (a != "not found")
                        {
                            Dispatcher.Invoke(() =>
                            {
                                password.Text = a;
                                tajm.Text = (time.Elapsed.TotalSeconds).ToString();
                            });

                            Thread.CurrentThread.Abort();

                        }
                        else
                        {
                            Thread.CurrentThread.Abort();
                        }

                    });
                    t.Start();

                }

            }

            else
            {
            }
        }

Answer 1

MD5 md55 = System.Security.Cryptography.MD5.Create();

Do yourself a favor, an stick using System.Security.Cryptography; at the top of the code file - then you can shorten this line to MD5 md55 = MD5.Create(); - but md55 is a bad name that doesn't mean anything. Perhaps engine, or hashProvider... anything but md55!

char[] words

That is a confusing name - characters aren't words, a better name for this array could be tokens, perhaps.

if (k == 0) return "not found";

What if the password happened to be "not found"? I think this is a situation where you would want to throw an exception of some sort, instead of tweaking the meaning of the return value.

Thread t = new Thread(() => ...

Starting your own thread is rarely a good idea. Look into the Task Parallel Library (TPL) in the System.Threading.Tasks namespace, and let the framework handle all the threading dirtiness for you. Also, starting a new thread incurs some overhead - if I'm not sticking a foot in my mouth here, using the TPL the framework determines if it's a good idea to start a new thread or to let code run sequentially... which may buy you some extra performance right here.

I would extract a method here, the anonymous method isn't buying you anything and contributes to make your click handler look like it's some "God button" that knows everything and does everything.

Thread.CurrentThread.Abort();

That's part of the threading nastiness you get to avoid with tasks. Aborting a thread is rarely a sane thing to do. I don't know much about multithreading, but this definitely rings a "no-no" bell.

        else
        {
        }

Dead code. Don't think, remove.

tajm.Text = (time.Elapsed.TotalSeconds).ToString();

You really need to work on naming. It's not going to make your code run any faster, but tajm doesn't mean anything to anyone maintaining your code. Be descriptive!

Answer 2

To start with, C# conventions often state that you should always use braces around if statements, for loops, and similar statements. That means this statement:

if (k == 0) return "not found";

should be changed to this, at least:

if (k == 0) { return "not found"; }

---

This also does not follow C# standards:

if (t == words.Length - 1) {konec[k - 1] = true;
    signal[k - 1] = 0;

    break;
}

The braces should always be on the next line:

if (t == words.Length - 1)
{
    konec[k - 1] = true;
    signal[k - 1] = 0;

    break;
}

---

In your Button_Click method, you have this:

if (md5)
{

    if (brute)
    {

You do not have any calls in the if (md5) block that are not also in the if (brute) block, so that should be like this:

if (md5 && brute) { ... }

---

After your if (md5) block, you have an empty else statement:

else
{
}

Please remove that.

---

The last statement in this if/else set is Thread.CurrentThread.Abort();:

if (a != "not found")
{
    Dispatcher.Invoke(() =>
    {
        password.Text = a;
        tajm.Text = (time.Elapsed.TotalSeconds).ToString();
    });

    Thread.CurrentThread.Abort();

}
else
{
    Thread.CurrentThread.Abort();
}

This can be simplified:

if (a != "not found")
{
    Dispatcher.Invoke(() =>
    {
        password.Text = a;
        tajm.Text = (time.Elapsed.TotalSeconds).ToString();
    });
}

Thread.CurrentThread.Abort();

---

You have one letter variable names scattered around your code: int l = Convert.ToInt32(stevilo.Text);. Please change this to be more descriptive.

---

If you don't need this commented code (presumably you don't, because it is commented), remove it:

// lenght = 1;
// string a = "";

Also, lenght should be spelled correctly as length (not to criticize your spelling, I make similar mistakes too).

---

Also, you have spare returns scattered around your code, especially before ending braces. C# does not consider whitespace when compiling, so those are most unnecessary, and make it hard to read your code. You may want to consider putting a single space after the brace instead of before the brace if your code is separated into logically similar groups or if you want to split it up some.

---

As for optimizing code, I am not too good at that myself, so I'll leave that for the experts to cover.

Answer 3

I am going to look at your button call a little bit and see if we can make it a little bit cleaner, first and see where we end up.

private void Button_Click(object sender, RoutedEventArgs e)
    {
        int l = Convert.ToInt32(stevilo.Text);
       // lenght = 1;
       // string a = "";

        if (md5)
        {

            if (brute)
            {
                Stopwatch time = new Stopwatch();
                time.Start();


                target = zgoscevalna.Text;
                for (int i = 1; i < l+1; i++)
                {
                    int _i = i;
                     Thread t = new Thread(() =>
                    {
                        string a = bruteForce(_i);
                        if (a != "not found")
                        {
                            Dispatcher.Invoke(() =>
                            {
                                password.Text = a;
                                tajm.Text = (time.Elapsed.TotalSeconds).ToString();
                            });

                            Thread.CurrentThread.Abort();

                        }
                        else
                        {
                            Thread.CurrentThread.Abort();
                        }

                    });
                    t.Start();

                }

            }

            else
            {
            }
        }

First thing, your indentation is off a bit.

Second, you can get rid of the nested if statement by combining them, probably not going to buy you any processing, but it will save you on nesting space and vertical space.

now it looks like this

private void Button_Click(object sender, RoutedEventArgs e)
{
    int l = Convert.ToInt32(stevilo.Text);
    if (md5 && brute)
    {
        Stopwatch time = new Stopwatch();
        time.Start();

        target = zgoscevalna.Text;
        for (int i = 1; i < l+1; i++)
        {
            int _i = i;
            Thread t = new Thread(() =>
            {
                string a = bruteForce(_i);
                if (a != "not found")
                {
                    Dispatcher.Invoke(() =>
                    {
                        password.Text = a;
                        tajm.Text = (time.Elapsed.TotalSeconds).ToString();
                    });
                    Thread.CurrentThread.Abort();
                }
                else
                {
                    Thread.CurrentThread.Abort();
                }
            });
            t.Start();
        }
    }
}

next thing that I see is

            int _i = i;
            Thread t = new Thread(() =>
            {
                string a = bruteForce(_i);

this is not necessary at all, don't do this, you just took up more memory space when you didn't have to.

just call

string a = bruteForce(i);

use the current iteration variable, because that is really what you are doing anyway.

Next, we are going to pull Thread.CurrentThread.Abort(); out of the if/else statement, so now it is going to look like this instead.

private void Button_Click(object sender, RoutedEventArgs e)
{
    int l = Convert.ToInt32(stevilo.Text);
    if (md5 && brute)
    {
        Stopwatch time = new Stopwatch();
        time.Start();

        target = zgoscevalna.Text;
        for (int i = 1; i < l+1; i++)
        {
            Thread t = new Thread(() =>
            {
                string a = bruteForce(i);
                if (a != "not found")
                {
                    Dispatcher.Invoke(() =>
                    {
                        password.Text = a;
                        tajm.Text = (time.Elapsed.TotalSeconds).ToString();
                    });
                }
                Thread.CurrentThread.Abort();
            });
            t.Start();
        }
    }
}

I also don't see a need for target = zgoscevalna.Text; this could probably be removed from the code, my guess is that it is left over from previous attempts.

Something else that I would do is move the declaration of l (you should name this better too) inside the if statement at the least so that you don't create the variable unless you enter into the if statement, you don't need the variable outside of that if statement so don't create it outside of the if statement.

All said and done....

private void Button_Click(object sender, RoutedEventArgs e)
{
    if (md5 && brute)
    {
        Stopwatch time = new Stopwatch();
        time.Start();
        int l = Convert.ToInt32(stevilo.Text);

        for (int i = 1; i < l+1; i++)
        {
            Thread t = new Thread(() =>
            {
                string a = bruteForce(i);
                if (a != "not found")
                {
                    Dispatcher.Invoke(() =>
                    {
                        password.Text = a;
                        tajm.Text = (time.Elapsed.TotalSeconds).ToString();
                    });
                }
                Thread.CurrentThread.Abort();
            });
            t.Start();
        }
    }
}

---

You might want to check out everything that you can do with the Stopwatch class, since you are looking at the performance you might want a higher resolution on your time between threads and such.

Stopwatch Class MSDN