1
\$\begingroup\$

The code connects multiple clients to a server using TCP sockets asynchronously. I have added code to attach a digital signature to the outgoing messages from the server, to authenticate the message the signature is verified on the client and the message is displayed. The signing and verifying of the digital signature uses RSA 4096 bit asymmetric keys hard-coded into the application.

The server's send method takes a string and a socket, signs the string and constructs the data packet by concatenating the message, salt, and signature bytes before sending:

private static void SendData(string text, Socket socket)
{
    byte[] data = Encoding.UTF8.GetBytes(text);
    byte[] signature = SignMessage(data);

    byte[] signedmessage = data.Concat(Encoding.UTF8.GetBytes("::Salt::"))
                                .Concat(salt)
                                .Concat(Encoding.UTF8.GetBytes("::Signature::"))
                                .Concat(signature)
                                .ToArray();

    socket.BeginSend(signedmessage, 0, signedmessage.Length, SocketFlags.None, new AsyncCallback(SendCallback), socket);
    Console.WriteLine("Sent data: " + text + " To: " + c.Socket.RemoteEndPoint.ToString());
}

private static byte[] SignMessage(byte[] message)
{
    RNGCryptoServiceProvider randCSP = new RNGCryptoServiceProvider();

    salt = new byte[32];
    randCSP.GetBytes(salt);
    salt = Encoding.UTF8.GetBytes(Convert.ToBase64String(salt));

    byte[] msgSalt = message.Concat(salt).ToArray();

    var hasher = new SHA256Managed();
    var messageHash = hasher.ComputeHash(msgSalt);

    CspParameters parms = new CspParameters();
    parms.Flags = CspProviderFlags.NoFlags;
    parms.KeyContainerName = Guid.NewGuid().ToString().ToUpperInvariant();
    parms.ProviderType = ((Environment.OSVersion.Version.Major > 5) || ((Environment.OSVersion.Version.Major == 5) && (Environment.OSVersion.Version.Minor >= 1))) ? 0x18 : 1;
    RSACryptoServiceProvider RSA = new RSACryptoServiceProvider(parms);
    RSA.FromXmlString(_rsaKeys);

    //Create a signature for HashValue and return it 
    return RSA.SignHash(messageHash, CryptoConfig.MapNameToOID("SHA256"));
}

The _rsaKeys value on the server is a string const with the public and private keys:

private const string _rsaKeys = @"<RSAKeyValue>
    <Modulus>...</Modulus>
    <Exponent>...</Exponent>
    <P>...</P>
    <Q>...</Q>
    <DP>...</DP>
    <DQ>...</DQ>
    <InverseQ>...</InverseQ>
    <D>...</D>
</RSAKeyValue>";

The receive callback function on the clients deconstructs the data packet, then verifies the signature:

private static void ReceiveCallback(IAsyncResult AR)
{
    try
    {
        Socket socket = (Socket)AR.AsyncState;
        int received = socket.EndReceive(AR);
        byte[] dataBuf = new byte[received];
        Array.Copy(_buffer, dataBuf, received);

        //covert data to text, NOTE this scrambles signature
        string text = Encoding.UTF8.GetString(dataBuf);

        //get everything before the signature
        var sigSplit = text.Split(new string[] { "::Signature::" }, StringSplitOptions.RemoveEmptyEntries);

        //get salt and message strings as bytes again
        var msgSplit = sigSplit[0].Split(new string[] { "::Salt::" }, StringSplitOptions.RemoveEmptyEntries);
        byte[] message = Encoding.UTF8.GetBytes(msgSplit[0]);
        salt = Encoding.UTF8.GetBytes(msgSplit[1]);

        //work out signature length - could possibly be hardcoded to 512
        var sigLength = received - (Encoding.UTF8.GetBytes(sigSplit[0]).Length + 13);
        //copy raw bytes from data buffer
        byte[] signature = new byte[sigLength];
        Array.Copy(dataBuf, (Encoding.UTF8.GetBytes(sigSplit[0]).Length + 13), signature, 0, sigLength);

        if (VerifyMessage(message, signature))
        {
            Console.WriteLine("Message received: " + msgSplit[0]);
        }
        else
        {
            Console.WriteLine("Signature was not verified.");
        }

        _clientSocket.BeginReceive(_buffer, 0, _buffer.Length, SocketFlags.None, new AsyncCallback(ReceiveCallback), _clientSocket);
    }
    catch (Exception)
    {
        Connect();
    }
}

private static bool VerifyMessage(byte[] message, byte[] signature)
{
    byte[] msgSalt = message.Concat(salt).ToArray();

    var hasher = new SHA256Managed();
    var messageHash = hasher.ComputeHash(msgSalt);

    CspParameters parms = new CspParameters();
    parms.Flags = CspProviderFlags.NoFlags;
    parms.KeyContainerName = Guid.NewGuid().ToString().ToUpperInvariant();
    parms.ProviderType = ((Environment.OSVersion.Version.Major > 5) || ((Environment.OSVersion.Version.Major == 5) && (Environment.OSVersion.Version.Minor >= 1))) ? 0x18 : 1;
    RSACryptoServiceProvider RSA = new RSACryptoServiceProvider(parms);
    RSA.FromXmlString(_rsaKey);

    //verify a signature for HashValue and return true/false 
    return RSA.VerifyHash(messageHash, CryptoConfig.MapNameToOID("SHA256"), signature);
}

The clients _rsaKey value is a string const with the public key only:

private const string _rsaKeys = @"<RSAKeyValue>
    <Modulus>...</Modulus>
    <Exponent>...</Exponent>
</RSAKeyValue>";

The main reason for this code review is to check that the above code is secure. Would it be better to have the keys in .pem or .cer files distributed with the client application? Does the salt make this code more secure, or less? Would using the RSAPKCS1SignatureFormatter class be better than using the SignHash method?

Also is there a better way of constructing and deconstructing the data packet?

\$\endgroup\$
  • 1
    \$\begingroup\$ If your main concern is security, the question would probably be better off on security.stackexchange.com \$\endgroup\$ – jcaron Feb 5 '16 at 0:15
  • \$\begingroup\$ @jcaron I have posted this on security.stackexchange.com/questions/112859/… \$\endgroup\$ – DafyddNZ Feb 5 '16 at 1:26
  • \$\begingroup\$ That code is headache inducing. Can't you simply use TLS instead? \$\endgroup\$ – CodesInChaos Feb 8 '16 at 13:58
  • \$\begingroup\$ You have many "magic" values without explanations (13, 5, 1, 0x18, etc.), you should put them instead in constants properly named. \$\endgroup\$ – Patrick Mevzek May 1 '18 at 2:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.