[WIP] Bug : Use random IV in EncryptDecryptUsingTpm

[shjala]

Description

The EncryptDecryptUsingTpm function was using a zero-initialized IV for AES-CFB encryption, which is cryptographically insecure.

This commit fixes the issue by generating a cryptographically random IV for each encryption and prepending the IV to the ciphertext (IV is public, not secret). We subsequently extract the IV from the first aes.BlockSize bytes during decryption.

PR dependencies

None.

How to test and validate this PR

Please describe how the changes in this PR can be validated or verified. For
example:

• If your PR fixes a bug, outline the steps to confirm the issue is resolved.
• If your PR introduces a new feature, explain how to test and validate it.

This will be used

1. to provide test scenarios for the QA team
2. by a reviewer to validate the changes in this PR.

The first is especially important, so, please make sure to provide as much
detail as possible.

If it's covered by an automated test, please mention it here.

Changelog notes

Fix AES-CFB encryption in EVE by using a random IV instead of a zero-initialized IV.

PR Backports

For all current LTS branches, please state explicitly if this PR should be
backported or not. This section is used by our scripts to track the backports,
so, please, do not omit it.

Here is the list of current LTS branches (it should be always up to date):

• 14.5-stable
• 13.4-stable

For example, if this PR fixes a bug in a feature that was introduced in 14.5,
you can write:

- 14.5-stable: To be backported.
- 13.4-stable: No, as the feature is not available there.

Also, to the PRs that should be backported into any stable branch, please
add a label stable.

Checklist

• I've provided a proper description
• I've added the proper documentation
• I've tested my PR on amd64 device
• I've tested my PR on arm64 device
• I've written the test verification instructions
• I've set the proper labels to this PR

For backport PRs (remove it if it's not a backport):

• I've added a reference link to the original PR
• PR's title follows the template

And the last but not least:

• I've checked the boxes above, or I've provided a good reason why I didn't
  check them.

Please, check the boxes above after submitting the PR in interactive mode.

[shjala]

Note to self: publish an advisory after fix got merged.

[eriknordmark]

This assumes that the controllers do not care about the length of the encrypted blob. Need to check whether that is the case. Does it place any other requirements on the controllers?

Also, presumably the decryption needs to handle the case when there is no prepended IV and use the old zeros approach so that one can transition.
(When it does that, does it always send the new encrypted blob to the controller meaning the IV would be added on the next boot of a device?)

Last but nost least, the API proto files which describe this as an encrypted key should have their comments expanded; key thing is to not assume anything about the length of the blob since the blob may or may not include an IV (for new vs old EVE versions).

[shjala]

This assumes that the controllers do not care about the length of the encrypted blob. Need to check whether that is the case. Does it place any other requirements on the controllers?

EVE-API places no restriction on the size. The implementation should threat this data as is and just stored it. Adam doesn't care about the size or content, I'll check to make sure the same is true for commercial controller.

Also, presumably the decryption needs to handle the case when there is no prepended IV and use the old zeros approach so that one can transition. (When it does that, does it always send the new encrypted blob to the controller meaning the IV would be added on the next boot of a device?)

Added fallback for backward compatibility on the decryption path.

Last but nost least, the API proto files which describe this as an encrypted key should have their comments expanded; key thing is to not assume anything about the length of the blob since the blob may or may not include an IV (for new vs old EVE versions).

I'll open a PR when this is ready to be merged.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 27.88%. Comparing base (2281599) to head (629eaf3).
⚠️ Report is 168 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/pillar/evetpm/encryptdecypt.go	0.00%	22 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5494      +/-   ##
==========================================
+ Coverage   19.52%   27.88%   +8.35%     
==========================================
  Files          19       19              
  Lines        3021     2331     -690     
==========================================
+ Hits          590      650      +60     
+ Misses       2310     1537     -773     
- Partials      121      144      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[justincormack]

You are making this very hard to change in the future, you should properly version the new interface. CFB encryption is no longer recommended (as it is not authenticated), and CFB modes are marked as deprectaed in Go standard library, so you are going to have to change this again, so you might as well make this easier.

[shjala]

You are making this very hard to change in the future, you should properly version the new interface. CFB encryption is no longer recommended (as it is not authenticated), and CFB modes are marked as deprectaed in Go standard library, so you are going to have to change this again, so you might as well make this easier.

I did consider that, with complete json output or magic bytes, but I need to first make sure how controller handles this filed.