2

I know we should never "roll our own crypto," but I want to understand the inner workings. I've written the following two encryption/decryption functions in PHP:

function encrypt($message) {

    $key = hash("sha256", base64_decode($secretpw64));    // get key from secret password
    $iv = random_bytes(16);                               // create an iv
    $output = openssl_encrypt($message, "AES-256-CBC", $key, 0, $iv);    // encrypt using openssl

    return implode(".", array(base64_encode($iv), $output));             // join iv.output
}

function decrypt($message) {

    $key = hash("sha256", base64_decode($secretpw64));    // get key from secret password
    $segs = explode(".", $message);    // separate the message
    $iv = base64_decode($segs[0]);     // get the iv
    $emsg = $segs[1];                  // get the encrypted message

    return openssl_decrypt($emsg, "AES-256-CBC", $key, 0, $iv);    // decrypt and return
}

I assume (which I know I should not) that random_bytes produces a crypto-safe random set of bytes, which according to the docs it does. I also use an Initialization Vector (iv) which causes the encryption to be different every time given the same input.

Is this code fundamentally flawed from a security point of view in any way? And, supposing it is not then is hash-then-encrypt (or similar) the next "level up" ?

12
  • 2
    You're working with relatively low-level crypto primitives. This always has the risk of minor mistakes which are more dangerous than you would have expected. The general advise here is to use a more high level library like libsodium, which is also available for php.
    – allo
    Apr 26, 2018 at 12:26
  • 3
    As for using hash-then-encrypt, you may want to simply use AES in GCM mode. Apr 26, 2018 at 12:32
  • 1
    @Antinous I am not 100% sure about how it works in PHP, but generally you may want to put the IV un-encoded to the openssl_encrypt and only encode it when it is in transport/storage. So you would do something like: return implode(".", array(base64_encode($iv), $output)); So don't encode it before using it. Of course decode here: $iv = base64_decode($segs[0]); Apr 26, 2018 at 12:34
  • 2
    Yes, you should always try to do as little as you can yourself to prevent mistakes. The last thing that may be improved, though this one is entirely dependent on your design and requirements is to replace the hash of the key with full PBKDF2. Using PBKDF2 with large amount of iterations massively improves security but also massively degrades performance. PS: It is meaningless if you have at least 256 bits of entropy in your password. Apr 26, 2018 at 12:40
  • 1
    @Antinous That is not 12 iterations, it is 2^12 = 4096 interations. It is a factor, so increasing it by one will double the time. Also, if you encrypt more than one string, you should derive the key separately, as that is the slowest part. Apr 26, 2018 at 12:53

1 Answer 1

1

There are some issues with your code.

$key = hash("sha256", base64_decode($secretpw64));

SHA-256 is very fast, and there is no salt here. An attacker can build a dictionary of (secretpw64, key) and get the key through a quick and dirty dictionary attack. As others have said, consider using a PBKDF. Even iterating the SHA for, say, 1000 times can be OK. For more information on how to derive a cryptographic key from a password, see this answer on Crypto.SE.

 $iv = random_bytes(16); 

You are assuming random_bytes is a cryptographically secure PRNG, but this is not something you can handwave. Be careful!

Moreover, take a look at how IVs are generated.

If the application crashes, or you shut down the OS, and start again, is the first IV always the same? It could happen.

If it does, an attacker can crash the application and get two messages with the same key and IV whenever he wants. No need to wait for 2^64 messages!

Confidentiality is not everything in security. Integrity and message authentication are also important in most applications.

As it turns out, CBC is also vulnerable to bit flipping (aka byte flipping) attacks.

Basically, you can flip a byte in the ciphertext so to flip a corresponding byte in the plaintext. This normally scrambles the previous block, but it becomes a practical attack when you want to modify the first block and you can change the IV or, in some cases, by bypassing input validation. For more details, see this Defcamp writeup.

Finally, you should also consider other, non cryptanalytic attacks, depending on your threat model, including:

  • theft/disclosure of encryption keys (e.g. from the RAM);
  • malicious modifications or hijacking of the RNG. If, for example, an attacker can replace your RNG with a weak PRNG, everything breaks up.
6
  • It's bit flipping not byte flipping, and it scrambles the previous block. Apr 26, 2018 at 18:40
  • Thanks for a great answer. Seems there are many holes to fill !
    – user156220
    Apr 26, 2018 at 18:42
  • @AndrolGenhald you're right, it scrambles the previous block. As for the name, sometimes it is known as byte flipping.
    – A. Darwin
    Apr 26, 2018 at 18:43
  • 2
    @antinous indeed. This is why, as a rule, you shouldn't roll your own. Of course, if you just want to experiment with your own data on your computer, that's fine, but if this is part of a product you want to sell or share, or if you are using it to protect other people's data, avoid rolling your own!
    – A. Darwin
    Apr 26, 2018 at 18:49
  • 1
    I see some references to "byte flipping" when I google it, but I think it's mostly a terminology mistake. What would it mean to flip a byte? Flipping a bit is obvious, it only has 2 possible values. Probably not worth arguing about though, it's pretty clear either way. Apr 26, 2018 at 18:49

You must log in to answer this question.