1921728 - Binary OpenPGP keys ending with whitespace cannot be imported

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, defect)

Description

[u761096]

Attachment: 61FB8C42F9F5892D-pub-ends_with_whitespace.pgp

Steps to reproduce:

In the OpenPGP key manager, attempted to import an OpenPGP public key file in binary format (i.e. not ASCII-armored) that ends with a whitespace (more precisely: any character that is removed by the "trim" method of the JavaScript string type, see attached file for an example).

Actual results:

An alert popup with "Importing the keys failed" was shown and a warning "rnp_import_keys FAILED; rv=285212673" was logged.

Expected results:

The key should have been imported. (The example file can be imported with other OpenPGP tools such as rnpkeys and GPG.)

---

This happens because in "RNP.sys.mjs", the trim function is called on the input string before passing it to RNP independent of whether it is ASCII-armored or not -- see the statement

const trimmed = keyBlockStr.replace(/^Comment:.*(\r?\n|\r)/gm, "").trim();

in mail/extensions/openpgp/content/modules/RNP.sys.mjs in the comm-central repository 1.

The issue seems to be introduced in the changeset b615fc82fe5c19ba59b07e23f35aecf686827d03 2 when fixing the bug 1883855 3. The RNP issue 4 opened in the context of the aforementioned Bugzilla ticket appears to have been fixed in RNP 0.17.1, so the changes made in "RNP.sys.mjs" could probably be reverted to fix this problem.

Comment 1

[Magnus Melin [:mkmelin]]

Aras, thanks for the test case.
What commands did you use to create the test key? Can you attach a test case that hasn't expired?

61FB8C42F9F5892D-pub-ends_with_whitespace.pgp is expired, so I can't use it in my test. Or well, the primary key is expired, but the encryption isn't. RNP.findKeyByEmail can't find such keys. I'm not sure that's correct, but...

Comment 2

[u761096]

Thanks for the prompt reply.

I wrote a small Go program to "brute force" test cases, which I will attach in a moment. Originally it created keys that expired 1 second after creation to exclude the possibility of the test keys being unintentionally used for actual communication.

I have created a test key that doesn't expire, which I will also upload.

Comment 3

[u761096]

Attachment: Go program that generates an OpenPGP key ending with a whitespace byte

Comment 4

[u761096]

Attachment: DC1A523730F62AE8-pub-does_not_expire-ends_with_whitespace.pgp

Comment 5

[Nickolay Olshevsky]

JFYI: This key artifact includes subpacket 39 (v6 preferred AEAD ciphersuites), which support is defined only in RFC 9580.

Comment 6

[Magnus Melin [:mkmelin]]

Attachment: Bug 1921728 - Handle binary OpenPGP keys ending with whitespace. r=kaie

Comment 7

[Kai Engert [:KaiE:]]

(In reply to Nickolay Olshevsky from comment #5)

JFYI: This key artifact includes subpacket 39 (v6 preferred AEAD ciphersuites), which support is defined only in RFC 9580.

Nickolay, thanks for highlighting that.

As I understand it, the subpacket is in the unhashed area, and is NOT marked critical.
According to RFC 4880, section 5.2.3.1:
"An implementation SHOULD ignore any subpacket of a type that it does not recognize."

I understand that RNP v 0.17.1 currently ignores this subpacket, which matches the recommendation of the RFC.

Can we expect that RNP will continue to ignore such subpackets?

If yes, it might be acceptable to use this example key for this text.

Comment 8

[Nickolay Olshevsky]

Hi Kai,
This subpacket is actually in hashed area, but, correct, it's not critical so RNP just ignores it. And, yeah, we do not plan to change this behaviour.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1ffe45a6b0a9
Handle binary OpenPGP keys ending with whitespace. r=kaie

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9428224 [details]
Bug 1921728 - Handle binary OpenPGP keys ending with whitespace. r=kaie

[Approval Request Comment]
Regression caused by (bug #): bug 1883855
User impact if declined: can't import keys that happen to binary-end with whitespace
Testing completed (on c-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): safe

Comment 11

[Corey Bryant]

Comment on attachment 9428224 [details]
Bug 1921728 - Handle binary OpenPGP keys ending with whitespace. r=kaie

[Triage Comment]
Approved for beta

Comment 12

[Corey Bryant]

Thunderbird 132.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/89eb23984b51

Comment 13

[Corey Bryant]

Comment on attachment 9428224 [details]
Bug 1921728 - Handle binary OpenPGP keys ending with whitespace. r=kaie

[Triage Comment]
Approved for esr128

Comment 14

[Kai Engert [:KaiE:]]

I'd clarify that it is NOT about a "binary file with additional appended whitespace",
but rather, it's about
"binary file where the trailing binary byte(s) happen to have the same encoding as whitespace characters".

Comment 15

[Rob Lemley [:rjl]]

Thunderbird 128.3.2esr:
https://hg.mozilla.org/releases/comm-esr128/rev/b849043bf8c1

Comment 16

[Magnus Melin [:mkmelin]]

Aras, could you attach the Go program you used? And/or a key that does not expire.

The key from comment 4 expired, and since it was used in a test the test now fails...

Comment 17

[u761096]

Hello, AFAICT the key from that comment does not expire, the summary I see is:

pub   rsa2048 2024-09-30 [SC]
      8CCEFEAC2E8895ECF1A1E419DC1A523730F62AE8
uid           TB 128 Trim Bug Test No. 35 (DO NOT USE) <[e-mail]>
sub   rsa2048 2024-09-30 [E]

(Also, the program I used to generate was attached here.)

I think the issue in bug 1982971 is that the Apple security key included in the repo, which is imported by the failed test has expired.

Comment 18

[Magnus Melin [:mkmelin]]

Ah, thanks! I was looking at the wrong lines apparently