search

rnpgp / rnp

RNP: high performance C++ OpenPGP library used by Mozilla Thunderbird
https://www.rnpgp.org
Other
198 stars 55 forks source link

test_stream_verify_no_key and hash_test_success fail when ENABLE_SM2=OFF (v0.16.0) #1836

Closed jirutka closed 2 years ago

jirutka commented 2 years ago

Description

test_stream_verify_no_key and hash_test_success fail when building with ENABLE_SM2=OFF.

Steps to Reproduce

cmake -B build \
        -DCMAKE_BUILD_TYPE=MinSizeRel \
        -DCMAKE_INTERPROCEDURAL_OPTIMIZATION=ON \
        -DCMAKE_INSTALL_PREFIX=/usr \
        -DCMAKE_INSTALL_LIBDIR=lib \
        -DCMAKE_VERBOSE_MAKEFILE=ON \
        -DBUILD_SHARED_LIBS=ON \
        -DBUILD_TESTING=ON \
        -DGIT_EXECUTABLE=/bin/false \
        -DDOWNLOAD_GTEST=OFF \
        -DDOWNLOAD_RUBYRNP=OFF \
        -DENABLE_SM2=OFF \
        .
cmake --build build

cd build
CTEST_OUTPUT_ON_FAILURE=TRUE ctest -j4

Expected Behavior

The tests should pass.

Actual Behavior

 26/244 Test  #28: rnp_tests.test_stream_verify_no_key ...........................................***Failed    0.05 sec
Note: Google Test filter = rnp_tests.test_stream_verify_no_key
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from rnp_tests
[ RUN      ] rnp_tests.test_stream_verify_no_key
NO PUBLIC KEY for signature made Sat Apr 28 23:17:59 2018
Valid until Sat Apr 28 23:27:59 2018

using RSA (Encrypt or Sign) key 7bc6709b15c23a4a
Signature verification failure: 0 invalid signature(s), 1 unknown signature(s)
rnp-0.16.0/src/tests/streams.cpp:1202: Failure
Value of: (cli_rnp_process_file(&rnp))
  Actual: true
Expected: false
rnp-0.16.0/src/tests/streams.cpp:1203: Failure
Expected equality of these values:
  (file_size("output.dat"))
    Which is: 4
  (-1)
    Which is: -1
NO PUBLIC KEY for signature made Sat Apr 28 23:17:59 2018
Valid until Sat Apr 28 23:27:59 2018

using RSA (Encrypt or Sign) key 7bc6709b15c23a4a
Signature verification failure: 0 invalid signature(s), 1 unknown signature(s)
[  FAILED  ] rnp_tests.test_stream_verify_no_key (42 ms)
198/244 Test #200: rnp_tests.test_ffi_set_log_fd .................................................***Failed    0.05 sec
Note: Google Test filter = rnp_tests.test_ffi_set_log_fd
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from rnp_tests
[ RUN      ] rnp_tests.test_ffi_set_log_fd
rnp-0.16.0/src/tests/ffi.cpp:10636: Failure
Expected: ((rnp_ffi_set_log_fd(ffi, 100))) != (RNP_SUCCESS), actual: 0 vs 0
[  FAILED  ] rnp_tests.test_ffi_set_log_fd (35 ms)
[----------] 1 test from rnp_tests (35 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (35 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] rnp_tests.test_ffi_set_log_fd

 1 FAILED TEST

Environment:

ni4 commented 2 years ago

@jirutka Thanks for spotting this!

antonsviridenko commented 2 years ago

@jirutka are you packaging RNP for Alpine Linux? Not related to this issue, just curious :)

jirutka commented 2 years ago

are you packaging RNP for Alpine Linux?

Yes, see here. ;)

antonsviridenko commented 2 years ago

Nice

andrey-utkin commented 2 years ago

~Note: this happens with -DCRYPTO_BACKEND=openssl, but with -DCRYPTO_BACKEND=botan tests pass.~

Note: this happens with -DCRYPTO_BACKEND=botan, but with -DCRYPTO_BACKEND=openssl tests pass.

jirutka commented 2 years ago

Nope, I’m building it with the botan backend, not openssl.

andrey-utkin commented 2 years ago

Sorry, my bad, I've stated the opposite of what I observed :)

andrey-utkin commented 2 years ago

Summary: How about we relax the tests in this case?

From the build system interface perspective, ENABLE_SM2 really means "ensure that SM2/SM3/SM4 works" (at the time of compilation, linkage and testing). The tests in question make assertions that if !ENABLE_SM2, then any related functionality certainly fails. Obviously that's not the same. I also think we don't have to live up to these tests' expectations. As far as I'm aware it's not a real product spec requirement that such functionality fails in such case. It's definitely useful to run some tests on SMx functionality when it wasn't requested to be enabled, but I'd suggest to make the assertions less specific and more obviously useful, such as "doesn't crash". Insisting that the tested program throws an exception when it happens to be able to run successfully is not obviously useful to me.

As is evident from the report, sometimes the functionality just works despite ENABLE_SM2=OFF, so the missing code to make the program "fail as expected" is of "policy" nature, not "mechanism". The codebase has a few pieces of code to enforce this policy, but it's not great to maintain it and having more code like that is not something I'm looking forward to.

Surely ENABLE_SM2 build-time configuration switch has a purpose, but its usage in the source code can be reduced to merely guarding e.g. library calls which may result in linkage errors, or the code which may dereference NULL if the dependency doesn't implement SMx, or like that. In a word, let's use this configuration option in the source code to serve really useful invariants, such as "doesn't crash".

Turns out quite some distros (Arch, Gentoo) enable SM2 in openssl and it seems botan has it enabled by default (and in Alpine Linux too?).

While I am not a fan of adding code dedicated to blocking functionality which otherwise might just work, but I explored this direction just for understanding.

With hash_test_success, it is sufficient to replicate the same explicit block as what is there for openssl:

--- a/src/lib/crypto/hash.cpp
+++ b/src/lib/crypto/hash.cpp
@@ -68,6 +68,13 @@ Hash::Hash(pgp_hash_alg_t alg)
         throw rnp_exception(RNP_ERROR_BAD_PARAMETERS);
     }

+#if !defined(ENABLE_SM2)
+if (alg == PGP_HASH_SM3) {
+    RNP_LOG("SM3 hash is not available.");
+    throw rnp_exception(RNP_ERROR_BAD_PARAMETERS);
+}
+#endif
+
     auto hash_fn = Botan::HashFunction::create(name);
     if (!hash_fn) {
         RNP_LOG("Error creating hash object for '%s'", name);

With test_stream_verify_no_key, i still looking where would be the "right" place to put the block.

ni4 commented 2 years ago

@andrey-utkin Thanks for investigating this. test_stream_verify_no_key should be fixed with this patch as well, as source of problem is SM3 hash algo usage in S2K. To avoid such problems in future we should also:

andrey-utkin commented 2 years ago

test_stream_verify_no_key should be fixed with this patch as well.

test_stream_verify_no_key is not fixed by the same patch according to my results.

ni4 commented 2 years ago

@andrey-utkin Ah, forgot that RNP uses Botan's S2K code directly. Please see the funtion pgp_s2k_derive_key() from s2k.cpp

ni4 commented 2 years ago

Fixed via #1850