11
\$\begingroup\$

The "Cyraz" encryption algorithm was presented on SOPT community.

This algorithm makes a cypher using a private key given in byte[] format with any arbitrary length. It will then "swap" bytes on the data depedending on the key value. Additionally it will append a byte to end of the cipher, that byte is a hash of the key. Therefore this algorithm only increases message size by 1 byte.

The original algorithm is only available on pastebin. This implementation only handles data in the format byte[]. My job was to make it handle Streams as well, so the algorithm would be capable of handling files that do not fit in memory. When encrypting the data with stream interface I needed it to work with the array interface as well. To make that work I added an additional footer (2 bytes) that tells me if the data has been encrypted using Streams or not.

We are aware this cypher is easly breakable so please consider this to be a "toy" cypher

public class Cryraz {
    private byte[] key = new byte[0];
    private const int bufferLength = 4096;
    //footer to include when encrypting data as a stream
    private static readonly byte[] streamFooter = new byte[]{255, 255};

    /// <summary>
    /// Represents the standart operation for encrypting or decrypting types.
    /// </summary>
    public enum Operation {
        /// <summary>
        /// Byte decrypting algorithin.
        /// </summary>
        Decrypt,
        /// <summary>
        /// Byte encrypting algorithin.
        /// </summary>
        Encrypt
    }

    /// <summary>
    /// Creates a new <seealso cref="Cryraz"/> instance with specified key array.
    /// </summary>
    /// <param name="key">The new instance byte-based key.</param>
    public Cryraz(byte[] key){
        if(key == null){
            throw new ArgumentNullException(nameof(key));
        }
        this.key = key;
    }

    /// <summary>
    /// Updates the key. You cannot get it directly.
    /// </summary>
    public byte[] Key { set { key = value; } }

    internal static byte performKeyHash(byte[] key) {
        int x = 0;
        foreach (byte b in key) {
            x += b;
            x *= 1 + (b % 2);
        }
        return (byte)(x / key.Length);
    }

    internal static byte computePos(byte[] inputArray, int pos) {
        int length = inputArray.Length;
        if (pos <= length - 1) return inputArray[pos];

        int divisor = pos / length;
        pos -= length * divisor;
        return inputArray[pos];
    }

    /// <summary>
    /// Validates if the file can be decrytped with the current key
    /// </summary>
    /// <param name="data">The encrypted data</param>
    /// <returns>True if the file was encrypted with streams</returns>
    private bool Verify(byte[] data){
        if (data == null) 
            throw new ArgumentNullException("entryByteData", "Input data cannot be nothing.");

        var length = data.Length;
        var isStreamEncrypted = data.Length >= streamFooter.Length + 1;
        for(int i = 1; i <= streamFooter.Length; ++i){
            isStreamEncrypted &= data[length - i] == streamFooter[streamFooter.Length - i];
        }

        var checkSum = isStreamEncrypted ? data[length - streamFooter.Length - 1] : data[length - 1];

        byte hash_x = performKeyHash(key);
        if (hash_x != checkSum)
            throw new System.Security.SecurityException("Invalid key for this data."); 

        return isStreamEncrypted;
    }

    private Stream DecryptData(Stream reader, bool createFile, bool verify){

        var isStream = !verify; 
        if(verify){
            reader.Position = reader.Length - streamFooter.Length - 1;
            var footer = new byte[streamFooter.Length + 1];
            reader.Read(footer, 0, footer.Length);
            isStream = Verify(footer);
            reader.Position = 0;
        }
        if(!isStream){
            var memStream = new MemoryStream();
            reader.CopyTo(memStream);
            var data = memStream.ToArray();
            return DecryptData(ref data, createFile);
        }

        var writer = createFile 
            ? (Stream)File.Create(Path.GetTempFileName()) 
            : (Stream)new MemoryStream();
        var buffer = new byte[bufferLength];
        int bytes;
        while((bytes = reader.Read(buffer, 0, buffer.Length)) > 0){
            Array.Resize(ref buffer, bytes);
            performData(ref buffer, Operation.Decrypt, 0);
            writer.Write(buffer, 0, bytes);
        }
        if(verify){
            writer.SetLength(writer.Length - streamFooter.Length - 1); 
        }
        writer.Flush();
        writer.Position = 0;
        return writer;
    }

    public Stream DecryptData(Stream stream, bool createFile = false) {
        return DecryptData(stream, createFile, true);
    }

    /// <summary>
    /// Decrypts an given single dimension byte-array with this class key.
    /// </summary>
    /// <param name="entryByteData">The input byte-array to decrypting.</param>
    /// <returns>A temporary file path containing the decrypted data if createFile is true. 
    /// Null if createFile is false</returns>
    public Stream DecryptData(ref byte[] data, bool createFile = false) {
        var isStreamEncrypted = Verify(data);

        if(!isStreamEncrypted){
            performData(ref data, Operation.Decrypt, 0);
            data[data.Length - 1] = 0;
            Array.Resize(ref data, data.Length - 1);
            var writer = createFile 
                ? (Stream)File.Create(Path.GetTempFileName()) 
                : (Stream)new MemoryStream(data);
            return writer;
        }else{
            Array.Resize(ref data, data.Length - streamFooter.Length - 1);
            var stream = DecryptData(new MemoryStream(data), createFile, false);
            stream.Read(data, 0, data.Length);
            stream.Position = 0;
            return stream;
        }
    }

    /// <summary>
    /// Encrypts an given single dimension byte-array with this class key.
    /// </summary>
    /// <param name="entryByteData">The input byte-array to encrypting.</param>
    public void EncryptData(ref byte[] entryByteData, bool createFile = false) {
        if (entryByteData == null) 
            throw new ArgumentNullException("entryByteData", "Input data cannot be nothing.");

        performData(ref entryByteData, Operation.Encrypt, 0);
        Array.Resize(ref entryByteData, entryByteData.Length + 1);

        byte hash_x = performKeyHash(key);
        entryByteData[entryByteData.Length - 1] = hash_x;
    }

    public Stream EncryptData(Stream stream, bool createFile = false) {
        if (stream == null) 
            throw new ArgumentNullException("stream", "Input data cannot be nothing.");

        var writer = createFile 
            ? (Stream)File.Create(Path.GetTempFileName()) 
            : (Stream)new MemoryStream();

        byte hash = performKeyHash(key);

        var buffer = new byte[bufferLength];
        int bytes;
        while((bytes = stream.Read(buffer, 0, buffer.Length)) > 0){
            Array.Resize(ref buffer, bytes);
            performData(ref buffer, Operation.Encrypt, 0);
            writer.Write(buffer, 0, bytes);
        }
        writer.WriteByte(hash);
        writer.Write(streamFooter, 0, streamFooter.Length);
        writer.Position = 0;
        return writer;
    }

    internal void performData(ref byte[] entryByteData, Operation op, int keyOffset) {
        for (int i = 0; i <= entryByteData.Length - 1; i++) {
            int pos = computePos(key, keyOffset + i);
            int a = entryByteData[i];
            if (op == Operation.Encrypt) {
                a += pos;
            } else a -= pos;
            entryByteData[i] = ((byte)a);
        }
    }
}

And a sample to test it. (Not for review)

public class Sample{
    public static void Main(string[] args)
    {
        var cypher = new Cryraz(new byte[]{10, 11, 12});
        var original = Enumerable.Range(0 , 1024 * 1024).
            Select(i => (byte)(i % (byte.MaxValue + 1)))
            .ToArray();
        var data = new byte[original.Length];
        Array.Copy(original, data, original.Length);

        var sum = data.Sum(d => (long)d);
        cypher.EncryptData(ref data);
        cypher.DecryptData(ref data, false);
        if(!Enumerable.SequenceEqual(original, data)){
            Console.WriteLine("Can not encrypt and desencrypt arrays");
        }else{
            Console.WriteLine("Can encrypt and desencrypt arrays");
        }

        var stream = cypher.EncryptData(new MemoryStream(data)) as MemoryStream;
        var memStream = cypher.DecryptData(stream) as MemoryStream;
        data = memStream.ToArray();
        if(!Enumerable.SequenceEqual(original, data)){
            Console.WriteLine("Can not encrypt and desencrypt streams");
        }else{
            Console.WriteLine("Can encrypt and desencrypt streams");
        }

        memStream = cypher.EncryptData(new MemoryStream(data)) as MemoryStream;
        data = memStream.ToArray();
        cypher.DecryptData(ref data);
        if(!Enumerable.SequenceEqual(original, data)){
            Console.WriteLine("Can not encrypt streams and desencrypt arrays");
        }else{
            Console.WriteLine("Can encrypt streams and desencrypt arrays");
        }

        cypher.EncryptData(ref data);
        memStream = cypher.DecryptData(new MemoryStream(data)) as MemoryStream;
        data = memStream.ToArray();
        if(!Enumerable.SequenceEqual(original, data)){
            Console.WriteLine("Can not encrypt arrays and desencrypt streams");
        }else{
            Console.WriteLine("Can encrypt arrays and desencrypt streams");
        }
    }
}
\$\endgroup\$
0

4 Answers 4

17
\$\begingroup\$

You are using in your code some non-standard loops.

for (int i = 1; i <= streamFooter.Length; ++i)

There should be a comment explaining why this loop is starting at 1 and not as expected at 0. I guess there is a reason for this. Even better would be to define a const for the 1 that could probably explain this offset without a comment.


for (int i = 0; i <= entryByteData.Length - 1; i++)

I must admit I've never seen a for loop written like this. We usually don't write the - 1 but use simpler < condition rather then <=. Most APIs consider the upper bound as exclusive (like Enumerable.Repeat or Random.Next). This is a more popular form:

for (int i = 0; i < entryByteData.Length; i++)

var memStream = new MemoryStream();

You are not disposing your streams. (I know, there are rumors that some disposables don't have to actually be be disposed, but it's a good practice to always do it. The implementation might change.)


 if (verify)

When I see the verify argument I wonder verify what?. It's not expressive enough.


 internal void performData(ref byte[] entryByteData, Operation op, int keyOffset)

You use a nice descriptive name for the first argument, but only two letters for the operation :( The Operation enum needs an upgrade too. What operation?


 else a -= pos;

Outch! Not only the {} are missing but the variable name is just an a.

\$\endgroup\$
4
\$\begingroup\$
┻━┻ ︵}else{︵ ┻━┻

Some white spaces between operators and braces would be nice. A minor thing, but it really helps reading the code.

\$\endgroup\$
3
\$\begingroup\$

I'm not sure if there is something rather special going on with C# but this part of your code caught my attention:

var isStream = !verify; 
if(verify){
    reader.Position = reader.Length - streamFooter.Length - 1;
    var footer = new byte[streamFooter.Length + 1];
    reader.Read(footer, 0, footer.Length);
    isStream = Verify(footer);
    reader.Position = 0;
}
if(!isStream){
    var memStream = new MemoryStream();
    reader.CopyTo(memStream);
    var data = memStream.ToArray();
    return DecryptData(ref data, createFile);
}

I can't connect the meanings of the variables from their names, also it looks like if one if is executed, the other one must be as well.

\$\endgroup\$
2
  • 2
    \$\begingroup\$ The first if alters the value of isStream so it's not the case that executing the first one implies executing the second. If verify is false, then !isStrream is false, so neither if will trigger. But if verify is true, then the line isStream = Verify(footer) determines whether the second if triggers. \$\endgroup\$ Commented Jul 10, 2017 at 12:23
  • \$\begingroup\$ @DavidRicherby Oh, I missed that. The second if could be nested inside the first to make this dependency clear. (Although if nesting is really more readable in this case is debatable) \$\endgroup\$
    – kalsowerus
    Commented Jul 10, 2017 at 12:28
0
\$\begingroup\$

One thing I'd like to see in this is 2 types of comments:

  1. just general comments which clarify the trickier stuff: ternaries, array manipulations, loops and ifs. State what you are doing and why you are doing it in human-readable text. This helps future maintainers figure out what everything does without having to debug it.

  2. A BIG and OBVIOUS disclaimer that this algorithm is not intended for production usage and should NEVER, EVER be used in a live environment . In fact, I'd appreciate it if you were to edit your question ASAP and add a disclaimer like that (no, the current one line disclaimer right above the reviewable segment isn't obvious enough). It doesn't have to be in the code itself (I think rules disallow making changes to code in your question anyway), but at least add it at the top of your question. Because I can almost guarantee you that no matter how much you think it won't happen, someone WILL google "encryption", come across this algorithm and use it for their company software without considering the merits. Especially since this is posted on a site linked to Stack Overflow. Think that won't happen? Nissan did it. There are millions of hits on Github for Stack Overflow. Even codereview.stackexchange.com has thousands and thousands of hits on Github.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ I left such disclaimer We are aware this cypher is easly breakable so please consider this to be a "toy" cypher \$\endgroup\$ Commented Jul 10, 2017 at 13:33
  • \$\begingroup\$ @BrunoCosta I've seen that you left such a disclaimer, but I don't consider it to stand out compared to the rest of the question. it's just a single line of text that appears in the middle of your post without any indication that this line is an important disclaimer about the intent of this code. Yes, it's there, but I personally don't consider it visible enough in this question, and you should definitely add it as a documentation comment in the code itself. \$\endgroup\$
    – Nzall
    Commented Jul 10, 2017 at 13:45
  • \$\begingroup\$ also all content/code posted on SE is licensed under CC-BY-SA which states the author is not liable. Aka - use at your own risk. \$\endgroup\$ Commented Jul 10, 2017 at 13:59

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.