Decryption Oracle based on replying to PGP or S/MIME (CVE-2019-10732)
ClosedPublic

Authored by knauss on Apr 22 2019, 8:51 PM.

Details

Summary

ObjectTreeParser.htmlContent/plainTextContent may concats different encrypted parts without the user noticing it. That would lead to a Decryption oracle. We have already a logic to find the topleveltextNode in MimeTreeParser, this does not take HTML nodes into account nor that TEXT nodes may have several PGP Inline blocks.

That plaintextContent for HtmlMessagePart is not return QString(), that bubbled up by testing the stuff.

BUG: 404698

Test Plan

Current status:

"x" - means the reply does not leak information from other blocks

reply as text:

  • PGP Mime text
  • PGP Mime html
  • S/MIME
  • PGP inline

reply as html:

  • PGP Mime text
  • PGP Mime html ( and it removes the first text node, if it finds a HtmlMessagePart in leaf nodes)
  • S/MIME
  • PGP inline

will moved to another review request:
forward (text/html) (see D20847)

Diff Detail

Repository
R94 PIM: Message Library
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
knauss created this revision.Apr 22 2019, 8:51 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptApr 22 2019, 8:51 PM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript
knauss requested review of this revision.Apr 22 2019, 8:51 PM

So these pass? There's no bug?

So these pass? There's no bug?

Yes they pass. It looks for me, that we have no bug anymore.

But I needed to enable D20753 before the data is taken into account.

See the bug for further communication.

Sorry if this is a stupid question :D

But does this test make sense?

I mean how is the test supposed to decrypt the encrypted part of it? How does it know the keys/password/whatever to decrypt it?

This makes sense I think (and should go in IMHO), but I'm not sure if it is complete to cover all of bug 404698.

  • Do we need to be concerned about forwarding (which does include attachments AFAIK)?
  • Do we need to consider inline PGP? That is, have the encrypted part you want to leak in the main body?

Sorry if this is a stupid question :D

But does this test make sense?

I mean how is the test supposed to decrypt the encrypted part of it? How does it know the keys/password/whatever to decrypt it?

All the keys are inside messagecore/autotests/gnupg_home/, but this has some *.conf.in files, so can't use the version in sourcedir and need to use the gnupg_home in builddir.

Than we have cmake/modules/kdepim_add_gpg_crypto_test.cmake with the macro add_gpg_crypto_test that makes sure, that you have a gpg-agent running for executing the tests.

As you can see that the add_templateparser_unittest use this marco, so MimeTreeParser can decrypt stuff and some other tests in templateparserjob like test_processWithTemplatesForBody_data do this decryption tests.

This makes sense I think (and should go in IMHO), but I'm not sure if it is complete to cover all of bug 404698.

  • Do we need to be concerned about forwarding (which does include attachments AFAIK)?

make sense, yes.

  • Do we need to consider inline PGP? That is, have the encrypted part you want to leak in the main body?

inline PGP and also html content inside the encrypted parts may trigger issues.

knauss updated this revision to Diff 56831.Tue, Apr 23, 3:27 PM
  • Add test for a normal encrypted message, to make sure decryption works.
  • templateparserjobtest: HTML mime parts are handled differently
  • WIP: Decryption Oracle is also possible with PGP inline.

Decryption Oracle with PGP inline is successful.

knauss edited the summary of this revision. (Show Details)Tue, Apr 23, 3:34 PM
knauss edited the summary of this revision. (Show Details)Tue, Apr 23, 10:20 PM
knauss updated this revision to Diff 56856.Tue, Apr 23, 10:26 PM
  • MimeTreeParser returns wrong content for inline mesasges with multiple encrypted blocks.
  • TemplateParser: test htmlReply.
knauss updated this revision to Diff 56953.Thu, Apr 25, 12:27 PM
  • Add test for a normal encrypted message, to make sure decrpytion works.
  • templateparserjobtest: HTML mime parts are handled differently
  • WIP: Decryption Oracle is also possible with PGP inline.
  • TemplateParser: test htmlReply.
  • Rename convertedHtmlContent-> convertedHtmlContent for reply_Plain
  • Make unexpected data leak harder via reply.
knauss updated this revision to Diff 56954.Thu, Apr 25, 12:29 PM
  • Add test cases for a Decryption Oracle based on PGP inline.
  • TemplateParser: test htmlReply.
  • Rename convertedHtmlContent-> convertedHtmlContent for reply_Plain
  • Make unexpected data leak harder via reply.
knauss edited the summary of this revision. (Show Details)Thu, Apr 25, 12:30 PM

The review is getting a little bit messy, because of all those new test data.

I fixed the oracle based on reply to PGP inline and reply to html data.

The relevant changes are in:

  • mimetreeparser/src/messagepart.*
  • templateparser/src/templateparserjob.*

I'll create a new review for the tests for "Forward".

I think the patch is ready for review. So please give feedback.

knauss retitled this revision from Test mails for Decryption Oracle based on replying to PGP or S/MIME. to Decryption Oracle based on replying to PGP or S/MIME (CVE-2019-10732).Thu, Apr 25, 12:42 PM
knauss edited the summary of this revision. (Show Details)
knauss edited the test plan for this revision. (Show Details)
knauss added a subscriber: security-team.
vkrause accepted this revision.Fri, Apr 26, 7:45 AM

Looks good to me, thanks for taking care of this!

This revision is now accepted and ready to land.Fri, Apr 26, 7:45 AM
knauss edited the test plan for this revision. (Show Details)Fri, Apr 26, 3:48 PM

@security-team: should I split the commits code/test so they are easier to backport? On the other side adding the tests also to old code makes it more clear that this is fixed.

I don't think splitting is needed. As you say, unittests belong with the fixes.

This revision was automatically updated to reflect the committed changes.