App to Encrypt and Decrypt file

Question

public sealed class Class1
{
    Class1()
    {

    }
    private static readonly object padlock = new object();
    private static Class1 instance = null;
    public static Class1 Instance
    {
        get
        {
            if (instance == null)
            {
                lock (padlock)
                {
                    if (instance == null)
                    {
                        instance = new Class1();
                    }
                }
            }
            return instance;
        }
    }

    public bool EncryptFile(string path)
    {
        string ext = Path.GetExtension(path).Replace(".","");
        string filename = Path.GetFileNameWithoutExtension(path);

        byte[] b= System.Text.ASCIIEncoding.ASCII.GetBytes(ext);
        string encrypted = Convert.ToBase64String(b);
        var newPath = path.Replace(filename+"."+ext, filename+"_"+ encrypted + ".vik");

        try
        {
            string password = @"vik12389"; // Your Key Here
            UnicodeEncoding UE = new UnicodeEncoding();
            byte[] key = UE.GetBytes(password);

            string cryptFile = newPath;
            FileStream fsCrypt = new FileStream(cryptFile, FileMode.Create);


            RijndaelManaged RMCrypto = new RijndaelManaged();

            CryptoStream cs = new CryptoStream(fsCrypt,
                RMCrypto.CreateEncryptor(key, key),
                CryptoStreamMode.Write);

            FileStream fsIn = new FileStream(path, FileMode.Open);

            int data;
            while ((data = fsIn.ReadByte()) != -1)
                cs.WriteByte((byte)data);
            fsIn.Close();
            cs.Close();
            fsCrypt.Close();
            File.Delete(path);

            return true;

        }
        catch(Exception e)
        {
            return false;
        }
    }

    public bool DecryptFile(string path)
    {
        string ext = Path.GetExtension(path).Replace(".", "");
        string filename = Path.GetFileNameWithoutExtension(path);

        string[] splitFilename = filename.Split('_');

        byte[] b = Convert.FromBase64String(splitFilename[1]);

        string decryptedfileext = System.Text.ASCIIEncoding.ASCII.GetString(b);

        var newPath = path.Replace(filename + ".vik", splitFilename[0] + '.'+ decryptedfileext);
        try
        {
            string password = @"vik12389"; // Your Key Here

            UnicodeEncoding UE = new UnicodeEncoding();
            byte[] key = UE.GetBytes(password);


            FileStream fsCrypt = new FileStream(path, FileMode.Open);

            RijndaelManaged RMCrypto = new RijndaelManaged();

            CryptoStream cs = new CryptoStream(fsCrypt,
                RMCrypto.CreateDecryptor(key, key),
                CryptoStreamMode.Read);

            FileStream fsOut = new FileStream(newPath, FileMode.Create);

            int data;
            while ((data = cs.ReadByte()) != -1)
                fsOut.WriteByte((byte)data);

            fsOut.Close();
            cs.Close();
            fsCrypt.Close();

            return true;

        }
        catch (Exception e)
        {
            return false;
        }

    }

}

My Project Link on GitHub :https://github.com/vivianm94/EncryptFile. how do I improve my code any suggestion.any features that could be added to the project

Answer 1

So, the most interesting bits about this code are the wildly differing styles throughout. It seems as it's almost copy-and-pasted from several sources. The reason I bring this up is because consistency is a hallmark of clean and maintainable code. A few things I noticed:

1. There's a debunked use of "double-checked locking" to attempt to use a lazily-instantiated singleton. Two things with this: A. This class doesn't have state and could be static to begin with, and thus do not need to worry about instances at all, and B. There's a very easy way these days to have a lazy singleton: the Lazy<T> class. You'll see its usage below.

2. There's a mish-mash of ASCIIEncoding.ASCII and new UnicodeEncoding(). The canonical way to access predefined encodings is through static members of the Encoding class: Encoding.ASCII and Encoding.Unicode.

3. Path.Combine() is wisely used to, of course, combine paths. But then string concatenation is used to pop an extension on the file name. Path.ChangeExtension() exists for just this task.

4. x.Close() isn't needed if you wrap your IDisposable resources in using blocks. This also has the benefit of guaranteeing disposal/closing in the case of an exception (it's syntactic sugar for a try..finally construct`.

5. There's a mix of using and not using var for variable declarations. Since the latter is less than the former, standardize on just using the actual type name for declarations.

6. I added two more - you don't need to read/write the file byte by byte. These are streams for a reason, and .NET gives us a handy method CopyTo to take care of the details. Also, CreateEncrpytor and CreateDecryptor return IDisposable-implementing classes, so I extracted them into their own usings.

Putting all that together, the class now looks like (note that I went with Lazy<T> rather than static class):

public sealed class Class1
{
    private static readonly Lazy<Class1> instance = new Lazy<Class1>(() => new Class1());
    public static Class1 Instance => instance.Value;

    public bool EncryptFile(string path)
    {
        string ext = Path.GetExtension(path).Replace(".", string.Empty);
        string filename = Path.GetFileNameWithoutExtension(path);

        byte[] b = Encoding.ASCII.GetBytes(ext);
        string encrypted = Convert.ToBase64String(b);
        string newPath = path.Replace(Path.ChangeExtension(filename, ext), filename + "_" + encrypted + ".vik");

        try
        {
            string password = @"vik12389"; // Your Key Here
            byte[] key = Encoding.Unicode.GetBytes(password);

            string cryptFile = newPath;
            using (FileStream fsCrypt = new FileStream(cryptFile, FileMode.Create))
            using (RijndaelManaged RMCrypto = new RijndaelManaged())
            using (ICryptoTransform ct = RMCrypto.CreateEncryptor(key, key))
            using (CryptoStream cs = new CryptoStream(fsCrypt,
                ct,
                CryptoStreamMode.Write))
            using (FileStream fsIn = new FileStream(path, FileMode.Open))
            {
                fsIn.CopyTo(cs);
            }

            File.Delete(path);

            return true;

        }
        catch(Exception e)
        {
            return false;
        }
    }

    public bool DecryptFile(string path)
    {
        string ext = Path.GetExtension(path).Replace(".", string.Empty);
        string filename = Path.GetFileNameWithoutExtension(path);

        string[] splitFilename = filename.Split('_');

        byte[] b = Convert.FromBase64String(splitFilename[1]);

        string decryptedfileext = Encoding.ASCII.GetString(b);

        string newPath = path.Replace(Path.ChangeExtension(filename, "vik"), splitFilename[0] + '.'+ decryptedfileext);
        try
        {
            string password = @"vik12389"; // Your Key Here

            byte[] key = Encoding.Unicode.GetBytes(password);

            using (FileStream fsCrypt = new FileStream(path, FileMode.Open))
            using (RijndaelManaged RMCrypto = new RijndaelManaged())
            using (ICryptoTransform ct = RMCrypto.CreateDecryptor(key, key))
            using (CryptoStream cs = new CryptoStream(fsCrypt,
                ct,
                CryptoStreamMode.Read))
            using (FileStream fsOut = new FileStream(newPath, FileMode.Create))
            {
                cs.CopyTo(fsOut);
            }

            return true;

        }
        catch (Exception e)
        {
            return false;
        }

    }

}

Answer 2

I'll not go into the C# coding part as much, as Jesse already covered that part.

It' not clear to me why any class should be called Class1. That a bad name. Even if it is just used for a particular function, then it should be named after this particular function.

It's not a good idea to have two instances instance and Instance.

---

There is nothing to indicate that a new filename is generated, nor how it will be generated. And possibly that's a good thing, as none of it makes sense. The extension is not encrypted but encoded. The filename + extension is replaced by calling replace, which means that if the file + extension is present elsewhere that the function will fail. Furthermore, there are a lot of changes to extension and filename just to create it back again.

The key should be configurable, and not be in code. Keys are not strings, they should be bytes where the bytes are not predictable to an adversary. Strings cannot hold any byte value, and therefore the precondition fails. Even worse, if you use Unicode then you will get 2 bytes for each character. If those characters are common ASCII then one of those bytes will simply be valued zero. Basically you have about 33% of the key space left.

Even worse, people are terrible at creating strong passwords, so the amount of entropy and password space to search is likely to be low enough to make the scheme easy to break. If you want to use a password, then use e.g. PBKDF2, a salt and a high iteration count. As it is, your password will be easier to crack than single DES.

You are using the same key as IV. The key should never ever be used as IV. The IV is considered public, and the key is not. It is unlikely that the platform will even try to keep the IV a secret. For instance, such values are more likely to be kept unprotected in memory. Any CPA secure cipher requires a unique IV / key pair, and CBC - which you're using - requires more: a fully unpredictable IV. As you fail to provide that it will be easy to guess information within files that start with the same plaintext.

Generally we nowadays try and use an authenticated mode such as GCM by default. If that's not available to you, then calculating an additional HMAC over IV and ciphertext can be used to generate an authentication tag. This tag can be verified before the data is decrypted again. This requires you to store the IV with the ciphertext. Note that verification should be performed using a time-constant compare.

Changes of the file can be detected if you use an authentication tag. Currently, your decryption may successfully finish even if the wrong key is used. PKCS#7 padding - which is used by default for CBC mode in .NET (and most other frameworks / crypto libs) - has about a 1 in 256 chance to unpad successfully given random bytes. In that case you would get a random plaintext, and no error whatsoever.

I really don't like the false that is returned if anything fails. The possible error conditions are just swept under the rug. So there is no chance of cleaning up problems when something fails.

Answer 3

As others have covered the technical details of the encodings and encryption, here's example of a more object-oriented approach (via inheritance rather than encapsulation). As encrypting and decrypting have a lot of common elements, this example also aims to reduce code duplication.

class App_FileEncryption
{
    public void Run()
    {
        var originalFile = new UnencryptedFile(@"c:\temp\myFile.txt", "vik123");
        originalFile.Encrypt();

        var encryptedFile = new EncryptedFile(@"c:\temp\myFile_encrypted.txt", "vik123");
        encryptedFile.Decrypt();
    }
}

public abstract class File
{
    protected string path;
    protected string password;

    protected FileStream fsCrypt => new FileStream(path, FileMode.Open);
    protected byte[] key => new UnicodeEncoding().GetBytes(password);
    protected RijndaelManaged RMCrypto = new RijndaelManaged();
    protected string targetPath => getTargetPath();
    protected string ext => Path.GetExtension(path).Replace(".", "");
    protected string filename => Path.GetFileNameWithoutExtension(path);

    protected abstract CryptoStream cs { get; }

    protected File(string path, string password)
    {
        this.path = path;
        this.password = password;
    }

    protected abstract string getTargetPath();
}

public class EncryptedFile : File
{
    protected override CryptoStream cs =>
        new CryptoStream(fsCrypt, RMCrypto.CreateEncryptor(key, key), CryptoStreamMode.Read);

    public EncryptedFile(string path, string password) : base(path, password)
    {
    }

    public void Decrypt()
    {
        ///do stuff, output to targetPath
    }

    protected override string getTargetPath()
    {
        var splitFilename = filename.Split('_');
        var b = Convert.FromBase64String(splitFilename[1]);
        var decryptedfileext = ASCIIEncoding.ASCII.GetString(b);
        return path.Replace(filename + ".vik", splitFilename[0] + '.' + decryptedfileext);
    }
}

public class UnencryptedFile : File
{
    protected override CryptoStream cs =>
        new CryptoStream(fsCrypt, RMCrypto.CreateDecryptor(key, key), CryptoStreamMode.Read);

    public UnencryptedFile(string path, string password) : base(path, password)
    {
    }

    public void Encrypt()
    {
        ///do stuff, output to targetPath
    }

    protected override string getTargetPath()
    {
        var b = ASCIIEncoding.ASCII.GetBytes(ext);
        var encrypted = Convert.ToBase64String(b);
        return path.Replace(filename + "." + ext, filename + "_" + encrypted + ".vik");
    }
}