rnpgp / rnp

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

Extend tests coverage on KBX/G10 #1997

Closed ni4 closed 1 year ago

ni4 commented 1 year ago

Description

As of Codecov report it should be possible to extend coverage on files related to KBX/G10.

antonsviridenko commented 1 year ago

@ni4 do you mean these files in https://github.com/rnpgp/rnp/tree/main/src/librekey subdirectory?

Are these functions (declared in header files, like rnp_key_store_g10_from_src()) considered as a part of public API? They are not present in include/rnp/ header files.

ni4 commented 1 year ago

@antonsviridenko Yeah, these ones. Those functions are internal API, however many of code lines are not covered with tests. So test suite should be extended with test cases which would hit those lines.

antonsviridenko commented 1 year ago

Here https://github.com/rnpgp/rnp/blob/ca9d7ee162a2a20d28378307c009430f199a8c68/src/librekey/key_store_kbx.cpp#L387 if the KBX file/memory source is less than 4 bytes, the function returns success without any error messages, is that an intended behavior?

Valid KBX must be at least 32 bytes long, according to this spec https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=kbx/keybox-blob.c;h=2564d1f4890644cb90998efe0530eb0f6f9f8e15;hb=refs/heads/master#l21

Although there are various length checks in other parts of KBX parsing code, they are never reached in this case.

ni4 commented 1 year ago

if the KBX file/memory source is less than 4 bytes, the function returns success without any error messages, is that an intended behavior?

I don't think so, it should be fixed.

Valid KBX must be at least 32 bytes long

Yeah, this check could be added as well.

antonsviridenko commented 1 year ago

@ni4 If no keys were found in otherwise valid KBX file, should rnp_load_keys() return an error? It returns success now.

ni4 commented 1 year ago

@antonsviridenko As it is FFI function, we should not change the behaviour. However, function documentation may be updated to mention this exact case.