[tests] Fix tampering tests for protected files #1980
Conversation
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @g2flyer)
tools/sgx/pf_tamper/pf_tamper.c line 353 at r1 (raw file):
/* Note: we do not test generally whether file is longer than meta_dec_file indicates, in * particular we do not test it for the case where the header says it is empty. So test only * size modification which interfer with the mht tree but not with meta_dec->file_size = 0. */
modifications which interfere
Code quote:
modification which interfer
g2flyer
changed the title
There was a problem hiding this comment.
Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @omeg)
tools/sgx/pf_tamper/pf_tamper.c line 353 at r1 (raw file):
Previously, omeg (Rafał Wojdyła) wrote…
modifications which interfere
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners
There was a problem hiding this comment.
Reviewed 3 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @g2flyer)
libos/test/fs/conftest.py line 6 at r2 (raw file):
def pytest_addoption(parser): parser.addoption("--skip-teardown", action='store_true')
This is quite useful, thanks!
libos/test/fs/conftest.py line 6 at r2 (raw file):
def pytest_addoption(parser): parser.addoption("--skip-teardown", action='store_true')
Could you mention this flag in the README: https://github.com/gramineproject/gramine/blob/master/libos/test/fs/README.md?plain=1
libos/test/fs/test_enc.py line 180 at r2 (raw file):
# prepare valid encrypted file (largest one for maximum possible corruptions) original_input = self.OUTPUT_FILES[-1]
Can we rename original_input to input_path? It's hard to read this code, too many variables.
libos/test/fs/test_enc.py line 182 at r2 (raw file):
original_input = self.OUTPUT_FILES[-1] self.__encrypt_file(self.INPUT_FILES[-1], original_input) shutil.copy(original_input, original_input+".save") # Save for debugging as we overwrite original below
Exceeds 100 char-per-line limit, please move the comment line above the code line
libos/test/fs/test_enc.py line 182 at r2 (raw file):
original_input = self.OUTPUT_FILES[-1] self.__encrypt_file(self.INPUT_FILES[-1], original_input) shutil.copy(original_input, original_input+".save") # Save for debugging as we overwrite original below
Isn't a more conventional extension .orig?
libos/test/fs/test_enc.py line 189 at r2 (raw file):
failed = False for name in os.listdir(invalid_dir): invalid = os.path.join(invalid_dir, name)
Please rename invalid to invalid_path
tools/sgx/pf_tamper/pf_tamper.c line 217 at r2 (raw file):
/* Note: as mentioned below in tamper_modify() for meta_dec.file_size, we mostly allow the * actual file to be longer than what the header size, the only thing we require is that the * file size is a multiple of PF_NODE_SIZE, so just verify this */
Confusing...
- Why do you say
mostly allow? What is the condition when this is disallowed? - By
wedo you meanGramine's encrypted filesystem? than what the header size-- do you meanthat what is specified in the header's file size?just verify this-- who verifies what? How is this related to this particular code here?
tools/sgx/pf_tamper/pf_tamper.c line 274 at r2 (raw file):
/* if update is true, also create a file with correct metadata MAC */ #define BREAK_PLN(suffix, update, ...) \
What does PLN stand for? "Plaintext"? Could you please add some quick comments what each of these macros does?
tools/sgx/pf_tamper/pf_tamper.c line 327 at r2 (raw file):
BREAK_PLN("meta_plain_version_1", /*update=*/false, { meta->plaintext_part.major_version = 0xff; }); /* Note: we only test (equality) on major version but nothing about minor_version, so no
we -> Gramine's encrypted filesystem
tools/sgx/pf_tamper/pf_tamper.c line 328 at r2 (raw file):
{ meta->plaintext_part.major_version = 0xff; }); /* Note: we only test (equality) on major version but nothing about minor_version, so no * point in tampering with meta->plaintext_part.minor_version ... */
No need for ... at the end, it only makes the reader stop and wonder if there's some text missing.
tools/sgx/pf_tamper/pf_tamper.c line 354 at r2 (raw file):
* particular we do not test it for the case where the header says it is empty. 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; });
Could you explain why for example this file size breaks? Isn't it less than the real file size, which is not caught (ignored) by Gramine's encrypted filesystem?
tools/sgx/pf_tamper/pf_tamper.c line 364 at r2 (raw file):
BREAK_DEC("meta_enc_mht_mac_1", { LAST_BYTE(meta_dec->root_mht_node_mac) ^= 0xfe; }); /* Note: no poing 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 .... */
No need for ... at the end, it only makes the reader stop and wonder if there's some text missing.
tools/sgx/pf_tamper/pf_tamper.c line 366 at r2 (raw file):
* 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 */
meta.padding -> meta->padding (for consistency with other comments)
There was a problem hiding this comment.
Reviewable status: 2 of 5 files reviewed, 12 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @omeg)
libos/test/fs/conftest.py line 6 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Could you mention this flag in the README: https://github.com/gramineproject/gramine/blob/master/libos/test/fs/README.md?plain=1
Good idea, done
libos/test/fs/test_enc.py line 180 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Can we rename
original_inputtoinput_path? It's hard to read this code, too many variables.
Done.
libos/test/fs/test_enc.py line 182 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Isn't a more conventional extension
.orig?
Done.
libos/test/fs/test_enc.py line 182 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Exceeds 100 char-per-line limit, please move the comment line above the code line
Done.
libos/test/fs/test_enc.py line 189 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please rename
invalidtoinvalid_path
Done.
tools/sgx/pf_tamper/pf_tamper.c line 217 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Confusing...
- Why do you say
mostly allow? What is the condition when this is disallowed?- By
wedo you meanGramine's encrypted filesystem?than what the header size-- do you meanthat what is specified in the header's file size?just verify this-- who verifies what? How is this related to this particular code here?
rephrased, hopefully clarifing it
tools/sgx/pf_tamper/pf_tamper.c line 274 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What does
PLNstand for? "Plaintext"? Could you please add some quick comments what each of these macros does?
added a corresponding comment
tools/sgx/pf_tamper/pf_tamper.c line 327 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
we->Gramine's encrypted filesystem
Done.
tools/sgx/pf_tamper/pf_tamper.c line 328 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
No need for
...at the end, it only makes the reader stop and wonder if there's some text missing.
Done.
tools/sgx/pf_tamper/pf_tamper.c line 354 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Could you explain why for example this file size breaks? Isn't it less than the real file size, which is not caught (ignored) by Gramine's encrypted filesystem?
Good question. I was wondering myself and after (admittedly very quick investigation) couldn't immediately get a firm explanation. As it was one of the second-level of defense reliability tests and not an adverserial attack, i gave up with the vague intuition that probably some inviarient in the meta-data vs data node vs data in header filling was violated ...
tools/sgx/pf_tamper/pf_tamper.c line 364 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
No need for
...at the end, it only makes the reader stop and wonder if there's some text missing.
Done.
tools/sgx/pf_tamper/pf_tamper.c line 366 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
meta.padding->meta->padding(for consistency with other comments)
Done.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @g2flyer)
libos/test/fs/README.md line 22 at r3 (raw file):
Encrypted file tests assume that Gramine was built with SGX enabled (see comment in `test_enc.py`).
Accidental trailing space, please remove
libos/test/fs/README.md line 27 at r3 (raw file):
pytest option `--skip-teardown` which prevents the removal of generated test files (in sub-directory `tmp`) and allows to easily replicate and investigate manually the test scenarios, e.g., with help of `gdb`.
A bit better rewording:
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`.
libos/test/fs/test_enc.py line 183 at r3 (raw file):
self.__encrypt_file(self.INPUT_FILES[-1], input_path) # Save for debugging as we overwrite original below shutil.copy(input_path, input_path+".orig")
- shutil.copy(input_path, input_path+".orig")
+ shutil.copy(input_path, input_path + '.orig')tools/sgx/pf_tamper/pf_tamper.c line 217 at r2 (raw file):
Previously, g2flyer (Michael Steiner) wrote…
rephrased, hopefully clarifing it
Thanks, this is now very clear!
tools/sgx/pf_tamper/pf_tamper.c line 274 at r3 (raw file):
} while (0) /* Three different macros to tamper with a file
Please reformat like this (it's more common in our code base, and also easier to read imho):
/*
* Three different macros ...
* ...
*/
tools/sgx/pf_tamper/pf_tamper.c line 274 at r3 (raw file):
} while (0) /* Three different macros to tamper with a file
Please add : at the end of the sentence
tools/sgx/pf_tamper/pf_tamper.c line 276 at r3 (raw file):
/* 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-encrypted it.
re-encrypt (no past tense)
tools/sgx/pf_tamper/pf_tamper.c line 278 at r3 (raw file):
* - BREAK_DEC: tamper with the decrypted part of the metadata and re-encrypted it. * (Not something an adversary can do to attack the system but still tests overall reliability.) * - BREAK_MHT: tamper with the MHT nodes
Please add . at the end of the sentence (for uniformity)
tools/sgx/pf_tamper/pf_tamper.c line 278 at r3 (raw file):
* - BREAK_DEC: tamper with the decrypted part of the metadata and re-encrypted it. * (Not something an adversary can do to attack the system but still tests overall reliability.) * - BREAK_MHT: tamper with the MHT nodes
Please add a new empty line, to visually separate the list from the next paragraph.
tools/sgx/pf_tamper/pf_tamper.c line 360 at r3 (raw file):
'\0'; // shorten path by one character }); /* Note: As mentioned above, Gramine's encrypted filesystem does not generally test whether a
Please remove As mentioned above, it was like 100 lines above, nobody will scroll to search for that place :)
g2flyer
force-pushed
the
msteiner/pf_tamper-fix
branch
from
4330b30
to
3a50ad9
Compare
There was a problem hiding this comment.
To fix up i had to re-order fixups and hence force-push. Given that the changes were small, i took also the liberty to autosquash, so should do future merge easier
Reviewable status: 2 of 5 files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv)
libos/test/fs/README.md line 22 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Accidental trailing space, please remove
Done.
libos/test/fs/README.md line 27 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
A bit better rewording:
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`.
Done.
libos/test/fs/test_enc.py line 183 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
- shutil.copy(input_path, input_path+".orig") + shutil.copy(input_path, input_path + '.orig')
Done.
tools/sgx/pf_tamper/pf_tamper.c line 274 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please reformat like this (it's more common in our code base, and also easier to read imho):
/* * Three different macros ... * ... */
Done.
tools/sgx/pf_tamper/pf_tamper.c line 274 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add
:at the end of the sentence
Done.
tools/sgx/pf_tamper/pf_tamper.c line 276 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
re-encrypt(no past tense)
Done.
tools/sgx/pf_tamper/pf_tamper.c line 278 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add
.at the end of the sentence (for uniformity)
Done.
tools/sgx/pf_tamper/pf_tamper.c line 278 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add a new empty line, to visually separate the list from the next paragraph.
Done.
tools/sgx/pf_tamper/pf_tamper.c line 360 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please remove
As mentioned above, it was like 100 lines above, nobody will scroll to search for that place :)
Done.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
|
Jenkins, test this please |
There was a problem hiding this comment.
Looks good, modulo some nitpicks. And sorry for the late review.
Reviewed 2 of 4 files at r1, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @g2flyer)
-- commits line 3 at r4:
Please add proper component tags to the commit one-liners.
-- commits line 13 at r4:
I guess that was the intended meaning? Or something else? I have problems understanding what you meant here.
Suggestion:
due to invalid path if didn't fail already due to plain text tampering)
tools/sgx/pf_tamper/pf_tamper.c line 359 at r4 (raw file):
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", {
please assert(strlen(meta_dec->file_path) > 0) before this test, just to be sure
tools/sgx/pf_tamper/pf_tamper.c line 364 at r4 (raw file):
}); /* 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
Suggestion:
what `meta_dec->file_size` indicateslibos/test/fs/test_enc.py line 182 at r4 (raw file):
input_path = self.OUTPUT_FILES[-1] self.__encrypt_file(self.INPUT_FILES[-1], input_path) # Save for debugging as we overwrite original below
Suggestion:
# Save for debugging as we overwrite the original below
g2flyer
force-pushed
the
msteiner/pf_tamper-fix
branch
from
3a50ad9
to
9585996
Compare
There was a problem hiding this comment.
I hope the change addresses your concerns. Note I also rebased to the latest master to test against that version.
Reviewable status: 2 of 5 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv, @mkow, and @omeg)
Previously, mkow (Michał Kowalczyk) wrote…
Please add proper component tags to the commit one-liners.
Done.
Previously, mkow (Michał Kowalczyk) wrote…
I guess that was the intended meaning? Or something else? I have problems understanding what you meant here.
Done.
tools/sgx/pf_tamper/pf_tamper.c line 359 at r4 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
please
assert(strlen(meta_dec->file_path) > 0)before this test, just to be sure
Done.
libos/test/fs/test_enc.py line 182 at r4 (raw file):
input_path = self.OUTPUT_FILES[-1] self.__encrypt_file(self.INPUT_FILES[-1], input_path) # Save for debugging as we overwrite original below
Done.
tools/sgx/pf_tamper/pf_tamper.c line 364 at r4 (raw file):
}); /* 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
Done.
There was a problem hiding this comment.
Looks good now, we can fix the rest during the rebase (but please check that "ift" for whether my suggestion is right).
Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @g2flyer)
-- commits line 3 at r5:
We just list tests in this case, as this isn't changing the Python production code itself (this way it's clear that the commit touches only tests). I guess it's not too clear in our guidelines, sorry!
Suggestion:
[tests]
-- commits line 10 at r5:
ditto, but with LibOS
Suggestion:
[tests]
Suggestion:
if it
* 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 <[email protected]>
g2flyer
force-pushed
the
msteiner/pf_tamper-fix
branch
from
9585996
to
7465218
Compare
* Make sure decrypt is called on correct path (or it will fail always due to invalid path if it didn't already fail 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 <[email protected]>
g2flyer
force-pushed
the
msteiner/pf_tamper-fix
branch
from
7465218
to
0cd0dbd
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @mkow)
Previously, mkow (Michał Kowalczyk) wrote…
We just list tests in this case, as this isn't changing the Python production code itself (this way it's clear that the commit touches only tests). I guess it's not too clear in our guidelines, sorry!
Done.
Previously, mkow (Michał Kowalczyk) wrote…
ditto, but with LibOS
Done.
-- commits line 13 at r5:
Done.
Description of the changes
Test
test_500_invalidalways failed as test files to decrypt always had an illegal filename masking the actual intended tampering. Fixing this inlibos/test/fs/test_enc.pyshowed that a number of intended tampering use-cases in fact were uncatchable. This PR fixes the file path in the test drivertest_nec.py, removes ineffective tampering use-cases and adds some tampering with ciphertext on headers.The PR also (in a separate commit) improves debuggability of python tests by optionally allow to skip the teardown of the test artifacts and run all tests for 500 instead of aborting on first failure.
How to test this PR?
Run usual regression tests
This change is