Closed Bug 1641720 Opened 4 years ago Closed 4 years ago

Need an OpenPGP crypto policy, only show security indicators for mechanisms that are on the allowlist

Categories

(MailNews Core :: Security: OpenPGP, enhancement, P1)

enhancement

Tracking

(thunderbird_esr78+ fixed, thunderbird81 fixed)

RESOLVED FIXED
82 Branch
Tracking Status
thunderbird_esr78 + fixed
thunderbird81 --- fixed

People

(Reporter: KaiE, Assigned: KaiE)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

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.

Summary: Need an OpenPGP crypto policy, only show security indicators for mechanisms that are on the whitelist → Need an OpenPGP crypto policy, only show security indicators for mechanisms that are on the allowlist
Priority: -- → P1
Attached patch 1641720-v1.patch (obsolete) — Splinter Review
Assignee: nobody → kaie
Attached patch 1641720-v2.patch (obsolete) — Splinter Review
Attachment #9172217 - Attachment is obsolete: true

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.)

Attachment #9172219 - Flags: feedback?(o.nickolay)

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.

Attached patch 1641720-disable-using-SM2.patch (obsolete) — Splinter Review

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.

Attachment #9172219 - Flags: review?(mkmelin+mozilla)
Attachment #9172240 - Flags: review?(mkmelin+mozilla)
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...
Attachment #9172219 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9172240 - Flags: review?(mkmelin+mozilla) → review+
Status: NEW → ASSIGNED
Target Milestone: --- → 82 Branch

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.

(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

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED

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?

Attachment #9172219 - Flags: feedback?(o.nickolay)

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.

Updated patch - only change is the proper commit header. Has r=mkmelin

Attachment #9172219 - Attachment is obsolete: true
Attachment #9172452 - Flags: review+

Updated patch - only change is the proper commit header. Has r=mkmelin

Attachment #9172452 - Attachment is obsolete: true
Attachment #9172453 - Flags: review+
Attachment #9172452 - Attachment is obsolete: false
Attachment #9172240 - Attachment is obsolete: true

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.

Attachment #9172452 - Flags: approval-comm-esr78?
Attachment #9172452 - Flags: approval-comm-beta?

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.

Attachment #9172453 - Flags: approval-comm-esr78?
Attachment #9172453 - Flags: approval-comm-beta?

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?

Flags: needinfo?(rob)

(In reply to Kai Engert (:KaiE:) from comment #16)

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?

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.

Flags: needinfo?(rob)

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.

Attachment #9172452 - Flags: approval-comm-beta? → approval-comm-beta+

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.

Attachment #9172453 - Flags: approval-comm-beta? → approval-comm-beta+

(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!

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.

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 on attachment 9172453 [details] [diff] [review]
1641720-disable-using-SM2-with-header.patch

[Triage Comment]
Approved for esr78

Attachment #9172453 - Flags: approval-comm-esr78? → approval-comm-esr78+

Comment on attachment 9172452 [details] [diff] [review]
1641720-v2-with-header.patch

[Triage Comment]
Approved for esr78

Attachment #9172452 - Flags: approval-comm-esr78? → approval-comm-esr78+
Blocks: 1662581
See Also: → 1666634
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: