[CryptoAuthLib provider] PsaCipherEncrypt and PsaCipherDecrypt implementation #563

akazimierskigl · 2021-11-30T08:54:36Z

Implementation for PsaCipherEncrypt and PsaCipherDecrypt operations.
Implementation of end-2-end test cases for Cipher operations.
Updated to newest parsec-client-rust which includes Cipher operations.

Signed-off-by: artur.kazimierski artur.kazimierski@globallogic.com

ionut-arm

Thanks for the patch!

One thing that would be nice to add is a test that imports an AES key, encrypts something with a crypto crate (maybe ring?) and then decrypts it with Parsec (or the other way around).

ionut-arm · 2021-11-30T15:04:27Z

src/providers/cryptoauthlib/cipher.rs

+
+impl Provider {
+    /// Generate random non-zero initialization vector
+    pub fn generate_iv(&self) -> Result<zeroize::Zeroizing<Vec<u8>>> {


This method isn't needed outside of the provider, is it? Would be better to limit it: pub(super)

Also, I don't think there's a need to return a Zeroizing here, since the IV isn't sensitive, you could just return Vec<u8>. That makes it easier later in the function (and in cipher_encrypt_internal), if you do

let random_bytes = self.psa_generate_random_internal(random_op)?.random_bytes.to_vec();

ionut-arm · 2021-11-30T15:18:43Z

src/providers/cryptoauthlib/cipher.rs

+            let random_bytes = self.psa_generate_random_internal(random_op)?.random_bytes;
+            match random_bytes.to_vec() != zero_iv {


I'm confused. If the RNG cannot generate random bytes, it should return an error. I'm not sure if this was discussed before, and it's probably partially our fault as the operation page in the Parsec book doesn't list "specific error codes". However, the spec does mention some. I guess the most appropriate would be PSA_ERROR_INSUFFICIENT_ENTROPY. Do you have any idea why the backend might return all zeroes?

ionut-arm · 2021-11-30T15:47:41Z

src/providers/cryptoauthlib/cipher.rs

+        let mut op_iv: Vec<u8> = Vec::new();
+        op_iv.extend_from_slice(&op.ciphertext[..CIPHER_IV_SIZE]);
+        let mut ciphertext: Vec<u8> = Vec::new();
+        ciphertext.extend_from_slice(&op.ciphertext[CIPHER_IV_SIZE..]);


I think you need to check that op.ciphertext is at least as long as CIPHER_IV_SIZE before you index into it, otherwise you might get a panic.

Also, getting the two parts in an easier way:

let mut op_iv = op.ciphertext.to_vec(); let ciphertext = op_iv.split_off(CIPHER_IV_SIZE);

akazimierskigl · 2021-12-03T09:35:02Z

I've delivered fixes for comments to the code and now I will try figure out your proposition about decrypting something which was not encrypted by parsec.

ionut-arm · 2021-12-03T10:28:42Z

Hi @akazimierskigl , I think the more pressing question is whether the psa_generate_random implementation from the CAL provider can actually return a vector of 0 bytes instead of completely random, while claiming that the operation was successful. If that's the case, I'd be inclined to move the retry and error-throwing into the generate_random implementation - we can hopefully include this in our 0.9.0 release.

akazimierskigl · 2021-12-03T10:59:15Z

@ionut-arm I will clarify this with our CAL expert. Previously I completely ignored the fact that generate_random can check itself if vector is cryptographically secure and I added this checking and loop just in case.

Meanwhile I have a first proposition for decryption tests. Instead of encrypting message by external crate we could use already prepared and proven set of IVs, plaintexts and ciphertexts from publications. The same how unit tests were done for rust-cryptoauthlib. https://github.com/PelionIoT/rust-cryptoauthlib/blob/master/cryptoauthlib/src/unit_tests/hw_backend_aes_cipher_stream.rs

ionut-arm · 2021-12-03T11:09:27Z

Sure, that sounds like a sensible plan. It's likely that if we expand the functionality to other providers in the future we'll add more stuff, but hardcoded values is enough for now.

akazimierskigl · 2021-12-06T11:41:27Z

Ok so I have an answer and it is possible to generate vector of zeros by CAL without any error returned. So we need to make decision how cover this extremely rare case.

ionut-arm · 2021-12-06T12:09:58Z

Ok so I have an answer and it is possible to generate vector of zeros by CAL without any error returned. So we need to make decision how cover this extremely rare case.

I'd say integrating the retry logic from this PR into psa_generate_random and sending back a PSA_ERROR_INSUFFICIENT_ENTROPY if that fails would work just fine.

akazimierskigl · 2021-12-10T08:05:20Z

@ionut-arm Good idea with only decrypt test cases. One algorithm didn't work properly so implementation of operations is changed too. Code is ready for re-review.

ionut-arm

Thanks for the update, and glad to hear a bug has been squashed before it was even born! 🐛

ionut-arm · 2021-12-13T10:02:33Z

e2e_tests/tests/per_provider/normal_tests/cipher.rs

+// Test Vector from:
+// https://csrc.nist.gov/CSRC/media/Projects/Cryptographic-Standards-and-Guidelines/documents/examples/AES_CFB.pdf
+// https://csrc.nist.gov/CSRC/media/Projects/Cryptographic-Standards-and-Guidelines/documents/examples/AES_OFB.pdf
+// https://csrc.nist.gov/CSRC/media/Projects/Cryptographic-Standards-and-Guidelines/documents/examples/AES_CTR.pdf
+// https://csrc.nist.gov/CSRC/media/Projects/Cryptographic-Standards-and-Guidelines/documents/examples/AES_ECB.pdf
+// https://csrc.nist.gov/CSRC/media/Projects/Cryptographic-Standards-and-Guidelines/documents/examples/AES_CBC.pdf


This is really nice, thank you for that!

ionut-arm · 2021-12-13T10:04:40Z

src/providers/cryptoauthlib/generate_random.rs

+                    if loop_count < 3 {
+                        continue;
+                    } else {
+                        return Err(ResponseStatus::PsaErrorInsufficientEntropy);


Could you also put an error! message here as well?

ionut-arm · 2021-12-13T10:08:07Z

src/providers/cryptoauthlib/generate_random.rs

+            // internal loop for vector size greater than buffer size
+            for _i in 0..call_count {
+                let mut buffer = Vec::with_capacity(rust_cryptoauthlib::ATCA_RANDOM_BUFFER_SIZE);
+                let err = self.device.random(&mut buffer);


Hmm, I just realised something - since a bigger request (i.e. one asking for more than ATCA_RANDOM_BUFFER_SIZE) can end up calling this random more than once, is it possible that one individual call returns all zeroes, but the others return something else? I'm just wondering if the loop checking for zeroes should instead be after each random call, instead of the whole combined thing.

"is it possible that one individual call returns all zeroes, but the others return something else?"

Yes, every other call of random should return different result so it is near impossible to get vector ATCA_RANDOM_BUFFER_SIZE * 2 of zeros. This is why I think checking only one time at the end if returned vector is all zeros is needed. Also I think when returned vector has at least one non zero value then is no longer considered "unsafe".

Hi @akazimierskigl - I am substituting for @ionut-arm here because he is now on leave until the new year. I am slightly lacking some context, so forgive me if I'm misunderstanding. To resolve this question I think I need to understand the underlying cause of a vector of zeroes from the back-end HRNG. Is it purely a probabilistic thing? Is a sequence of 32 zeroes as likely as any other result? If so, then I'm not sure why it needs any special handling, because you might just as well worry about about any other specific set of values. However, if the source of the zeroes is due to some sort of known transient failure or unready state in the HRNG, then I would agree with Ionut that we really should be checking it on every call to the hardware, because otherwise it is compromising the overall entropy of the result. I hope this makes sense.

"Is it purely a probabilistic thing? Is a sequence of 32 zeroes as likely as any other result?"
Yes.
"However, if the source of the zeroes is due to some sort of known transient failure or unready state in the HRNG"
It's not.
I've added handling in a first place only because it was required by cipher algorithm to use random non-zero initialization vector.

Okay, thanks. In that case it strikes me that we're worrying over nothing, really. It's a vanishingly-unlikely case. A single check on the collected final result is probably good enough, and for all practical purposes that check would never fail anyway.

Thanks @paulhowardarm, @akazimierskigl ! I misunderstood what the initial cause for the checks for 0 was, that's a lesson in communication for me :) I thought they'd result from an error in the CAL chip. Shall review once again, apologies for the massive delay this has caused.

Implementation of end-2-end test cases for Cipher operations. New parsec-client-rust commit. Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

…r fixes Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

…nerate_random error handling Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

…essage Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

ionut-arm

👍🏻 thanks!

ionut-arm reviewed Nov 30, 2021

View reviewed changes

akazimierskigl changed the title ~~DRAFT: [CryptoAuthLib provider] PsaCipherEncrypt and PsaCipherDecrypt implementation~~ [CryptoAuthLib provider] PsaCipherEncrypt and PsaCipherDecrypt implementation Dec 2, 2021

akazimierskigl force-pushed the calib-psa-cipher branch 2 times, most recently from d7c6ccf to a01f8e2 Compare December 3, 2021 08:54

akazimierskigl force-pushed the calib-psa-cipher branch from 30c15c6 to ff089c7 Compare December 9, 2021 14:55

ionut-arm reviewed Dec 13, 2021

View reviewed changes

akazimierskigl force-pushed the calib-psa-cipher branch 2 times, most recently from 63bba0c to 4911584 Compare December 20, 2021 08:26

akazimierskigl added 5 commits January 12, 2022 14:36

Implementation for PsaCipherEncrypt and PsaCipherDecrypt operations.

ea2ba57

Implementation of end-2-end test cases for Cipher operations. New parsec-client-rust commit. Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

removed unused match arm in backend handler, fmt fixes

7545dce

Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

updated parsec-client-rust commit, review fixes, formatting and linte…

0e7b1af

…r fixes Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

new decrypt e2e tests, fixed logic for ecb algorithm case, new psa_ge…

0bbfbb8

…nerate_random error handling Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

review fixes: remove unnecessary check for zero vector, added error m…

fb3408e

…essage Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

akazimierskigl force-pushed the calib-psa-cipher branch from 4911584 to fb3408e Compare January 12, 2022 13:37

key identity interface adaptation

ed63c4d

Signed-off-by: artur.kazimierski <artur.kazimierski@globallogic.com>

ionut-arm approved these changes Jan 14, 2022

View reviewed changes

ionut-arm merged commit e8a2c4e into parallaxsecond:main Jan 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CryptoAuthLib provider] PsaCipherEncrypt and PsaCipherDecrypt implementation #563

[CryptoAuthLib provider] PsaCipherEncrypt and PsaCipherDecrypt implementation #563

akazimierskigl commented Nov 30, 2021 •

edited

Loading

ionut-arm left a comment

ionut-arm Nov 30, 2021 •

edited

Loading

ionut-arm Nov 30, 2021

ionut-arm Nov 30, 2021

ionut-arm Nov 30, 2021

akazimierskigl commented Dec 3, 2021

ionut-arm commented Dec 3, 2021

akazimierskigl commented Dec 3, 2021

ionut-arm commented Dec 3, 2021

akazimierskigl commented Dec 6, 2021

ionut-arm commented Dec 6, 2021

akazimierskigl commented Dec 10, 2021

ionut-arm left a comment

ionut-arm Dec 13, 2021

ionut-arm Dec 13, 2021

ionut-arm Dec 13, 2021

akazimierskigl Dec 16, 2021

paulhowardarm Dec 17, 2021

akazimierskigl Dec 17, 2021

paulhowardarm Dec 17, 2021

ionut-arm Jan 4, 2022

ionut-arm left a comment

		let random_bytes = self.psa_generate_random_internal(random_op)?.random_bytes;
		match random_bytes.to_vec() != zero_iv {

[CryptoAuthLib provider] PsaCipherEncrypt and PsaCipherDecrypt implementation #563

[CryptoAuthLib provider] PsaCipherEncrypt and PsaCipherDecrypt implementation #563

Conversation

akazimierskigl commented Nov 30, 2021 • edited Loading

ionut-arm left a comment

Choose a reason for hiding this comment

ionut-arm Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akazimierskigl commented Dec 3, 2021

ionut-arm commented Dec 3, 2021

akazimierskigl commented Dec 3, 2021

ionut-arm commented Dec 3, 2021

akazimierskigl commented Dec 6, 2021

ionut-arm commented Dec 6, 2021

akazimierskigl commented Dec 10, 2021

ionut-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ionut-arm left a comment

Choose a reason for hiding this comment

akazimierskigl commented Nov 30, 2021 •

edited

Loading

ionut-arm Nov 30, 2021 •

edited

Loading