gh-ARMmbed/mbed-crypto

Watch

Remove a potential call to fwrite with null buffer. Add UBsan testing #239

Merged

AndrzejKurek merged 3 commits from psa-its-file-fixes into development

2019-09-05 10:40:09 -05:00

AndrzejKurek commented

2019-08-28 01:33:55 -05:00

(Migrated from github.com)

This PR handles issue https://github.com/ARMmbed/mbed-crypto/issues/203.
First commit removes a potential call to fwrite with a null buffer, and second extends testing.

Tested locally on Ubuntu 16.04 with gcc 5.4 - after extending the testing in CMakeLists and before adding the null check Asan type build fails (and therefore all.sh too). With these two commits - everything passes.

Before merging, this PR will also need a changelog entry to credit nanokatze who has filed this issue.

This PR handles issue https://github.com/ARMmbed/mbed-crypto/issues/203. First commit removes a potential call to fwrite with a null buffer, and second extends testing. Tested locally on Ubuntu 16.04 with gcc 5.4 - after extending the testing in CMakeLists and before adding the null check Asan type build fails (and therefore all.sh too). With these two commits - everything passes. Before merging, this PR will also need a changelog entry to credit nanokatze who has filed this issue.

k-stachowiak (Migrated from github.com) reviewed

2019-08-28 08:58:37 -05:00

library/psa_its_file.c

					
				@ -214,9 +214,12 @@ psa_status_t psa_its_set( psa_storage_uid_t uid,

				    n = fwrite( &header, 1, sizeof( header ), stream );

Is this enough? The Doxygen for this function says, among others, it will return PSA_ERROR_INVALID_ARGUMENT, if:

The operation failed because one of the provided pointers(p_data) is invalid, for example is NULL or references memory the caller cannot access.

If the docs are up to date, I suggest returning the above error.

Is this enough? The Doxygen for this function says, among others, it will return `PSA_ERROR_INVALID_ARGUMENT`, if: > The operation failed because one of the provided pointers(`p_data`) is invalid, for example is `NULL` or references memory the caller cannot access. If the docs are up to date, I suggest returning the above error.

athoelke (Migrated from github.com) reviewed

2019-08-28 09:48:55 -05:00

library/psa_its_file.c

					
				@ -214,9 +214,12 @@ psa_status_t psa_its_set( psa_storage_uid_t uid,

				    n = fwrite( &header, 1, sizeof( header ), stream );

It is not required for the implementation to report this error code in the case of an invalid memory reference, as a memory access violation is also a reasonable outcome of such a call. But an implementation is permitted to detect and report invalid memory references using this error code.

It would be helpful if the spec provided guidance on how memory buffers are handled in the API, for comparison see the PSA-FF and PSA Crypto specifications.

In this API p_data should be permitted to have any value if data_length==0. In that specific case:

fwrite() has fully defined behaviour (do nothing)
psa_its_set() has agreed behaviour (but it is not explicit in the spec), when it will create a zero-length data asset.

However, I can see that avoiding the potential UB when (data_length > 0 && p_data == NULL) is important due to the risk of leaving a half-populated ITS storage file in the system. So maybe you need:

if (p_data == NULL && data_length > 0)
    return PSA_ERROR_INVALID_ARGUMENT;

further up the function, before creating/opening the file and storing any data.

It is not required for the implementation to report this error code in the case of an invalid memory reference, as a memory access violation is also a reasonable outcome of such a call. But an implementation is permitted to detect and report invalid memory references using this error code. It would be helpful if the spec provided guidance on how memory buffers are handled in the API, for comparison see the PSA-FF and PSA Crypto specifications. In this API `p_data` should be permitted to have any value if `data_length==0`. In that specific case: * `fwrite()` has fully defined behaviour (do nothing) * `psa_its_set()` has agreed behaviour (but it is not explicit in the spec), when it will create a zero-length data asset. However, I can see that avoiding the potential UB when `(data_length > 0 && p_data == NULL)` is important due to the risk of leaving a half-populated ITS storage file in the system. So maybe you need: ``` if (p_data == NULL && data_length > 0) return PSA_ERROR_INVALID_ARGUMENT; ``` further up the function, before creating/opening the file and storing any data.

gilles-peskine-arm (Migrated from github.com) requested changes

2019-09-03 04:38:18 -05:00

gilles-peskine-arm (Migrated from github.com) left a comment

This is ok in that it makes the test pass, but the check could be made more robust at zero cost.

This is ok in that it makes the test pass, but the check could be made more robust at zero cost.

library/psa_its_file.c

					
				@ -214,9 +214,12 @@ psa_status_t psa_its_set( psa_storage_uid_t uid,

				    n = fwrite( &header, 1, sizeof( header ), stream );

data_length == 0 && p_data == NULL is a valid use case. It creates an empty file. Crypto is unlikely to need it, but this implementation of ITS is meant to be generic.

data_length != 0 && p_data == NULL is not a valid input. In the PSA Crypto API, there is implementation guidance to use the normal handling of invalid pointer dereferences (typically terminating the application and if possible logging the fault), and not return an error from the function. (The rationale for this guidance is that returning an error has no benefit since it's a programmer error and not a recoverable error, it makes errors harder to debug, it confuses static analyzers that have heuristics around nullable pointers by making them think that a null input is ok, and the code to check for null is pure overhead with no benefit.) I recommend following this guidance elsewhere in PSA.

`data_length == 0 && p_data == NULL` is a valid use case. It creates an empty file. Crypto is unlikely to need it, but this implementation of ITS is meant to be generic. `data_length != 0 && p_data == NULL` is not a valid input. In the PSA Crypto API, there is implementation guidance to use the normal handling of invalid pointer dereferences (typically terminating the application and if possible logging the fault), and not return an error from the function. (The rationale for this guidance is that returning an error has no benefit since it's a programmer error and not a recoverable error, it makes errors harder to debug, it confuses static analyzers that have heuristics around nullable pointers by making them think that a null input is ok, and the code to check for null is pure overhead with no benefit.) I recommend following this guidance elsewhere in PSA.

This should be if( data_length == 0 ). Checking for null silently writes nothing instead of faulting if p_data is null but data_length != 0.

This should be `if( data_length == 0 )`. Checking for null silently writes nothing instead of faulting if `p_data` is null but `data_length != 0`.

AndrzejKurek commented

2019-09-03 06:53:29 -05:00

(Migrated from github.com)

Updated the PR. Did not add an additional test to fill in the testing gap after removing MEMORY_BUFFER_ALLOC, since it's bound to be removed anyway here: https://github.com/ARMmbed/mbedtls/pull/2469

Updated the PR. Did not add an additional test to fill in the testing gap after removing MEMORY_BUFFER_ALLOC, since it's bound to be removed anyway here: https://github.com/ARMmbed/mbedtls/pull/2469

gilles-peskine-arm (Migrated from github.com) reviewed

2019-09-03 07:02:20 -05:00

library/psa_its_file.c

I'd prefer not to have that check, mainly because it fools static analyzers into thinking that it's ok to call this function with p_data == NULL, also because it makes calls with NULL harder to debug. However, the ITS interface explicitly documents that the function returns PSA_ERROR_INVALID_ARGUMENT in that case. So we need to make a call between friendliness to static analysis (and ease of debugging) and compliance. I vote for static analysis. @Patater What's your call?

I'd prefer not to have that check, mainly because it fools static analyzers into thinking that it's ok to call this function with `p_data == NULL`, also because it makes calls with NULL harder to debug. However, the ITS interface explicitly documents that the function returns `PSA_ERROR_INVALID_ARGUMENT` in that case. So we need to make a call between friendliness to static analysis (and ease of debugging) and compliance. I vote for static analysis. @Patater What's your call?

Patater (Migrated from github.com) reviewed

2019-09-03 07:18:29 -05:00

library/psa_its_file.c

I think not being compliant will complicate compliance testing, which hurts maintainability as much as fooling static analyzers does. Can we change the standard?

I think not being compliant will complicate compliance testing, which hurts maintainability as much as fooling static analyzers does. Can we change the standard?

gilles-peskine-arm (Migrated from github.com) reviewed

2019-09-03 07:26:33 -05:00

library/psa_its_file.c

How important compliance is depends on what the objective of psa_its.c is. It started out as solely for testing Mbed Crypto without relying on any third-party code. But I believe @paulhowardarm now intends to use it in production on Linux.

PSA storage is now officially at 1.0, so we can't change it until some future release (1.1 or 2.0).

How important compliance is depends on what the objective of `psa_its.c` is. It started out as solely for testing Mbed Crypto without relying on any third-party code. But I believe @paulhowardarm now intends to use it in production on Linux. PSA storage is now officially at 1.0, so we can't change it until some future release (1.1 or 2.0).

Patater (Migrated from github.com) reviewed

2019-09-03 07:53:04 -05:00

library/psa_its_file.c

Ah, this is for PSA ITS File. We don't care about compliance with this code. I'm up for smaller code size and more effective static analysis: which means no NULL check here. We don't intend to do compliance testing on our PSA ITS file implementation.

@simonqhughes is working on implementing PSA ITS for Linux. He used our adapter as a starting point, but has built on it quite a bit. He intends his implementation to pass compliance. @paulhowardarm should get in touch and use that implementation.

Ah, this is for PSA ITS File. We don't care about compliance with this code. I'm up for smaller code size and more effective static analysis: which means no NULL check here. We don't intend to do compliance testing on our PSA ITS file implementation. @simonqhughes is working on implementing PSA ITS for Linux. He used our adapter as a starting point, but has built on it quite a bit. He intends his implementation to pass compliance. @paulhowardarm should get in touch and use that implementation.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-09-03 07:53:19 -05:00

library/psa_its_file.c

I filed a change request internally: https://github.com/ARMmbed/psa_trusted_storage_api/issues/86

I filed a change request internally: https://github.com/ARMmbed/psa_trusted_storage_api/issues/86

athoelke (Migrated from github.com) reviewed

2019-09-03 08:12:05 -05:00

library/psa_its_file.c

I don't think that it is required that the API returns INVALID_ARGUMENT in case of an invalid buffer pointer (not all are easy to detect unless we are in a PSA-FF type environment). buf==NULL is an example of a argument error can can result in that error code.

The basic principle: as for almost any library API ever written, passing garbage pointers can have undefined behaviour, ranging from application termination to loss of control flow when overwriting the stack to bricking the device if writing to some flash control register.

The concern here is that because:

The standard explicitly states that passing p_data==NULL && len>0 to fwrite() is UNDEFINED BEHAVIOUR, AND
Compilers build in knowledge of standard library functions, AND
Compilers have the ability to statically determine that some calls to fwrite() will fall into this UB trap, AND
Aggressive optimisation exploits the standard definition of UB => program is meaningless => it doesn't matter what code is generated AT ALL, AND
The generated code might not fail at runtime in the way the developer expected: exception when NULL is written to. (It is unknown how bad the result could actually be - see 4)
So, UB sanitizers (which are often a good thing) can be conservative in identifying potential sites where UB is provoked, driving the developer to insert excessive NULL checks to keep the sanitizer quiet.

So can we manage without the NULL check in psa_it_set():

does a data_len > 0 check eliminate the problem cases?
if UBSAN doesn't like it - are there specific call sites of psa_its_set() where it can see a potential data flow path which results in p_data==NULL? Can we ensure that doesn't happen?

Side note: On the the whole UB is evil, it would have been much better if the ISO C standards committee had been less lazy and documented reasonable constraints on implementation behaviour in these circumstances

I don't think that it is _required_ that the API returns `INVALID_ARGUMENT` in case of an invalid buffer pointer (not all are easy to detect unless we are in a PSA-FF type environment). `buf==NULL` is an _example_ of a argument error can _can_ result in that error code. The basic principle: as for almost any library API ever written, passing garbage pointers can have undefined behaviour, ranging from application termination to loss of control flow when overwriting the stack to bricking the device if writing to some flash control register. The concern here is that because: 1. The standard explicitly states that passing `p_data==NULL && len>0` to `fwrite()` is `UNDEFINED BEHAVIOUR`, AND 2. Compilers build in knowledge of standard library functions, AND 3. Compilers have the ability to statically determine that _some_ calls to `fwrite()` will fall into this UB trap, AND 4. Aggressive optimisation exploits the standard definition of UB => program is meaningless => it doesn't matter what code is generated AT ALL, AND 5. The generated code might not fail at runtime in the way the developer expected: exception when NULL is written to. (It is unknown how bad the result could actually be - see 4) 6. So, UB sanitizers (which are often a good thing) can be conservative in identifying potential sites where UB is provoked, driving the developer to insert excessive NULL checks to keep the sanitizer quiet. So can we manage without the NULL check in `psa_it_set()`: - does a `data_len > 0` check eliminate the problem cases? - if UBSAN doesn't like it - are there specific call sites of `psa_its_set()` where it can see a potential data flow path which results in `p_data==NULL`? Can we ensure that doesn't happen? --- _Side note: On the the whole UB is evil, it would have been much better if the ISO C standards committee had been less lazy and documented reasonable constraints on implementation behaviour in these circumstances_

gilles-peskine-arm (Migrated from github.com) reviewed

2019-09-03 08:26:24 -05:00

library/psa_its_file.c

does a data_len > 0 check eliminate the problem cases?

Wrapping the fwrite call in if (data_len > 0) (with no else) makes the function behave correctly when called with valid arguments (p_data is valid or data_len is zero).

are there specific call sites of psa_its_set() where it can see a potential data flow path which results in p_data==NULL?

In PSA Crypto, at the moment, no. In the future, maybe, but of course only with data_len==0. Unless there's a bug in PSA crypto (or a compiler bug, or a runtime glitch…).

it would have been much better if the ISO C standards committee had been less lazy and documented reasonable constraints on implementation behaviour in these circumstances

If they'd done that, they'd be called the ISO Rust standards committee. There's no realistic way to limit the behavior of free(p); *p = 3; on common hardware. Most processors don't even have a nasal demon peripheral.

> does a data_len > 0 check eliminate the problem cases? Wrapping the `fwrite` call in `if (data_len > 0)` (with no `else`) makes the function behave correctly when called with valid arguments (`p_data` is valid or `data_len` is zero). > are there specific call sites of psa_its_set() where it can see a potential data flow path which results in p_data==NULL? In PSA Crypto, at the moment, no. In the future, maybe, but of course only with `data_len==0`. Unless there's a bug in PSA crypto (or a compiler bug, or a runtime glitch…). > it would have been much better if the ISO C standards committee had been less lazy and documented reasonable constraints on implementation behaviour in these circumstances If they'd done that, they'd be called the ISO Rust standards committee. There's no realistic way to limit the behavior of `free(p); *p = 3;` on common hardware. Most processors don't even have a nasal demon peripheral.

athoelke (Migrated from github.com) reviewed

2019-09-03 10:42:22 -05:00

library/psa_its_file.c

does a data_len > 0 check eliminate the problem cases?

Wrapping the fwrite call in if (data_len > 0) (with no else) makes the function behave correctly when called with valid arguments (p_data is valid or data_len is zero).

Sorry, I meant: does adding just the data_len>0 check stop the UB sanitizer complaining about UB calls to fwrite().

are there specific call sites of psa_its_set() where it can see a potential data flow path which results in p_data==NULL?

In PSA Crypto, at the moment, no. In the future, maybe, but of course only with data_len==0. Unless there's a bug in PSA crypto (or a compiler bug, or a runtime glitch…).

Well, those are the bugs we want UB Sanitizer to detect - definite cases of passing NULL pointers without zero length data.

it would have been much better if the ISO C standards committee had been less lazy and documented reasonable constraints on implementation behaviour in these circumstances

If they'd done that, they'd be called the ISO Rust standards committee. There's no realistic way to limit the behavior of free(p); *p = 3; on common hardware. Most processors don't even have a nasal demon peripheral.

By 'reasonable constraints' I meant that the implementation doesn't do "anything surprising". In your scenario, causing an exception, or overwriting some other object with 3 is "reasonable behaviour" - even if the consequences are hard to enumerate. The compiler deciding to generate a program that computes PI to a million decimal places instead of attempting to do what the program appears to do is not "reasonable behaviour", but this is permitted by the standard. For more background see https://blog.regehr.org/archives/213

> > does a data_len > 0 check eliminate the problem cases? > > Wrapping the `fwrite` call in `if (data_len > 0)` (with no `else`) makes the function behave correctly when called with valid arguments (`p_data` is valid or `data_len` is zero). Sorry, I meant: does adding just the `data_len>0` check stop the UB sanitizer complaining about UB calls to `fwrite()`. > > are there specific call sites of psa_its_set() where it can see a potential data flow path which results in p_data==NULL? > > In PSA Crypto, at the moment, no. In the future, maybe, but of course only with `data_len==0`. Unless there's a bug in PSA crypto (or a compiler bug, or a runtime glitch…). Well, those are the bugs we want UB Sanitizer to detect - definite cases of passing NULL pointers without zero length data. > > it would have been much better if the ISO C standards committee had been less lazy and documented reasonable constraints on implementation behaviour in these circumstances > > If they'd done that, they'd be called the ISO Rust standards committee. There's no realistic way to limit the behavior of `free(p); *p = 3;` on common hardware. Most processors don't even have a nasal demon peripheral. By 'reasonable constraints' I meant that the _implementation_ doesn't do "anything surprising". In your scenario, causing an exception, or overwriting some other object with `3` is "reasonable behaviour" - even if the consequences are hard to enumerate. The *compiler* deciding to generate a program that computes PI to a million decimal places instead of attempting to do what the program appears to do is not "reasonable behaviour", but this is permitted by the standard. For more background see https://blog.regehr.org/archives/213

gilles-peskine-arm (Migrated from github.com) reviewed

2019-09-03 12:10:17 -05:00

library/psa_its_file.c

Adding the check for data_len > 0 fixes the actual problem and also causes UBsan not to complain. UBsan analyzed this correctly.

In practice, free(p); *p = 3; (or some variant like free(p); p->foo = 3;) corrupts data structures used by malloc and the behavior is truly undefined up to the capabilities of the hardware. Especially for us in the security business, where the choice to run this particular code is likely to have been made by an adversary whose objective is to do something that the defender considers unreasonable. Removing this undefined behavior hole means imposing constraints on memory management, and then C wouldn't be C anymore. I'm not saying that I like it, but C is what it is.

Adding the check for `data_len > 0` fixes the actual problem and also causes UBsan not to complain. UBsan analyzed this correctly. In practice, `free(p); *p = 3;` (or some variant like `free(p); p->foo = 3;`) corrupts data structures used by malloc and the behavior is truly undefined up to the capabilities of the hardware. Especially for us in the security business, where the choice to run this particular code is likely to have been made by an adversary whose objective is to do something that the defender considers unreasonable. Removing this undefined behavior hole means imposing constraints on memory management, and then C wouldn't be C anymore. I'm not saying that I like it, but C is what it is.

athoelke (Migrated from github.com) reviewed

2019-09-03 17:20:44 -05:00

library/psa_its_file.c

In practice, free(p); *p = 3; (or some variant like free(p); p->foo = 3;) corrupts data structures used by malloc and the behavior is truly undefined up to the capabilities of the hardware.

You underestimate the potential interpretation of the standard. The C standard considers that the compiler and runtime and system are all "the C abstract machine" that interprets the program.

We might expect that free(p); *p = 3; results in some machine code like:

mov r0, r11
bl free
mov r0, #3 
str [r11], r0

after which all hell could break loose in the system because we have just scribbled on allocator meta-data. This is only a moderately bad result.

The C standard says that because writing to memory that has been freed is undefined behaviour, the program is incorrect and the "C abstract machine" can do whatever it wants. In practice the compiler detecting this might eliminate output code that only serves to implement scenarios that exhibit undefined behaviour - so here it might choose to make the entire function containing the above code a NOP.

It is not possible for the developer to reason about the impact that such unexpected transformations will have on the operation of their code.

> In practice, `free(p); *p = 3;` (or some variant like `free(p); p->foo = 3;`) corrupts data structures used by malloc and the behavior is truly undefined up to the capabilities of the hardware. You underestimate the potential interpretation of the standard. The C standard considers that the compiler **and** runtime **and** system are all "the C abstract machine" that interprets the program. We might expect that `free(p); *p = 3;` results in some machine code like: mov r0, r11 bl free mov r0, #3 str [r11], r0 after which all hell could break loose in the system because we have just scribbled on allocator meta-data. This is only a _moderately bad_ result. The C standard says that because writing to memory that has been freed is undefined behaviour, the **program** is incorrect and the "C abstract machine" can do whatever it wants. In practice the compiler detecting this might eliminate output code that only serves to implement scenarios that exhibit undefined behaviour - so here it might choose to make **the entire function** containing the above code a NOP. It is not possible for the developer to reason about the impact that such unexpected transformations will have on the operation of their code.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-09-04 04:17:38 -05:00

library/psa_its_file.c

I'm well aware of what undefined behavior is, thank you. My remark was about how to approach UB when thinking about security. As a defender, the worst result is that the attacker gets to execute arbitrary code in your execution space.

And the secondmost reliable way to avoid UB is to root it out with static analysis. (The most reliable is of course to program in a language that doesn't have UB.) Hence my push towards 1. actually using static analysis tools and 2. making the code friendly to these tools.

I'm well aware of what undefined behavior is, thank you. My remark was about how to approach UB when thinking about security. As a defender, the worst result is that the attacker gets to execute arbitrary code in your execution space. And the secondmost reliable way to avoid UB is to root it out with static analysis. (The most reliable is of course to program in a language that doesn't have UB.) Hence my push towards 1. actually using static analysis tools and 2. making the code friendly to these tools.

athoelke (Migrated from github.com) reviewed

2019-09-04 06:02:19 -05:00

library/psa_its_file.c

Sorry, I should have not started my reply with "you" - this wasn't meant to be a personal response.

I totally agree in the importance of using static tooling to identify UB, and ensuring that the code is both friendly to such tools and that all reports of issues from the tool are followed up.

My comment was attempting to dispel the myth that the results of UB in source code are merely the side effects of memory scribbles or numeric overflows (or similar) that result from the execution of the expected machine code. UB makes these expectations invalid in the long run (new versions or different toolchains), if not at present.

Which just reinforces the point about the need for using static tools that help identify and eliminate UB.

Sorry, I should have not started my reply with "you" - this wasn't meant to be a personal response. I totally agree in the importance of using static tooling to identify UB, and ensuring that the code is both friendly to such tools and that all reports of issues from the tool are followed up. My comment was attempting to dispel the myth that the results of UB in source code are merely the side effects of memory scribbles or numeric overflows (or similar) that result from the execution of the **expected** machine code. UB makes these expectations invalid in the long run (new versions or different toolchains), if not at present. Which just reinforces the point about the need for using static tools that help identify and eliminate UB.

AndrzejKurek (Migrated from github.com) reviewed

2019-09-04 06:09:56 -05:00

library/psa_its_file.c

Ok, so do we agree on removing this check?

Ok, so do we agree on removing this check?

AndrzejKurek (Migrated from github.com) reviewed

2019-09-05 08:35:27 -05:00

library/psa_its_file.c

Done.

Done.

gilles-peskine-arm (Migrated from github.com) approved these changes

2019-09-05 08:47:09 -05:00

k-stachowiak (Migrated from github.com) approved these changes

2019-09-05 08:52:16 -05:00

Sign in to join this conversation.

No reviewers

gilles-peskine-arm

No labels

good first issue

HEADER FORMATTING ISSUE

Jira status: AWAITING REVIEW

Jira status: CLOSED

Jira status: DONE

Jira status: IN PROGRESS

Jira status: IN REVIEW

Jira status: OPEN

Jira status: REOPENED

Jira status: RESOLVED

mirror internally

needs: backports

needs: design review

needs: preceding PR

open for community contribution

Product Backlog

ready for merge

type: enhancement

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-ARMmbed/mbed-crypto!239

No description provided.