Always check soter_rand() for success (#570)

* Warn about unused result of soter_rand()

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

* 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.

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).

* 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 [1].

[1]: https://github.com/cossacklabs/themis/blob/5f886e1fa1742beb2070da93a2036ad79030dac5/src/soter/ed25519/gen_rand_32.c#L31-L35

* Typo in clang-tidy configuration

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.

* Reorder SOTER_MUST_USE to make Clang happy

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.

.clang-tidy

@@ -2,6 +2,6 @@
 Checks: 'bugprone-*,cppcoreguidelines-*,clang-analyzer-*,misc-*,performance-*,portability-*,readability-*,-readability-implicit-bool-conversion,-readability-isolate-declaration,-*-magic-numbers,-cppcoreguidelines-pro-type-vararg'
 FormatStyle: none
 CheckOptions:
-  - key: cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor:
+  - key: cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor
     value: 1
 ...

src/soter/soter_error.h

@@ -79,6 +79,14 @@ typedef int32_t soter_status_t;
 #endif
 #endif
 
+#if __cplusplus >= 201402L
+#define SOTER_MUST_USE [[nodiscard]]
+#elif defined(__GNUC__) || defined(__clang__)
+#define SOTER_MUST_USE __attribute__((warn_unused_result))
+#else
+#define SOTER_MUST_USE
+#endif
+
 /**@}*/
 
 /**

src/soter/soter_rand.h

@@ -49,6 +49,7 @@ extern "C" {
  * SOTER_FAIL indicates that there is not enough entropy available
  * to fill the entire buffer. Please try again later.
  */
+SOTER_MUST_USE
 SOTER_API
 soter_status_t soter_rand(uint8_t* buffer, size_t length);
 

src/wrappers/themis/themispp/secure_rand.hpp

@@ -37,7 +37,10 @@ class secure_rand_t
 
     std::vector<uint8_t>& get()
     {
-        soter_rand(&n_[0], block_length_t_p);
+        soter_status_t res = soter_rand(&n_[0], block_length_t_p);
+        if (res != SOTER_SUCCESS) {
+            throw exception_t("failed to generate random bytes", res);
+        }
         return n_;
     }
 

tests/common/test_utils.c

@@ -67,7 +67,10 @@ size_t rand_int(size_t max_val)
 
 	do
 	{
-		soter_rand((uint8_t*)(&res), sizeof(size_t));
+		if (soter_rand((uint8_t*)(&res), sizeof(res)) != SOTER_SUCCESS)
+		{
+			exit(1);
+		}
 		res %= max_val;
 	} while (!res);
 

tests/themispp/main.cpp

@@ -19,6 +19,7 @@
 #include "secure_cell_test.hpp"
 #include "secure_comparator_test.hpp"
 #include "secure_message_test.hpp"
+#include "secure_rand_test.hpp"
 #include "secure_session_test.hpp"
 
 int main()
@@ -29,6 +30,7 @@ int main()
         themispp::secure_message_test::run_secure_message_test();
         themispp::secure_session_test::run_secure_session_test();
         themispp::secure_session_test::run_secure_comparator_test();
+        themispp::secure_rand_test::run_secure_rand_test();
         sput_finish_testing();
         return sput_get_return_value();
     } catch (const std::exception& e) {

tests/themispp/secure_rand_test.hpp

@@ -0,0 +1,70 @@
+/*
+ * Copyright (c) 2019 Cossack Labs Limited
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef THEMISPP_RAND_TEST
+#define THEMISPP_RAND_TEST
+
+#include <vector>
+
+#include <common/sput.h>
+
+#include <themispp/secure_rand.hpp>
+
+namespace themispp
+{
+namespace secure_rand_test
+{
+
+static bool not_all_zeros(const std::vector<uint8_t>& data)
+{
+    for (size_t i = 0; i < data.size(); i++) {
+        if (data[i] != 0) {
+            return true;
+        }
+    }
+    return false;
+}
+
+static void secure_rand_test()
+{
+    themispp::secure_rand_t<32> random_32_bytes;
+
+    std::vector<uint8_t> result = random_32_bytes.get();
+    sput_fail_unless(result.size() == 32, "return 32 random bytes", __LINE__);
+    sput_fail_unless(not_all_zeros(result), "they are actually random", __LINE__);
+
+    std::vector<uint8_t> another = random_32_bytes.get();
+    sput_fail_unless(another != result, "random bytes do not repeat", __LINE__);
+
+    try {
+        themispp::secure_rand_t<0> no_random_bytes;
+        no_random_bytes.get();
+        sput_fail_unless(false, "throws on zero size", __LINE__);
+    } catch (themispp::exception_t& e) {
+        sput_fail_unless(true, "throws on zero size", __LINE__);
+    }
+}
+
+inline void run_secure_rand_test()
+{
+    sput_enter_suite("ThemisPP secure rand test:");
+    sput_run_test(secure_rand_test, "secure_rand_test", __FILE__);
+}
+
+} // namespace secure_rand_test
+} // namespace themispp
+
+#endif /* THEMISPP_RAND_TEST */