[#1275] support decrypting messages with hidden recipients

[andrey-utkin]

[lgtm-com[bot]]

This pull request introduces 2 alerts when merging 3f1a6d622789e2f4e7f5b7963596d5eecffb1f1b into 63837dda031c321eb8dd57e2d7f0ec55bbd59b9b - view on LGTM.com

new alerts:

• 2 for Local variable hides global variable

[codecov[bot]]

Codecov Report

Merging #1792 (c1651e6) into master (8adffe2) will increase coverage by 0.10%. The diff coverage is 91.77%.

@@            Coverage Diff             @@
##           master    #1792      +/-   ##
==========================================
+ Coverage   81.09%   81.19%   +0.10%     
==========================================
  Files         136      138       +2     
  Lines       28710    28838     +128     
==========================================
+ Hits        23281    23416     +135     
+ Misses       5429     5422       -7     

Impacted Files	Coverage Δ
src/librepgp/stream-key.cpp	75.35% <ø> (-0.08%)	:arrow_down:
src/librepgp/stream-key.h	100.00% <ø> (ø)
src/tests/streams.cpp	93.29% <ø> (ø)
src/tests/support.h	100.00% <ø> (ø)
src/librepgp/stream-write.cpp	76.86% <63.63%> (-0.18%)	:arrow_down:
src/tests/ffi-key.cpp	79.20% <80.00%> (-0.06%)	:arrow_down:
src/librepgp/stream-armor.h	90.90% <88.88%> (ø)
src/librepgp/stream-armor.cpp	88.71% <89.18%> (+0.99%)	:arrow_up:
src/tests/support.cpp	77.79% <92.59%> (+0.69%)	:arrow_up:
src/tests/ffi.cpp	86.93% <93.54%> (+0.11%)	:arrow_up:
... and 14 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5282075...c1651e6. Read the comment docs.

[andrey-utkin]

• added FFI tests
• cleaned up the history and the code
• rebased on master

One thing I'm unsure about is how to go to further purify the runtime logs. E.g. elgamal_decrypt_pkcs1() itself emits an error message straight to stderr. It is technically simple to just pass further the bool speculative function argument to switch that logging off, but this feels unnecessarily complicating the interface. I'd appreciate any ideas of more subtle way to control emission of these error messages.

Oh and I still haven't implemented a SecurityContext switch to control this new feature.

[lgtm-com[bot]]

This pull request introduces 2 alerts when merging e020f6e606b69f51642f98619183c61463498780 into 8adffe214d0ed5188693dde417ad10a7c59e151b - view on LGTM.com

new alerts:

• 2 for Local variable hides global variable

[andrey-utkin]

What I don't like is the name of a new subroutine I've created, key_fetch_and_try(). Name doesn't convey the meaning well, and there's already a similarly-named encrypted_try_key() which does a related bit of work. Suggestions for better names or a different approach to factoring out that bit of code are welcome.

[antonsviridenko]

LGTM so far, waiting for tests to be fixed

[ni4]

What I don't like is the name of a new subroutine I've created, key_fetch_and_try(). Name doesn't convey the meaning well, and there's already a similarly-named encrypted_try_key() which does a related bit of work. Suggestions for better names or a different approach to factoring out that bit of code are welcome.

I added few PR comments, but will review it more carefully later, as it has a bunch of changes. Also need some time to think on the questions you asked )

[andrey-utkin]

Thanks for the comments Nickolay.

Meanwhile I was reviewing codecov CI check results. The lines which are not covered now are error handling paths and the new disabled test.

An example of a line missing coverage:

@@ -1483,7 +1483,7 @@ encrypted_try_key(pgp_source_encrypted_param_t *param,
         pgp_fingerprint_t fingerprint;
         if (pgp_fingerprint(fingerprint, *seckey)) {
             if (!speculative) {
                 RNP_LOG("ECDH fingerprint calculation failed");  // <=====
             }
             return false;
         }

RNP_LOG() was there, what changed is that now it's conditional upon !speculative. What this means is that we don't have any tests where this message is emitted and where that message actually is desirable! But this is the kind of error log message for a condition which is not intended to happen normally, and it may be uneconomical to write a test just to target it. But if we want it, I guess the easiest way to do it is to build failure injection into the codebase itself. What are your thoughts on that?

[ni4]

But this is the kind of error log message for a condition which is not intended to happen normally, and it may be uneconomical to write a test just to target it. But if we want it, I guess the easiest way to do it is to build failure injection into the codebase itself. What are your thoughts on that?

@andrey-utkin you are right, most of these are left to have some diagnostic input in case when something unusal happens, so such codecov CI failures may be skipped. Things which must be covered should be triggered by external data or system configuration.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging cd192c7dcbe70bf4ae66073d93ea91f17efef9c7 into 92fb07b6b7369edbfa2ff228fdd5c732ecf4fa3a - view on LGTM.com

new alerts:

• 1 for Clear-text storage of sensitive information

[andrey-utkin]

Here's some strange CI failure. It says type mismatch (char const* vs const char *?), but I don't see any in the sources. Also I would expect such a mismatch would have been flagged up at compilation level. Also it seems unrelated to this PR's changes. https://github.com/rnpgp/rnp/runs/5837249724?check_suite_focus=true

2022-04-05T16:50:13.4223420Z /__w/rnp/rnp/builds/rnp-build/src/rnp/rnp --homedir /tmp/rnpctmpixokxhmr/.rnp --pass-fd 4 --decrypt /tmp/rnpctmpixokxhmr/cleartext.gpg --output /tmp/rnpctmpixokxhmr/cleartext.rnp
2022-04-05T16:50:13.4224079Z /__w/rnp/rnp/src/lib/rnp.cpp:667:12: runtime error: call to function ffi_pass_callback_file(rnp_ffi_st*, void*, rnp_key_handle_st*, char const*, char*, unsigned long) through pointer to incorrect function type 'bool (*)(rnp_ffi_st *, void *, rnp_key_handle_st *, const char *, char *, unsigned long)'
2022-04-05T16:50:13.4224322Z /__w/rnp/rnp/src/rnp/fficli.cpp:441: note: ffi_pass_callback_file(rnp_ffi_st*, void*, rnp_key_handle_st*, char const*, char*, unsigned long) defined here
2022-04-05T16:50:13.4224642Z SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /__w/rnp/rnp/src/lib/rnp.cpp:667:12 in

[andrey-utkin]

Also the rnp-ruby test suite failure seems unrelated, maybe flakiness?

  1) #<Method: Rnp.s2k_iterations(hash:, msec:) /home/runner/work/rnp/rnp/installs/ruby-rnp/lib/rnp/misc.rb:196> returns a higher iterations count for MD5 vs SM3
     Failure/Error:
       expect(
         Rnp.s2k_iterations(hash: 'MD5', msec: MSEC)
       ).to be > Rnp.s2k_iterations(hash: 'SM3', msec: MSEC)

       expected: > 425984
            got:   163840
     # ./spec/misc_spec.rb:160:in `block (2 levels) in <top (required)>'

Finished in 17.82 seconds (files took 0.42674 seconds to load)
247 examples, 1 failure

[lgtm-com[bot]]

This pull request introduces 1 alert when merging ac221e815ee526a1f4c382327f3867447387f2ed into 92fb07b6b7369edbfa2ff228fdd5c732ecf4fa3a - view on LGTM.com

new alerts:

• 1 for Clear-text storage of sensitive information

[andrey-utkin]

This "stderr clean" requirement doesn't come easy or fast. It would be good to just not print from the libraries, but just wiping logging statements from the codebase feels barbaric.

[ni4]

Here's some strange CI failure. It says type mismatch (char const* vs const char *?), but I don't see any in the sources.

Yeah, most likely it's some compiler issue or something like that. I filed issue #1329 for this some time ago, but don't have a solution yet.

[ni4]

Also the rnp-ruby test suite failure seems unrelated, maybe flakiness?

1) #<Method: Rnp.s2k_iterations(hash:, msec:) /home/runner/work/rnp/rnp/installs/ruby-rnp/lib/rnp/misc.rb:196> returns a higher iterations count for MD5 vs SM3

Yeah, this is due to timing issue, and happens sometimes. Just feel free to restart failed run in such case.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 400ffec64616d9e08c8b3e91bea52278b696381c into 92fb07b6b7369edbfa2ff228fdd5c732ecf4fa3a - view on LGTM.com

new alerts:

• 1 for Clear-text storage of sensitive information

[lgtm-com[bot]]

This pull request introduces 7 alerts when merging b1db5d69c375d8c56ee08cdd1cf316f7b5c070a0 into 92fb07b6b7369edbfa2ff228fdd5c732ecf4fa3a - view on LGTM.com

new alerts:

• 7 for Clear-text storage of sensitive information

[lgtm-com[bot]]

This pull request introduces 7 alerts when merging ccb240467a6804522c52b7a8b1c7a5afc7a7ed69 into 92fb07b6b7369edbfa2ff228fdd5c732ecf4fa3a - view on LGTM.com

new alerts:

• 7 for Clear-text storage of sensitive information

[lgtm-com[bot]]

This pull request introduces 7 alerts when merging a5a4ac24a94d061a04e4af0a957b87fed95672e5 into 92fb07b6b7369edbfa2ff228fdd5c732ecf4fa3a - view on LGTM.com

new alerts:

• 7 for Clear-text storage of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 514c58683246449a3169a7026f95a34c8db75685 into 52820759f60f72021213f0c606001f1669326d35 - view on LGTM.com

new alerts:

• 1 for Clear-text storage of sensitive information

[andrey-utkin]

I guess we can drop the new bool speculative parameter since we have LogStop.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging b58aece794bcad200ba661762d7c478cf7431609 into d6f871d6412fa7bc33a87624621de44bac24cc8f - view on LGTM.com

new alerts:

• 1 for Clear-text storage of sensitive information

[lgtm-com[bot]]

This pull request introduces 1 alert when merging c1651e62806af0e2eca8f5419c5267613b0d618b into d6f871d6412fa7bc33a87624621de44bac24cc8f - view on LGTM.com

new alerts:

• 1 for Clear-text storage of sensitive information

[andrey-utkin]

@ni4 @antonsviridenko I believe this is finally ready for merging. May I ask you guys to have a thorough final review soonish? Also, as this is my first work on this codebase, I would highly appreciate any feedback from you about how is it to work with me - here or in private email.

[ni4]

@andrey-utkin Thanks, I'll review this later today. Usually we discuss issues here on Github, however if there are some questions which is handier to discuss via chat - feel free to drop a message in Skype.

[andrey-utkin]

One major comment was not addressed - we cannot use FFI interface on the lower layer in stream-parse.cpp. To request a key we should handle zero keyid key request on the lib/rnp.cpp layer's crypto provider.

Thanks for the review. Sorry, indeed I forgot about it. I will revise the code in question.

[andrey-utkin]

One major comment was not addressed - we cannot use FFI interface on the lower layer in stream-parse.cpp. To request a key we should handle zero keyid key request on the lib/rnp.cpp layer's crypto provider.

So I've prototyped the way to have this try-all-keys work under search. See branch autkin-1275-decrypt-hidden-to-under-search . The case of any other search is knowingly broken right now, can be fixed. The actual decryption to hidden recipient somehow doesn't work too, but probably due to UB or a problem in my code. I am sharing this early now for discussion - whether I understood the suggestion correctly, does this approach still look right, etc One problem which I stumbled upon is that during search the key objects we look at are const, and trying the key requires it to be non-const. When I just used copy-constructor of pgp_key_pkt_t to get a mutable object, my test emitted src/librepgp/stream-parse.cpp:1354] checksum check failed which I wasn't expecting, so I guess I am not supposed to just copy pgp_key_pkt_t.

[ni4]

@andrey-utkin I'd suggest another approach, making key provider responsible to check for zero keyid:

• in key_fetch_and_try() just call a key provider, without checking for zero keyid.
• in lib/rnp.cpp, ffi_key_provider check whether zero keyid requested. On first call return a first secret key and store somewhere in userdata which key was returned. rnp_identifier_iterator_t could be useful here.
• back in key_fetch_and_try() attempt to start decryption. If it failed, and we have zero keyid - call key provider again.
• iterate it until we succeed to start decryption or key provider doesn't return any key.

Does this sound reasonable, or I missed some possible issues?

[andrey-utkin]

@andrey-utkin I'd suggest another approach, making key provider responsible to check for zero keyid: * in key_fetch_and_try() just call a key provider, without checking for zero keyid. * in lib/rnp.cpp, ffi_key_provider check whether zero keyid requested. On first call return a first secret key and store somewhere in userdata which key was returned. rnp_identifier_iterator_t could be useful here. * back in key_fetch_and_try() attempt to start decryption. If it failed, and we have zero keyid - call key provider again. * iterate it until we succeed to start decryption or key provider doesn't return any key. Does this sound reasonable, or I missed some possible issues?

I very much like the idea! It's much simpler to implement than what I tried to do the last time. It may be counterintuitive why such a loop should be used in key_fetch_and_try() in general case, but this can be explained in a comment, and keeping the interface free of special cases, albeit more complex, is good for reliability. I will do that.