Always check soter_rand() for success #570
Conversation
Checking random number generation for failures is pretty imporant [1, 2]
so let's make it easier for users to track such issues. Introduce a
SOTER_MUST_USE macro that adds an attribute to the function which will
cause a compiler warning if the return value is not used (or not
explicitly ignored by casting to void). See [3] for description.
Since we treat warnings as errors, CI compilation will fail like this:
compile build/obj/tests/common/test_utils.c.o
tests/common/test_utils.c:70:3: warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result]
soter_rand((uint8_t*)(&res), sizeof(size_t));
^~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
if anyone adds a soter_rand() call without checking the return value.
[1]: https://github.com/veorq/cryptocoding#use-strong-randomness
[2]: http://jbp.io/2014/01/16/openssl-rand-api
[3]: http://releases.llvm.org/9.0.0/tools/clang/docs/AttributeReference.html#nodiscard-warn-unused-result
We have this API, but it's not tested (or documented either). However, it's there and we should at least exercise it in the test suite. Turns out that it was not checking return value of soter_rand() and it's not visible if there are no tests. The API is... wacky, but that's what it is. I'm not really motivated to clean it up at the moment (and that's out of scope).
Add missing error handling. For ThemisPP throw an exception. For tests just abort with non-zero exit code as the API does not allow for returning an error. That's actually the case for ed25519 code too [1]. [1]: https://github.com/cossacklabs/themis/blob/5f886e1fa1742beb2070da93a2036ad79030dac5/src/soter/ed25519/gen_rand_32.c#L31-L35
ilammy
added
core
ilammy
requested review from
Lagovas,
storojs72 and
vixentael
as code owners
|
Looks like tests are failing |
|
Yeah, I'm looking into it. It's weird since I have also noticed that apparently we do not test ThemisPP with C++14 and C++17. I was under impression that we do. |
Nothing interestring. I have found this typo when trying to run clang-tidy manually. It is obscured during normal runs due to "2>/dev/null" which is added there because clang-tidy likes to talk even when no errors are detected and this is treated as warning output by our build system.
Make sure that it goes first because when compiling for C++14 this gets
expanded into
__attribute__((visibility("default")))
[[nodiscard]]
soter_status_t soter_rand(uint8_t* buffer, size_t length);
which throws Clang off-guard with this GCC compatibility syntax for
attributes. For some reason Clang thinks that [[nodiscard]] applies
only to "soter_status_t" which is not allowed.
Make sure that C++ attributes go before non-standard attributes.
|
Okay, I got it. That’s because Clang behaves weirdly when non-standard attributes get mixed with standard ones (only C++ has standard attributes). If you expand the macros the compiler sees __attribute__((visibility("default")))
[[nodiscard]]
soter_status_t soter_rand(uint8_t* buffer, size_t length);and that triggers something in Clang, but not in GCC. Making I wonder whether we should check the whole codebase with both GCC and Clang now. The analysis stage compiles and runs tests with some recent versions of GCC and Clang, but we do not check ThemisPP compilation with both compilers, for example. |
|
you've fixed the build!
this might be a good idea! |
|
I believe we can merge this, @ilammy? |
|
Uh... yeah, the issue with the build got fixed, and now we test ThemisPP with both GCC and Clang. |
As previously discussed, let's make sure that return value of
soter_rand()is always checked. I have added these failsafes, manually reviewed all existing callsites, and ensured that we abort processing if we fail to generate random data.Warn about unused result of soter_rand()
Checking random number generation for failures is pretty important [1] [2] so let's make it easier for users to track such issues. Introduce a SOTER_MUST_USE macro that adds an attribute to the function which will cause a compiler warning if the return value is not used (or not explicitly ignored by casting to void). See Clang docs.
Since we treat warnings as errors, CI compilation will fail like this:
if anyone adds a soter_rand() call without checking the return value.
Add missing tests for themispp::secure_rand_t
We have this API, but it's not tested (or documented either). However, it's there and we should at least exercise it in the test suite. Turns out that it was not checking return value of soter_rand() and it's not visible if there are no tests.
Handle soter_rand() failures
Add missing error handling. For ThemisPP throw an exception. For tests just abort with non-zero exit code as the API does not allow for returning an error. That's actually the case for ed25519 code too.
Checklist
Benchmark results are attached(N/A)Example projects and code samples are updated(N/A)Changelog is updated if needed(internal change, no need to mention it right now)