MD5 hash generator

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

-
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 = 0 && i = 16 && i = 32 && i = 48 && i > (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 > (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 > (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  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 ap

StackExchange Code Review Q#106554, answer score: 2