1907282 - RNP failed to parse signature in certificate breaking OpenPGP support (due to experimental 1pa3pc signature in pubring.gpg)

Status: RESOLVED DUPLICATE of bug 1884337

(MailNews Core :: Security: OpenPGP, defect)

Description

[Justus Winter]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:109.0) Gecko/20100101 Firefox/115.0

Steps to reproduce:

I imported all my OpenPGP certificates (~1500) into Thunderbird in order to debug a scalability issue:

$ sq cert export > ./obj-x86_64-pc-linux-gnu/tmp/profile-default/pubring.gpg

Actual results:

RNP fails to parse a single signature in the certificates, and as a result the whole OpenPGP support in Thunderbird broke. I see on stderr:

[parse() /home/teythoon/repos/pep/thunderbird/source/comm/third_party/rnp/src/librepgp/stream-sig.cpp:405] unknown subpacket : 37
[parse_v4() /home/teythoon/repos/pep/thunderbird/source/comm/third_party/rnp/src/librepgp/stream-sig.cpp:1297] failed to parse hashed subpackets
[process_pgp_key_signatures() /home/teythoon/repos/pep/thunderbird/source/comm/third_party/rnp/src/librepgp/stream-key.cpp:243] failed to parse signature at 50707494
[...]
console.warn: "Get key info from handle FAILED for 0xundefined"
console.warn: "Get key info from handle FAILED for 0xundefined"
console.warn: "Get key info from handle FAILED for 0xundefined"

Thunderbird's OpenPGP certificate manager is empty, except for three obviously malformed and empty entries.

Expected results:

Thunderbird's OpenPGP certificate manager should list all my certificates.

Comment 1

[Corey Bryant]

Adding Kai to see if he can take a look

Comment 2

[Kai Engert [:KaiE:]]

Justus, thanks for the report.

I would like to reproduce the issue.
It would help me to have examples of keys that trigger this behavior.

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: Bug 1907282 - Improve/fix logging when there are issues with OpenPGP keys. r=kaie

This fixes the logging, and just skips the problematic entries.

Comment 4

[Kai Engert [:KaiE:]]

Thanks Magnus, have you tested that?

What worries me is Justus' statement that we end up in a broken state.

I would like to understand if that broken state is persistent across restarts.

Even if it's just for the one session, it's still bad and shouldn't happen. I would like to investigate in which state we arrive. We shouldn't get there, even if we import something that RNP cannot process.

Comment 5

[Magnus Melin [:mkmelin]]

No I don't have a test case, but at least the logging should now be able to say which key has an issue.
Justus, are you able to try the patch?

Comment 6

[Kai Engert [:KaiE:]]

The key that caused Justus' import to fail had a 1pa3pc subpacket (type 37).

I found an example key here:
https://gitlab.com/sequoia-pgp/sequoia/-/issues/335#note_528740078

I'll attach it to this bug.

I cannot reproduce a failure with this bug.
When trying to import with TB 128, it gives me a listing and offers to import, but then import fails.
(The reason is that we're relaxed when listing, and strict when importing.)

After the failed import, I still have keys in OpenPGP key manager.
I don't experience corruption.

Comment 7

[Nickolay Olshevsky]

Here is the issue which was created for the same purpose on RNP tracker: https://github.com/rnpgp/rnp/issues/2223
Nothing new here - we don't support this subpacket yet, and it is marked as hashed and critical so we do reject the signature. And depending on permissive import flag that would reject the single signature or the whole certificate block.

Comment 8

[Kai Engert [:KaiE:]]

FTR, I get the same console output as from comment 0 with the test key.

[parse() /home/user/moz/comm-beta/mozilla/comm/third_party/rnp/src/librepgp/stream-sig.cpp:405] unknown subpacket : 37
[parse_v4() /home/user/moz/comm-beta/mozilla/comm/third_party/rnp/src/librepgp/stream-sig.cpp:1297] failed to parse hashed subpackets
[process_pgp_key_signatures() /home/user/moz/comm-beta/mozilla/comm/third_party/rnp/src/librepgp/stream-key.cpp:243] failed to parse signature at 1019

I also tried to set mail.openpgp.allow_permissive_import to true, still no corruption.

Comment 9

[Justus Winter]

Thanks for the sleuthing! Yeah, looks like the experimental 1pa3pc signature is the cause of this.

Note that I bypassed Thunderbird's import logic by writing the certs directly to the pubring.gpg, which is arguably not what normal users would do, but I can see some scenarios where the same can happen without meddling with the file:

For example, suppose a user tries the Octopus, imports a cert with the 1pa3pc signature in it (I'm assuming it would pass Thunderbird's import screening then, but I haven't looked closely or tried that), now it is written with that signature to the pubring.gpg. Later, Thunderbird automatically upgrades, overwriting the Octopus with RNP, or maybe the user wants to switch back to RNP. Now, they are in the situation I was in.

Or, suppose there is a bug discovered in RNP and fixing it requires rejecting a signature that previously was considered valid. If a certificate was imported prior to the fix, and RNP is later updated, they end up in the same situation.

Or, suppose the pubring.gpg file is corrupted, say a bit is flipped in a signature due to cosmic radiation. Same situation.

I believe the best thing to do for both end users and the OpenPGP ecosystem as a whole is to gracefully deal with unexpected artifacts, no matter how they came into the store. Screening on import and trying to curate the certificate store is less robust than to be permissive all the time.

I haven't had a chance to try the patch as I'm currently traveling. I'll do so when I come back and do some more experiments.

Comment 10

[Kai Engert [:KaiE:]]

The attached patch is just for logging. Let's keep the bug open after commiting it.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0aa3e1da67ba
Improve/fix logging when there are issues with OpenPGP keys. r=kaie

Comment 12

[Magnus Melin [:mkmelin]]

Attachment: bug1907282-testkey.asc - from comment 6

This can be imported when I set mail.openpgp.allow_permissive_import true but not otherwise

Comment 13

[Kai Engert [:KaiE:]]

I assume this now works in TB version 135 and later, after the change in bug 1884337