From 23661cc2327d06b12dfe2962a1f82d97feb88d5e Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 16 Oct 2023 19:31:41 +0100 Subject: [PATCH 01/37] Detailed design of memory protection strategy Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 176 ++++++++++++++++++++++++- 1 file changed, 173 insertions(+), 3 deletions(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 783b118841..a65ed26ac0 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -232,11 +232,60 @@ Justification: see “[Susceptibility of different mechanisms](susceptibility-of ### Implementation of copying -Copy what needs copying. This seems straightforward. +Copy what needs copying. This is broadly straightforward, however there are a few things to consider. + +#### Compiler optimization of copies + +It is unclear whether the compiler will attempt to optimize away copying operations. + +Once the copying code is implemented, it should be evaluated to see whether compiler optimization is a problem. Specifically, for the major compilers and platforms supported by Mbed TLS: +* Build Mbed TLS with the compiler set to a high level of optimization (e.g. `-O2` for `gcc`). +* Inspect the generated code with `objdump` or a similar tool to see if copying operations are preserved. + +If copying behaviour is preserved by all major compilers and platforms then assume that compiler optimization is not a problem. + +If copying behaviour is optimized away by the compiler, further investigation is needed. Experiment with using the `volatile` keyword to force the compiler not to optimize accesses to the copied buffers. + +**Open questions: Will the compiler optimize away copies? If so, can it be prevented from doing so in a portable way?** + +#### Copying code + +We may either copy buffers on an ad-hoc basis using `memcpy()` in each PSA function, or use a unified set of functions for copying input and output data. The advantages of the latter are obvious: + +* Any test hooks need only be added in one place. +* Copying code must only be reviewed for correctness in one place, rather than in all functions where it occurs. +* Copy bypass is simpler as we can just replace these functions with no-ops in a single place. +* Any complexity needed to prevent the compiler optimizing copies away does not have to be duplicated. + +On the other hand, the only advantage of ad-hoc copying is slightly greater flexibility. + +**Design decision: Create a unified set of functions for copying input and output data.** + +#### Copying in multipart APIs + +Multipart APIs may follow one of 2 possible approaches for copying of input: + +##### 1. Allocate a buffer and copy input on each call to `update()` + +This is simple and mirrors the approach for one-shot APIs nicely. However, allocating memory in the middle of a multi-part operation is likely to be bad for performance. Multipart APIs are designed in part for systems that do not have time to perform an operation at once, so introducing poor performance may be a problem here. + +**Open question: Does memory allocation in `update()` cause a performance problem? If so, to what extent?** + +##### 2. Allocate a buffer at the start of the operation and subdivide calls to `update()` + +In this approach, input and output buffers are allocated at the start of the operation that are large enough to hold the expected average call to `update()`. When `update()` is called with larger buffers than these, the PSA API layer makes multiple calls to the driver, chopping the input into chunks of the temporary buffer size and filling the output from the results until the operation is finished. + +This would be more complicated than approach (1) and introduces some extra issues. For example, if one of the intermediate calls to the driver's `update()` returns an error, it is not possible for the driver's state to be rolled back to before the first call to `update()`. It is unclear how this could be solved. + +However, this approach would reduce memory usage in some cases and prevent memory allocation during an operation. Additionally, since the input and output buffers would be fixed-size it would be possible to allocate them statically, avoiding the need for any dynamic memory allocation at all. + +**Design decision: Initially use approach (1) and treat approach (2) as an optimization to be done if necessary.** ### Validation of copying -TODO: how to we validate that we didn't forget to copy? +#### Validation of copying by review + +This is fairly self-explanatory. Review all functions that use shared memory and ensure that they each copy memory. This is the simplest strategy to implement but is less reliable than automated validation. #### Validation of copying with memory pools @@ -263,9 +312,62 @@ static void copy_to_user(void *copy_buffer, void *const input_buffer, size_t len #endif } ``` - The reason to poison the memory before calling the library, rather than after the copy-in (and symmetrically for output buffers) is so that the test will fail if we forget to copy, or we copy the wrong thing. This would not be the case if we relied on the library's copy function to do the poisoning: that would only validate that the driver code does not access the memory on the condition that the copy is done as expected. +Note: Extra work may be needed when allocating buffers to ensure that each shared buffer lies in its own separate page, allowing its permissions to be set independently. + +**Question: Should we try to build memory poisoning validation on existing Mbed TLS tests, or write new tests for this?** + +##### Validation with existing tests + +It should be possible to integrate memory poisoning validation with existing tests. This has two main advantages: +* All of the tests are written already, potentially saving development time. +* The code coverage of these tests is already much greater than would be achievable writing new tests from scratch. + +In an ideal world, we would be able to take a grand, encompassing approach whereby we would simply replace the implementation of `mbedtls_calloc()` and all tests would transparently run with memory poisoning enabled. Unfortunately, there are some significant difficulties with this idea: +* We cannot automatically distinguish which allocated buffers are shared buffers that need memory poisoning enabled. +* Some input buffers to tested functions may be stack allocated so cannot be poisoned automatically. + +Instead, consider a more modest strategy. Create a function: +```c +uint8_t *mbedtls_test_get_poisoned_copy(uint8_t *buffer, size_t len) +``` +that creates a poisoned copy of a buffer ready to be passed to the PSA function. Also create: +```c +uint8_t *mbedtls_test_copy_free_poisoned_buffer(uint8_t *poisoned_buffer, uint8_t *original_buffer, size_t len) +``` +which copies the poisoned buffer contents back into the original buffer and frees the poisoned copy. + +In each test case, manually wrap any calls to PSA functions in code that substitutes a poisoned buffer. For example, the code: +```c +psa_api_do_some_operation(input, input_len, output, output_len); +``` +Would be transformed to: +```c +input_poisoned = mbedtls_test_get_poisoned_copy(input, input_len); +output_poisoned = mbedtls_test_get_poisoned_copy(output, output_len); +psa_api_do_some_operation(input_poisoned, input_len, output_poisoned, output_len); +mbedtls_test_copy_free_poisoned_buffer(input_poisoned, input, input_len); +mbedtls_test_copy_free_poisoned_buffer(output_poisoned, output, output_len); +``` +Further interface design or careful use of macros may make this a little less cumbersome than it seems in this example. + +The poison copying functions should be written so as to evaluate to no-ops based on the value of a config option. They also need not be added to all tests, only to a 'covering set' of important tests. + +##### Validation with new tests + +Validation with newly created tests is initially simpler to implement than using the existing tests, since the tests can know about memory poisoning from the start. However, re-implementing testing for most PSA interfaces (even only basic positive testing) is a large undertaking. Furthermore, not much is gained over the previous approach, given that it seems straightforward to wrap PSA function calls in existing tests with poisoning code. + +**Design decision: Add memory poisoning code to existing tests rather than creating new ones, since this is simpler and produces greater coverage.** + +#### Discussion + +Of all discussed approaches, validation by memory poisoning appears as the best. This is because it: +* Does not require complex linking against different versions of `malloc()` (as is the case with the memory pool approach). +* Allows automated testing (unlike the review approach). + +**Design decision: Use a memory poisoning approach to validate copying.** + ### Shared memory protection requirements TODO: write document and reference it here. @@ -298,6 +400,12 @@ Idea: call `mmap` to allocate memory for arguments and `mprotect` to deny or ree Record the addresses that are accessed. Mark the test as failed if the same address is read twice. This part might be hard to do in the gdb language, so we may want to just log the addresses and then use a separate program to analyze the logs, or do the gdb tasks from Python. +#### Discussion + +The best approach for validating the correctness of memory accesses is an open question that requires further investigation and prototyping. The above sections discuss some possibilities. + +However, there is one additional consideration that may make this easier. The careful-access approach to memory protection is only planned for hash and MAC algorithms. These lend themselves to a linear access pattern on input data; it may be simpler to test that a linear pattern is followed, rather than a random-access single-access-per-location pattern. + ## Analysis of argument protection in built-in drivers TODO: analyze the built-in implementations of mechanisms for which there is a requirement on drivers. By code inspection, how satisfied are we that they meet the requirement? @@ -309,3 +417,65 @@ For efficiency, we are likely to want mechanisms to bypass the copy and process Expand this section to document any mechanisms that bypass the copy. Make sure that such mechanisms preserve the guarantees when buffers overlap. + +## Detailed design + +### Copying functions + +As discussed above, it is simpler to use a single unified API for copying. Therefore, we create the following functions: + +* `psa_crypto_copy_input(const uint8_t *input, size_t input_length, uint8_t *input_copy, size_t input_copy_length)` +* `psa_crypto_copy_output(const uint8_t *output_copy, size_t output_copy_length, uint8_t *output, size_t output_length)` + +These seem to be a repeat of the same function, however it is useful to retain two separate functions for input and output parameters so that we can use different test hooks in each when using memory poisoning for tests. + +Given that the majority of functions will be allocating memory on the heap to copy, it may help to build convenience functions that allocate the memory as well. One function allocates and copies the buffers: + +* `psa_crypto_alloc_and_copy(const uint8_t *input, size_t input_length, uint8_t *output, size_t output_length, struct {uint8_t *inp, uint8_t *out} *buffers)` + +This function allocates an input and output buffer in `buffers` and copy the input from the user-supplied input buffer to `buffers->inp`. + +An analogous function is needed to copy and free the buffers: + +* `psa_crypto_copy_and_free(struct {uint8_t *inp, uint8_t *out} buffers, const uint8_t *input, size_t input_length, const uint8_t *output, size_t output_length)` + +This function would first copy the `buffers->out` buffer to the user-supplied output buffer and then free `buffers->inp` and `buffers->out`. + +Some PSA functions may not use these convenience functions as they may have local optimizations that reduce memory usage. For example, ciphers may be able to use a single intermediate buffer for both input and output. + +### Implementation by module + +Module | Input protection strategy | Output protection strategy | Notes +---|---|---|--- +Hash and MAC | Careful access | Careful access | Low risk of multiple-access as the input and output are raw unformatted data. +Cipher | Copying | Copying | +AEAD | Copying (careful access for additional data) | Copying | +Key derivation | Careful access | Careful access | +Asymmetric signature | Careful access | Copying | Inputs to signatures are passed to a hash. This will no longer hold once PureEdDSA support is implemented. +Asymmetric encryption | Copying | Copying | +Key agreement | Copying | Copying | +PAKE | Copying | Copying | +Key import / export | Copying | Copying | Keys may be imported and exported in DER format, which is a structured format and therefore susceptible to read-read inconsistencies and potentially write-read inconsistencies. + +### Validation of copying + +As discussed above, the best strategy for validation of copies appears to be validation by memory poisoning. + +To implement this validation, we need several things: +1. The ability to allocate memory in individual pages. +2. The ability to poison memory pages in the copy functions. +3. Tests that exercise this functionality. + +We can implement (1) as a test helper function that allocates full pages of memory so that we can safely set permissions on them: +```c +unsigned char *mbedtls_test_get_buffer_poisoned_page(size_t nmemb, size_t size) +``` +This allocates a buffer of the requested size that is guaranteed to lie entirely within its own memory page. It also calls `mprotect()` so that the page is inaccessible. + +Requirement (2) can be implemented by creating a function as alluded to above: +```c +void mbedtls_psa_core_poison_memory(unsigned char *buffer, size_t len, int poisoned) +``` +This function should call `mprotect()` on the buffer to prevent it from being accessed (when `poisoned == 1`) or to allow it to be accessed (when `poisoned == 0`). Note that `mprotect()` requires a page-aligned address, so the function may have to do some preliminary work to find the correct page-aligned address that contains `buffer`. + +### Validation of protection by careful access From 0bd87f59595316b5656dda310b9445e0810282c7 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 19 Oct 2023 13:45:21 +0100 Subject: [PATCH 02/37] Change unsigned int to uint8_t Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index a65ed26ac0..d0c19b002a 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -468,13 +468,13 @@ To implement this validation, we need several things: We can implement (1) as a test helper function that allocates full pages of memory so that we can safely set permissions on them: ```c -unsigned char *mbedtls_test_get_buffer_poisoned_page(size_t nmemb, size_t size) +uint8_t *mbedtls_test_get_buffer_poisoned_page(size_t nmemb, size_t size) ``` This allocates a buffer of the requested size that is guaranteed to lie entirely within its own memory page. It also calls `mprotect()` so that the page is inaccessible. Requirement (2) can be implemented by creating a function as alluded to above: ```c -void mbedtls_psa_core_poison_memory(unsigned char *buffer, size_t len, int poisoned) +void mbedtls_psa_core_poison_memory(uint8_t *buffer, size_t len, int poisoned) ``` This function should call `mprotect()` on the buffer to prevent it from being accessed (when `poisoned == 1`) or to allow it to be accessed (when `poisoned == 0`). Note that `mprotect()` requires a page-aligned address, so the function may have to do some preliminary work to find the correct page-aligned address that contains `buffer`. From dae0ad439f71ae4114e1fc547a7e19d903dee59d Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 19 Oct 2023 14:03:51 +0100 Subject: [PATCH 03/37] Add more detail in design of memory poisoning Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index d0c19b002a..b68c874ff8 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -472,10 +472,24 @@ uint8_t *mbedtls_test_get_buffer_poisoned_page(size_t nmemb, size_t size) ``` This allocates a buffer of the requested size that is guaranteed to lie entirely within its own memory page. It also calls `mprotect()` so that the page is inaccessible. +We also need a function to reset the permissions and free the memory: +```c +void mbedtls_test_free_buffer_poisoned_page(uint8_t *buffer, size_t len) +``` +This calls `mprotect()` to restore read and write permissions to the pages of the buffer and then frees the buffer. + +On top of this function we can build the functions for testing mentioned above: +```c +uint8_t *mbedtls_test_get_poisoned_copy(uint8_t *buffer, size_t len) +uint8_t *mbedtls_test_copy_free_poisoned_buffer(uint8_t *poisoned_buffer, uint8_t *original_buffer, size_t len) +``` + Requirement (2) can be implemented by creating a function as alluded to above: ```c void mbedtls_psa_core_poison_memory(uint8_t *buffer, size_t len, int poisoned) ``` This function should call `mprotect()` on the buffer to prevent it from being accessed (when `poisoned == 1`) or to allow it to be accessed (when `poisoned == 0`). Note that `mprotect()` requires a page-aligned address, so the function may have to do some preliminary work to find the correct page-aligned address that contains `buffer`. +Requirement (3) is implemented by wrapping calls to PSA functions with code that creates poisoned copies of its inputs and outputs as described above. + ### Validation of protection by careful access From 3f7e42a7501b43f5efbb1420fdb10e09b2412b77 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 19 Oct 2023 15:14:20 +0100 Subject: [PATCH 04/37] Move implementation by module table earlier Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 28 +++++++++++++------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index b68c874ff8..279e8651df 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -420,6 +420,20 @@ Make sure that such mechanisms preserve the guarantees when buffers overlap. ## Detailed design +### Implementation by module + +Module | Input protection strategy | Output protection strategy | Notes +---|---|---|--- +Hash and MAC | Careful access | Careful access | Low risk of multiple-access as the input and output are raw unformatted data. +Cipher | Copying | Copying | +AEAD | Copying (careful access for additional data) | Copying | +Key derivation | Careful access | Careful access | +Asymmetric signature | Careful access | Copying | Inputs to signatures are passed to a hash. This will no longer hold once PureEdDSA support is implemented. +Asymmetric encryption | Copying | Copying | +Key agreement | Copying | Copying | +PAKE | Copying | Copying | +Key import / export | Copying | Copying | Keys may be imported and exported in DER format, which is a structured format and therefore susceptible to read-read inconsistencies and potentially write-read inconsistencies. + ### Copying functions As discussed above, it is simpler to use a single unified API for copying. Therefore, we create the following functions: @@ -443,20 +457,6 @@ This function would first copy the `buffers->out` buffer to the user-supplied ou Some PSA functions may not use these convenience functions as they may have local optimizations that reduce memory usage. For example, ciphers may be able to use a single intermediate buffer for both input and output. -### Implementation by module - -Module | Input protection strategy | Output protection strategy | Notes ----|---|---|--- -Hash and MAC | Careful access | Careful access | Low risk of multiple-access as the input and output are raw unformatted data. -Cipher | Copying | Copying | -AEAD | Copying (careful access for additional data) | Copying | -Key derivation | Careful access | Careful access | -Asymmetric signature | Careful access | Copying | Inputs to signatures are passed to a hash. This will no longer hold once PureEdDSA support is implemented. -Asymmetric encryption | Copying | Copying | -Key agreement | Copying | Copying | -PAKE | Copying | Copying | -Key import / export | Copying | Copying | Keys may be imported and exported in DER format, which is a structured format and therefore susceptible to read-read inconsistencies and potentially write-read inconsistencies. - ### Validation of copying As discussed above, the best strategy for validation of copies appears to be validation by memory poisoning. From a72b4ca734b80758e23d7c87c2d6409d153e8f5c Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 19 Oct 2023 15:22:15 +0100 Subject: [PATCH 05/37] Modify optimize-testing instructions Mention -flto and whole-program optimization as this is the most important aspect. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 279e8651df..081095f0d1 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -239,7 +239,8 @@ Copy what needs copying. This is broadly straightforward, however there are a fe It is unclear whether the compiler will attempt to optimize away copying operations. Once the copying code is implemented, it should be evaluated to see whether compiler optimization is a problem. Specifically, for the major compilers and platforms supported by Mbed TLS: -* Build Mbed TLS with the compiler set to a high level of optimization (e.g. `-O2` for `gcc`). +* Write a small program that uses a PSA function which copies inputs or outputs. +* Build the program with link-time optimization / full-program optimization enabled (e.g. `-flto` with `gcc`). * Inspect the generated code with `objdump` or a similar tool to see if copying operations are preserved. If copying behaviour is preserved by all major compilers and platforms then assume that compiler optimization is not a problem. From 05ca3d9a1be46e5bc005b6292c64004eb3ddb67f Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 19 Oct 2023 16:45:37 +0100 Subject: [PATCH 06/37] Expand design for validation of careful access Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 081095f0d1..278a6fee45 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -373,13 +373,27 @@ Of all discussed approaches, validation by memory poisoning appears as the best. TODO: write document and reference it here. -### Validation of protection for built-in drivers +### Validation of careful access for built-in drivers -TODO: when there is a requirement on drivers, how to we validate that our built-in implementation meets these requirements? (This may be through testing, review, static analysis or any other means or a combination.) +For PSA functions whose inputs and outputs are not copied, it is important that we validate that the builtin drivers are correctly accessing their inputs and outputs so as not to cause a security issue. Specifically, we must check that each memory location in a shared buffer is not accessed more than once by a driver function. In this section we examine various possible methods for performing this validation. -Note: focusing on read-read inconsistencies for now, as most of the cases where we aren't copying are inputs. +Note: We are focusing on read-read inconsistencies for now, as most of the cases where we aren't copying are inputs. -#### Linux mprotect+ptrace +#### Review + +As with validation of copying, the simplest method of validation we can implement is careful code review. This is the least desirable method of validation for several reasons: +1. It is tedious for the reviewers. +2. Reviewers are prone to make mistakes (especially when performing tedious tasks). +3. It requires engineering time linear in the number of PSA functions to be tested. +4. It cannot assure the quality of third-party drivers, whereas automated tests can be ported to any driver implementation in principle. + +If all other approaches turn out to be prohibitively difficult, code review exists as a fallback option. However, it should be understood that this is far from ideal. + +#### Tests using `mprotect()` + +Checking that a memory location is not accessed more than once may be achieved by using `mprotect()` on a Linux system to cause a segmentation fault whenever a memory access happens. Tests based on this approach are sketched below. + +##### Linux mprotect+ptrace Idea: call `mmap` to allocate memory for arguments and `mprotect` to deny or reenable access. Use `ptrace` from a parent process to react to SIGSEGV from a denied access. On SIGSEGV happening in the faulting region: @@ -390,7 +404,7 @@ Idea: call `mmap` to allocate memory for arguments and `mprotect` to deny or ree Record the addresses that are accessed. Mark the test as failed if the same address is read twice. -#### Debugger + mprotect +##### Debugger + mprotect Idea: call `mmap` to allocate memory for arguments and `mprotect` to deny or reenable access. Use a debugger to handle SIGSEGV (Gdb: set signal catchpoint). If the segfault was due to accessing the protected region: From 4e54abf182a4525540b160938f43a8e8722cee79 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 19 Oct 2023 17:59:45 +0100 Subject: [PATCH 07/37] Add section on possible use of Valgrind tracing Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 278a6fee45..7e2c280202 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -415,6 +415,24 @@ Idea: call `mmap` to allocate memory for arguments and `mprotect` to deny or ree Record the addresses that are accessed. Mark the test as failed if the same address is read twice. This part might be hard to do in the gdb language, so we may want to just log the addresses and then use a separate program to analyze the logs, or do the gdb tasks from Python. +#### Instrumentation (Valgrind) + +An alternative approach is to use a dynamic instrumentation tool (the most obvious being Valgrind) to trace memory accesses and check that each of the important memory addresses is accessed no more than once. + +Valgrind has no tool specifically that checks the property that we are looking for. However, it is possible to generate a memory trace with Valgrind using the following: + +``` +valgrind --tool=lackey --trace-mem=yes --log-file=logfile ./myprogram +``` +This will execute `myprogram` and dump a record of every memory access to `logfile`, with its address and data width. If `myprogram` is a test that does the following: +1. Set up input and output buffers for a PSA function call. +2. Leak the start and end address of each buffer via `print()`. +3. Write data into the input buffer exactly once. +4. Call the PSA function. +5. Read data from the output buffer exactly once. + +Then it should be possible to parse the output from the program and from Valgrind and check that each location was accessed exactly twice: once by the program's setup and once by the PSA function. + #### Discussion The best approach for validating the correctness of memory accesses is an open question that requires further investigation and prototyping. The above sections discuss some possibilities. From 17b3716c5ae33d30229dc55f565878f971aa6b1c Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 20 Oct 2023 18:14:36 +0100 Subject: [PATCH 08/37] Tweak compiler optimization evaluation section * Remove references to the platform - this is unlikely to affect whether copies are optimized. * Note that the evaluation should test extreme optimisation settings. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 7e2c280202..a962839e1b 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -238,12 +238,12 @@ Copy what needs copying. This is broadly straightforward, however there are a fe It is unclear whether the compiler will attempt to optimize away copying operations. -Once the copying code is implemented, it should be evaluated to see whether compiler optimization is a problem. Specifically, for the major compilers and platforms supported by Mbed TLS: +Once the copying code is implemented, it should be evaluated to see whether compiler optimization is a problem. Specifically, for the major compilers supported by Mbed TLS: * Write a small program that uses a PSA function which copies inputs or outputs. -* Build the program with link-time optimization / full-program optimization enabled (e.g. `-flto` with `gcc`). +* Build the program with link-time optimization / full-program optimization enabled (e.g. `-flto` with `gcc`). Try also enabling the most extreme optimization options such as `-Ofast` (`gcc`) and `-Oz` (`clang`). * Inspect the generated code with `objdump` or a similar tool to see if copying operations are preserved. -If copying behaviour is preserved by all major compilers and platforms then assume that compiler optimization is not a problem. +If copying behaviour is preserved by all major compilers then assume that compiler optimization is not a problem. If copying behaviour is optimized away by the compiler, further investigation is needed. Experiment with using the `volatile` keyword to force the compiler not to optimize accesses to the copied buffers. From 51fc6cf3783436a5d4a495d6a7c3aedb7fa75b29 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 20 Oct 2023 18:07:38 +0100 Subject: [PATCH 09/37] Explore sanitizers for memory poisoning Consider MSan, ASan and Valgrind as options for implementing memory poisoning tests. Come to the altered conclusion that Valgrind is the best option. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index a962839e1b..94c2fbc3a1 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -315,7 +315,25 @@ static void copy_to_user(void *copy_buffer, void *const input_buffer, size_t len ``` The reason to poison the memory before calling the library, rather than after the copy-in (and symmetrically for output buffers) is so that the test will fail if we forget to copy, or we copy the wrong thing. This would not be the case if we relied on the library's copy function to do the poisoning: that would only validate that the driver code does not access the memory on the condition that the copy is done as expected. -Note: Extra work may be needed when allocating buffers to ensure that each shared buffer lies in its own separate page, allowing its permissions to be set independently. +##### Options for implementing poisoning + +There are several different ways that poisoning could be implemented: +1. Using Valgrind's memcheck tool. Valgrind provides a macro `VALGRIND_MAKE_MEM_NO_ACCESS` that allows manual memory poisoning. Valgrind memory poisoning is already used for constant-flow testing in Mbed TLS. +2. Using Memory Sanitizer (MSan), which allows us to mark memory as uninitialized. This is also used for constant-flow testing. It is suitable for input buffers only, since it allows us to detect when a poisoned buffer is read but not when it is written. +3. Using Address Sanitizer (ASan). This provides `ASAN_POISON_MEMORY_REGION` which marks memory as inaccessible. +4. Allocating buffers separate pages and calling `mprotect()` to set pages as inaccessible. This has the disadvantage that we will have to manually ensure that buffers sit in their own pages, which likely means making a copy. + +Approach (2) is insufficient for the full testing we require as we need to be able to check both input and output buffers. + +All three remaining approaches are suitable for our purposes. However, approach (4) is more complex than the other two. To implement it, we would need to allocate poisoned buffers in separate memory pages. They would require special handling and test code would likely have to be designed around this special handling. + +Meanwhile, approaches (1) and (3) are much more convenient. We are simply required to call a special macro on some buffer that was allocated by us and the sanitizer takes care of everything else. Of these two, ASan appears to have a limitation related to buffer alignment. From code comments quoted in [the documentation](https://github.com/google/sanitizers/wiki/AddressSanitizerManualPoisoning): + +> This function is not guaranteed to poison the whole region - it may poison only subregion of [addr, addr+size) due to ASan alignment restrictions. + +It is not clear how easy it would be to work around this. Valgrind does not appear to have this limitation (unless Valgrind is simply more poorly documented). Valgrind is therefore slightly preferable compared with ASan, but ASan is an acceptable fallback option. + +**Design decision: Use Valgrind's memcheck tool to implement memory poisoning tests.** **Question: Should we try to build memory poisoning validation on existing Mbed TLS tests, or write new tests for this?** From be868347f4757e42b75261f45baac7ae5bc2d1d6 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 20 Oct 2023 19:25:11 +0100 Subject: [PATCH 10/37] Rewrite design exploration of copy validation Main changes: * New tests are easier to write than first stated * Use of existing tests is ledd beneficial * But using existing tests is a benefit if it can be done transparently Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 55 ++++++++------------------ 1 file changed, 17 insertions(+), 38 deletions(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 94c2fbc3a1..31722337e0 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -337,47 +337,26 @@ It is not clear how easy it would be to work around this. Valgrind does not appe **Question: Should we try to build memory poisoning validation on existing Mbed TLS tests, or write new tests for this?** -##### Validation with existing tests - -It should be possible to integrate memory poisoning validation with existing tests. This has two main advantages: -* All of the tests are written already, potentially saving development time. -* The code coverage of these tests is already much greater than would be achievable writing new tests from scratch. - -In an ideal world, we would be able to take a grand, encompassing approach whereby we would simply replace the implementation of `mbedtls_calloc()` and all tests would transparently run with memory poisoning enabled. Unfortunately, there are some significant difficulties with this idea: -* We cannot automatically distinguish which allocated buffers are shared buffers that need memory poisoning enabled. -* Some input buffers to tested functions may be stack allocated so cannot be poisoned automatically. - -Instead, consider a more modest strategy. Create a function: -```c -uint8_t *mbedtls_test_get_poisoned_copy(uint8_t *buffer, size_t len) -``` -that creates a poisoned copy of a buffer ready to be passed to the PSA function. Also create: -```c -uint8_t *mbedtls_test_copy_free_poisoned_buffer(uint8_t *poisoned_buffer, uint8_t *original_buffer, size_t len) -``` -which copies the poisoned buffer contents back into the original buffer and frees the poisoned copy. - -In each test case, manually wrap any calls to PSA functions in code that substitutes a poisoned buffer. For example, the code: -```c -psa_api_do_some_operation(input, input_len, output, output_len); -``` -Would be transformed to: -```c -input_poisoned = mbedtls_test_get_poisoned_copy(input, input_len); -output_poisoned = mbedtls_test_get_poisoned_copy(output, output_len); -psa_api_do_some_operation(input_poisoned, input_len, output_poisoned, output_len); -mbedtls_test_copy_free_poisoned_buffer(input_poisoned, input, input_len); -mbedtls_test_copy_free_poisoned_buffer(output_poisoned, output, output_len); -``` -Further interface design or careful use of macros may make this a little less cumbersome than it seems in this example. - -The poison copying functions should be written so as to evaluate to no-ops based on the value of a config option. They also need not be added to all tests, only to a 'covering set' of important tests. - ##### Validation with new tests -Validation with newly created tests is initially simpler to implement than using the existing tests, since the tests can know about memory poisoning from the start. However, re-implementing testing for most PSA interfaces (even only basic positive testing) is a large undertaking. Furthermore, not much is gained over the previous approach, given that it seems straightforward to wrap PSA function calls in existing tests with poisoning code. +Validation with newly created tests would be simpler to implement than using existing tests, since the tests can be written to take into account memory poisoning. It is also possible to build such a testsuite on top of existing tests - `mbedtls_test_psa_exercise_key` is a test helper that already exercises most PSA interfaces, so implementing the tests could be as simple as extending it. -**Design decision: Add memory poisoning code to existing tests rather than creating new ones, since this is simpler and produces greater coverage.** +Additionally, we can ensure that all functions are exercised by automatically generating test data files. + +##### Validation with existing tests + +An alternative approach would be to integrate memory poisoning validation with existing tests. This has two main advantages: +* All of the tests are written already, potentially saving development time. +* The code coverage of these tests is greater than would be achievable writing new tests from scratch. In practice this advantage is small as buffer copying will take place in the dispatch layer. The tests are therefore independent of the values of parameters passed to the driver, so extra coverage in these parameters does not gain anything. + +It may be possible to transparently implement memory poisoning so that existing tests can work without modification. This would be achieved by replacing the implementation of `malloc()` with one that allocates poisoned buffers. However, there are some difficulties with this: +* Not all buffers allocated by tests are used as inputs and outputs to PSA functions being tested. +* Those buffers that are inputs to a PSA function need to be unpoisoned right up until the function is called, so that they can be filled with input data. +* Those buffers that are outputs from a PSA function need to be unpoisoned straight after the function returns, so that they can be read to check the output is correct. + +These issues may be solved by creating some kind of test wrapper around every PSA function call that poisons the memory. However, it is unclear how straightforward this will be in practice. If this is simple to achieve, the extra coverage and time saved on new tests will be a benefit. If not, writing new tests is the best strategy. + +**Design decision: Attempt to add memory poisoning transparently to existing tests. If this proves difficult, write new tests instead.** #### Discussion From 16dac00cb9b5d53b36541eb8132cbb935b32ada8 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 23 Oct 2023 18:34:43 +0100 Subject: [PATCH 11/37] Add skeleton of detailed design rewrite In light of choosing Valgrind/ASan over mprotect()-based poisoning, update the detailed design of copy validation. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 42 +++++++++++--------------- 1 file changed, 17 insertions(+), 25 deletions(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 31722337e0..0d48324262 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -489,37 +489,29 @@ Some PSA functions may not use these convenience functions as they may have loca ### Validation of copying -As discussed above, the best strategy for validation of copies appears to be validation by memory poisoning. +As discussed in the [design exploration of copying validation](#validation-of-copying), the best strategy for validation of copies appears to be validation by memory poisoning, implemented using Valgrind. -To implement this validation, we need several things: -1. The ability to allocate memory in individual pages. -2. The ability to poison memory pages in the copy functions. -3. Tests that exercise this functionality. - -We can implement (1) as a test helper function that allocates full pages of memory so that we can safely set permissions on them: +To perform memory poisoning, we must implement the function alluded to in [Validation of copying by memory poisoning](#validation-of-copying-by-memory-poisoning): ```c -uint8_t *mbedtls_test_get_buffer_poisoned_page(size_t nmemb, size_t size) +mbedtls_psa_core_poison_memory(uint8_t *buffer, size_t length, int should_poison); ``` -This allocates a buffer of the requested size that is guaranteed to lie entirely within its own memory page. It also calls `mprotect()` so that the page is inaccessible. +This should either poison or unpoison the given buffer based on the value of `should_poison`: +* When `should_poison == 1`, this is equivalent to calling `VALGRIND_MAKE_MEM_NOACCESS(buffer, length)`. +* When `should_poison == 0`, this is equivalent to calling `VALGRIND_MAKE_MEM_DEFINED(buffer, length)`. -We also need a function to reset the permissions and free the memory: -```c -void mbedtls_test_free_buffer_poisoned_page(uint8_t *buffer, size_t len) -``` -This calls `mprotect()` to restore read and write permissions to the pages of the buffer and then frees the buffer. +We may choose one of two approaches. As discussed in [the design exploration](#validation-with-existing-tests), the first is preferred: +* Use transparent allocation-based memory poisoning. +* Use memory poisoning functions and a new testsuite. -On top of this function we can build the functions for testing mentioned above: -```c -uint8_t *mbedtls_test_get_poisoned_copy(uint8_t *buffer, size_t len) -uint8_t *mbedtls_test_copy_free_poisoned_buffer(uint8_t *poisoned_buffer, uint8_t *original_buffer, size_t len) -``` +We will specify the particularities of each approach's implementation below. -Requirement (2) can be implemented by creating a function as alluded to above: -```c -void mbedtls_psa_core_poison_memory(uint8_t *buffer, size_t len, int poisoned) -``` -This function should call `mprotect()` on the buffer to prevent it from being accessed (when `poisoned == 1`) or to allow it to be accessed (when `poisoned == 0`). Note that `mprotect()` requires a page-aligned address, so the function may have to do some preliminary work to find the correct page-aligned address that contains `buffer`. +#### Transparent allocation-based memory poisoning + +In order to implement transparent memory poisoning we require a wrapper around all PSA function calls that poisons any input and output buffers. + +The easiest way to do this is to create a header that `#define`s PSA function names to be wrapped versions of themselves. + +#### Memory poisoning functions and a new testsuite -Requirement (3) is implemented by wrapping calls to PSA functions with code that creates poisoned copies of its inputs and outputs as described above. ### Validation of protection by careful access From ded14a2c0214d30744f719ed4a1892e2aff3cba6 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 23 Oct 2023 18:58:41 +0100 Subject: [PATCH 12/37] Add example wrapper function implementation Give an example wrapper foir psa_aead_update for the transparent testing option. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 0d48324262..b10953824e 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -509,7 +509,27 @@ We will specify the particularities of each approach's implementation below. In order to implement transparent memory poisoning we require a wrapper around all PSA function calls that poisons any input and output buffers. -The easiest way to do this is to create a header that `#define`s PSA function names to be wrapped versions of themselves. +The easiest way to do this is to create wrapper functions that poison the memory and then `#define` PSA function names to be wrapped versions of themselves. For example, to replace `psa_aead_update()`: +```c +psa_status_t mem_poison_psa_aead_update(psa_aead_operation_t *operation, + const uint8_t *input, + size_t input_length, + uint8_t *output, + size_t output_size, + size_t *output_length) +{ + mbedtls_psa_core_poison_memory(input, input_length, 1); + mbedtls_psa_core_poison_memory(output, output_size, 1); + psa_status_t status = psa_aead_update(operation, input, input_length, + output, output_size, output_length); + mbedtls_psa_core_poison_memory(input, input_length, 0); + mbedtls_psa_core_poison_memory(output, output_size, 0); + + return status; +} + +#define psa_aead_update(...) mem_poison_psa_aead_update(__VA_ARGS__) +``` #### Memory poisoning functions and a new testsuite From f889e0fa0a534fc06277d693732545570bc29558 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 23 Oct 2023 19:01:21 +0100 Subject: [PATCH 13/37] Replace vague 'above' with a reference for ease-of-navigation Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index b10953824e..2c2da8d883 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -466,7 +466,7 @@ Key import / export | Copying | Copying | Keys may be imported and exported in D ### Copying functions -As discussed above, it is simpler to use a single unified API for copying. Therefore, we create the following functions: +As discussed in [Copying code](#copying-code), it is simpler to use a single unified API for copying. Therefore, we create the following functions: * `psa_crypto_copy_input(const uint8_t *input, size_t input_length, uint8_t *input_copy, size_t input_copy_length)` * `psa_crypto_copy_output(const uint8_t *output_copy, size_t output_copy_length, uint8_t *output, size_t output_length)` From cbf068dbee8abc647899953b93590c215c619703 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 23 Oct 2023 19:03:10 +0100 Subject: [PATCH 14/37] Fix broken reference Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 2c2da8d883..f0f27811cd 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -99,7 +99,7 @@ Example: the PSA Firmware Framework 1.0 forbids shared memory between partitions #### Careful accesses -The following rules guarantee that shared memory cannot result in a security violation other than [write-read feedback](#write-read feedback): +The following rules guarantee that shared memory cannot result in a security violation other than [write-read feedback](#write-read-feedback): * Never read the same input twice at the same index. * Never read back from an output. From c61ddb2089c9d07ed6e9241620d5dc14c6134110 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 23 Oct 2023 19:18:50 +0100 Subject: [PATCH 15/37] Add C language annotation to code block Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index f0f27811cd..5ae3ecbbe3 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -298,7 +298,7 @@ Proposed general idea: in test code, “poison” the memory area used by input In the library, the code that does the copying temporarily unpoisons the memory by calling a test hook. -``` +```c static void copy_to_user(void *copy_buffer, void *const input_buffer, size_t length) { #if defined(MBEDTLS_TEST_HOOKS) if (mbedtls_psa_core_poison_memory != NULL) { From 52df6207368f0cc51b720f882e32eb88bc12b6c1 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 23 Oct 2023 19:38:01 +0100 Subject: [PATCH 16/37] Use ASan for memory poisoning as well as Valgrind Also add information about ASan from Microsoft docs. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 5ae3ecbbe3..c3894a31f5 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -331,9 +331,11 @@ Meanwhile, approaches (1) and (3) are much more convenient. We are simply requir > This function is not guaranteed to poison the whole region - it may poison only subregion of [addr, addr+size) due to ASan alignment restrictions. -It is not clear how easy it would be to work around this. Valgrind does not appear to have this limitation (unless Valgrind is simply more poorly documented). Valgrind is therefore slightly preferable compared with ASan, but ASan is an acceptable fallback option. +Specifically, ASan will round the buffer size down to 8 bytes before poisoning due to details of its implementation. For more information on this, see [Microsoft documentation of this feature](https://learn.microsoft.com/en-us/cpp/sanitizers/asan-runtime?view=msvc-170#alignment-requirements-for-addresssanitizer-poisoning). -**Design decision: Use Valgrind's memcheck tool to implement memory poisoning tests.** +It should be possible to work around this by manually rounding buffer lengths up to the nearest multiple of 8 in the poisoning function, although it's remotely possible that this will cause other problems. Valgrind does not appear to have this limitation (unless Valgrind is simply more poorly documented). However, running tests under Valgrind causes a much greater slowdown compared with ASan. As a result, it would be beneficial to implement support for both Valgrind and ASan, to give the extra flexibility to choose either performance or accuracy as required. This should be simple as both have very similar memory poisoning interfaces. + +**Design decision: Implement memory poisoning tests with both Valgrind's memcheck and ASan manual poisoning.** **Question: Should we try to build memory poisoning validation on existing Mbed TLS tests, or write new tests for this?** @@ -489,15 +491,15 @@ Some PSA functions may not use these convenience functions as they may have loca ### Validation of copying -As discussed in the [design exploration of copying validation](#validation-of-copying), the best strategy for validation of copies appears to be validation by memory poisoning, implemented using Valgrind. +As discussed in the [design exploration of copying validation](#validation-of-copying), the best strategy for validation of copies appears to be validation by memory poisoning, implemented using Valgrind and ASan. To perform memory poisoning, we must implement the function alluded to in [Validation of copying by memory poisoning](#validation-of-copying-by-memory-poisoning): ```c mbedtls_psa_core_poison_memory(uint8_t *buffer, size_t length, int should_poison); ``` This should either poison or unpoison the given buffer based on the value of `should_poison`: -* When `should_poison == 1`, this is equivalent to calling `VALGRIND_MAKE_MEM_NOACCESS(buffer, length)`. -* When `should_poison == 0`, this is equivalent to calling `VALGRIND_MAKE_MEM_DEFINED(buffer, length)`. +* When `should_poison == 1`, this is equivalent to calling `VALGRIND_MAKE_MEM_NOACCESS(buffer, length)` or `ASAN_POISON_MEMORY_REGION(buffer, length)`. +* When `should_poison == 0`, this is equivalent to calling `VALGRIND_MAKE_MEM_DEFINED(buffer, length)` or `ASAN_UNPOISON_MEMORY_REGION(buffer, length)`. We may choose one of two approaches. As discussed in [the design exploration](#validation-with-existing-tests), the first is preferred: * Use transparent allocation-based memory poisoning. From 806055edbf5b953aa9b5d5cec5e161c885fbb2f2 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 23 Oct 2023 19:53:30 +0100 Subject: [PATCH 17/37] Refactor note on preferred poison-test approach Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index c3894a31f5..9929573fc0 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -501,10 +501,12 @@ This should either poison or unpoison the given buffer based on the value of `sh * When `should_poison == 1`, this is equivalent to calling `VALGRIND_MAKE_MEM_NOACCESS(buffer, length)` or `ASAN_POISON_MEMORY_REGION(buffer, length)`. * When `should_poison == 0`, this is equivalent to calling `VALGRIND_MAKE_MEM_DEFINED(buffer, length)` or `ASAN_UNPOISON_MEMORY_REGION(buffer, length)`. -We may choose one of two approaches. As discussed in [the design exploration](#validation-with-existing-tests), the first is preferred: +We may choose one of two approaches: * Use transparent allocation-based memory poisoning. * Use memory poisoning functions and a new testsuite. +As discussed in [the design exploration](#validation-with-existing-tests), the transparent approach is preferred. + We will specify the particularities of each approach's implementation below. #### Transparent allocation-based memory poisoning From 8f905c289d7ba60d339a7906836b3922f81a05a0 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 23 Oct 2023 20:08:38 +0100 Subject: [PATCH 18/37] Add reference to test hooks in detailed design Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 9929573fc0..dd2f87f2a8 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -501,7 +501,9 @@ This should either poison or unpoison the given buffer based on the value of `sh * When `should_poison == 1`, this is equivalent to calling `VALGRIND_MAKE_MEM_NOACCESS(buffer, length)` or `ASAN_POISON_MEMORY_REGION(buffer, length)`. * When `should_poison == 0`, this is equivalent to calling `VALGRIND_MAKE_MEM_DEFINED(buffer, length)` or `ASAN_UNPOISON_MEMORY_REGION(buffer, length)`. -We may choose one of two approaches: +The PSA copying function must then have test hooks implemented as outlined in [Validation of copying by memory poisoning](#validation-of-copying-by-memory-poisoning). + +For test implementation, we may choose one of two approaches: * Use transparent allocation-based memory poisoning. * Use memory poisoning functions and a new testsuite. From 6c51207602ecb9482988c0b7829e4ff8f8550df5 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 23 Oct 2023 20:25:14 +0100 Subject: [PATCH 19/37] Add notes about configuration of poisoning tests Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index dd2f87f2a8..874621b10d 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -539,5 +539,8 @@ psa_status_t mem_poison_psa_aead_update(psa_aead_operation_t *operation, #### Memory poisoning functions and a new testsuite +#### Configuration of poisoning tests + +Since the memory poisoning tests will require the use of interfaces specific to the sanitizers used to poison memory, they must be guarded by new config options, for example `PSA_TEST_COPYING_ASAN` and `PSA_TEST_COPYING_VALGRIND`, as well as `MBEDTLS_TEST_HOOKS`. These would be analogous to the existing `MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN` and `MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND`. Since they require special tooling, these options should not be enabled in either the `default` or `full` configurations. Instead, as with the constant flow testing options, they should be enabled in a new component in `all.sh` that performs the copy testing with Valgrind or ASan. ### Validation of protection by careful access From 730dea31cbc710fadb48bc4f7295eca0b51a581e Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 23 Oct 2023 20:35:35 +0100 Subject: [PATCH 20/37] Rewrite incorrect description of psa_exercise_key And clarify our potential use of it as a starting point for writing memory poisoning tests from scratch. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 874621b10d..36fc569dbc 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -341,7 +341,7 @@ It should be possible to work around this by manually rounding buffer lengths up ##### Validation with new tests -Validation with newly created tests would be simpler to implement than using existing tests, since the tests can be written to take into account memory poisoning. It is also possible to build such a testsuite on top of existing tests - `mbedtls_test_psa_exercise_key` is a test helper that already exercises most PSA interfaces, so implementing the tests could be as simple as extending it. +Validation with newly created tests would be simpler to implement than using existing tests, since the tests can be written to take into account memory poisoning. It is also possible to build such a testsuite using existing tests as a starting point - `mbedtls_test_psa_exercise_key` is a test helper that already exercises many PSA operations on a key. This would need to be extended to cover operations without keys (e.g. hashes) and multipart operations, but it provides a good base from which to build all of the required testing. Additionally, we can ensure that all functions are exercised by automatically generating test data files. From 09c84ef0cd39610c5070b93dbcbb095bc1e7ded1 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 23 Oct 2023 20:43:03 +0100 Subject: [PATCH 21/37] Add lengths to convenience interface sketch Add lengths to structs in the convenience functions to allocate and copy input and output buffers. It seems better to ensure we always store a buffer with its length. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 36fc569dbc..251c699b07 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -477,13 +477,13 @@ These seem to be a repeat of the same function, however it is useful to retain t Given that the majority of functions will be allocating memory on the heap to copy, it may help to build convenience functions that allocate the memory as well. One function allocates and copies the buffers: -* `psa_crypto_alloc_and_copy(const uint8_t *input, size_t input_length, uint8_t *output, size_t output_length, struct {uint8_t *inp, uint8_t *out} *buffers)` +* `psa_crypto_alloc_and_copy(const uint8_t *input, size_t input_length, uint8_t *output, size_t output_length, struct {uint8_t *inp, size_t inp_len, uint8_t *out, size_t out_len} *buffers)` This function allocates an input and output buffer in `buffers` and copy the input from the user-supplied input buffer to `buffers->inp`. An analogous function is needed to copy and free the buffers: -* `psa_crypto_copy_and_free(struct {uint8_t *inp, uint8_t *out} buffers, const uint8_t *input, size_t input_length, const uint8_t *output, size_t output_length)` +* `psa_crypto_copy_and_free(struct {uint8_t *inp, size_t inp_len, uint8_t *out, size_t out_len} buffers, const uint8_t *input, size_t input_length, const uint8_t *output, size_t output_length)` This function would first copy the `buffers->out` buffer to the user-supplied output buffer and then free `buffers->inp` and `buffers->out`. From 56aa1b3fbb5064b814fc8be88f314860b1de8bc7 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 23 Oct 2023 21:20:01 +0100 Subject: [PATCH 22/37] Add exploration section on FVP testing Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 251c699b07..bb8be2b864 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -432,6 +432,14 @@ This will execute `myprogram` and dump a record of every memory access to `logfi Then it should be possible to parse the output from the program and from Valgrind and check that each location was accessed exactly twice: once by the program's setup and once by the PSA function. +#### Fixed Virtual Platform testing + +It may be possible to measure double accesses by running tests on a Fixed Virtual Platform such as Corstone 310 ecosystem FVP, available [here](https://developer.arm.com/downloads/-/arm-ecosystem-fvps). There exists a pre-packaged example program for the Corstone 310 FVP available as part of the Open IoT SDK [here](https://git.gitlab.arm.com/iot/open-iot-sdk/examples/sdk-examples/-/tree/main/examples/mbedtls/cmsis-rtx/corstone-310) that could provide a starting point for a set of tests. + +Running on an FVP allows two approaches to careful-access testing: +* Convenient scripted use of a debugger with [Iris](https://developer.arm.com/documentation/101196/latest/). This allows memory watchpoints to be set, perhaps more flexibly than with GDB. +* Tracing of all memory accesses with [Tarmac Trace](https://developer.arm.com/documentation/100964/1123/Plug-ins-for-Fast-Models/TarmacTrace). To validate the single-access properties, the [processor memory access trace source](https://developer.arm.com/documentation/100964/1123/Plug-ins-for-Fast-Models/TarmacTrace/Processor-memory-access-trace) can be used to output all memory accesses happening on the FVP. This output can then be easily parsed and processed to ensure that the input and output buffers are accessed only once. The addresses of buffers can either be leaked by the program through printing to the serial port or set to fixed values in the FVP's linker script. + #### Discussion The best approach for validating the correctness of memory accesses is an open question that requires further investigation and prototyping. The above sections discuss some possibilities. From c7ccbf51574a24a0830f95d1c19e591d5d1384f1 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 24 Oct 2023 15:43:12 +0100 Subject: [PATCH 23/37] Add detailed design section for careful access This consists in outlining the prototyping and evaluation of different possible testing approaches. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index bb8be2b864..14751ea897 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -552,3 +552,22 @@ psa_status_t mem_poison_psa_aead_update(psa_aead_operation_t *operation, Since the memory poisoning tests will require the use of interfaces specific to the sanitizers used to poison memory, they must be guarded by new config options, for example `PSA_TEST_COPYING_ASAN` and `PSA_TEST_COPYING_VALGRIND`, as well as `MBEDTLS_TEST_HOOKS`. These would be analogous to the existing `MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN` and `MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND`. Since they require special tooling, these options should not be enabled in either the `default` or `full` configurations. Instead, as with the constant flow testing options, they should be enabled in a new component in `all.sh` that performs the copy testing with Valgrind or ASan. ### Validation of protection by careful access + +As concluded in [Validation of careful access for built-in drivers](validation-of-careful-access-for-built-in-drivers), the best approach to validation of careful accesses is an open question. Designing this will involve prototyping each possible approach and choosing the one that seems most fruitful. + +For each of the test strategies discussed: +1. Take 1-2 days to create a basic prototype of a test that uses the approach. +2. Document the prototype - write a short guide that can be followed to arrive at the same prototype. +3. Evaluate the prototype according to its usefulness. The criteria of evaluation should include: + * Ease of implementation - Was the prototype simple to implement? Having implemented it, is it simple to extend it to do all of the required testing? + * Flexibility - Could the prototype be extended to cover other careful-access testing that may be needed in future? + * Performance - Does the test method perform well? Will it cause significant slowdown to CI jobs? + * Ease of reproduction - Does the prototype require a particular platform or tool to be set up? How easy would it be for an external user to run the prototype? + * Comprehensibility - Accounting for the lower code quality of a prototype, would developers unfamiliar with the tests based on the prototype be able to understand them easily? + +Once each prototype is complete, choose the best approach to implement the careful-access testing. Implement tests using this approach for each of the PSA interfaces that require careful-access testing: +* Hash +* MAC +* AEAD (additional data only) +* Key derivation +* Asymmetric signature (input only) From f95767ad565b8d8a7c9fd33859d696a1ceb545ee Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 24 Oct 2023 16:16:36 +0100 Subject: [PATCH 24/37] Clarify use of new tests for careful-access New tests are needed (rather than existing ones) because the complexity of setting up careful-access tests would make it difficult to build atop existing tests. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 14751ea897..452ea7c54d 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -446,6 +446,12 @@ The best approach for validating the correctness of memory accesses is an open q However, there is one additional consideration that may make this easier. The careful-access approach to memory protection is only planned for hash and MAC algorithms. These lend themselves to a linear access pattern on input data; it may be simpler to test that a linear pattern is followed, rather than a random-access single-access-per-location pattern. +##### New vs existing tests + +Most of the test methods discussed above need extra setup. Some require leaking of buffer bounds, predictable memory access patterns or allocation of special buffers. FVP testing even requires the tests to be run on a non-host target. + +With this complexity in mind it does not seem feasible to run careful-access tests using existing testsuites. Instead, new tests should be written that exercise the drivers in the required way. Fortunately, the only interfaces that need testing are hash, MAC, AEAD (testing over AD only), Key derivation and Asymmetric signature, which limits the number of new tests that must be written. + ## Analysis of argument protection in built-in drivers TODO: analyze the built-in implementations of mechanisms for which there is a requirement on drivers. By code inspection, how satisfied are we that they meet the requirement? From 2711d23976d6b4c562f51efdc9e44fbf6e151ca2 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 25 Oct 2023 15:07:58 +0100 Subject: [PATCH 25/37] Fix broken links Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 452ea7c54d..c812387eec 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -228,7 +228,7 @@ This section explains how Mbed TLS implements the shared memory protection strat * The built-in implementations of cryptographic mechanisms with arguments whose access needs to be protected shall protect those arguments. -Justification: see “[Susceptibility of different mechanisms](susceptibility-of-different-mechanisms)”. +Justification: see “[Susceptibility of different mechanisms](#susceptibility-of-different-mechanisms)”. ### Implementation of copying @@ -559,7 +559,7 @@ Since the memory poisoning tests will require the use of interfaces specific to ### Validation of protection by careful access -As concluded in [Validation of careful access for built-in drivers](validation-of-careful-access-for-built-in-drivers), the best approach to validation of careful accesses is an open question. Designing this will involve prototyping each possible approach and choosing the one that seems most fruitful. +As concluded in [Validation of careful access for built-in drivers](#validation-of-careful-access-for-built-in-drivers), the best approach to validation of careful accesses is an open question. Designing this will involve prototyping each possible approach and choosing the one that seems most fruitful. For each of the test strategies discussed: 1. Take 1-2 days to create a basic prototype of a test that uses the approach. From 8e58ccb4f6690aa2f045c3897aee4433ef3ff1ce Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 25 Oct 2023 15:13:29 +0100 Subject: [PATCH 26/37] Add blank lines before lists This widens compatibility with different dialects of Markdown. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index c812387eec..c1e6ba1809 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -318,6 +318,7 @@ The reason to poison the memory before calling the library, rather than after th ##### Options for implementing poisoning There are several different ways that poisoning could be implemented: + 1. Using Valgrind's memcheck tool. Valgrind provides a macro `VALGRIND_MAKE_MEM_NO_ACCESS` that allows manual memory poisoning. Valgrind memory poisoning is already used for constant-flow testing in Mbed TLS. 2. Using Memory Sanitizer (MSan), which allows us to mark memory as uninitialized. This is also used for constant-flow testing. It is suitable for input buffers only, since it allows us to detect when a poisoned buffer is read but not when it is written. 3. Using Address Sanitizer (ASan). This provides `ASAN_POISON_MEMORY_REGION` which marks memory as inaccessible. @@ -348,10 +349,12 @@ Additionally, we can ensure that all functions are exercised by automatically ge ##### Validation with existing tests An alternative approach would be to integrate memory poisoning validation with existing tests. This has two main advantages: + * All of the tests are written already, potentially saving development time. * The code coverage of these tests is greater than would be achievable writing new tests from scratch. In practice this advantage is small as buffer copying will take place in the dispatch layer. The tests are therefore independent of the values of parameters passed to the driver, so extra coverage in these parameters does not gain anything. It may be possible to transparently implement memory poisoning so that existing tests can work without modification. This would be achieved by replacing the implementation of `malloc()` with one that allocates poisoned buffers. However, there are some difficulties with this: + * Not all buffers allocated by tests are used as inputs and outputs to PSA functions being tested. * Those buffers that are inputs to a PSA function need to be unpoisoned right up until the function is called, so that they can be filled with input data. * Those buffers that are outputs from a PSA function need to be unpoisoned straight after the function returns, so that they can be read to check the output is correct. @@ -363,6 +366,7 @@ These issues may be solved by creating some kind of test wrapper around every PS #### Discussion Of all discussed approaches, validation by memory poisoning appears as the best. This is because it: + * Does not require complex linking against different versions of `malloc()` (as is the case with the memory pool approach). * Allows automated testing (unlike the review approach). @@ -381,6 +385,7 @@ Note: We are focusing on read-read inconsistencies for now, as most of the cases #### Review As with validation of copying, the simplest method of validation we can implement is careful code review. This is the least desirable method of validation for several reasons: + 1. It is tedious for the reviewers. 2. Reviewers are prone to make mistakes (especially when performing tedious tasks). 3. It requires engineering time linear in the number of PSA functions to be tested. @@ -424,6 +429,7 @@ Valgrind has no tool specifically that checks the property that we are looking f valgrind --tool=lackey --trace-mem=yes --log-file=logfile ./myprogram ``` This will execute `myprogram` and dump a record of every memory access to `logfile`, with its address and data width. If `myprogram` is a test that does the following: + 1. Set up input and output buffers for a PSA function call. 2. Leak the start and end address of each buffer via `print()`. 3. Write data into the input buffer exactly once. @@ -437,6 +443,7 @@ Then it should be possible to parse the output from the program and from Valgrin It may be possible to measure double accesses by running tests on a Fixed Virtual Platform such as Corstone 310 ecosystem FVP, available [here](https://developer.arm.com/downloads/-/arm-ecosystem-fvps). There exists a pre-packaged example program for the Corstone 310 FVP available as part of the Open IoT SDK [here](https://git.gitlab.arm.com/iot/open-iot-sdk/examples/sdk-examples/-/tree/main/examples/mbedtls/cmsis-rtx/corstone-310) that could provide a starting point for a set of tests. Running on an FVP allows two approaches to careful-access testing: + * Convenient scripted use of a debugger with [Iris](https://developer.arm.com/documentation/101196/latest/). This allows memory watchpoints to be set, perhaps more flexibly than with GDB. * Tracing of all memory accesses with [Tarmac Trace](https://developer.arm.com/documentation/100964/1123/Plug-ins-for-Fast-Models/TarmacTrace). To validate the single-access properties, the [processor memory access trace source](https://developer.arm.com/documentation/100964/1123/Plug-ins-for-Fast-Models/TarmacTrace/Processor-memory-access-trace) can be used to output all memory accesses happening on the FVP. This output can then be easily parsed and processed to ensure that the input and output buffers are accessed only once. The addresses of buffers can either be leaked by the program through printing to the serial port or set to fixed values in the FVP's linker script. @@ -512,12 +519,14 @@ To perform memory poisoning, we must implement the function alluded to in [Valid mbedtls_psa_core_poison_memory(uint8_t *buffer, size_t length, int should_poison); ``` This should either poison or unpoison the given buffer based on the value of `should_poison`: + * When `should_poison == 1`, this is equivalent to calling `VALGRIND_MAKE_MEM_NOACCESS(buffer, length)` or `ASAN_POISON_MEMORY_REGION(buffer, length)`. * When `should_poison == 0`, this is equivalent to calling `VALGRIND_MAKE_MEM_DEFINED(buffer, length)` or `ASAN_UNPOISON_MEMORY_REGION(buffer, length)`. The PSA copying function must then have test hooks implemented as outlined in [Validation of copying by memory poisoning](#validation-of-copying-by-memory-poisoning). For test implementation, we may choose one of two approaches: + * Use transparent allocation-based memory poisoning. * Use memory poisoning functions and a new testsuite. @@ -562,6 +571,7 @@ Since the memory poisoning tests will require the use of interfaces specific to As concluded in [Validation of careful access for built-in drivers](#validation-of-careful-access-for-built-in-drivers), the best approach to validation of careful accesses is an open question. Designing this will involve prototyping each possible approach and choosing the one that seems most fruitful. For each of the test strategies discussed: + 1. Take 1-2 days to create a basic prototype of a test that uses the approach. 2. Document the prototype - write a short guide that can be followed to arrive at the same prototype. 3. Evaluate the prototype according to its usefulness. The criteria of evaluation should include: @@ -572,6 +582,7 @@ For each of the test strategies discussed: * Comprehensibility - Accounting for the lower code quality of a prototype, would developers unfamiliar with the tests based on the prototype be able to understand them easily? Once each prototype is complete, choose the best approach to implement the careful-access testing. Implement tests using this approach for each of the PSA interfaces that require careful-access testing: + * Hash * MAC * AEAD (additional data only) From 2b86df87da757322703a19ba9d6ebad6f3664aa0 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 25 Oct 2023 15:26:27 +0100 Subject: [PATCH 27/37] De-duplicate section titles Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index c1e6ba1809..cdcccd9287 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -363,7 +363,7 @@ These issues may be solved by creating some kind of test wrapper around every PS **Design decision: Attempt to add memory poisoning transparently to existing tests. If this proves difficult, write new tests instead.** -#### Discussion +#### Discussion of copying validation Of all discussed approaches, validation by memory poisoning appears as the best. This is because it: @@ -447,7 +447,7 @@ Running on an FVP allows two approaches to careful-access testing: * Convenient scripted use of a debugger with [Iris](https://developer.arm.com/documentation/101196/latest/). This allows memory watchpoints to be set, perhaps more flexibly than with GDB. * Tracing of all memory accesses with [Tarmac Trace](https://developer.arm.com/documentation/100964/1123/Plug-ins-for-Fast-Models/TarmacTrace). To validate the single-access properties, the [processor memory access trace source](https://developer.arm.com/documentation/100964/1123/Plug-ins-for-Fast-Models/TarmacTrace/Processor-memory-access-trace) can be used to output all memory accesses happening on the FVP. This output can then be easily parsed and processed to ensure that the input and output buffers are accessed only once. The addresses of buffers can either be leaked by the program through printing to the serial port or set to fixed values in the FVP's linker script. -#### Discussion +#### Discussion of careful-access validation The best approach for validating the correctness of memory accesses is an open question that requires further investigation and prototyping. The above sections discuss some possibilities. @@ -510,7 +510,7 @@ This function would first copy the `buffers->out` buffer to the user-supplied ou Some PSA functions may not use these convenience functions as they may have local optimizations that reduce memory usage. For example, ciphers may be able to use a single intermediate buffer for both input and output. -### Validation of copying +### Implementation of copying validation As discussed in the [design exploration of copying validation](#validation-of-copying), the best strategy for validation of copies appears to be validation by memory poisoning, implemented using Valgrind and ASan. From c59913822e6817afced3978f1591bcd4306af683 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 25 Oct 2023 15:33:50 +0100 Subject: [PATCH 28/37] Remove references to new-test approach in design This is already covered in the design exploration and since the other approach was chose, we do not need to discuss it in the detailed design section. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index cdcccd9287..82f4bd0610 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -525,14 +525,7 @@ This should either poison or unpoison the given buffer based on the value of `sh The PSA copying function must then have test hooks implemented as outlined in [Validation of copying by memory poisoning](#validation-of-copying-by-memory-poisoning). -For test implementation, we may choose one of two approaches: - -* Use transparent allocation-based memory poisoning. -* Use memory poisoning functions and a new testsuite. - -As discussed in [the design exploration](#validation-with-existing-tests), the transparent approach is preferred. - -We will specify the particularities of each approach's implementation below. +As discussed in [the design exploration](#validation-with-existing-tests), the preferred approach for implementing copy-testing is to implement it transparently using existing tests. This is specified in more detail below. #### Transparent allocation-based memory poisoning @@ -560,8 +553,6 @@ psa_status_t mem_poison_psa_aead_update(psa_aead_operation_t *operation, #define psa_aead_update(...) mem_poison_psa_aead_update(__VA_ARGS__) ``` -#### Memory poisoning functions and a new testsuite - #### Configuration of poisoning tests Since the memory poisoning tests will require the use of interfaces specific to the sanitizers used to poison memory, they must be guarded by new config options, for example `PSA_TEST_COPYING_ASAN` and `PSA_TEST_COPYING_VALGRIND`, as well as `MBEDTLS_TEST_HOOKS`. These would be analogous to the existing `MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN` and `MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND`. Since they require special tooling, these options should not be enabled in either the `default` or `full` configurations. Instead, as with the constant flow testing options, they should be enabled in a new component in `all.sh` that performs the copy testing with Valgrind or ASan. From 78bd77f5749e2d5c9eb89c1f807a54bc15c71279 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 25 Oct 2023 18:04:39 +0100 Subject: [PATCH 29/37] Careful-access prototyping to design exploration Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 42 +++++++++++--------------- 1 file changed, 17 insertions(+), 25 deletions(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 82f4bd0610..85e26ad502 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -449,9 +449,24 @@ Running on an FVP allows two approaches to careful-access testing: #### Discussion of careful-access validation -The best approach for validating the correctness of memory accesses is an open question that requires further investigation and prototyping. The above sections discuss some possibilities. +The best approach for validating the correctness of memory accesses is an open question that requires further investigation. To answer this question, each of the test strategies discussed above must be prototyped as follows: -However, there is one additional consideration that may make this easier. The careful-access approach to memory protection is only planned for hash and MAC algorithms. These lend themselves to a linear access pattern on input data; it may be simpler to test that a linear pattern is followed, rather than a random-access single-access-per-location pattern. +1. Take 1-2 days to create a basic prototype of a test that uses the approach. +2. Document the prototype - write a short guide that can be followed to arrive at the same prototype. +3. Evaluate the prototype according to its usefulness. The criteria of evaluation should include: + * Ease of implementation - Was the prototype simple to implement? Having implemented it, is it simple to extend it to do all of the required testing? + * Flexibility - Could the prototype be extended to cover other careful-access testing that may be needed in future? + * Performance - Does the test method perform well? Will it cause significant slowdown to CI jobs? + * Ease of reproduction - Does the prototype require a particular platform or tool to be set up? How easy would it be for an external user to run the prototype? + * Comprehensibility - Accounting for the lower code quality of a prototype, would developers unfamiliar with the tests based on the prototype be able to understand them easily? + +Once each prototype is complete, choose the best approach to implement the careful-access testing. Implement tests using this approach for each of the PSA interfaces that require careful-access testing: + +* Hash +* MAC +* AEAD (additional data only) +* Key derivation +* Asymmetric signature (input only) ##### New vs existing tests @@ -556,26 +571,3 @@ psa_status_t mem_poison_psa_aead_update(psa_aead_operation_t *operation, #### Configuration of poisoning tests Since the memory poisoning tests will require the use of interfaces specific to the sanitizers used to poison memory, they must be guarded by new config options, for example `PSA_TEST_COPYING_ASAN` and `PSA_TEST_COPYING_VALGRIND`, as well as `MBEDTLS_TEST_HOOKS`. These would be analogous to the existing `MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN` and `MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND`. Since they require special tooling, these options should not be enabled in either the `default` or `full` configurations. Instead, as with the constant flow testing options, they should be enabled in a new component in `all.sh` that performs the copy testing with Valgrind or ASan. - -### Validation of protection by careful access - -As concluded in [Validation of careful access for built-in drivers](#validation-of-careful-access-for-built-in-drivers), the best approach to validation of careful accesses is an open question. Designing this will involve prototyping each possible approach and choosing the one that seems most fruitful. - -For each of the test strategies discussed: - -1. Take 1-2 days to create a basic prototype of a test that uses the approach. -2. Document the prototype - write a short guide that can be followed to arrive at the same prototype. -3. Evaluate the prototype according to its usefulness. The criteria of evaluation should include: - * Ease of implementation - Was the prototype simple to implement? Having implemented it, is it simple to extend it to do all of the required testing? - * Flexibility - Could the prototype be extended to cover other careful-access testing that may be needed in future? - * Performance - Does the test method perform well? Will it cause significant slowdown to CI jobs? - * Ease of reproduction - Does the prototype require a particular platform or tool to be set up? How easy would it be for an external user to run the prototype? - * Comprehensibility - Accounting for the lower code quality of a prototype, would developers unfamiliar with the tests based on the prototype be able to understand them easily? - -Once each prototype is complete, choose the best approach to implement the careful-access testing. Implement tests using this approach for each of the PSA interfaces that require careful-access testing: - -* Hash -* MAC -* AEAD (additional data only) -* Key derivation -* Asymmetric signature (input only) From 599b0879907d5d815dacefd16403c232e42fa555 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 25 Oct 2023 18:09:17 +0100 Subject: [PATCH 30/37] Rename and specify config options * Rename config options to have MBEDTLS_TEST_ prefix * Clarify that these config options should not exist in mbedtls_config.h Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 85e26ad502..7bcb45c62e 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -570,4 +570,4 @@ psa_status_t mem_poison_psa_aead_update(psa_aead_operation_t *operation, #### Configuration of poisoning tests -Since the memory poisoning tests will require the use of interfaces specific to the sanitizers used to poison memory, they must be guarded by new config options, for example `PSA_TEST_COPYING_ASAN` and `PSA_TEST_COPYING_VALGRIND`, as well as `MBEDTLS_TEST_HOOKS`. These would be analogous to the existing `MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN` and `MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND`. Since they require special tooling, these options should not be enabled in either the `default` or `full` configurations. Instead, as with the constant flow testing options, they should be enabled in a new component in `all.sh` that performs the copy testing with Valgrind or ASan. +Since the memory poisoning tests will require the use of interfaces specific to the sanitizers used to poison memory, they must be guarded by new config options, for example `MBEDTLS_TEST_PSA_COPYING_ASAN` and `MBEDTLS_TEST_PSA_COPYING_VALGRIND`, as well as `MBEDTLS_TEST_HOOKS`. These would be analogous to the existing `MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN` and `MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND`. Since they require special tooling and are for testing only, these options should not be present in `mbedtls_config.h`. Instead, they should be set only in a new component in `all.sh` that performs the copy testing with Valgrind or ASan. From d081e52685bbeb237dab09bd8dff11b9938b4223 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 30 Oct 2023 15:22:07 +0000 Subject: [PATCH 31/37] Discuss plain-overwriting memory poisoning Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 7bcb45c62e..fa07a65252 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -323,9 +323,12 @@ There are several different ways that poisoning could be implemented: 2. Using Memory Sanitizer (MSan), which allows us to mark memory as uninitialized. This is also used for constant-flow testing. It is suitable for input buffers only, since it allows us to detect when a poisoned buffer is read but not when it is written. 3. Using Address Sanitizer (ASan). This provides `ASAN_POISON_MEMORY_REGION` which marks memory as inaccessible. 4. Allocating buffers separate pages and calling `mprotect()` to set pages as inaccessible. This has the disadvantage that we will have to manually ensure that buffers sit in their own pages, which likely means making a copy. +5. Filling buffers with random data, keeping a copy of the original. For input buffers, keep a copy of the original and copy it back once the PSA function returns. For output buffers, fill them with random data and keep a separate copy of it. In the memory poisoning hooks, compare the copy of random data with the original to ensure that the output buffer has not been written directly. Approach (2) is insufficient for the full testing we require as we need to be able to check both input and output buffers. +Approach (5) is simple and requires no extra tooling. It is likely to have good performance as it does not use any sanitizers. However, it requires the memory poisoning test hooks to maintain extra copies of the buffers, which seems difficult to implement in practice. Additionally, it does not precisely test the property we want to validate, so we are relying on the tests to fail if given random data as input. It is possible (if unlikely) that the PSA function will access the poisoned buffer without causing the test to fail. This becomes more likely when we consider test cases that call PSA functions on incorrect inputs to check that the correct error is returned. For these reasons, this memory poisoning approach seems unsuitable. + All three remaining approaches are suitable for our purposes. However, approach (4) is more complex than the other two. To implement it, we would need to allocate poisoned buffers in separate memory pages. They would require special handling and test code would likely have to be designed around this special handling. Meanwhile, approaches (1) and (3) are much more convenient. We are simply required to call a special macro on some buffer that was allocated by us and the sanitizer takes care of everything else. Of these two, ASan appears to have a limitation related to buffer alignment. From code comments quoted in [the documentation](https://github.com/google/sanitizers/wiki/AddressSanitizerManualPoisoning): From e88a6f836885b099405d8114b0de567da2f5bdef Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 30 Oct 2023 15:26:21 +0000 Subject: [PATCH 32/37] Add portability consideration to careful-access It's important that we be able to test for target-specific bugs. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index fa07a65252..b5314ef05d 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -462,6 +462,7 @@ The best approach for validating the correctness of memory accesses is an open q * Performance - Does the test method perform well? Will it cause significant slowdown to CI jobs? * Ease of reproduction - Does the prototype require a particular platform or tool to be set up? How easy would it be for an external user to run the prototype? * Comprehensibility - Accounting for the lower code quality of a prototype, would developers unfamiliar with the tests based on the prototype be able to understand them easily? + * Portability - How well can this approach be ported to multiple platforms? This would allow us to ensure that there are no double-accesses due to a bug that only affects a specific target. Once each prototype is complete, choose the best approach to implement the careful-access testing. Implement tests using this approach for each of the PSA interfaces that require careful-access testing: From e045b55c652d0ba486f321dbbdb495517a460bd9 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 30 Oct 2023 17:00:16 +0000 Subject: [PATCH 33/37] Add sections on validation of validation These cover the fact that we need to test our test framework to make sure it really detects incorrect accesses. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index b5314ef05d..fc179fa2ac 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -478,6 +478,15 @@ Most of the test methods discussed above need extra setup. Some require leaking With this complexity in mind it does not seem feasible to run careful-access tests using existing testsuites. Instead, new tests should be written that exercise the drivers in the required way. Fortunately, the only interfaces that need testing are hash, MAC, AEAD (testing over AD only), Key derivation and Asymmetric signature, which limits the number of new tests that must be written. +#### Validation of validation for careful-access + +In order to ensure that the careful-access validation works, it is necessary to write tests to check that we can correctly detect careful-access violations when they occur. To do this, write a test function that: + +* Reads its input multiple times at the same location. +* Writes to its output multiple times at the same location. + +Then, write a careful-access test for this function and ensure that it fails. + ## Analysis of argument protection in built-in drivers TODO: analyze the built-in implementations of mechanisms for which there is a requirement on drivers. By code inspection, how satisfied are we that they meet the requirement? @@ -575,3 +584,12 @@ psa_status_t mem_poison_psa_aead_update(psa_aead_operation_t *operation, #### Configuration of poisoning tests Since the memory poisoning tests will require the use of interfaces specific to the sanitizers used to poison memory, they must be guarded by new config options, for example `MBEDTLS_TEST_PSA_COPYING_ASAN` and `MBEDTLS_TEST_PSA_COPYING_VALGRIND`, as well as `MBEDTLS_TEST_HOOKS`. These would be analogous to the existing `MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN` and `MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND`. Since they require special tooling and are for testing only, these options should not be present in `mbedtls_config.h`. Instead, they should be set only in a new component in `all.sh` that performs the copy testing with Valgrind or ASan. + +#### Validation of validation for copying + +To make sure that we can correctly detect functions that access their input/output buffers rather than the copies, it is necessary to write a test function that misbehaves and test it with memory poisoning. Specifically, the function should: + +* Read its input buffer and after calling the input-buffer-copying function to create a local copy of its input. +* Write to its output buffer before and after calling the output-buffer-copying function to copy-back its output. + +Then, write a test that uses this function with memory poisoning and ensure that it fails. Since we are expecting a failure due to memory-poisoning, run this test separately from the rest of the memory-poisoning testing. From 15b5beea0c568857afbd235450e2f440db15878e Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 30 Oct 2023 17:13:54 +0000 Subject: [PATCH 34/37] Add note on platform-specific barriers Describe the approach of platform-specific code and draw a comparison with the constant-time module. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index fc179fa2ac..3968af142f 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -245,7 +245,7 @@ Once the copying code is implemented, it should be evaluated to see whether comp If copying behaviour is preserved by all major compilers then assume that compiler optimization is not a problem. -If copying behaviour is optimized away by the compiler, further investigation is needed. Experiment with using the `volatile` keyword to force the compiler not to optimize accesses to the copied buffers. +If copying behaviour is optimized away by the compiler, further investigation is needed. Experiment with using the `volatile` keyword to force the compiler not to optimize accesses to the copied buffers. If the `volatile` keyword is not sufficient, we may be able to use compiler or target-specific techniques to prevent optimization, for example memory barriers or empty `asm` blocks. These may be implemented and verified for important platforms while retaining a C implementation that is likely to be correct on most platforms as a fallback - the same approach taken by the constant-time module. **Open questions: Will the compiler optimize away copies? If so, can it be prevented from doing so in a portable way?** From 2531dab296b65ff4c53ed852c3dcef8cc9823caf Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 30 Oct 2023 18:27:10 +0000 Subject: [PATCH 35/37] Add auto-generation of test wrappers to design Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 3968af142f..385f48fe17 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -580,6 +580,18 @@ psa_status_t mem_poison_psa_aead_update(psa_aead_operation_t *operation, #define psa_aead_update(...) mem_poison_psa_aead_update(__VA_ARGS__) ``` +A header containing these wrappers should be auto-generated using a template and annotations to the input and output parameters to the PSA API. This will look something like the following: +```c +MBEDTLS_PSA_SHARED_BUFFER(input, input_length) +MBEDTLS_PSA_SHARED_BUFFER(output, output_size) +psa_status_t psa_aead_update(psa_aead_operation_t *operation, + const uint8_t *input, + size_t input_length, + uint8_t *output, + size_t output_size, + size_t *output_length); +``` +The `MBEDTLS_PSA_SHARED_BUFFER()` annotation expands to an empty expression but can be parsed by the wrapper generation script, which generates a new header file with the wrappers. #### Configuration of poisoning tests From 413dd07a4947f86cfefc16c7331d2bf3347c79cf Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 31 Oct 2023 12:16:53 +0000 Subject: [PATCH 36/37] Downgrade auto testing testing to a nice-to-have Automatic testing of our testing is not essential, as our testing framework may be manually tested. Having automated tests to test our tests may be left to future work. Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 385f48fe17..01b42b0cb2 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -599,9 +599,11 @@ Since the memory poisoning tests will require the use of interfaces specific to #### Validation of validation for copying -To make sure that we can correctly detect functions that access their input/output buffers rather than the copies, it is necessary to write a test function that misbehaves and test it with memory poisoning. Specifically, the function should: +To make sure that we can correctly detect functions that access their input/output buffers rather than the copies, it would be best to write a test function that misbehaves and test it with memory poisoning. Specifically, the function should: * Read its input buffer and after calling the input-buffer-copying function to create a local copy of its input. * Write to its output buffer before and after calling the output-buffer-copying function to copy-back its output. -Then, write a test that uses this function with memory poisoning and ensure that it fails. Since we are expecting a failure due to memory-poisoning, run this test separately from the rest of the memory-poisoning testing. +Then, we could write a test that uses this function with memory poisoning and ensure that it fails. Since we are expecting a failure due to memory-poisoning, we would run this test separately from the rest of the memory-poisoning testing. + +However, performing this testing automatically is not urgent. It will suffice to manually verify that the test framework works, automatic tests are a 'nice to have' feature that may be left to future work. From f63a52ed6317371c89cbbc2c168fab1cd2dff270 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 31 Oct 2023 14:25:30 +0000 Subject: [PATCH 37/37] Remove auto-generation of test wrappers Signed-off-by: David Horstmann --- docs/architecture/psa-shared-memory.md | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/docs/architecture/psa-shared-memory.md b/docs/architecture/psa-shared-memory.md index 01b42b0cb2..0b98f25fcc 100644 --- a/docs/architecture/psa-shared-memory.md +++ b/docs/architecture/psa-shared-memory.md @@ -580,18 +580,6 @@ psa_status_t mem_poison_psa_aead_update(psa_aead_operation_t *operation, #define psa_aead_update(...) mem_poison_psa_aead_update(__VA_ARGS__) ``` -A header containing these wrappers should be auto-generated using a template and annotations to the input and output parameters to the PSA API. This will look something like the following: -```c -MBEDTLS_PSA_SHARED_BUFFER(input, input_length) -MBEDTLS_PSA_SHARED_BUFFER(output, output_size) -psa_status_t psa_aead_update(psa_aead_operation_t *operation, - const uint8_t *input, - size_t input_length, - uint8_t *output, - size_t output_size, - size_t *output_length); -``` -The `MBEDTLS_PSA_SHARED_BUFFER()` annotation expands to an empty expression but can be parsed by the wrapper generation script, which generates a new header file with the wrappers. #### Configuration of poisoning tests