A simple encryption scheme 2.0

Question

This is a follow up to my previous post.

I've used a BCrypt library to create a key from the passed in password. I then separated out the salt from the hash. The encryption is done using the salt concatenated to the hash, but only the hash is returned to the user. The salt is prepended to the encrypted data.

The key(salt + hash) is used to create a 512 bit hash using SHA3512. Each byte of data is XOR'ed with a byte of the key and a byte of the new hash. This byte of the hash is replaced by being XOR'ed with the byte of the key. When the end of the hash is reached, this byte array with all new values is then hashed and used for the next 64 bytes of data.

The Decryption separates out the salt and combines it with the passed in key to create the original hash and continue with the original encryption steps.

using System.Text;
using System.Linq;
using System.Collections.Generic;
using SHA3.Net;
using BCrypt.Net;
class Cipher
{
 public static IEnumerable<byte> Encrypt(string password, IEnumerable<byte> data,out string key)
 {
 if (password == null)
 {
 password = "";
 }
 if (data == null)
 {
 data = new byte[] { 0 };
 }
 var hash = MakeKey(password);
 key = new string(hash, 22, 31);
 string salt = new string(hash, 0, 22);
 return GetBytes(data, key,salt);
 }
 public static IEnumerable<byte> Decrypt(IEnumerable<byte> data, string key)
 {
 if (data == null)
 {
 data = new byte[] { 0 };
 }
 return GetBytes(data, key);
 }
 static IEnumerable<byte> GetBytes(IEnumerable<byte> data, string key, string salt = "")
 {
 int saltLength = 0;
 if(salt == "")
 {
 salt = new string(data.Take(22).Select(x => (char)x).ToArray());
 saltLength = 22;
 }
 else
 {
 foreach(char c in salt)
 {
 yield return (byte)c;
 }
 }
 key = $"{salt}{key}"; 
 var hash = Sha3.Sha3512().ComputeHash(Encoding.UTF8.GetBytes(key));
 int i = 0;
 foreach (var b in data.Skip(saltLength))
 {
 //modulo 64
 int hashIndex = (int)(((uint)i << 26) >> 26);
 if (i > 0 && hashIndex == 0)
 {
 hash = Sha3.Sha3512().ComputeHash(hash);
 }
 byte offset = (byte)key[i % key.Length];
 var retVal = (byte)(b ^ offset ^ hash[hashIndex]);
 hash[hashIndex] ^= offset;
 ++i;
 yield return retVal;
 }
 }
 static char[] MakeKey(string password) => BCrypt.Net.BCrypt.HashPassword(password).Skip(7).ToArray();
}

Here's what the encrypted data can look like:

{86, 110, 68, 77, 68, 75, 46, 116, 73, 99, 50, 53, 114, 57, 89, 47, 109, 70, 88, 105, 114, 117, 233, 83, 221, 231, 216, 45, 118, 223, 227, 185, 170, 177, 131, 154, 240, 170, 72, 192, 182, 112, 149, 208, 102, 235, 252, 156, 127, 189, 255, 240, 203, 152, 142, 189, 140, 64, 166, 196, 13, 254, 138, 159, 229, 199, 80, 80, 113, 162, 137, 87, 216, 59, 81, 140, 123, 199, 211, 75, 43, 62, 11, 3}

• aepot: Could you provide the link to NuGet package you're using to run this code? I can't find the package containing Sha3 class. You well described how it works but it doesn't clear what's its purpose? What do you expect from a Review?
• aepot: (int)(((uint)i << 26) >> 26) is that the same as i & 63 and i % 64?
• user33306: The shifts are a shortcut for modulo 64. I'm not home I'll add the using when I get back

Answer 1

Too fancy, too hard to read. The whole code can be compressed into something like BCrypt.Net.BCrypt.HashPassword(salt+password).ToArray()

Take a look at how to use this function here: https://jasonwatmore.com/post/2020/07/16/aspnet-core-3-hash-and-verify-passwords-with-bcrypt

Most importantly: there's no point in using mighty hashing like SHA3 and BCrypt only to use some made-up XOR encryption scheme. Use a standard encryption algorithm from a reputable library. If you don't trust any library or algorithm, then encrypt 2 times with two different algorithms, different keys and maybe using libraries from different vendors.

Beyond that, here are other issues I have with this code:

• Is .Skip(7) supposed to somehow remove the salt? You can't "remove" the salt, the string you get from that function is not your input anymore, it's a completely different string (aka hash).

• All these IEnumerable aren't efficient. Just use byte[].

• there's an attempt to be efficient with bitshifts. Just use % and leave it to the compiler. Given the rest of the code doesn't look like the most efficient implementation (IEnumerable, yield...) it makes no sense to sacrifice readability

• to that end - add other comments. It's not clear what GetBytes is supposed to achieve, given its generic name and lack of comments.

• if the password is null, empty, or string below a certain length - throw an exception. There's no point in encrypting something with an empty password

• name things properly. byte offset is not an offset (that would have been i % key.Length), it's a byte from the key

• use constants instead of magic values sprinkled across the code.

• get familiarized with common encryption libraries and try to follow their patterns, don't reinvent wheels that need no reinventing