From fecdb64e59148b80bf34b3cbb56d901adc74b94d Mon Sep 17 00:00:00 2001 From: g2flyer Date: Thu, 22 Aug 2024 11:19:20 -0700 Subject: [PATCH 1/2] Make pytest debugging a bit more user-friendly * use --skip-teardown to prevent removal of artifacts * for tamper tests, run all of them instead of abort on first failure Signed-off-by: g2flyer --- libos/test/fs/README.md | 6 ++++++ libos/test/fs/conftest.py | 10 ++++++++++ libos/test/fs/test_enc.py | 8 +++++++- libos/test/fs/test_fs.py | 7 +++++-- 4 files changed, 28 insertions(+), 3 deletions(-) create mode 100644 libos/test/fs/conftest.py diff --git a/libos/test/fs/README.md b/libos/test/fs/README.md index d1bee53105..c8854cfc8a 100644 --- a/libos/test/fs/README.md +++ b/libos/test/fs/README.md @@ -20,3 +20,9 @@ How to execute Encrypted file tests assume that Gramine was built with SGX enabled (see comment in `test_enc.py`). + +This test suite automatically creates files-under-test on startup and removes +them afterwards. When some test fails and you want to debug this failure, it's +more convenient to skip this automatic removal of files (and manually +investigate the test scenario, e.g. with the help of GDB). In such cases, use +the pytest option `--skip-teardown`. diff --git a/libos/test/fs/conftest.py b/libos/test/fs/conftest.py new file mode 100644 index 0000000000..bfe6c7e0e5 --- /dev/null +++ b/libos/test/fs/conftest.py @@ -0,0 +1,10 @@ +import pytest + +option = None + +def pytest_addoption(parser): + parser.addoption("--skip-teardown", action='store_true') + +def pytest_configure(config): + global option + option = config.option diff --git a/libos/test/fs/test_enc.py b/libos/test/fs/test_enc.py index 3b83a33879..201b249bcd 100644 --- a/libos/test/fs/test_enc.py +++ b/libos/test/fs/test_enc.py @@ -179,10 +179,13 @@ def test_500_invalid(self): # prepare valid encrypted file (largest one for maximum possible corruptions) original_input = self.OUTPUT_FILES[-1] self.__encrypt_file(self.INPUT_FILES[-1], original_input) + # Save for debugging as we overwrite original below + shutil.copy(original_input, original_input + '.orig') # generate invalid files based on the above self.__corrupt_file(original_input, invalid_dir) # try to decrypt invalid files + failed = False for name in os.listdir(invalid_dir): invalid = os.path.join(invalid_dir, name) output_path = os.path.join(self.OUTPUT_DIR, name) @@ -190,6 +193,7 @@ def test_500_invalid(self): # copy the file so it has the original file name (for allowed path check) shutil.copy(invalid, input_path) + # test decryption using the pf-crypt tool try: args = ['decrypt', '-V', '-w', self.WRAP_KEY, '-i', input_path, '-o', output_path] self.__pf_crypt(args) @@ -198,4 +202,6 @@ def test_500_invalid(self): self.assertEqual(exc.returncode, 255) else: print('[!] Fail: successfully decrypted file: ' + name) - self.fail() + failed = True + if failed: + self.fail() diff --git a/libos/test/fs/test_fs.py b/libos/test/fs/test_fs.py index 2fb768d252..4345d191c0 100644 --- a/libos/test/fs/test_fs.py +++ b/libos/test/fs/test_fs.py @@ -2,6 +2,7 @@ import os import shutil import unittest +import conftest from graminelibos.regression import ( HAS_SGX, @@ -31,7 +32,8 @@ def setUpClass(cls): @classmethod def tearDownClass(cls): - shutil.rmtree(cls.TEST_DIR) + if not conftest.option.skip_teardown: + shutil.rmtree(cls.TEST_DIR) def setUp(self): # clean output for each test @@ -336,7 +338,8 @@ def setUp(self): os.mkdir(self.TEST_DIR) def tearDown(self): - shutil.rmtree(self.TEST_DIR) + if not conftest.option.skip_teardown: + shutil.rmtree(self.TEST_DIR) def _test_multiple_writers(self, n_lines, n_processes, n_threads): output_path = os.path.join(self.TEST_DIR, 'output.txt') From 3a50ad94b28fb0287a9d5f58122e77b8a9324886 Mon Sep 17 00:00:00 2001 From: g2flyer Date: Thu, 22 Aug 2024 11:19:20 -0700 Subject: [PATCH 2/2] Fix pf_tamper testing * Make sure decrypt is called on correct path (or it will fail always due to invalid path it didn't fail already due to plain text tampering) * Remove undetectable "tampering" test-cases but also add a few use-cases tampering with header ciphertext Signed-off-by: g2flyer --- libos/test/fs/test_enc.py | 13 ++- tools/sgx/pf_tamper/pf_tamper.c | 170 ++++++++++++++++++-------------- 2 files changed, 100 insertions(+), 83 deletions(-) diff --git a/libos/test/fs/test_enc.py b/libos/test/fs/test_enc.py index 201b249bcd..046e4fe742 100644 --- a/libos/test/fs/test_enc.py +++ b/libos/test/fs/test_enc.py @@ -177,21 +177,20 @@ def test_500_invalid(self): os.mkdir(invalid_dir) # prepare valid encrypted file (largest one for maximum possible corruptions) - original_input = self.OUTPUT_FILES[-1] - self.__encrypt_file(self.INPUT_FILES[-1], original_input) + input_path = self.OUTPUT_FILES[-1] + self.__encrypt_file(self.INPUT_FILES[-1], input_path) # Save for debugging as we overwrite original below - shutil.copy(original_input, original_input + '.orig') + shutil.copy(input_path, input_path + '.orig') # generate invalid files based on the above - self.__corrupt_file(original_input, invalid_dir) + self.__corrupt_file(input_path, invalid_dir) # try to decrypt invalid files failed = False for name in os.listdir(invalid_dir): - invalid = os.path.join(invalid_dir, name) + invalid_path = os.path.join(invalid_dir, name) output_path = os.path.join(self.OUTPUT_DIR, name) - input_path = os.path.join(invalid_dir, os.path.basename(original_input)) # copy the file so it has the original file name (for allowed path check) - shutil.copy(invalid, input_path) + shutil.copy(invalid_path, input_path) # test decryption using the pf-crypt tool try: diff --git a/tools/sgx/pf_tamper/pf_tamper.c b/tools/sgx/pf_tamper/pf_tamper.c index 004663f0e1..52d832ca36 100644 --- a/tools/sgx/pf_tamper/pf_tamper.c +++ b/tools/sgx/pf_tamper/pf_tamper.c @@ -212,10 +212,11 @@ static void tamper_truncate(void) { truncate_file("trunc_data_3", 3 * PF_NODE_SIZE + PF_NODE_SIZE / 2); /* extend */ + /* Note: The code implementing Gramine's encrypted filesystem in `common/src/protected_files.c` + * tolerates the actual file to be longer than what the `file_size` attribute in the decrypted + * header says. The only required invariant enforced in the code is ensuring that the file size + * is a multiple of PF_NODE_SIZE, so let us validate this below. */ truncate_file("extend_0", g_input_size + 1); - truncate_file("extend_1", g_input_size + PF_NODE_SIZE / 2); - truncate_file("extend_2", g_input_size + PF_NODE_SIZE); - truncate_file("extend_3", g_input_size + PF_NODE_SIZE + PF_NODE_SIZE / 2); } /* returns mmap'd output contents */ @@ -270,24 +271,48 @@ static void pf_encrypt(const void* decrypted, size_t size, const pf_key_t* key, munmap(meta, g_input_size); \ } while (0) -/* if update is true, also create a file with correct metadata MAC */ -#define BREAK_PF(suffix, update, ...) do { \ - __BREAK_PF(suffix, __VA_ARGS__); \ - if (update) { \ - __BREAK_PF(suffix "_fixed", __VA_ARGS__ { \ - pf_encrypt(meta_dec, sizeof(*meta_dec), &g_meta_key, \ - &meta->plaintext_part.metadata_mac, meta->encrypted_part, \ - "metadata"); \ - } ); \ - } \ -} while (0) +/* + * Three different macros to tamper with a file: + * - BREAK_PLN: tamper with the plaintext part of the metadata. + * - BREAK_DEC: tamper with the decrypted part of the metadata and re-encrypt it. + * (Not something an adversary can do to attack the system but still tests overall reliability.) + * - BREAK_MHT: tamper with the MHT nodes. + * + * The tampering is done by creating a new file with the tampered part and the rest copied from the + * original file. + */ -#define BREAK_MHT(suffix, ...) do { \ - __BREAK_PF(suffix, __VA_ARGS__ { \ - pf_encrypt(mht_dec, sizeof(*mht_dec), &meta_dec->root_mht_node_key, \ - &meta_dec->root_mht_node_mac, mht_enc, "mht"); \ - } ); \ -} while (0) +/* if update is true, also create a file with correct metadata MAC */ +#define BREAK_PLN(suffix, update, ...) \ + do { \ + __BREAK_PF(suffix, __VA_ARGS__); \ + if (update) { \ + __BREAK_PF( \ + suffix "_fixed", __VA_ARGS__ { \ + pf_encrypt(meta_dec, sizeof(*meta_dec), &g_meta_key, \ + &meta->plaintext_part.metadata_mac, meta->encrypted_part, \ + "metadata"); \ + }); \ + } \ + } while (0) + +#define BREAK_DEC(suffix, ...) \ + do { \ + __BREAK_PF( \ + suffix, __VA_ARGS__ { \ + pf_encrypt(meta_dec, sizeof(*meta_dec), &g_meta_key, \ + &meta->plaintext_part.metadata_mac, meta->encrypted_part, "metadata"); \ + }); \ + } while (0) + +#define BREAK_MHT(suffix, ...) \ + do { \ + __BREAK_PF( \ + suffix, __VA_ARGS__ { \ + pf_encrypt(mht_dec, sizeof(*mht_dec), &meta_dec->root_mht_node_key, \ + &meta_dec->root_mht_node_mac, mht_enc, "mht"); \ + }); \ + } while (0) #define LAST_BYTE(array) (((uint8_t*)&array)[sizeof(array) - 1]) @@ -303,60 +328,55 @@ static void tamper_modify(void) { FATAL("Out of memory\n"); /* plain part of the metadata isn't covered by the MAC so no point updating it */ - BREAK_PF("meta_plain_id_0", /*update=*/false, - { meta->plaintext_part.file_id = 0; }); - BREAK_PF("meta_plain_id_1", /*update=*/false, - { meta->plaintext_part.file_id = UINT64_MAX; }); - BREAK_PF("meta_plain_version_0", /*update=*/false, - { meta->plaintext_part.major_version = 0; }); - BREAK_PF("meta_plain_version_1", /*update=*/false, - { meta->plaintext_part.major_version = 0xff; }); - BREAK_PF("meta_plain_version_2", /*update=*/false, - { meta->plaintext_part.minor_version = 0xff; }); + BREAK_PLN("meta_plain_id_0", /*update=*/false, + { meta->plaintext_part.file_id = 0; }); + BREAK_PLN("meta_plain_id_1", /*update=*/false, + { meta->plaintext_part.file_id = UINT64_MAX; }); + BREAK_PLN("meta_plain_version_0", /*update=*/false, + { meta->plaintext_part.major_version = 0; }); + BREAK_PLN("meta_plain_version_1", /*update=*/false, + { meta->plaintext_part.major_version = 0xff; }); + /* Note: Gramine's encrypted filesystem only tests (equality) on major version but nothing about + * minor_version, so no point in tampering with meta->plaintext_part.minor_version. */ /* metadata_key_nonce is the keying material for encrypted metadata key derivation, so create * also PFs with updated MACs */ - BREAK_PF("meta_plain_nonce_0", /*update=*/true, - { meta->plaintext_part.metadata_key_nonce[0] ^= 1; }); - BREAK_PF("meta_plain_nonce_1", /*update=*/true, - { LAST_BYTE(meta->plaintext_part.metadata_key_nonce) ^= 0xfe; }); - BREAK_PF("meta_plain_mac_0", /*update=*/true, - { meta->plaintext_part.metadata_mac[0] ^= 0xfe; }); - BREAK_PF("meta_plain_mac_1", /*update=*/true, - { LAST_BYTE(meta->plaintext_part.metadata_mac) &= 1; }); - - BREAK_PF("meta_enc_filename_0", /*update=*/true, - { meta_dec->file_path[0] = 0; }); - BREAK_PF("meta_enc_filename_1", /*update=*/true, - { meta_dec->file_path[0] ^= 1; }); - BREAK_PF("meta_enc_filename_2", /*update=*/true, - { LAST_BYTE(meta_dec->file_path) ^= 0xfe; }); - BREAK_PF("meta_enc_size_0", /*update=*/true, - { meta_dec->file_size = 0; }); - BREAK_PF("meta_enc_size_1", /*update=*/true, - { meta_dec->file_size = g_input_size - 1; }); - BREAK_PF("meta_enc_size_2", /*update=*/true, - { meta_dec->file_size = g_input_size + 1; }); - BREAK_PF("meta_enc_size_3", /*update=*/true, - { meta_dec->file_size = UINT64_MAX; }); - BREAK_PF("meta_enc_mht_key_0", /*update=*/true, - { meta_dec->root_mht_node_key[0] ^= 1; }); - BREAK_PF("meta_enc_mht_key_1", /*update=*/true, - { LAST_BYTE(meta_dec->root_mht_node_key) ^= 0xfe; }); - BREAK_PF("meta_enc_mht_mac_0", /*update=*/true, - { meta_dec->root_mht_node_mac[0] ^= 1; }); - BREAK_PF("meta_enc_mht_mac_1", /*update=*/true, - { LAST_BYTE(meta_dec->root_mht_node_mac) ^= 0xfe; }); - BREAK_PF("meta_enc_data_0", /*update=*/true, - { meta_dec->file_data[0] ^= 0xfe; }); - BREAK_PF("meta_enc_data_1", /*update=*/true, - { LAST_BYTE(meta_dec->file_data) ^= 1; }); - - /* padding is ignored */ - BREAK_PF("meta_padding_0", /*update=*/false, - { meta->padding[0] ^= 1; }); - BREAK_PF("meta_padding_1", /*update=*/false, - { LAST_BYTE(meta->padding) ^= 0xfe; }); + BREAK_PLN("meta_plain_nonce_0", /*update=*/true, + { meta->plaintext_part.metadata_key_nonce[0] ^= 1; }); + BREAK_PLN("meta_plain_nonce_1", /*update=*/true, + { LAST_BYTE(meta->plaintext_part.metadata_key_nonce) ^= 0xfe; }); + BREAK_PLN("meta_plain_mac_0", /*update=*/false, // update would overwrite the tampering + { meta->plaintext_part.metadata_mac[0] ^= 0xfe; }); + BREAK_PLN("meta_plain_mac_1", /*update=*/false, // update would overwrite the tampering + { LAST_BYTE(meta->plaintext_part.metadata_mac) ^= 1; }); + BREAK_PLN("meta_plain_encrypted_0", /*update=*/false, // update would overwrite the tampering + { meta->encrypted_part[0] ^= 1; }); + BREAK_PLN("meta_plain_encrypted_1", /*update=*/false, // update would overwrite the tampering + { LAST_BYTE(meta->encrypted_part) ^= 1; }); + + BREAK_DEC("meta_enc_filename_0", { meta_dec->file_path[0] = 0; }); + BREAK_DEC("meta_enc_filename_1", { meta_dec->file_path[0] ^= 1; }); + BREAK_DEC("meta_enc_filename_2", { + meta_dec->file_path[strlen(meta_dec->file_path) - 1] = + '\0'; // shorten path by one character + }); + /* Note: Gramine's encrypted filesystem does not generally test whether a file is longer than + * `meta_dec->file_size` indicates; in particular it does not test for the case where the header + * says the file is empty but file would contain (ignored) data blocks. So test only size + * modifications which interfere with the mht tree but not with `meta_dec->file_size = 0`. */ + BREAK_DEC("meta_enc_size_0", { meta_dec->file_size = g_input_size - PF_NODE_SIZE; }); + BREAK_DEC("meta_enc_size_1", { meta_dec->file_size = g_input_size - 1; }); + BREAK_DEC("meta_enc_size_2", { meta_dec->file_size = g_input_size + PF_NODE_SIZE; }); + BREAK_DEC("meta_enc_size_3", { meta_dec->file_size = UINT64_MAX; }); + BREAK_DEC("meta_enc_size_4", { meta_dec->file_size = g_input_size + 1; }); + BREAK_DEC("meta_enc_mht_key_0", { meta_dec->root_mht_node_key[0] ^= 1; }); + BREAK_DEC("meta_enc_mht_key_1", { LAST_BYTE(meta_dec->root_mht_node_key) ^= 0xfe; }); + BREAK_DEC("meta_enc_mht_mac_0", { meta_dec->root_mht_node_mac[0] ^= 1; }); + BREAK_DEC("meta_enc_mht_mac_1", { LAST_BYTE(meta_dec->root_mht_node_mac) ^= 0xfe; }); + /* Note: no point in tampering with (decrypted) meta_dec->file_data as there is no way to + * detect such tampering, the re-encryption would turn it in authentic (different) data. */ + + /* Note: padding is ignored during processing, so no point in tampering meta->padding */ BREAK_MHT("mht_0", { mht_dec->data_nodes_crypto[0].key[0] ^= 1; }); BREAK_MHT("mht_1", { mht_dec->data_nodes_crypto[0].mac[0] ^= 1; }); @@ -380,11 +400,9 @@ static void tamper_modify(void) { }); /* data nodes start from node #2 */ - BREAK_PF("data_0", /*update=*/false, - { *(out + 2 * PF_NODE_SIZE) ^= 1; }); - BREAK_PF("data_1", /*update=*/false, - { *(out + 3 * PF_NODE_SIZE - 1) ^= 1; }); - BREAK_PF("data_2", /*update=*/false, { + BREAK_PLN("data_0", /*update=*/false, { *(out + 2 * PF_NODE_SIZE) ^= 1; }); + BREAK_PLN("data_1", /*update=*/false, { *(out + 3 * PF_NODE_SIZE - 1) ^= 1; }); + BREAK_PLN("data_2", /*update=*/false, { /* swap data nodes */ memcpy(out + 2 * PF_NODE_SIZE, g_input_data + 3 * PF_NODE_SIZE, PF_NODE_SIZE); memcpy(out + 3 * PF_NODE_SIZE, g_input_data + 2 * PF_NODE_SIZE, PF_NODE_SIZE);