5
\$\begingroup\$

After much research, I was able to implement the twofish algorithm using Bouncy Castle Portable for .NET. The code has been tested and is working perfectly, to the best of my knowledge. But I do not know if it was well implemented and I would like someone's opinion. It would be nice to know if it is done correctly because it would be for other developers to use. Is there any improvement in security and performance that could be made? Any idea?

public class BCEngine
{
    private readonly Encoding _encoding;
    private readonly IBlockCipher _blockCipher;
    private PaddedBufferedBlockCipher _cipher;
    private IBlockCipherPadding _padding;

    public BCEngine()
    {

    }

    BCEngine(IBlockCipher blockCipher, Encoding encoding)
    {
        _blockCipher = blockCipher;
        _encoding = encoding;
    }

    #region Public Methods


    public static string TwoFishEncryption(string TextPlain, string Password, byte[] Salt)
    {
        Sha3Digest Sha3Digest = new Sha3Digest();
        Pkcs5S2ParametersGenerator gen = new Pkcs5S2ParametersGenerator(Sha3Digest);
        gen.Init(Encoding.UTF8.GetBytes(Password), Salt, 1000);
        KeyParameter param = (KeyParameter)gen.GenerateDerivedParameters(new TwofishEngine().AlgorithmName, 256);

        BCEngine bcEngine = new BCEngine(new TwofishEngine(), Encoding.UTF8);
        bcEngine.SetPadding(new Pkcs7Padding());
        return bcEngine.Encrypt(TextPlain, param);
    }

    public static string TwoFishDecryption(string TextEncripted, string Password, byte[] Salt)
    {
        Sha3Digest Sha3Digest = new Sha3Digest();
        Pkcs5S2ParametersGenerator gen = new Pkcs5S2ParametersGenerator(Sha3Digest);
        gen.Init(Encoding.UTF8.GetBytes(Password), Salt, 1000);
        KeyParameter param = (KeyParameter)gen.GenerateDerivedParameters(new TwofishEngine().AlgorithmName, 256);

        BCEngine bcEngine = new BCEngine(new TwofishEngine(), Encoding.UTF8);
        bcEngine.SetPadding(new Pkcs7Padding());
        return bcEngine.Decrypt(TextEncripted, param);
    }

    #endregion

    #region Private Methods

    void SetPadding(IBlockCipherPadding padding)
    {
        if (padding != null)
            _padding = padding;
    }

    string Encrypt(string plain, ICipherParameters SetKeyParameter)
    {
        byte[] result = BouncyCastleCrypto(true, _encoding.GetBytes(plain), SetKeyParameter);
        return Convert.ToBase64String(result);
    }

    string Decrypt(string cipher, ICipherParameters SetKeyParameter)
    {
        byte[] result = BouncyCastleCrypto(false, Convert.FromBase64String(cipher), SetKeyParameter);
        return _encoding.GetString(result, 0, result.Length);
    }

    byte[] BouncyCastleCrypto(bool forEncrypt, byte[] input, ICipherParameters SetKeyParameter)
    {
        try
        {
            _cipher = _padding == null ?
            new PaddedBufferedBlockCipher(_blockCipher) : new PaddedBufferedBlockCipher(_blockCipher, _padding);

            _cipher.Init(forEncrypt, SetKeyParameter);

            byte[] ret = _cipher.DoFinal(input);
            return ret;

        }
        catch (CryptoException ex)
        {
            // throw new CryptoException(ex);
        }
        return null;
    }

    #endregion

}
\$\endgroup\$
0

3 Answers 3

3
\$\begingroup\$

static vs instance

public BCEngine()
{

}

BCEngine(IBlockCipher blockCipher, Encoding encoding)
{
    _blockCipher = blockCipher;
    _encoding = encoding;
}

Default constructors should provide reasonable default values for the two fields initialized by the other constructor but wait... both constructors are useless because all public APIs are static. The only instance members are private methods. I can create an instance of BCEngine but I cannot do anything with it.

BCEngine bcEngine = new BCEngine(new TwofishEngine(), Encoding.UTF8);
bcEngine.SetPadding(new Pkcs7Padding());
return bcEngine.Encrypt(TextPlain, param);

But wait again.. you instantiate it to call the private instance methods. This is very inconsistent and chaotic.

You should either make everything static or everything instance.

It's ok if an instance uses private static methods as helpers but the other way around is kind of weird. At least the default constructor should be private too.

However your implementation uses private fields where two of them are not readonly so you use them to store some state but fortunately they are not static. You should remove these fields and pass the required arguments from method to method. Then you won't need any instances and the methods would be pure <-- LINK.


using and disposables

You should check your code for disposable types. I'm pretty sure there are plenty of them. Encryption usually requires to dispose something.


#region

Regions are rarely necessary and for member grouping virtually never. It's easier to read the code without them.

\$\endgroup\$
5
  • \$\begingroup\$ Sometimes region is useful in this case maybe not that much but I still wouldn't mind having it. \$\endgroup\$
    – Denis
    Jun 14, 2017 at 16:24
  • \$\begingroup\$ @Denis I can name at most like two examples where they might be usefull... I sometimes use them to wrap an interface implementation (if it's something large or I have a few of them) or I might sometimes use them to wrap a bunch of similar methods like initialization etc. do you know more? \$\endgroup\$
    – t3chb0t
    Jun 14, 2017 at 16:28
  • 1
    \$\begingroup\$ I tend to use regions for separating my methods in winforms applications in the following way : Event Handlers/ Helper functions (short functions such as converting a DateTime to a short string version). The rest I leave without a region because they're usually sorted above the previous 2. \$\endgroup\$
    – Denis
    Jun 14, 2017 at 16:33
  • \$\begingroup\$ I've changed the code according to the feedbacks I hope it has improved. I accept more suggestions! Thank you! \$\endgroup\$
    – Diogo Luiz
    Jun 14, 2017 at 19:46
  • \$\begingroup\$ @dilpo unfortunatelly editing the code in the question is not allowed when there are already answers. You can post a follow-up question or a self answer instead. \$\endgroup\$
    – t3chb0t
    Jun 14, 2017 at 19:48
3
\$\begingroup\$
  • You should notify the user if your method prohibits some values:

    void SetPadding(IBlockCipherPadding padding)
    {
        if (padding != null)
        _padding = padding;
    }
    

    If the value is null, throw an exception for example.

    There is no clear indication that _padding is not set to null.

  • TwoFishEncryption & TwoFishDecryption appear to have almost identical bodies, consider refactoring the repetitive code and just call the 1 different line at the end.

  • Your ternary operator ordering is a bit unusual. This looks cleaner to me:

    _cipher = _padding == null
        ? new PaddedBufferedBlockCipher(_blockCipher)
        : new PaddedBufferedBlockCipher(_blockCipher, _padding);
    
\$\endgroup\$
1
  • \$\begingroup\$ I've changed the code according to the feedbacks I hope it has improved. I accept more suggestions! Thank you! \$\endgroup\$
    – Diogo Luiz
    Jun 14, 2017 at 19:46
0
\$\begingroup\$

I've changed the code according to the feedbacks I hope it has improved. I accept more suggestions! Thanks to all who helped me!

using Org.BouncyCastle.Crypto;
using Org.BouncyCastle.Crypto.Digests;
using Org.BouncyCastle.Crypto.Generators;
using Org.BouncyCastle.Crypto.Paddings;
using Org.BouncyCastle.Crypto.Parameters;
using Org.Cerberus.Utils;
using System;
using System.Text;

namespace BCEngine
{
    public class BCEngineCore
    {
        Encoding _encoding;
        IBlockCipher _blockCipher;
        private PaddedBufferedBlockCipher _cipher;
        private IBlockCipherPadding _padding;
        KeyParameter KeyParameter;

        #region Public Methods

        public string TwoFishEncryption(string TextPlain, string Password, byte[] Salt, string Prefix = "")
        {
            TwoFish(TextPlain, Password, Salt, Prefix);
            return Encrypt(TextPlain, KeyParameter);
        }

        public string TwoFishDecryption(string TextEncripted, string Password, byte[] Salt, string Prefix = "")
        {
            TwoFish(TextEncripted, Password, Salt, Prefix);
            return Decrypt(TextEncripted, KeyParameter);
        }

        #endregion

        #region Private Methods

        void TwoFish(string TextPlain, string Password, byte[] Salt, string Prefix = "")
        {

            Sha3Digest Sha3Digest = new Sha3Digest((int)KeySizes.F256);
            Pkcs5S2ParametersGenerator gen = new Pkcs5S2ParametersGenerator(Sha3Digest);
            gen.Init(Encoding.UTF8.GetBytes(Password), Salt.PreFixSaltBytes(Prefix), Constants.NUM_ITERATOR);
            KeyParameter = (KeyParameter)gen.GenerateDerivedParameters(BCEngines.TwofishEngine.AlgorithmName, 256);

            SetPadding(new Pkcs7Padding());
            SetBlockCipher(BCEngines.TwofishEngine);
            SetEncoding(Encoding.UTF8);
        }

        void SetPadding(IBlockCipherPadding padding)
        {
            if (padding != null)
                _padding = padding;
            else
                throw new NullReferenceException("Padding is null!");
        }

        void SetBlockCipher(IBlockCipher blockCipher)
        {
            if (blockCipher != null)
                _blockCipher = blockCipher;
            else
                throw new NullReferenceException("BlockCipher is null!");
        }

        void SetEncoding(Encoding encoding)
        {
            if (encoding != null)
                _encoding = encoding;
            else
                throw new NullReferenceException("Encoding is null!");
        }

        string Encrypt(string plain, ICipherParameters SetKeyParameter)
        {
            byte[] result = BouncyCastleCrypto(true, _encoding.GetBytes(plain), SetKeyParameter);
            return Convert.ToBase64String(result);
        }

        string Decrypt(string cipher, ICipherParameters SetKeyParameter)
        {
            byte[] result = BouncyCastleCrypto(false, Convert.FromBase64String(cipher), SetKeyParameter);
            return _encoding.GetString(result, 0, result.Length);
        }

        byte[] BouncyCastleCrypto(bool forEncrypt, byte[] input, ICipherParameters SetKeyParameter)
        {
            try
            {
                _cipher = _padding == null
                    ? new PaddedBufferedBlockCipher(_blockCipher)
                    : new PaddedBufferedBlockCipher(_blockCipher, _padding);

                _cipher.Init(forEncrypt, SetKeyParameter);

                byte[] ret = _cipher.DoFinal(input);
                return ret;

            }
            catch (CryptoException ex)
            {
                // throw new CryptoException(ex);
            }
            return null;
        }

        #endregion

    }

    public sealed class BCEngine
    {
        private static BCEngineCore instance;

        private BCEngine() { }

        public static BCEngineCore Instance
        {
            get
            {
                if (instance == null)
                    instance = new BCEngineCore();

                return instance;
            }
        }
    }

}
\$\endgroup\$
2
  • \$\begingroup\$ Self-answer is fine ;-) About the code... I'm still not entirely happy with the fields. Did you see the link to an article about pure functions in my answer? Let me post it here again: sitepoint.com/functional-programming-pure-functions you should really read this. It'll help you to refactor your code and get rid of the fields. \$\endgroup\$
    – t3chb0t
    Jun 14, 2017 at 20:12
  • \$\begingroup\$ Sorry I did not see it. I'll read you post an improved version! thank you so much. \$\endgroup\$
    – Diogo Luiz
    Jun 14, 2017 at 20:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.