1791195 - Changes required for updating to RNP v0.16.1+

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, task, P2)

Description

[Rob Lemley [:rjl]]

Attachment: image.png

After updating to RNP v0.16.1, two tests for "partial-signed" emails from Carol are failing in browser_viewPartialMessage.js. Both the test for partial-signed-from-carol-html.eml and partial-signed-from-carol-plaintext.eml fail with unknown verification icon is shown - false == true - JS frame :: chrome://mochitests/content/browser/comm/mail/test/browser/openpgp/browser_viewPartialMessage.js :: testPartialInlinePGPDecrypt :: line 219. The "Bad signature" icon rather than the "Unknown verificationn" icon is shown in both cases.

Nickolay Olshevsky was kind enough to triage (from bug 1790116 comment 15):

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://searchfox.org/comm-central/rev/1480d9f14070fee52641c2698fc9a104b02ceddb/mail/extensions/openpgp/content/modules/RNP.jsm#1163-1200

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

Comment 1

[Kai Engert (:KaiE:)]

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

How can an email have more than one signature?

PGP/MIME says there will be exactly one signature, see section 5:
https://www.rfc-editor.org/rfc/rfc3156.html

For PGP/INLINE, there should also be only one signature after the BEGIN PGP SIGNATURE line.

Comment 2

[Kai Engert (:KaiE:)]

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.

What's the error code returned in the new version?

Comment 3

[Kai Engert (:KaiE:)]

Oh, I guess it's RNP_ERROR_SIGNATURE_INVALID, because you quoted that code

Comment 4

[Kai Engert (:KaiE:)]

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

How can an email have more than one signature?

We currently handle only the first signature:
https://searchfox.org/comm-central/rev/1480d9f14070fee52641c2698fc9a104b02ceddb/mail/extensions/openpgp/content/modules/RNP.jsm#1389

Comment 5

[Nickolay Olshevsky]

Ah, that makes things easier. I was thinking of 'general' OpenPGP signed message, which could contain any number of signatures.

I would suggest to look for RNP_SUCCESS and RNP_SIGNATURE_INVALID as a result of successfull verification, and then check the status of the first signature (and see whether there are signatures at all). Other result would mean that source data is malformed/damaged/whatever else.
Please also note that RNP_SUCCESS may be returned if data was just succesfully decrypted, without being signed at all.

Comment 6

[Kai Engert (:KaiE:)]

I think it's necessary to work on this bug soon, updating our RNP base library and getting fixes is a good idea, and at least we need to start testing this update for potential other regressions on c-c.

The fix should be implemented in a way that is compatible for comm-esr102 as well, in case third part distributors ship newer RNP independently and combine it with stable 102.x

Comment 7

[Kai Engert (:KaiE:)]

If a message has one signature, how do I distinguish between:
(1) the signature couldn't be checked, because the signing key is missing
and
(2) the signature was checked, but doesn't match the contents?

Comment 8

[Kai Engert (:KaiE:)]

I guess I can no longer know by looking at the exit code, but must check myself.

I see I'm already checking this with rnp_op_verify_signature_get_key

Comment 9

[Kai Engert (:KaiE:)]

I found error code RNP_ERROR_KEY_NOT_FOUND, which can be returned by rnp_op_verify_signature_get_status.
I think I can use that.

Comment 10

[Kai Engert (:KaiE:)]

Removing needinfo, I think it's clear what I need to do.

Comment 11

[Kai Engert (:KaiE:)]

I'd like to use this bug for multiple patches to our application-level code that are necessary to version 0.16.1+

In addition to the signature handling, I'm also adding code that allows us to remove our RNP patches.

Comment 12

[Kai Engert (:KaiE:)]

Attachment: Bug 1791195 - Add RNP security rules to obsolete our patches to RNP. r=mkmelin

Comment 13

[Kai Engert (:KaiE:)]

Attachment: Bug 1791195 - Adjust OpenPGP signature handling for RNP >= 0.16.2. r=mkmelin

Depends on D158269

Comment 14

[Kai Engert (:KaiE:)]

The security rules patch configures:

• MD5 as insecure
• SHA1 as always acceptable for key signatures
• keeps RNP's default SHA1 configuration for data signatures

Rules are tied to an RNP library handle (ffi). All application code was changed to always use a handle with the rules applied.
I also added a runtime sanity check (executed once), to confirm that MD5 are really configured in the expected way.

The patch also sets 0.16.2 as the minimum library version we accept at runtime.

Comment 15

[Kai Engert (:KaiE:)]

Tests pass locally for me with these tests, including the signature status tests that Rob reported initially as failing.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/97d546d13f42
Add RNP security rules to obsolete our patches to RNP. r=mkmelin,o.nickolay
https://hg.mozilla.org/comm-central/rev/a6e391c0893e
Adjust OpenPGP signature handling for RNP >= 0.16.2. r=mkmelin

Comment 17

[Wayne Mery (:wsmwk)]

Should this be done in the same uplift as bug 1790116, i.e. 102.7.0 ?

Comment 18

[Kai Engert (:KaiE:)]

(In reply to Wayne Mery (:wsmwk) from comment #17)

Should this be done in the same uplift as bug 1790116, i.e. 102.7.0 ?

The following set of bugs has strong dependencies on each other.
We need all of them uplifted at the same time:

• bug 1753683
• bug 1790446
• bug 1790116
• bug 1790662
• bug 1791195

(5 bugs, 13 patches)

It's the same set of patches I had offered for early backporting to the RHEL people:
https://bugzilla.redhat.com/show_bug.cgi?id=2133263#c13

Comment 19

[Kai Engert (:KaiE:)]

Rob, Wayne, because I have not received any testing feedback regarding RNP v0.16.2 from the Thunderbird community, would it be possible to do the following?

• land all patches from comment 18 early into esr102
• create a candidate build
• ask community again for early testing of that candidate,
  like https://thunderbird.topicbox.com/groups/e2ee/T89acb2c9d0fac43c-M185f6b28db93cf2ec1726ed3

Comment 20

[Wayne Mery (:wsmwk)]

Comment on attachment 9296499 [details]
Bug 1791195 - Adjust OpenPGP signature handling for RNP >= 0.16.2. r=mkmelin

[Triage Comment]
Approved for esr102

Comment 21

[Wayne Mery (:wsmwk)]

Comment on attachment 9296498 [details]
Bug 1791195 - Add RNP security rules to obsolete our patches to RNP. r=mkmelin

[Triage Comment]
Approved for esr102

Comment 22

[Rob Lemley [:rjl]]

Thunderbird 102.7.0:
https://hg.mozilla.org/releases/comm-esr102/rev/7cc26adb9ca1
https://hg.mozilla.org/releases/comm-esr102/rev/4c402b79d558