fix key hashing for v5 / v6 signatures #2261
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2261 +/- ##
=======================================
Coverage 84.27% 84.27%
=======================================
Files 114 114
Lines 23324 23324
=======================================
Hits 19656 19656
Misses 3668 3668 ☔ View full report in Codecov by Sentry. |
TJ-91
force-pushed
the
fix-v6-key-sig-justus
branch
2 times, most recently
from
f3e99bb to
c74315c
Compare
|
@ni4 I'm not sure why a windows and Cirrus CI test fails. Does the problem still exists that tests need to be restarted occasionally? Also codecov complains about some code that is not tested. Are we required to provide full test coverage for new code? It's not always trivial. For example, one failure case I added tests whether the length of a key is longer than what can be encoded in 4 length octets, i.e., 4 gigabyte. |
Yeah, that could be caused by two reason: GnuPG is not designed to run in batch/multiple threads and it seem to fail occasionally in tests. Also there is issue with encryption to multiple password which in some cases not well handled by GnuPG (cannot find now, but we should have issue for that).
Codecov requires the patch coverage to be not smaller than overall project coverage, and sometimes that get tricky. In some cases which should never happen, like checks for memory failures, you may use LCOV_EXCL_LINE or LCOV_EXCL_START/LCOV_EXCL_END to disable check for certain code lines. |
|
So what would you suggest how can I best satisfy codecov here? I agree with the general idea to ideally test every line but for most of the changes it'll be a bit tricky and I suppose a typical test case would fail somewhere earlier already. Let's look for example at the codecov finding In summary, I'm not sure if it's reasonable to make the effort here and some more sophisticated testing utility framework to help write the specific test cases might be needed if that level of testing is desired. Would you be fine with just using |
src/librepgp/stream-sig.h
Outdated
| * Throws exception on error. | ||
| * @param key key packet, must be populated | ||
| * @param hash initialized hash context | ||
| */ | ||
| void signature_hash_key(const pgp_key_pkt_t &key, rnp::Hash &hash); | ||
| void fingerprint_hash_key(const pgp_key_pkt_t &key, rnp::Hash &hash); |
There was a problem hiding this comment.
I don't think we should introduce new functions, adding and describing just version field to already existing signature_hash_key() would be pretty enough.
src/librepgp/stream-sig.cpp
Outdated
| case PGP_V3: | ||
| FALLTHROUGH_STATEMENT; | ||
| case PGP_V4: | ||
| if (key.hashed_len > ((size_t) 1 << 16) - 1) { |
There was a problem hiding this comment.
Given that we cannot have key artifacts with length >65535 at the moment (we will not parse it nor generate) we could add just an assert() here and below. The price of mistake would be just an incorrect signature.
|
@TJ-91 Please see the review comments. Let's just keep things simpler :) |
TJ-91
force-pushed
the
fix-v6-key-sig-justus
branch
from
c74315c to
17845b3
Compare
|
@ni4 I hope now it's simple enough :) I think I need to restart tests for the macos test (fails |
|
@TJ-91 PRs which fix CI are finally merged, so could you please rebase this on main to get it merged? Thanks! |
TJ-91
force-pushed
the
fix-v6-key-sig-justus
branch
from
17845b3 to
c13ce03
Compare
|
@ni4 Passes all checks now, thanks! |
|
Merging with two approvals. Thanks all! |
Justus pointed out that the hashing of keys is implemented incorrectly for RNP and some other implementations, see https://mailarchive.ietf.org/arch/msg/openpgp/PyP-XDv0VM5bYPX1Iq41-Oyytds/
This concerns computing v5 and v6 signatures over keys.
This PR fixes that. I separated signature and fingerprint computation logically, but the same code is used in the background. I also added a check that we don't accidently hash too large a key.