Closed Bug 1658730 Opened 3 months ago Closed 3 months ago

Cannot import keys containing UTF-8-invalid strings

Categories

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

Thunderbird 81

Tracking

(thunderbird_esr78 fixed, thunderbird80 fixed)

RESOLVED FIXED
81 Branch
Tracking Status
thunderbird_esr78 --- fixed
thunderbird80 --- fixed

People

(Reporter: flo.sammueller, Assigned: flo.sammueller)

Details

Attachments

(3 files, 1 obsolete file)

Attached file test.asc

The import of a public key fails if a User-ID packet of the key contains characters that are invalid in UTF-8 encoding. I will attach a test key that is issued for "Test Müller" in ISO-8859-15 encoding that reveals this behaviour. "ü" in ISO-8859-15 has a hex value of 0xfc which is not a valid UTF-8 hexcode.
At import, the console shows this stacktrace:

TypeError: malformed UTF-8 character sequence at offset 6
    getKeyInfoFromHandle chrome://openpgp/content/modules/RNP.jsm:432
    getKeysFromFFI chrome://openpgp/content/modules/RNP.jsm:234
    importKeyBlockImpl chrome://openpgp/content/modules/RNP.jsm:1519
    importKeyBlockAPI chrome://openpgp/content/modules/cryptoAPI/RNPCryptoAPI.jsm:102
    importKeyAsync chrome://openpgp/content/modules/keyRing.jsm:801
    importKey chrome://openpgp/content/modules/keyRing.jsm:683
    EnigmailCommon_importObjectFromFile chrome://openpgp/content/ui/commonWorkflows.js:189
    oncommand chrome://openpgp/content/ui/enigmailKeyManager.xhtml:1

Note that the key can be handled by RNP on the command line, so it is not a problem of the library.
Especially users with quite old keys in their keyring (pre UTF-8-only times) could be affected by this bug.

Attached patch 1658730.patchSplinter Review

The thrown TypeError is actually the defined behaviour of readString() [1] which is used for collecting information about the key that is imported. Maybe this should be replaced by readStringReplaceMalformed() in all places where UTF-8-invalid characters can occur.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/js-ctypes/js-ctypes_reference/CData#readString

Florian, thanks a lot, I had seen this exception before, but it was still on my TODO list. Your analysis and example is very helpful. I'll look into this soon.

Priority: -- → P1
Assignee: nobody → kaie
Status: UNCONFIRMED → NEW
Ever confirmed: true

Your suggestion to use readStringReplaceMalformed for user IDs seems good. You missed one place, prim_uid_str.readString().

However, you also changed readString to readStringReplaceMalformed where we shouldn't do it. If our code produces an ASCII ARMOR buffer using RNP, then the result should be valid UTF-8. It would be good to keep that check, as a failure would indicate that unexpected data is given to us.

You're right, the returned char arrays after encryption/signing must be ASCII.

Status: NEW → ASSIGNED

Patrick didn't have time yet to review this, so let's do something else.

Because Florian has agreed to my suggested modifications, we can take this patch with Florian as the author, and myself as the reviewer.

Assignee: kaie → flo.sammueller
Attachment #9169713 - Attachment is obsolete: true
Attached patch 1658730-v2.patchSplinter Review

Patch by Florian, removed the places that are unnecessary, added one forgotton place.

r=kaie

Attachment #9170196 - Flags: review+

Pushed by kaie@kuix.de:
https://hg.mozilla.org/comm-central/rev/6f4962ef80ed
Allow importing of OpenPGP keys with user IDs that aren't UTF-8 encoded. r=kaie

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch

Comment on attachment 9170196 [details] [diff] [review]
1658730-v2.patch

OpenPGP correctness fix. For backwards compatibility, allow import/use of older keys that use a non UTF-8 encoding in the user-id meta information.

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

Comment on attachment 9170196 [details] [diff] [review]
1658730-v2.patch

[Triage Comment]
Approved for beta
Approved for esr78

Attachment #9170196 - Flags: approval-comm-esr78?
Attachment #9170196 - Flags: approval-comm-esr78+
Attachment #9170196 - Flags: approval-comm-beta?
Attachment #9170196 - Flags: approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.