Padding is broken

[Rob-Hague]

Originally this was just an exercise to further increase coverage for AesCipher when using padding, but it turns out padding is not implemented correctly at all: it is not added on block-sized lengths; and not removed during decryption.

So then this became a fix for that, with some unsatisfying logic in BlockCipher.Decrypt to accommodate existing usage where padding is specified but the plaintext is not actually padded.

However, upon reviewing usages in the library and reviewing #865 I want to discuss removing CipherPadding altogether. Here some notes:

1. CipherPadding is not used for SSH packets: instead packets are padded within the protocol to block-length.
2. The only usages in the library are decrypting keys in PrivateKeyFile, e.g.
   

   SSH.NET/src/Renci.SshNet/PrivateKeyFile.cs

   Lines 217 to 227 in 826222f

   	switch (cipherName)
   	{
   	case "DES-EDE3-CBC":
   	cipher = new CipherInfo(192, (key, iv) => new TripleDesCipher(key, new CbcCipherMode(iv), new PKCS7Padding()));
   	break;
   	case "DES-EDE3-CFB":
   	cipher = new CipherInfo(192, (key, iv) => new TripleDesCipher(key, new CfbCipherMode(iv), new PKCS7Padding()));
   	break;
   	case "DES-CBC":
   	cipher = new CipherInfo(64, (key, iv) => new DesCipher(key, new CbcCipherMode(iv), new PKCS7Padding()));
   	break;

   
   It is used (incorrectly) to allow decrypting non-block-sized inputs in a block-by-block manner in BlockCipher. We could equally just call Array.Resize to achieve the same effect.
3. The encrypted data in all packets sent in one direction SHOULD be considered a single data stream. This means that our cipher implementations need to preserve state across calls to Encrypt/Decrypt. But a call to these functions with padding specified would be expected to provide padding i.e. to "finalize" the state. So there are effectively two modes leading to logic like
   https://github.com/zybexXL/SSH.NET/blob/ada6ccbcc20e1e26463fbe22004556c9f884da57/src/Renci.SshNet/Security/Cryptography/Ciphers/AesCipher.cs#L225-L232 even though we never actually use the TransformFinalBlock logic in the library.

---

I think the options are:

1. Remove CipherPadding, PKCS7Padding etc. They are broken and effectively unused (or rather, used uneffectively). It's hard to imagine anyone depending on them. We then do not need a condition in the cipher implementations which tries to decide whether we are in a "stream" mode or not.
   This would disallow encrypting non-block-sized lengths, which we do not do in the library anyway.
2. Take this PR as a fix, with the awkward logic to satisfy existing call sites with the corrected decryption logic, and with the corrected logic for encryption which would change existing calls for padding of block-sized lengths (of which there are none in the library).
3. Do nothing and have a class called PKCS7Padding which neither pads nor depads correctly.

[zybexXL]

Hi @Rob-Hague , it seems we're on the same path again. I've mentioned the broken padding some days ago in 865 as I had seen the same issues you describe. I also worked on a fix this week - here's my code - I think it follows pretty much the same path as yours, with a slightly different implementation.

Just to drive the point home about how broken the padding is: the current BlockDecrypt() function is actually adding padding instead of removing it, which is only working because the SSH Session code actually never calls it with non-blocksized data (otherwise DecryptBlock() produces 100% garbage output).

While padding can be entirely removed for now, I think it's better to leave the (corrected) functionality in place as it may be needed in the future. The SSH Stream will always use the block-aligned BlockEncrypt/BlockDecrypt functions, but the more generic Encrypt/Decrypt functions are needed for secondary usages such as certificate decryption. These functions should fully support padding/unpadding, and should have 100% standard behavior. We could add Constructors that don't take a CipherPadding arg (defaulting to null) as almost all usages in SSH.Net set it to null anyway.

One other related issue is that some cipher modes should allow for non-blocksized data input/output, without padding, though this functionality is not needed for SSH.NET. Only CBC and ECB mandate a PKCS7 padding, while CTR/CFC/OFB do not (ie, you can encrypt 7 bytes and get 7 bytes back without padding). I've commited a change to CFB that allows that to happen, just as a proof of concept.

[zybexXL]

3. The encrypted data in all packets sent in one direction SHOULD be considered a single data stream. This means that our cipher implementations need to preserve state across calls to Encrypt/Decrypt.

I've considered this while I was doing #865 and I think this is not the case. The correct behavior is:

• Consecutive calls to EncryptBlock / DecryptBlock must preserve state - this is how the SSH Stream works
• Consecutive calls to Encrypt/Decrypt must not preserve state. Two consecutive calls with the same data input should return the same output.

This usually means that you cannot use the same Cipher instance to mix calls to both APIs, which I think is perfectly fine.
Consider what would happen if you call Encrypt() with 7 bytes, followed by EncryptBlock() with 16 bytes:

• if padding is enabled, the first call is padded to 16 bytes and the second one to 32 bytes - this won't work for SSH Stream as padding should not be added to the second call.
• if padding is disabled, the call to Encrypt() will fail under ECB or CBC, and under other modes a second call would produce undetermined/unreliable output - is the preserved IV supposed to be block-aligned or not? Should 9 bytes of unused IV be discarded or shifted for reuse on the second call?

This is the reason why the .Net APIs include a TransformFinalBlock, which adds the final padding and resets the IV back to the original value. Under .Net the Encrypt/Decrypt functions are deterministic, they don't preserve IV state.

[Rob-Hague]

• Consecutive calls to Encrypt/Decrypt must not preserve state. Two consecutive calls with the same data input should return the same output.

Doesn't that contradict https://github.com/zybexXL/SSH.NET/blob/ada6ccbcc20e1e26463fbe22004556c9f884da57/src/Renci.SshNet/Security/Cryptography/Ciphers/AesCipher.cs#L225-L232 ? It is using TransformBlock to preserve state (for the SSH data stream)

I agree with that code - the integration tests fail without it.

[zybexXL]

It is a partial contradiction. I did that so we can use ECB and CBC (which normally require padding) to encrypt data without padding or to decrypt without removing it. This is mostly to pass some test cases which are not expecting correct padding functionality. Some of the test PrivateKey/certs have the padding included when they should not, for instance, or vice versa.

As an aside - your (most excelent) AesCipherTest generator uses OpenSSH to generate the reference outputs, but it always calls it with the -nopad option. However, because you're piping the output to hd (hexdump) you're not seeing the STDERR errors that happen when using ECB and CBC with -nopad. OpenSSH warns that the output is wrong, and there are 16 missing bytes. The generated test can then only pass when calling Decrypt() with Padding.None, even though ECB/CBC require it. This is also why I added that code above, as TransformFinalBlock will always add padding.

With -nopad : 0x100 bytes output, StdErr message "bad decrypt":

>echo -n -e '\x99\x3a\xc9\x2b\xfb\x1d\x0e\x8e\x31\x0c\x96\x68\x4c\x46\x1d\xbb\xe1\x23\xc8\x99\x59\x90\x47\xcb\x63\x99\x5b\xf7\x91\x87\x44\x09\x2e\xff\xa4\x21\xdc\xc3\xd9\x89\xd7\x24\x0a\x32\x05\x36\x60\x25\xa4\x17\xda\xaf\x08\xbe\xc9\x08\xf3\xfe\xc7\x61\xc2\x17\xfd\xaa'
   | openssl enc -e -aes-128-ecb -K C78D3A4CA2FBDE1E493EC1348614C62D -nopad | C:\cygwin64\bin\hexdump.exe -C
bad decrypt
12032:error:0607F08A:digital envelope routines:EVP_EncryptFinal_ex:data not multiple of block length:crypto/evp/evp_enc.c:432:
00000000  8a 86 2e c3 f2 f6 a4 42  c6 f9 7a 51 8d 84 e7 06
00000010  b4 cf af 81 26 39 e1 97  33 7d a4 2a 35 84 2f b9
00000020  d6 6d 5d b2 82 54 00 f2  6e 33 78 76 b6 88 82 89
00000030  47 1d 47 6a 44 57 2f f9  5b d6 f9 74 42 f5 22 d3
00000040  c3 59 c7 da 7f 1d f8 4d  97 fa 17 22 57 65 da 18
00000050  d5 da 78 74 a0 0f 6a f4  ad b6 6f 3c 85 46 58 12
00000060  f2 e7 fb d2 8f 3f c4 82  9c e0 88 0e b0 0b 4f b3
00000070  58 49 87 65 8b 37 57 6c  0a 63 93 e5 05 98 2c 23
00000080  53 d7 17 02 c7 c9 38 08  28 74 d2 ce 96 89 b2 ad
00000090  4d 27 e1 b0 21 d0 e4 6d  bd 35 69 86 af 50 7e 4d
000000a0  f3 f6 aa cf 9f f2 1a 13  5f ab 19 95 03 1e 5a f6
000000b0  18 f3 82 5a cb 6e 87 6d  b2 f6 a3 60 e4 b0 e6 8f
000000c0  ae 50 90 81 a3 bc 0a 7c  11 a6 28 f1 b7 90 0b 12
000000d0  73 e4 06 c7 94 f6 2b 60  48 09 78 d7 9a 56 15 41
000000e0  48 3c 2d b7 45 7f 7e 70  99 d8 53 fd 7c 8e c7 c0
000000f0  64 40 c5 2e f8 1b bb ee  3b 21 f6 ba b1 a2 36 df

Without -nopad : 0x110 bytes output (padded), no error:

>echo -n -e '\x99\x3a\xc9\x2b\xfb\x1d\x0e\x8e\x31\x0c\x96\x68\x4c\x46\x1d\xbb\xe1\x23\xc8\x99\x59\x90\x47\xcb\x63\x99\x5b\xf7\x91\x87\x44\x09\x2e\xff\xa4\x21\xdc\xc3\xd9\x89\xd7\x24\x0a\x32\x05\x36\x60\x25\xa4\x17\xda\xaf\x08\xbe\xc9\x08\xf3\xfe\xc7\x61\xc2\x17\xfd\xaa'
   | openssl enc -e -aes-128-ecb -K C78D3A4CA2FBDE1E493EC1348614C62D | C:\cygwin64\bin\hexdump.exe -C
00000000  8a 86 2e c3 f2 f6 a4 42  c6 f9 7a 51 8d 84 e7 06
00000010  b4 cf af 81 26 39 e1 97  33 7d a4 2a 35 84 2f b9
00000020  d6 6d 5d b2 82 54 00 f2  6e 33 78 76 b6 88 82 89
00000030  47 1d 47 6a 44 57 2f f9  5b d6 f9 74 42 f5 22 d3
00000040  c3 59 c7 da 7f 1d f8 4d  97 fa 17 22 57 65 da 18
00000050  d5 da 78 74 a0 0f 6a f4  ad b6 6f 3c 85 46 58 12
00000060  f2 e7 fb d2 8f 3f c4 82  9c e0 88 0e b0 0b 4f b3
00000070  58 49 87 65 8b 37 57 6c  0a 63 93 e5 05 98 2c 23
00000080  53 d7 17 02 c7 c9 38 08  28 74 d2 ce 96 89 b2 ad
00000090  4d 27 e1 b0 21 d0 e4 6d  bd 35 69 86 af 50 7e 4d
000000a0  f3 f6 aa cf 9f f2 1a 13  5f ab 19 95 03 1e 5a f6
000000b0  18 f3 82 5a cb 6e 87 6d  b2 f6 a3 60 e4 b0 e6 8f
000000c0  ae 50 90 81 a3 bc 0a 7c  11 a6 28 f1 b7 90 0b 12
000000d0  73 e4 06 c7 94 f6 2b 60  48 09 78 d7 9a 56 15 41
000000e0  48 3c 2d b7 45 7f 7e 70  99 d8 53 fd 7c 8e c7 c0
000000f0  64 40 c5 2e f8 1b bb ee  3b 21 f6 ba b1 a2 36 df
00000100  a4 81 19 9c ff c0 bb 49  0a 83 40 00 3e 18 32 70

[zybexXL]

Many existing testcases are calling Encrypt/Decrypt under the expectation that this function ends up calling DecryptBlock/EncryptBlock. However after #865 this is no longer the case! So perhaps new/changed testcases are needed for DecryptBlock/EncryptBlock, which are still the main functions used during an SSH session.

[Rob-Hague]

This is mostly to pass some test cases which are not expecting correct padding functionality.

Calling TransformFinalBlock instead of TransformBlock in that code would give the same output with PaddingMode.None but the integration tests will fail. Your code is correct but I think your reasoning is confused. The current implementation preserves state across calls to Encrypt.

However, because you're piping the output to hd (hexdump) you're not seeing the STDERR errors

The script itself does not pipe to hd. It reads stderr and generates Assert.Fail code if it read anything. Did you get this example case from AesCipherTest?

Many existing testcases are calling Encrypt/Decrypt under the expectation that this function ends up calling DecryptBlock/EncryptBlock

I don't see such an expectation. EncryptBlock/DecryptBlock are not supposed to be used publicly: they are public because they are called from CipherMode.Encrypt. It's a weird design.

var cbc = new AesCipher(key, new CbcCipherMode(iv), null);

cbc.EncryptBlock(input); // This will be ECB, not CBC. It is not correct to call EncryptBlock

// It is only correct to call
cbc.Encrypt(input);

[zybexXL]

Calling TransformFinalBlock instead of TransformBlock in that code would give the same output with PaddingMode.None but the integration tests will fail. Your code is correct but I think your reasoning is confused. The current implementation preserves state across calls to Encrypt.

Yes, I added the change to call TransformBlock instead of TransformFinalBlock when padding is null so that it preserves state for the SSH stream mode. When Padding is not null, calling TransformFinalBlock will reset the IV.

The script itself does not pipe to hd. It reads stderr and generates Assert.Fail code if it read anything. Did you get this example case from AesCipherTest?

Yes I did. The point is that the testcase expects a given output with -nopad when that is not a valid case for ECB/CBC. I believe an SSH server using AES-CBC will add correct PKCS7 padding; the reason SSH.Net still works when using null Padding is likely because the SSH message contains a PaddingLength field which SSH.Net uses to strip all padding before after decryption. However some SSH servers might frown at the missing CBC padding in received messages?

I don't see such an expectation. EncryptBlock/DecryptBlock are not supposed to be used publicly: they are public because they are called from CipherMode.Encrypt. It's a weird design.

You're right, Session.cs calls Encrypt/Decrypt.

var cbc = new AesCipher(key, new CbcCipherMode(iv), null);

cbc.EncryptBlock(input); // This will be ECB, not CBC. It is not correct to call EncryptBlock

// It is only correct to call
cbc.Encrypt(input);

This is fixed in #865, you can safely call EncryptBlock/DecryptBlock and it will do the correct mode.

[Rob-Hague]

I believe an SSH server using AES-CBC will add correct PKCS7 padding

Where have you read this? I don't think it is true.

[zybexXL]

I didn't, I just think that it should because ECB and CBC mandate padding. Though it's fine to use them without padding, as long as both sides agree on the implementation.

[drieseng]

@Rob-Hague, off-topic: can I contact you directly? I have a JetBrains license (Resharper, ...) available for you to use, if you're interested. I contacted JetBrains and got three licenses (one for @WojciechNagorski, one for you and one for me).

[Rob-Hague]

Sure, thanks very much. You can use an email of mine in the commit history

[drieseng]

Sure, thanks very much. You can use an email of mine in the commit history

Send me an email, and I'll respond with the license URL.