4
\$\begingroup\$

I made this code for a simple WinForm for my school project where I want to add other hash and encryption algorithms. I have made it following the algorithm of the official RFC 1321 and the Wikipedia page. I checked if the algorithm is correct by generating a hash and indeed it's working. However, are there any optimisable points and should I remove/add something?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace eigenMD5
{
    class MD5
    {
        // Zie Wikipedia
        protected readonly static uint[] T = new uint[64] 
            {   
                0xd76aa478,0xe8c7b756,0x242070db,0xc1bdceee,
                0xf57c0faf,0x4787c62a,0xa8304613,0xfd469501,
                0x698098d8,0x8b44f7af,0xffff5bb1,0x895cd7be,
                0x6b901122,0xfd987193,0xa679438e,0x49b40821,
                0xf61e2562,0xc040b340,0x265e5a51,0xe9b6c7aa,
                0xd62f105d,0x2441453,0xd8a1e681,0xe7d3fbc8,
                0x21e1cde6,0xc33707d6,0xf4d50d87,0x455a14ed,
                0xa9e3e905,0xfcefa3f8,0x676f02d9,0x8d2a4c8a,
                0xfffa3942,0x8771f681,0x6d9d6122,0xfde5380c,
                0xa4beea44,0x4bdecfa9,0xf6bb4b60,0xbebfbc70,
                0x289b7ec6,0xeaa127fa,0xd4ef3085,0x4881d05,
                0xd9d4d039,0xe6db99e5,0x1fa27cf8,0xc4ac5665,
                0xf4292244,0x432aff97,0xab9423a7,0xfc93a039,
                0x655b59c3,0x8f0ccc92,0xffeff47d,0x85845dd1,
                0x6fa87e4f,0xfe2ce6e0,0xa3014314,0x4e0811a1,
                0xf7537e82,0xbd3af235,0x2ad7d2bb,0xeb86d391
            };

        // Zie Wikipedia
        protected readonly static int[] s = new int[64]
        {
            7, 12, 17, 22, 7, 12, 17, 22, 7, 12, 17, 22, 7, 12, 17, 22,
            5,  9, 14, 20, 5,  9, 14, 20, 5,  9, 14, 20, 5,  9, 14, 20, 
            4, 11, 16, 23, 4, 11, 16, 23, 4, 11, 16, 23, 4, 11, 16, 23, 
            6, 10, 15, 21, 6, 10, 15, 21, 6, 10, 15, 21, 6, 10, 15, 21
        };


        // Standaard waarden
        uint A = 0x67452301;
        uint B = 0xEFCDAB89;
        uint C = 0x98BADCFE;
        uint D = 0X10325476;

        protected uint[] X = new uint[16];
        protected byte[] m_byteInput;

        public MD5(string s)
        {
            m_byteInput = new byte[s.Length];
            for (int i = 0; i < s.Length; i++)
            {
                m_byteInput[i] = (byte)s[i];
            }

        }

        public string createMD5()
        {
            byte[] bMsg;
            uint N;

            bMsg = padBuffer();

            N = (uint)(bMsg.Length * 8) / 32;

            for (uint i = 0; i < N / 16; i++)
            {
                makeBlock(bMsg, i);
                calcTransform();
            }

            string s = ReverseByte(A).ToString("X8") +
                        ReverseByte(B).ToString("X8") +
                        ReverseByte(C).ToString("X8") +
                        ReverseByte(D).ToString("X8") ;
            return s;
        }

        private static uint ReverseByte(uint uiNumber)
        {
            return (((uiNumber & 0x000000ff) << 24) |
                   (  uiNumber               >> 24) |
                   (( uiNumber & 0x00ff0000) >> 8)  |
                   (( uiNumber & 0x0000ff00) << 8)) ;
        }

        protected void calcTransform()
        {
            uint AA = A, BB = B, CC = C, DD = D;
            uint F = 0;
            int g = 0;

            for (int i = 0; i < 64; i++)
            {
                if (i >= 0 && i <= 15)
                {
                    F = ((BB & CC) | (~(BB) & DD));
                    g = i;
                }
                else if (i >= 16 && i <= 31)
                {
                    F = ((BB & DD) | (CC & ~DD));
                    g = (5 * i + 1) % 16;
                }
                else if (i >= 32 && i <= 47)
                {
                    F = (BB ^ CC ^ DD);
                    g = (3 * i + 5) % 16;
                }
                else if (i >= 48 && i <= 63)
                {
                    F = (CC ^ (BB | ~DD));
                    g = (7 * i) % 16;
                }
                uint dtemp = DD;
                DD = CC;
                CC = BB;
                uint dtempt = AA + F + T[i] + X[g];
                dtempt = (dtempt << s[i]) | (dtempt >> (32 - s[i]));
                BB = BB + dtempt;
                AA = dtemp;
            }

            A += AA;
            B += BB;
            C += CC;
            D += DD;
        }

        protected void makeBlock(byte[] bMsg, uint block)
        {
            block = block << 6;
            for (uint i = 0; i < 61; i += 4)
            {
                X[i>>2] = (((uint)bMsg[block+(i+3)]) << 24) |
                          (((uint)bMsg[block+(i+2)]) << 16) |
                          (((uint)bMsg[block+(i+1)]) << 8)  |
                          (((uint)bMsg[block+(i)] ))        ;
            }
        }
        protected byte[] padBuffer()
        {
            uint pad;
            byte[] bMsg;
            ulong sizeMsg;
            uint sizeMsgBuff;

            int temp = (448 - ((m_byteInput.Length * 8) % 512));

            pad = (uint)((temp + 512) % 512);

            if (pad == 0)
            {
                pad = 512;
            }

            sizeMsgBuff = (uint)((m_byteInput.Length) + (pad / 8) + 8);
            sizeMsg = (ulong)m_byteInput.Length * 8;
            bMsg = new byte[sizeMsgBuff];

            for (int i = 0; i < m_byteInput.Length; i++)
            {
                bMsg[i] = m_byteInput[i];
            }

            bMsg[m_byteInput.Length] |= 0x80;

            for (int i = 8; i > 0; i--)
            {
                bMsg[sizeMsgBuff - i] = (byte)(sizeMsg >> ((8 - i) * 8) & 0x00000000000000ff);
            }

            return bMsg;

        }
    }
}

An MD5 hash can be generated by calling MD5 test = MD5(yourstring) and then doing test.createMD5(). I am aware that I could make the generation more direct.

\$\endgroup\$
2
\$\begingroup\$
  • Why isn't this class sealed which I would expect of a security targeting class. By omitting the sealed keyword together with these protected variables and methods opens this class to vulnerabilities. Why should this class ever be inherited from ?

  • according to the NET naming guidelines methods should be named using PascalCase casing. So for instance calcTransform() should become CalcTransform() or better without abbreviation CalculateTransform().

  • according to the same naming guidelines variables should be named using camelCase casing. So for instance uint[] X should be uint[] x. If variable names doesn't have a special domain meaning, they should be as descriptive than possible.


protected byte[] m_byteInput;

public MD5(string s)
{
    m_byteInput = new byte[s.Length];
    for (int i = 0; i < s.Length; i++)
    {
        m_byteInput[i] = (byte)s[i];
    }

}  

m_byteInput is no where changed in your code so better make it private readonly to be sure that you don't accidential change it.

protected readonly static uint[] T = new uint[64] 
        {   
            0xd76aa478,0xe8c7b756,0x242070db,0xc1bdceee,
            0xf57c0faf,0x4787c62a,0xa8304613,0xfd469501, 
            ...  

If you initialize a array at the creation, you don't need to add the length of it. So this can become

private readonly static uint[] t = new uint[] 
        {   
            0xd76aa478,0xe8c7b756,0x242070db,0xc1bdceee,
            0xf57c0faf,0x4787c62a,0xa8304613,0xfd469501,
            ...  

The same is true for int[] s.

public string createMD5()
{
    byte[] bMsg;
    uint N;

    bMsg = padBuffer();

    N = (uint)(bMsg.Length * 8) / 32;

    for (uint i = 0; i < N / 16; i++)
    {
        makeBlock(bMsg, i);
        calcTransform();
    }

    string s = ReverseByte(A).ToString("X8") +
                ReverseByte(B).ToString("X8") +
                ReverseByte(C).ToString("X8") +
                ReverseByte(D).ToString("X8") ;
    return s;
}

Hungarian notation should not be used anymore. Your IDE will show you the used type if you hoover over a variables name. If you have a method named padBuffer it would be more obvious to name the variable which holds the returned value like paddedBuffer.

Having the variable string s doesn't add any value here. Just return the concated values.

By assigning a value at the declaration your code will become shorter and better readable like so

public string CreateMD5()
{
    byte[] paddedBuffer = PadBuffer();

    uint N = (uint)(bMsg.Length * 8) / 32;

    for (uint i = 0; i < N / 16; i++)
    {
        makeBlock(paddedBuffer, i);
        calcTransform();
    }

    return ReverseByte(A).ToString("X8") +
                ReverseByte(B).ToString("X8") +
                ReverseByte(C).ToString("X8") +
                ReverseByte(D).ToString("X8") ;
}  

That is much clearer, isn't it ?

protected void calcTransform()
{
    uint AA = A, BB = B, CC = C, DD = D;
    uint F = 0;
    int g = 0;

    for (int i = 0; i < 64; i++)
    {
        if (i >= 0 && i <= 15)
        {
            F = ((BB & CC) | (~(BB) & DD));
            g = i;
        }
        else if (i >= 16 && i <= 31)
        {
            F = ((BB & DD) | (CC & ~DD));
            g = (5 * i + 1) % 16;
        }
        else if (i >= 32 && i <= 47)
        {
            F = (BB ^ CC ^ DD);
            g = (3 * i + 5) % 16;
        }
        else if (i >= 48 && i <= 63)
        {
            F = (CC ^ (BB | ~DD));
            g = (7 * i) % 16;
        }
        uint dtemp = DD;
        DD = CC;
        CC = BB;
        uint dtempt = AA + F + T[i] + X[g];
        dtempt = (dtempt << s[i]) | (dtempt >> (32 - s[i]));
        BB = BB + dtempt;
        AA = dtemp;
    }

    A += AA;
    B += BB;
    C += CC;
    D += DD;
}

Declarations of multiple variables on the same line should be avoided because it reduces readability of the code.

Because you have written the if conditions from low to high you can omit the checking of the lower value of i.

After this if..else if statement you are doing this

uint dtemp = DD;
DD = CC;
CC = BB;
uint dtempt = AA + F + T[i] + X[g];
dtempt = (dtempt << s[i]) | (dtempt >> (32 - s[i]));
BB = BB + dtempt;
AA = dtemp;  

it is very hard to distinguish between a variable dtemp and another dtempt.

Maybe currentDD (or swapedDD) and calculatedValue would be better.

Implementing these points will lead to

private void CalcTransform()
{
    uint AA = A;
    uint BB = B;
    uint CC = C;
    uint DD = D;
    uint F = 0;
    int g = 0;

    for (int i = 0; i < 64; i++)
    {
        if (i <= 15)
        {
            F = ((BB & CC) | (~(BB) & DD));
            g = i;
        }
        else if (i <= 31)
        {
            F = ((BB & DD) | (CC & ~DD));
            g = (5 * i + 1) % 16;
        }
        else if (i <= 47)
        {
            F = (BB ^ CC ^ DD);
            g = (3 * i + 5) % 16;
        }
        else if (i <= 63)
        {
            F = (CC ^ (BB | ~DD));
            g = (7 * i) % 16;
        }
        uint currentDD = DD;
        DD = CC;
        CC = BB;
        uint calculatedValue = AA + F + T[i] + X[g];
        calculatedValue = (calculatedValue << s[i]) | (calculatedValue >> (32 - s[i]));
        BB = BB + calculatedValue;
        AA = currentDD;
    }

    A += AA;
    B += BB;
    C += CC;
    D += DD;
}

protected byte[] padBuffer()
{
    uint pad;
    byte[] bMsg;
    ulong sizeMsg;
    uint sizeMsgBuff;

    int temp = (448 - ((m_byteInput.Length * 8) % 512));

    pad = (uint)((temp + 512) % 512);

    if (pad == 0)
    {
        pad = 512;
    }

    sizeMsgBuff = (uint)((m_byteInput.Length) + (pad / 8) + 8);
    sizeMsg = (ulong)m_byteInput.Length * 8;
    bMsg = new byte[sizeMsgBuff];

    for (int i = 0; i < m_byteInput.Length; i++)
    {
        bMsg[i] = m_byteInput[i];
    }

    bMsg[m_byteInput.Length] |= 0x80;

    for (int i = 8; i > 0; i--)
    {
        bMsg[sizeMsgBuff - i] = (byte)(sizeMsg >> ((8 - i) * 8) & 0x00000000000000ff);
    }

    return bMsg;

}  

Variables should be declared as near as possible to their usage. This can be easily achieved by assigning the values at declaration.

A variable named sizeMsgBuff would be more meaningful if it would be named messageBufferSize or bufferSize. The same applies for sizeMsg -> messageSize.

Regarding 0x00000000000000ff, this is a magic number, which should be extracted to a meaningful (maybe domain specific named) constant. In addition, it could be more easily expressed like 0xff.

This loop

for (int i = 0; i < m_byteInput.Length; i++)
{
    bMsg[i] = m_byteInput[i];
}  

can be replaced by a call to Array.Copy().

Applying these points leads to

private byte[] PadBuffer()
{

    uint pad = (uint)(((448 - ((m_byteInput.Length * 8) % 512)) + 512) % 512);

    if (pad == 0)
    {
        pad = 512;
    }

    uint bufferSize = (uint)((m_byteInput.Length) + (pad / 8) + 8);
    ulong messageSize = (ulong)m_byteInput.Length * 8;

    byte[] paddedBuffer = new byte[bufferSize];

    Array.Copy(m_byteInput, paddedBuffer, m_byteInput.Length);

    paddedBuffer[m_byteInput.Length] |= 0x80;

    for (int i = 8; i > 0; i--)
    {
        paddedBuffer[bufferSize - i] = (byte)(messageSize >> ((8 - i) * 8) & 0xff);
    }

    return paddedBuffer;

}
\$\endgroup\$
  • \$\begingroup\$ Very helpful, I really appreciate it! Thank you! \$\endgroup\$ – Thomas W Oct 5 '15 at 18:04

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.