1

I'm reading an encrypted file and want to throw a specific error whenever the password is incorrect. Since decrypting with an incorrect password would simply succeed but return gibberish data, I'd like to determine if the password is correct.

The way I do this is I write a "magic value" to the encrypted file; let's say I write the value MaGiC to the file. Whenever I'm decrypting the file I ensure the first 5 bytes contain MaGiC. If not, the password used to decrypt the file must be wrong and so I throw an InvalidPasswordException.

MAGICHEADER = new byte[] { 0x4D, 0x61, 0x47, 0x69, 0x43 };      // "MaGiC"

using (var fs = File.OpenRead(path))
using (var algo = ...)
using (var cs = new CryptoStream(fs, algo.CreateDecryptor(), CryptoStreamMode.Read))
{
    var hdr = new byte[MAGICHEADER.Length]; // Buffer
    cryptoStream.Read(hdr, 0, hdr.Length);  // Read magic header from encrypted stream

    // If the decrypted data doesn't equal the magic header this means the password is wrong
    if (!MAGICHEADER.SequenceEqual(hdr))
        throw new InvalidPasswordException();

    // ... read rest of file
}

For brevity I've left out a lot of stuff (which algorithm, key, IV, blocksize, padding, mode etc.). For this particular case, what may be important: I'm using PKCS7 for padding (but I'm quite sure all padding will cause the described issue).

Now, on the calling side I wrote code like:

try {
    var data = ReadFile("C:\foo\bar", "INCORRECTPASSWORD");
    // ... Do stuff with data
} catch (InvalidPasswordException) {
    // Oh noes! Wrong password! Ask to enter password again
}

However, to my surprise the InvalidPasswordException was not caught; I did get an exception but of type CryptographicException: Padding is invalid and cannot be removed..

When I debug the issue I see the InvalidPasswordException being thrown. I also think I know what's going on: the InvalidPasswordException is thrown, the using causes the CryptoStream to be disposed and the CryptoStream determines that since we're not at the end of the stream what is left must be "padding" which then causes an exception. Or at least something along those lines.

To 'force' MY exception getting thrown I could do something like:

if (!MAGICHEADER.SequenceEqual(hdr)) {
    try { cs.FlushFinalBlock(); } catch { }  // YUK!!
    throw new InvalidPasswordException();
}

Or:

if (!MAGICHEADER.SequenceEqual(hdr)) {
    try { cs.Dispose(); } catch { }  // YUK!!
    throw new InvalidPasswordException();
}

I was, however, hoping for a more elegant solution. Is there a way I can initialize the CryptoStream not to throw on disposal? Or a way to 'close' the CryptoStream cleanly without it throwing? I've tried Close(), Clear() etc. but all of these methods also cause the CryptoGraphicException forcing me to wrap them in a try { ... } catch { ... } just before throwing my own InvalidPasswordException.

RobIII
  • 8,488
  • 2
  • 43
  • 93
  • Rather than a magic cookie why not prefix with a strong hash of the password? - then you can easily test without needing to begin decryption with an invalid password. Having a known fixed constant value at the start of all ciphertext may also have security implications. – Alex K. Aug 09 '18 at 13:43
  • I'd rather not have any information related to the password stored in the file (even if used with a strong hash) since the file contains sensitive information which may be stored for decades and who knows when a flaw in "strong hash X" is found. The "magic cookie" (as you call it) is also not at the beginning of the file (nor at a fixed position); it's described it as such just to keep it simple(r). The file is bigger and has some ("random length") data before the "magic cookie". – RobIII Aug 09 '18 at 13:45
  • Hmm, considering using @AlexK.'s suggestion anyway (storing a strong hash of the password in the 'unencrypted header' of the file). But that still leaves the question: how would I throw **anything** from that part of the code if all I'll ever get is a `CryptoGraphicException` at the callsite? – RobIII Aug 09 '18 at 14:05
  • You could do away with the `using` part and perform the resource handling yourself. In the end using `using` is candy for the developer, but it isn't *required* for anything. – Maarten Bodewes Aug 11 '18 at 11:37

0 Answers0