Closed Bug 1790116 Opened 2 years ago Closed 2 years ago

Update vendored RNP to 0.16.2

Categories

(MailNews Core :: Security: OpenPGP, task)

Product:
MailNews Core
Component:
Security: OpenPGP
Type:
task

Tracking

(thunderbird_esr102+ fixed, thunderbird105 affected)

Status:
RESOLVED FIXED
Milestone:
107 Branch
Tracking Flags:
Tracking Status
thunderbird_esr102 + fixed
thunderbird105 --- affected

People

(Reporter: rjl, Assigned: rjl)

References

Details

(Whiteboard: [TM:102.7.0])

Attachments

(6 files, 1 obsolete file)

Bug 1790116 - update_rnp.sh changes for RNP v0.16.2. r=kaie
48 bytes, text/x-phabricator-request
wsmwk
: approval-comm-esr102+
Details | Review
Bug 1790116 - Update librnp to v0.16.2. r=kaie
48 bytes, text/x-phabricator-request
wsmwk
: approval-comm-esr102+
Details | Review
Bug 1790116 - mozbuild changes for RNP v0.16.2. r=kaie
48 bytes, text/x-phabricator-request
wsmwk
: approval-comm-esr102+
Details | Review
Bug 1790116 - Update rnp_export.h. r=kaie
48 bytes, text/x-phabricator-request
wsmwk
: approval-comm-esr102+
Details | Review
Bug 1790116 - Do not compile SM2 crypto with librnp. r=kaie
48 bytes, text/x-phabricator-request
wsmwk
: approval-comm-esr102+
Details | Review
image.png
272.83 KB, image/png
Details

RNP v0.16.1 was tagged on 2022-09-06.

JFYI: release with all the metadata and page update(s) will follow shortly. Anyway it will be sticked to this tag.

The changes in bug_1768424.patch are now included upstream in
https://github.com/rnpgp/rnp/commit/ac6f58ef7ccea270b735b53f87da2c3ca5b34290.

bug_1763641.patch updated due to upstream changes.

Assignee: nobody → rob
Status: NEW → ASSIGNED

Depends on D157010

hash_sha1cd.cpp moved up to its parent directory.

ENABLE_IDEA needs to be set to keep support enabled.
https://github.com/rnpgp/rnp/commit/17972d0238919d4abf88b04debce95844be4716d

Update rnp_symbols.py to not include deprecated functions.
Added new symbols to rnp.symbols for export.

Depends on D157011

Looks like the upgrade is causing a test failure on all platforms:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&selectedTaskRun=dTChLDVwSw6f5R3cXO8xkw.0

The CMake code that generates this file changed with RNP 0.16. The local copy
needs to be regenerated.

Depends on D157011

Depends on: 1790446

The CMake configuration in rnp/src/lib/CMakeLists.txt does not include
src/lib/crypto/sm2.cpp unless ENABLE_SM2 is defined.
Thunderbird builds do not set ENABLE_SM2, so there's no need to build this
file.

Depends on D157053

Fix for RNP bug: https://github.com/rnpgp/rnp/issues/1901

Depends on D157154

(In reply to Rob Lemley [:rjl] from comment #5)

Looks like the upgrade is causing a test failure on all platforms:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&selectedTaskRun=dTChLDVwSw6f5R3cXO8xkw.0

It looks like the Testing partial inline; filename=partial-signed-from-carol-html.eml and Testing partial inline; filename=partial-signed-from-carol-plaintext.eml are displaying the "Bad signature" icon rather than the "Unknown verificationn" icon.

FWIW, If I run 'rnp-cli -v' against a copy of the plaintext message (just the signed portion and the signature) I get:

NO PUBLIC KEY for signature made Thu Apr 15 11:56:23 2021
using RSA key 3099ff1238852b9f
Signature verification failure: 1 unknown signature

This is different than what I get when I run the same using RNP out of Thunderbird 102.2.2:

NO PUBLIC KEY for signature made Thu Apr 15 11:56:23 2021
using RSA (Encrypt or Sign) key 3099ff1238852b9f
Signature verification failure: 0 invalid signature(s), 1 unknown signature(s)

Maybe the "unknown signature" concept is broken?

Hi Rob, this is caused by the following commit: https://github.com/rnpgp/rnp/commit/2bd2597d3fb38773707ee43fc2fbeb598a6a4a5d
If there are no invalid or unknown signatures, then CLI just don't print corresponding part of the message.

I just realized the part of comment 9 referring to the cli doesn't make much sense. Thanks for clarifying the new expected behavior.

The tests for those two "partial-signed" emails from Carol are failing though. https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=896e8ddbfb63950c616b43116885d79169171523 is my latest try push. In the macOS and Linux64 "bct7" tests, you can see in the screenshot that the "invalid signature" icon is being displayed, while the test expects the "unknown" icon.

Could you please point me on where I should click to see the screenshot and where I can obtain the test email to check it faster? I'm not quite familiary with TB testing UI yet, so it looks a bit complicated to quickly dive in.

...in v 0.16.1 we changed a bit multiple signature handling code, so now by default library requires only single valid signature for successfull call to the rnp_op_verify_execute(). Probably that touched signature status retrieval as well (while it should not...).

Attached image image.png —

Open the link in comment 11, you should get the build in the screenshot.

The failing test is "bct7", and the "Linux64 18.04 x64 Webrender opt" test platform shows the incorrect icon. Click on "bct7" in that row and it will open up the bottom panel.

The right side of the bottom panel will be on "Failure Summary", click on "Artifacts and Debugging Tools". In the list of artifacts that comes up, it's "mozilla-test-fail-screenshot_u4rj2lwq.png". Click to open.

The test emails that fail are at https://hg.mozilla.org/comm-central/file/tip/mail/test/browser/openpgp/data/eml/partial-signed-from-carol-plaintext.eml and https://hg.mozilla.org/comm-central/file/tip/mail/test/browser/openpgp/data/eml/partial-signed-from-carol-html.eml.

There is a set of keys for Carol, but they are not imported for the test.

You'll see in the test screenshot the "OpenPGP" icon in the headers area has a red "!" indicating an invalid signature. It should be a blue "?" for an unknown key.

Hi Rob, thanks for the detailed instructions, now I get it. This happens due to changes introduced in v0.16.1. Previously rnp_op_verify_execute() call was returning RNP_SUCCESS for the unknown signing key, however now it would return RNP_SUCCESS only if there is at least one valid signature. This was done to avoid possible issue when caller would not do further signatures checks and think that verification succeeded.

And here is code piece responcible for result check :

      case RNPLib.RNP_ERROR_SIGNATURE_INVALID:
        result.statusFlags |= lazy.EnigmailConstants.BAD_SIGNATURE;
        useDecodedData = true;
        processSignature = false;
        break;

From the https://hg.mozilla.org/comm-central/file/tip/mail/extensions/openpgp/content/modules/RNP.jsm#l1163

My suggestion would be to update logic to check all signatures validity statues and then make a decisions. Also what would happen if email has two signatures, one of which is expired, and second is valid? Or one invalid, and second from the unknown key? And so on...

Depends on: 1791195

Thanks Nickolay for looking into that test failure! I opened bug 1791195 for it.

JFYI: RNP v0.16.2 release should be out soon. It fixes unfortunate issue with ENABLE_IDEA, and some other CMake-related issues.

Thanks Nickolay for the notice, and thanks Rob for pushing this work forward. I'll use your patches locally for early work on bug 1791195, but otherwise it seems reasonable to skip 0.16.1 and rather go directly to 0.16.2 - this will also simplify a potential uplift to esr102.

Summary: Update vendored RNP to 0.16.1 → Update vendored RNP to 0.16.2

Just so it doesn't get missed, in D157010 I update third_party/patches/rnp/bug_1763641.patch due to changes in rnp/src/lib/crypto/signatures.cpp. Kai, can you make sure to check that?

(In reply to Rob Lemley [:rjl] from comment #19)

Just so it doesn't get missed, in D157010 I update third_party/patches/rnp/bug_1763641.patch due to changes in rnp/src/lib/crypto/signatures.cpp. Kai, can you make sure to check that?

Yes, I'm currently reviewing that.

Nickolay, would you like to register yourself at phabricator.services.mozilla.com ? It would allow me to CC you to code reviews.

I'll say that here, because Nickolay isn't yet on phabricator:

I think https://phabricator.services.mozilla.com/D157010 needs a different patch for 0.16.1

In my understanding, we should continue to allow SHA1 for action VerifyKey (intention of the fix for bug 1763641), but we should reject SHA1 for action VerifyData.
I'll create an updated patch.

Kai, together with other changes, v0.16.1 updates SHA1 signature handling rules to not allow it for data signature(s), but to allow it for key signature(s) till Jan, 19 2024. This date may be changed via security rules handling functions, which now allow to distinct between data and key signatures.

P.S. Registering at phabricator, will write back once ready.

Ok, done. My nickname there should be o.nickolay.

Nickolay, thanks for the explanation. If I understand correctly, we no longer need the patch from https://phabricator.services.mozilla.com/D145220 - RNP 0.16.1+ already has equivalent handling to allow SHA1 in data signatures.

Rather, if we want to allow SHA1 for key signatures longer than 2024-01-19, we should make an application level call to add a security rule.

Yeah, that's correct. If you'd like to extend you should use rnp_add_security_rule() call to override or extend default settings.

Attachment #9294048 - Attachment description: Bug 1790116 - Update RNP source to v0.16.1. r=kaie → Bug 1790116 - Update librnp to v0.16.1. r=kaie

I updated the patch stack based on some review comments and also removed bug_1763641.patch per comment 26.

Once 0.16.2 is tagged I'll update the stack again without D157155.

JFYI: RNP v0.16.2 is tagged and released few hours ago.

(In reply to Nickolay Olshevsky from comment #28)

JFYI: RNP v0.16.2 is tagged and released few hours ago.

I saw, thank you. I will update the patch stack later today.

Attachment #9294047 - Attachment description: Bug 1790116 - update_rnp.sh changes for RNP v0.16.1. r=kaie → Bug 1790116 - update_rnp.sh changes for RNP v0.16.2. r=kaie
Attachment #9294048 - Attachment description: Bug 1790116 - Update librnp to v0.16.1. r=kaie → Bug 1790116 - Update librnp to v0.16.2. r=kaie
Attachment #9294049 - Attachment description: Bug 1790116 - mozbuild changes for RNP v0.16.1. r=kaie → Bug 1790116 - mozbuild changes for RNP v0.16.2. r=kaie
Attachment #9294322 - Attachment is obsolete: true

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

this will also simplify a potential uplift to esr102.

Since you mention it, this bug and bug 1790446 both uplift to comm-esr102 without any conflicts. Makes things easier.

Nickolay, with the security rules patch attached, the old patch from here:
https://searchfox.org/comm-central/source/third_party/patches/rnp/disable_obsolete_ciphers.patch
should no longer be necessary, do you agree?
We disable SM2 at build time (don't set ENABLE_SM2).

Flags: needinfo?(o.nickolay)

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

Nickolay, with the security rules patch attached,

The rules patch is here:
https://phabricator.services.mozilla.com/D158269

Nickolay, what's your recommendation regarding Botan?
We're still at 2.18.2, in both our stable and development branches.

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

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

Nickolay, with the security rules patch attached,

The rules patch is here:
https://phabricator.services.mozilla.com/D158269

Kai, could you please add me to the reviewers, as phabricator doesn't seem to allow me to submit comments and review?

Flags: needinfo?(o.nickolay)

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

Nickolay, what's your recommendation regarding Botan?
We're still at 2.18.2, in both our stable and development branches.

I don't see anything which could relate to the RNP functionality in the Botan changelog for v2.19.0-v2.19.1. Version 3.0 is still in alpha as well.

(In reply to Nickolay Olshevsky from comment #34)

The rules patch is here:
https://phabricator.services.mozilla.com/D158269

Kai, could you please add me to the reviewers, as phabricator doesn't seem to allow me to submit comments and review?

done

@Kai Hm, still got this trying to submit review (probably I should be added to the project, or whatever else? Newer used phabricator before so don't have a clue):

You do not have permission to edit this object.
Users with the "Can Edit" capability:
Members of the project "Restricted Project" can take this action.
The owner of a revision can always view and edit it.

Are you sure you are logged in to phabricator?
I've also added you as a subscriber, no idea if that helps.
I've never before heard the problems you describe, usually if people create an account, they can immediately add comments.

Yeah, I logged out/back in, tried Firefox instead of Safari - still the same, have 3 comments unsubmitted, and when trying to submit those at the bottom, requesting changes, receive the same error message.

Nickolay, please try again. Wayne gave you additional permissions.

Thanks, now it worked.

I think this is ready to land on comm-central for testing in Thunderbird Daily.
Please land together with the patches from bug 1791195.

Keywords: checkin-needed-tb
Target Milestone: --- → 107 Branch

Pushed by thunderbird@calypsoblue.org:
https://hg.mozilla.org/comm-central/rev/993606480caa
update_rnp.sh changes for RNP v0.16.2. r=kaie
https://hg.mozilla.org/comm-central/rev/ac79f53985cb
Update librnp to v0.16.2. r=kaie
https://hg.mozilla.org/comm-central/rev/ca4b3f6b3e3e
Update rnp_export.h. r=kaie
https://hg.mozilla.org/comm-central/rev/01c9e1069f75
Do not compile SM2 crypto with librnp. r=kaie
https://hg.mozilla.org/comm-central/rev/a63cf20cdf61
mozbuild changes for RNP v0.16.2. r=kaie

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED

We should consider to upgrade Thunderbird 102.x to this newer RNP version.
We have been asked for the backport in the scope of
https://bugzilla.redhat.com/show_bug.cgi?id=2133263

However, because we had some problems the last time we upgraded the stable Thunderbird branch (caused by functional changes in a past RNP release), I'd like to be more careful this time.

I've asked the TB e2ee community for testing and feedback.
https://thunderbird.topicbox.com/groups/e2ee/T89acb2c9d0fac43c-M185f6b28db93cf2ec1726ed3/testing-rnp-v0-16-2-with-thunderbird

The links I gave in that post refer to this try build:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=8d543c6ad4846d971eca309c0ea464ef445b90a2

Unfortunately, 5 days later, we have not yet received any feedback.

I'd prefer to get this uplifted to esr102. But there is some potential compatibility risk.

How should we proceed?

If we don't uplift, then for the remaining lifetime of the esr102 branch (until late 2023), we'd have to backport any (future) security fixes to RNP v0.16.0

We could either uplift and be prepared to back out if needed, nor just handle the uplift IF there are future security fixes that needs it.

The strategy to uplift on demand isn't ideal. With that, we'd have to deal with potential regressions at the same time, while facing the inability to backout because of a required security fix.

I'd prefer to uplift soon. I suggest to target early January.

Comment on attachment 9294048 [details]
Bug 1790116 - Update librnp to v0.16.2. r=kaie

requesting approval for 102.7.0 (mid january 2023)

[Approval Request Comment]
Regression caused by (bug #): none
User impact if declined: none
Testing completed (on c-c, etc.): in TB 107 beta
Risk to taking this patch (and alternatives if risky): slight risk for openpgp compatibility regressions

Attachment #9294048 - Flags: approval-comm-esr102?
Whiteboard: [TM:102.7.0]
Attachment #9294047 - Flags: approval-comm-esr102?
Attachment #9294049 - Flags: approval-comm-esr102?
Attachment #9294146 - Flags: approval-comm-esr102?
Attachment #9294321 - Flags: approval-comm-esr102?

Comment on attachment 9294321 [details]
Bug 1790116 - Do not compile SM2 crypto with librnp. r=kaie

[Triage Comment]
Approved for esr102

Attachment #9294321 - Flags: approval-comm-esr102? → approval-comm-esr102+

Comment on attachment 9294047 [details]
Bug 1790116 - update_rnp.sh changes for RNP v0.16.2. r=kaie

[Triage Comment]
Approved for esr102

Attachment #9294047 - Flags: approval-comm-esr102? → approval-comm-esr102+

Comment on attachment 9294049 [details]
Bug 1790116 - mozbuild changes for RNP v0.16.2. r=kaie

[Triage Comment]
Approved for esr102

Attachment #9294049 - Flags: approval-comm-esr102? → approval-comm-esr102+

Comment on attachment 9294146 [details]
Bug 1790116 - Update rnp_export.h. r=kaie

[Triage Comment]
Approved for esr102

Attachment #9294146 - Flags: approval-comm-esr102? → approval-comm-esr102+

Comment on attachment 9294048 [details]
Bug 1790116 - Update librnp to v0.16.2. r=kaie

[Triage Comment]
Approved for esr102

Attachment #9294048 - Flags: approval-comm-esr102? → approval-comm-esr102+
Regressions: 1830094
Blocks: 1688863
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: