Add U and V key handlers, and decrypt + run payload #197

lkatalin · 2021-06-07T21:37:28Z

I'll need to figure out how to avoid using

#![feature(string_remove_matches)]
#![feature(mutex_unlock)]

in order for CI to pass as these are unstable features. The former should be easy, the latter maybe not so much.

There are a few details still to fill in re: running the decrypted payload, checking if it's a zip file, etc.
~~The decrypted payload appears to be the excludes list; there is no autorun.sh sent.~~

Resolves #97
Resolves #203
Resolves #221
Resolves #226
Resolves #202
Resolves #99

puiterwijk · 2021-06-14T19:59:47Z

src/keys_handler.rs

+    let (iv, rest) = payload.split_at(AES_BLOCK_SIZE);
+    let (ciphertext, tag) = rest.split_at(rest.len() - AES_BLOCK_SIZE);
+
+    // note: it's unclear in the python codebase whether this is AES 128, but 128 seems


So, you could just automatically derive the Cipher value.

Since I'm pretty sure that for cryptography, the default depends on the length of the key.

(Also, given the fact that ukey, vkey and sym_key are 32 bytes, that means that this is 256 bit AES.)

puiterwijk · 2021-06-14T20:01:08Z

src/keys_handler.rs

+    // to be the default
+    // https://cryptography.io/en/latest/hazmat/primitives/symmetric-encryption/#symmetric-encryption-modes
+    decrypt_aead(
+        Cipher::aes_128_gcm(),


Suggested change

Cipher::aes_128_gcm(),

match symm_key.len() {

16 => Cipher::aes_128_gcm(),

32 => Cipher::aes_256_gcm(),

_ => return Err(....),

}

Terrific idea.

puiterwijk · 2021-06-14T20:04:28Z

src/keys_handler.rs

+
+use crate::{error::Error, QuoteData};
+
+// This value comes from and must match the Python codebase


I think a better reference for this constant could be FIPS 197, Introduction: This standard specifies the Rijndael algorithm ([3] and [4]), a symmetric block cipher that can process data blocks of 128 bits, .... (128 bits / 8 bits-per-byte gives you 16)

puiterwijk · 2021-06-14T20:18:12Z

src/keys_handler.rs

+        Cipher::aes_128_gcm(),
+        symm_key,
+        Some(iv),
+        &[0u8], // todo: check that we don't need additional auth data here?


There is no additional data: that would be a call like encryptor.authenticate_additional_data(associated_data) in the encrypt function, or decryptor.authenticate_additional_data(authenticated_data) in decrypt.

puiterwijk · 2021-06-14T20:19:45Z

src/keys_handler.rs

+    )
+}
+
+pub(crate) unsafe fn try_combine_keys() -> bool {


All the unsafe stuff makes me feel a bit worried. Would it be a possibility to put this data as a fully locked struct field in the overall HTTP state?

Good point, I've put this in the app_data now.

lkatalin · 2021-07-08T22:23:13Z

@puiterwijk @lukehinds @ashcrow This is updated, I'm able to run a small script with it. Still having the problem referenced in #202 but that is the fail point right now after the payload script runs.

lkatalin · 2021-07-09T20:25:14Z

I can also split these commits out into their own PRs if that makes it easier to review; just let me know.

lkatalin · 2021-07-12T14:31:10Z

@puiterwijk @lukehinds @ashcrow This is updated, I'm able to run a small script with it. Still having the problem referenced in #202 but that is the fail point right now after the payload script runs.

Happy to say #202 is fixed now in this PR. To use the revocation feature and ensure that the revocation certificate is sent correctly, you must run the tenant with the following command (example directory and script shown here):

[root@localhost ~]# ls /root/senddir/
autorun.sh

[root@localhost ~]# keylime_tenant -v 127.0.0.1 -t 127.0.0.1 --uuid d432fbb3-d2f1-4a97-9ef7-75bd81c00000 
--allowlist /root/allowlist.txt --include /root/senddir --cert /root/ca --exclude /root/excludes.txt -c add

... where autorun.sh is any script you want for testing. You must also ensure that the following values are set in /etc/keylime.conf:

[cloud_agent]
...
extract_payload_zip = True
...
listen_notfications = True
...
payload_script=autorun.sh

I'll include this in some documentation.

Signed-off-by: Lily Sturmann <[email protected]>

The Python code manually forces the IMA PCR (PCR 10) to be added from the SHA1 bank, even when that is not the bank used for the other PCRs. Because of the way it builds up the list of PCRs to check, we must also do this the same way. Signed-off-by: Lily Sturmann <[email protected]> Co-authored-by: Patrick Uiterwijk <[email protected]>

Signed-off-by: Lily Sturmann <[email protected]>

ashcrow · 2021-07-12T16:46:17Z

src/main.rs

+        let zipped_payload = config_get("cloud_agent", "dec_payload_file")?;
+        info!("Unzipping payload {} to {}", zipped_payload, unzipped);
+
+        let mut status = Command::new("unzip")


There is nothing wrong with this, though I do want to ask: Why use the system command instead of a library?

@ashcrow I tried a library, but it was a bit more fiddly than I expected and this just worked. I suppose we could always update it on a second pass if we want to make some enhancements.

The zip_rs is not nice I agree, check out compress tools, I find it a lot simpler https://crates.io/crates/compress-tools

@lukehinds Sure will do, I can push a new version of that commit if I get that working.

The library seems straightforward, but I keep getting

INFO keylime_agent > Unzipping payload decrypted_payload.zip to /tmp/secure/unzipped Error: Io(Os { code: 2, kind: NotFound, message: "No such file or directory" })

Despite

[keylime-ima@localhost rust-keylime]$ ls /tmp/secure/unzipped/ decrypted_payload.zip derived_tci_key [keylime-ima@localhost rust-keylime]$ file /tmp/secure/unzipped/decrypted_payload.zip /tmp/secure/unzipped/decrypted_payload.zip: Zip archive data, at least v2.0 to extract [keylime-ima@localhost rust-keylime]$ ls -la /tmp/secure/unzipped/decrypted_payload.zip -rw-r--r--. 1 root root 8738 Jul 12 14:02 /tmp/secure/unzipped/decrypted_payload.zip

The code:

let zipped_payload = config_get("cloud_agent", "dec_payload_file")?; info!("Unzipping payload {} to {}", &zipped_payload, &unzipped); let mut source = fs::File::open(&zipped_payload)?; assert!(Path::new(&zipped_payload).exists()); let dest = Path::new(&unzipped); assert!(Path::new(&dest).exists()); let file_list = list_archive_files(&mut source).expect("failed to list files"); info!("File list: {:?}", file_list); uncompress_archive(&mut source, &dest, Ownership::Ignore).expect("failed to unzip");

Still debugging...

Okay, it worked with a test example so it should be doable here shortly.

It says it succeeds, but then the unzipped files aren't there. All I can think of is maybe it isn't opening with the correct permissions, as it's apparently r only. This should be easy to update but for some reason is not.

I figured out the error... apparently compress_tools::list_archive_files mutates the src archive you give it so that there are no files left afterwards. Bizarre!

ashcrow

One question but overall 👍

lukehinds · 2021-07-12T17:06:12Z

This LGTM, would you like me to test again first thing tomorrow @lkatalin ?

lkatalin · 2021-07-12T17:12:39Z

Thanks @ashcrow and @lukehinds !

@lukehinds Thanks for the offer - if you test again with this branch, you will get the same intermittent quote error we saw at the end of last week. I bypassed that to test the U/V key handling behavior by setting the Python tpm2_checkquote function to return true so that I could at least build out the rest of the code following the quote check. So if you want to test the code in this branch, I could send you a branch of the Python code that I use for testing the interaction. If you want to test the whole system, we will need to fix the intermittent quote error first. I am about to give that a go, but could use help, as it seems to be esoteric TPM behavior.

lkatalin · 2021-07-13T13:45:28Z

@ashcrow @lukehinds Updated to use a zip library!

Signed-off-by: Lily Sturmann <[email protected]>

keylime-bot requested review from ashcrow, puiterwijk and lukehinds June 7, 2021 21:37

keylime-bot assigned lukehinds, puiterwijk and ashcrow Jun 7, 2021

lkatalin force-pushed the keyshandler branch from eb31d7b to b043be1 Compare June 7, 2021 21:49

puiterwijk reviewed Jun 14, 2021

View reviewed changes

lkatalin force-pushed the keyshandler branch from b043be1 to 8fb0a95 Compare June 17, 2021 21:48

lkatalin force-pushed the keyshandler branch 3 times, most recently from a22749d to 44cd92c Compare July 5, 2021 04:00

lkatalin marked this pull request as ready for review July 5, 2021 05:06

lkatalin force-pushed the keyshandler branch from 3a75be4 to 408c39d Compare July 5, 2021 05:18

lkatalin requested a review from puiterwijk July 5, 2021 05:20

lkatalin changed the title ~~Add U and V key handlers~~ Add U and V key handlers, and decrypt + run payload Jul 5, 2021

lkatalin force-pushed the keyshandler branch 5 times, most recently from 46b48d4 to 121185c Compare July 8, 2021 22:12

lkatalin force-pushed the keyshandler branch 3 times, most recently from dbe7019 to e6af73a Compare July 9, 2021 20:07

lkatalin added the critical must fix for rust agent release label Jul 9, 2021

lkatalin force-pushed the keyshandler branch 2 times, most recently from 3ab0bb5 to 56a32bd Compare July 12, 2021 14:09

lkatalin and others added 3 commits July 12, 2021 11:09

Do not flush AK context as this causes an error

c42dc78

Signed-off-by: Lily Sturmann <[email protected]>

Add helper functions for handling U and V keys

ae94116

Signed-off-by: Lily Sturmann <[email protected]>

lkatalin force-pushed the keyshandler branch from 56a32bd to 0abfe2b Compare July 12, 2021 15:09

ashcrow reviewed Jul 12, 2021

View reviewed changes

lkatalin force-pushed the keyshandler branch 3 times, most recently from 996683c to 3f97775 Compare July 13, 2021 13:32

lkatalin requested a review from ashcrow July 13, 2021 13:45

lkatalin added 3 commits July 13, 2021 09:48

Add handler for U and V keys

28f4223

Signed-off-by: Lily Sturmann <[email protected]>

Decrypt payload after key handlers find symm key

cf73b5d

Signed-off-by: Lily Sturmann <[email protected]>

Write out data and run payload

855086b

Signed-off-by: Lily Sturmann <[email protected]>

lkatalin force-pushed the keyshandler branch from 3f97775 to 855086b Compare July 13, 2021 13:48

lukehinds approved these changes Jul 13, 2021

View reviewed changes

lkatalin merged commit 4bb5ba8 into keylime:master Jul 13, 2021

lkatalin deleted the keyshandler branch July 16, 2021 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add U and V key handlers, and decrypt + run payload #197

Add U and V key handlers, and decrypt + run payload #197

lkatalin commented Jun 7, 2021 •

edited

Loading

puiterwijk Jun 14, 2021

puiterwijk Jun 14, 2021

puiterwijk Jun 14, 2021 •

edited

Loading

puiterwijk Jun 14, 2021

lkatalin Jun 14, 2021

puiterwijk Jun 14, 2021

This comment was marked as outdated.

puiterwijk Jun 14, 2021

puiterwijk Jun 14, 2021

lkatalin Jun 17, 2021

lkatalin commented Jul 8, 2021

lkatalin commented Jul 9, 2021

lkatalin commented Jul 12, 2021 •

edited

Loading

ashcrow Jul 12, 2021

lkatalin Jul 12, 2021

lukehinds Jul 12, 2021

lkatalin Jul 12, 2021

lkatalin Jul 12, 2021 •

edited

Loading

lkatalin Jul 12, 2021

lkatalin Jul 12, 2021

lkatalin Jul 12, 2021

ashcrow left a comment

lukehinds commented Jul 12, 2021

lkatalin commented Jul 12, 2021

lkatalin commented Jul 13, 2021

-        Cipher::aes_128_gcm(),
+        match symm_key.len() {
+=> Cipher::aes_128_gcm(),
+=> Cipher::aes_256_gcm(),
+          _ => return Err(....),
+        }


		use crate::{error::Error, QuoteData};

		// This value comes from and must match the Python codebase

Add U and V key handlers, and decrypt + run payload #197

Add U and V key handlers, and decrypt + run payload #197

Conversation

lkatalin commented Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puiterwijk Jun 14, 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

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkatalin commented Jul 8, 2021

lkatalin commented Jul 9, 2021

lkatalin commented Jul 12, 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

lkatalin Jul 12, 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

ashcrow left a comment

Choose a reason for hiding this comment

lukehinds commented Jul 12, 2021

lkatalin commented Jul 12, 2021

lkatalin commented Jul 13, 2021

lkatalin commented Jun 7, 2021 •

edited

Loading

puiterwijk Jun 14, 2021 •

edited

Loading

lkatalin commented Jul 12, 2021 •

edited

Loading

lkatalin Jul 12, 2021 •

edited

Loading