Need an OpenPGP crypto policy, only show security indicators for mechanisms that are on the allowlist
Categories
(MailNews Core :: Security: OpenPGP, enhancement, P1)
Tracking
(thunderbird_esr78+ fixed, thunderbird81 fixed)
People
(Reporter: KaiE, Assigned: KaiE)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 3 obsolete files)
5.88 KB,
patch
|
Details | Diff | Splinter Review | |
2.74 KB,
patch
|
KaiE
:
review+
rjl
:
approval-comm-beta+
wsmwk
:
approval-comm-esr78+
|
Details | Diff | Splinter Review |
1.30 KB,
patch
|
KaiE
:
review+
rjl
:
approval-comm-beta+
wsmwk
:
approval-comm-esr78+
|
Details | Diff | Splinter Review |
The RNP library supports a wide set of ciphersuites.
We should decide which ciphers we consider sufficiently secure, and maintain an allowlist of ciphers considered good.
When seeing encrypted data or a signature using a mechanism that isn't on the allowlist, we might want to omit security indicators.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 1•4 years ago
|
||
Assignee | ||
Comment 2•4 years ago
|
||
Assignee | ||
Comment 3•4 years ago
|
||
Comment on attachment 9172219 [details] [diff] [review]
1641720-v2.patch
Nickolay, I couldn't work on this earlier.
Because we don't yet have APIs in RNP that allow us to define a policy for allowed ciphers/hashes, I consider to apply this local patch as an initial solution for signatures, to block some ciphers and hashes which I think shouldn't be allowed.
Does this patch look technically correct?
For testing purposes, I've added SHA1 temporarily to the "false" section, and used a signed message that used a SHA1 hash. It was rejected as expected.
(Note I'm not suggesting to block SHA1 at this time, I just used it for testing, because that was the easiest for me.)
Assignee | ||
Comment 4•4 years ago
|
||
Of course this isn't complete. A separate follow-up step could be to check the size of the key material used with public key algorithms, and also reject signatures for small sizes, based on algorithm.
Assignee | ||
Comment 5•4 years ago
|
||
Thunderbird and RNP currently support the import of keys that use the non-standard SM2 public key algorithm.
This patch should stop Thunderbird from using such keys for signing or encryption.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Comment 6•4 years ago
|
||
Comment on attachment 9172219 [details] [diff] [review] 1641720-v2.patch Review of attachment 9172219 [details] [diff] [review]: ----------------------------------------------------------------- ::: third_party/rnp/src/lib/crypto/signatures.cpp @@ +248,5 @@ > rnp_result_t ret = RNP_ERROR_GENERIC; > > const pgp_hash_alg_t hash_alg = pgp_hash_alg_type(hash); > + if (!is_hash_alg_allowed_in_sig(hash_alg)) { > + return RNP_ERROR_SIGNATURE_INVALID; Would probably make sense to have a specific error code for cases like this...
Updated•4 years ago
|
Updated•4 years ago
|
Comment 7•4 years ago
|
||
While this code looks ok for now and could be used as quick-fix, I think that it should be done in more general way, to be able to distinguish wrong signature from weak signature, mark keys as weak, and so on.
For instance, signature with MD5/RSA768 made back in 1990s should not be rejected.
This requires some thinking. I will create an issue in the rnp repository (if we don't have it yet) to discuss the way it could be implemented.
Assignee | ||
Comment 8•4 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #6)
Would probably make sense to have a specific error code for cases like
this...
Yes, but this is internal RNP code, so it's RNP's decision if that makes sense. And that's deep inside many call levels, and only affects parts of the verifications that will be done, so likely this error code from here isn't directly reported back to the application.
Pushed by kaie@kuix.de:
https://hg.mozilla.org/comm-central/rev/9dd9372128b6
Patch RNP to disable several nonstandard or obsolete ciphers. r=mkmelin
https://hg.mozilla.org/comm-central/rev/1872902a18f1
Disable the use of keys with the SM2 public key algorithm. r=mkmelin
Assignee | ||
Comment 10•4 years ago
|
||
Nickolay, thanks for the feedback. I agree we should improve it further. It will also be inconvenient to carry a patch to RNP permanently, so a better solution will be much appreciated. Happy to talk on github about API specifics. What could potentially work: Allow Thunderbird to perform a one time configuration (either global, or if necessary, bound to an FFI). When verifying a signature inside that FFI, RNP should then ignore/skip any signatures that are outside of the policy allowance. I don't yet have a clear opinion what should happen to keys with an out-of-police signature, if the primary attributes of the key are allowed by the policy. Allow using the fine attributes of the key, or reject the key in general?
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 11•4 years ago
|
||
To verify the patch to RNP, I've executed the RNP tests. The patch causes the following tests to fail.
110 - rnp_tests.rnpkeys_generatekey_testSignature (Failed)
131 - rnp_tests.test_key_validate (Failed)
168 - rnp_tests.test_stream_key_signatures (Failed)
169 - rnp_tests.test_stream_key_signature_validate (Failed)
I've looked at the related data. And all of them involve the use of either SM3 or MD5.
With this patch applied (to RNP tests, not required for Thunderbird), which disables the testing of SM3, and negates the expectation for keys involving MD5, the RNP test suite passes.
Assignee | ||
Comment 12•4 years ago
|
||
Updated patch - only change is the proper commit header. Has r=mkmelin
Assignee | ||
Comment 13•4 years ago
|
||
Updated patch - only change is the proper commit header. Has r=mkmelin
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 14•4 years ago
|
||
Comment on attachment 9172452 [details] [diff] [review]
1641720-v2-with-header.patch
OpenPGP correctness, disable certain nonstandard or outdated algorithms, don't give users a false sense of security when using data that involves these algorithms.
Assignee | ||
Comment 15•4 years ago
|
||
Comment on attachment 9172453 [details] [diff] [review]
1641720-disable-using-SM2-with-header.patch
OpenPGP correctness, don't allow users to actively use keys that are based on nonstandard algorithms.
Assignee | ||
Comment 16•4 years ago
|
||
Comment on attachment 9172452 [details] [diff] [review]
1641720-v2-with-header.patch
Rob, for this patch, can you suggest how we can remember to re-apply this patch locally, whenever updating RNP to newer snapshots?
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 17•4 years ago
|
||
Comment 18•4 years ago
|
||
(In reply to Kai Engert (:KaiE:) from comment #16)
Comment on attachment 9172452 [details] [diff] [review]
1641720-v2-with-header.patchRob, for this patch, can you suggest how we can remember to re-apply this patch locally, whenever updating RNP to newer snapshots?
How about creating a patches directory under third_party and then make changes to the update script to apply the patch after updating from git. I can take care of it if that sounds good.
Comment 19•4 years ago
|
||
Comment on attachment 9172452 [details] [diff] [review]
1641720-v2-with-header.patch
[Triage Comment]
Approved by wsmwk via Matrix on 2020-08-27 for 81.0b2.
Comment 20•4 years ago
|
||
Comment on attachment 9172453 [details] [diff] [review]
1641720-disable-using-SM2-with-header.patch
[Triage Comment]
Approved by wsmwk via Matrix on 2020-08-27 for 81.0b2.
Assignee | ||
Comment 21•4 years ago
|
||
(In reply to Rob Lemley [:rjl] from comment #18)
How about creating a patches directory under third_party and then make changes to the update script to apply the patch after updating from git. I can take care of it if that sounds good.
Sounds great, thanks for your help!
Assignee | ||
Comment 22•4 years ago
|
||
I'm also fine with a need to manually update, because patches might fail to apply. Printing a big reminder at the end of the script would be sufficient for me, if you want to simplify.
Comment 23•4 years ago
|
||
Kai, I created an issue here: https://github.com/rnpgp/rnp/issues/1281
You and all others who interested in this discussion are welcome to join!
Comment 24•4 years ago
|
||
Comment on attachment 9172453 [details] [diff] [review]
1641720-disable-using-SM2-with-header.patch
[Triage Comment]
Approved for esr78
Comment 25•4 years ago
|
||
Comment on attachment 9172452 [details] [diff] [review]
1641720-v2-with-header.patch
[Triage Comment]
Approved for esr78
Assignee | ||
Comment 26•4 years ago
•
|
||
https://hg.mozilla.org/releases/comm-esr78/rev/fe45272cca54f1b02a7ae543571687347c54ef78
https://hg.mozilla.org/releases/comm-esr78/rev/b547a9c68f9ecae19f4c1bf114498eef15955160
78.2.1
This fix is only a part of the intended work. We will a follow-up bug for more actions.
Edit: fixed commit URLs
Description
•