10
\$\begingroup\$

I have created an SslStream Abstract Class for my Client and i want you to review and enhance it for me if needed

SecureStream.cs

using System;
using System.Net;
using System.Net.Security;
using System.Net.Sockets;
using System.Security.Cryptography.X509Certificates;
using System.Threading.Tasks;
using Program.Connections.Packets;
using Program.Core.Misc;

namespace Program.Connections.Network
{
    public abstract class SecureStream
    {
        private static readonly byte[] Buffer = new byte[0x1f80];
        private const string IpAddress = "192.168.1.65";
        private readonly int _port;
        private readonly Socket _socket;
        private SslStream _sslStream;

        public abstract void Receive(byte[] buffer);
        public abstract void Error(Exception e);

        protected SecureStream(int port)
        {
            _socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
            _socket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.KeepAlive, true);
            _socket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.DontLinger, true);
            _port = port;
        }

        public void Connect()
        {
            _socket.BeginConnect(new IPEndPoint(IPAddress.Parse(IpAddress), _port), Authenticate, null);            
        }

        private async void Authenticate(IAsyncResult ar)
        {
            try
            {
                var onCertificateValidationCallBack =
                    new RemoteCertificateValidationCallback(
                        OnCertificateValidation);
                _sslStream = new SslStream(new NetworkStream(_socket, true), false, onCertificateValidationCallBack);

                await _sslStream.AuthenticateAsClientAsync(IpAddress);
                if (!_sslStream.IsAuthenticated) throw new Exception("not^authenticated");

                do
                {
                    var size = await _sslStream.ReadAsync(Buffer, 0, Buffer.Length);
                    if (size == 0) throw new Exception("invalid^size");
                    var buffer = new byte[size];
                    Native.MemoryCopy(Buffer, buffer, (uint)buffer.Length);
                    Receive(buffer);
                    Array.Clear(Buffer, 0, Buffer.Length);

                } while (_sslStream.CanRead);
            }
            catch (Exception e)
            {
                Error(e);
            }
        }

        internal async Task Send(PacketWriter writer)
        {
            var buffer = writer.GetWorkspace();
            await _sslStream.WriteAsync(buffer, 0, buffer.Length);
        }

        private static bool OnCertificateValidation(object sender, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslpolicyerrors)
        {
            return true;
        }
    }
}

CoreServer.cs

using System;
using System.Windows.Forms;

namespace Program.Connections.Network
{
    public sealed class CoreServer : SecureStream
    {
        public CoreServer(int p) : base(p) { }

        public override void Receive(byte[] buffer)
        {
            //TODO: Handle The Buffer
        }

        public override void Error(Exception e)
        {
            //TODO: Add Error Writer.
            MessageBox.Show(
                string.Format("Disconnected from the server."),
                string.Format("Connection Error"), MessageBoxButtons.OK,
                MessageBoxIcon.Error);
            Environment.Exit(1);
        }
    }
}

And i Simply Execute it using

var coreServer = new CoreServer(/*port*/);
coreServer.Connect();
\$\endgroup\$
0

1 Answer 1

4
\$\begingroup\$
new byte[0x1f80]

I think using decimal would be more readable here. And I don't understand why is your buffer 8096 bytes, usual sizes are powers of two, like 8192.

IpAddress = "192.168.1.65"

This looks like a perfect example of something that should be configurable, not set in stone. You should accept the IP address in the constructor, like you already do with port number.

public abstract void Receive(byte[] buffer);
public abstract void Error(Exception e);

I'm not sure I like the use of inheritance here. I think using events, or something like that would make more sense here.

_socket.BeginConnect(…)

I don't like that you're using the old Asynchronous Programming Model when you could (and do in other parts of your code) use async-await. Unfortunately, Socket doesn't expose awaitable methods, but there are ways around that.

private async void Authenticate(IAsyncResult ar)

I think Authenticate is not a very good name for this method, it does much more than just authentication.

throw new Exception("not^authenticated")

You shouldn't throw Exception, you should instead create a custom class that derives from Exception and use that. And the exception message should be human-readable, not some error code like it seems in your case. If you need to differentiate different errors, either have different exception types for them, or add a property for that to your custom exception.

if (size == 0) throw new Exception("invalid^size");

When ReadAsync() returns 0, it's not invalid size, it means the end of the stream was reached, so you should act accordingly.

Native.MemoryCopy(Buffer, buffer, (uint)buffer.Length);

Why are using some custom memory copying method here? Do you have some reason for not using Buffer.BlockCopy()? Also, why are you even doing the copying here? It seems you class is intended only for limited use (since you have a constant IP address), so couldn't you make sure the buffer isn't used after Receive() returns, so you didn't have to copy at all?

return true;

I'm no expert on security, but I think you should perform at least some checks of the certificate here. Otherwise, your class isn't really a “secure stream”.

MessageBox.Show(…);

You shouldn't mix code like this with GUI code, they should be separated.

\$\endgroup\$
2
  • \$\begingroup\$ i am throwing an exception on reading 0 because it means that it is disconnected. Also i am using MemoryCopy because it is much faster than regular BlockCopy \$\endgroup\$
    – RuneS
    Oct 27, 2012 at 20:04
  • \$\begingroup\$ Instead of always returning true from OnCertificateValidation it's better to return sslpolicyerrors == SslPolicyErrors.None. Example here: msdn.microsoft.com/en-us/library/… \$\endgroup\$ Feb 13, 2017 at 15:33

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.