1507218 - (CVE-2018-18509) Wrong Thunderbird S/MIME signature status shown, if CMS signed data has data content.

Status: RESOLVED FIXED

(MailNews Core :: Security: S/MIME, defect)

Description

[Damian Poddebniak]

Attachment: eContentConfusion.eml

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

Steps to reproduce:

Build a special *.eml file with conflicting requirements. I used the der2ascii tool¹ to create this email. The error is further explained in the uploaded file itself.

¹ https://github.com/google/der-ascii


Actual results:

The email is showed as being signed by Damian Poddebniak (me), even though the text can be freely changed.


Expected results:

Error out due to "conflicting requirements" OR display of the verified content only.

Comment 1

[Damian Poddebniak]

This works in the latest Thunderbirds (60.0, 60.2.1, 60.3.0) but I expect it to work in all (?) previous versions supporting opaque signing.

Comment 2

[Ben Bucksch (:BenB)]

Content of message with more explanation:

This is arbitrary text not covered by any signature. However,
Thunderbird shows this message together with all UI elements
indicating a valid signature.

In order to verify this, (1) change this text,
                         (2) reopen in Thunderbird, and
			 (3) see that nothing changes 

The problem is, that Thunderbird validates the signature against the
text contained in the CMS object in the second part of this email (the
eContent), but displays the detached part (this text here).

The solution is simple: error out in case that both the eContent and
some external content is present OR only show what was verified.

Comment 3

[Damian Poddebniak]

By the way, this could also potentially affect xpi addons. But as far as I tested, I was not able to bypass the signature check and change the manifest. I will continue testing.

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

Thanks for the report. Magnus, can you have someone look into this or do so yourself?

Comment 5

[Damian Poddebniak]

Any updates on this? :-)

Comment 6

[Magnus Melin [:mkmelin]]

Yeah I was looking at this earlier with not a huge success. Will investigate some more now.

Comment 7

[Damian Poddebniak]

If I can assist somehow, please ask. It should be enough to open the provided mail in thunderbird as the used certificate is valid.

Comment 8

[Damian Poddebniak]

I am not familiar with the Thunderbird codebase (yet), but I expect that the signature is first checked somewhere and then some other code decided to take the detached text without noticing that the eContent was used for verification.

Comment 9

[Magnus Melin [:mkmelin]]

As far as I can tell, this bug is in NSS. The structure of the message is not really odd, but once the signature part is sent off for verification it verifies against the internal text inside what's supposed to be the signature (though I suppose that's legal, as a non-detached signature?)

The comment at https://searchfox.org/comm-central/source/mailnews/mime/src/nsCMS.cpp#258-264 is not exactly encouraging... but maybe not the core issue.

Kai, can you make any sense of this?

Comment 10

[Damian Poddebniak]

S/MIME supports multiple signers (signerInfos is a list of signerInfo) in one CMS object. I think that the comment means that multiple signers are not supported at all. I like that. Especially, because having multiple signers may introduce problems like showing "good signature" even if some signatures are invalid. Also, the UI must be changed to support multiple signers. I think this will introduce more problems that it solves.

The internal text (called eContent in CMS-speak) is used in opaque signatures, i.e. those that are not send via multipart/signed. Having an empty eContent signals that the signed data must come from somewhere else. In the case of S/MIME this is typically the text in the detached part. But having a non-empty eContent AND a multipart/signed message makes no sense.

Maybe it is okay to just error out in this situation? This is what Claws Mail does.

Comment 11

[Kai Engert [:KaiE:]]

Damian, thanks for the bug report, test case and the explanations.

(In reply to Damian Poddebniak from comment #10)

But having a non-empty eContent AND a multipart/signed message makes no sense.
Maybe it is okay to just error out in this situation? This is what Claws Mail does.

The current implementation probably doesn't know about this potential inconsistency, and doesn't try to detect it. If it finds signature data at the matching MIME nesting level, then it will update the UI. I'm reading through the code, to find the right place to fix it. We could simply ignore the signature (not display signature UI) if we find eContent in the context of multipart/signed, or, we could report the "invalid signature icon" without trying to verify.

(The source comment that Magnus mentioned in comment 9 seems to be unrelated.)

Comment 12

[Kai Engert [:KaiE:]]

Analyzing this bug raises concerns about inconsistencies in the code.

The MIME processing code calculates the digest/hash of the displayed message contents. We arrive in nsCMSMessage::CommonVerifySignature, which uses the locally computed digest data. The digest parameter has a variable size. If the multipart message mentioned micalg-sha256, the digest parameter has the expected size (32 bytes, 256 bits).

However, nsCMSMessage::CommonVerifySignature proceeds to tell NSS about the digest, by calling NSS_CMSSignedData_SetDigestValue. Here it uses a hardcoded value SEC_OID_SHA1.

Despite this inconsistency, NSS happily accepts the digest into and stores it. Later we arrive in NSS_CMSSignedData_VerifySignerInfo, which finds the digest, and thinks it's a digest of type SHA256, and the comparison succeeds.

I don't understand yet why this works. But it seems this is an inconsistency that should get analyzed further. The data structures used here by NSS have the ability to store multiple digests of different types, so maybe it only works for most messages, because there's usually just one digest calculated.

I think that code should be fixed, it should no longer hardcode SEC_OID_SHA1, but should use the same digest identifier that was used during digest calculation (e.g. hash_type in mimemcms.cpp), and pass on together as an additioan parameter, along with the digest data and length.

Now, back to this bug.

In this scenario, the decoding of the CMS content data by NSS has already triggered the calculation of a digest value, and NSS already has it remembered. It's of type SHA256.

nsCMSMessage::CommonVerifySignature attempts to tell NSS about the message content data that the MIME code has calulated. Because it uses the (incorrect) SEC_OID_SHA1 identifier, NSS stores it as an additional digest. Later, when verifying inside NSS_CMSSignedData_VerifySignerInfo, NSS picks the remembered SHA256 digest, and is happy.

As a quick test, I changed the hardcoded value to SEC_OID_SHA256. With this change, when nsCMSMessage::CommonVerifySignature calls NSS_CMSSignedData_SetDigestValue, it overrides the earlier SHA256 digest. And later in NSS_CMSSignedData_VerifySignerInfo, a mismatch is detected and the user interface shows the "invalid signature" icon.

So, in order to fix this bug, we should:

• try to understand if the hardcoded _SHA1 tag could cause other signature status display with other messages, too

• replace the hardcoded _SHA1 with the correct information, the same as used during MIME digest calculation

• ensure we correctly handle a scenario in which multiple digests are present. For example, the multipart message could request a migalg-sha384, but the CMS content info signature might use SHA256. When arriving inside NSS_CMSSignedData_VerifySignerInfo, it would find two difference digests, and might select any of it, and if it selects the SHA256 digest, it might again cause a correct signature display. I think when arriving in nsCMSMessage::CommonVerifySignature, we should remove all digests from the NSS structure, which NSS might have already created during CMS decoding, and ensure that only a single digest will be set, which is the one obtained from the MIME calculation, which hashed the displayed content. With this, we'll either get a mismatch, if the signature was created for a different content, or with a different hash algorithm. Or, if the CMS content is identical to the message content, and both use the same hash algorithm, and it matches the digest used in the signature, then we'll correctly get a valid signature display.

Comment 13

[Kai Engert [:KaiE:]]

Attachment: testcase2.eml

This is an additional testcase. It's almost identical to Damian's file, only difference is that it says micalg-sha384 in the header.

After fixing the hardcoded SHA1 parameter, Damian's file no longer triggers the bug. But this sha384 file still triggers is.

Comment 14

[Kai Engert [:KaiE:]]

Attachment: 1507218-v1.patch (don't use yet)

Comment 15

[Kai Engert [:KaiE:]]

Attachment: no comment

Comment 16

[Damian Poddebniak]

We may disclose this/and similar issues soon. I really don't want to stress, especially as the fix may not be trivial. Bit do you have any updates on this?

PS: we would also like to register a CVE as to document it appropriately. How should I proceed?

Comment 17

[Kai Engert [:KaiE:]]

PS: we would also like to register a CVE as to document it appropriately. How should I proceed?

Al, could you please help out and assign a CVE?

Comment 18

[Kai Engert [:KaiE:]]

(In reply to Damian Poddebniak from comment #16)

We may disclose this/and similar issues soon. I really don't want to stress, especially as the fix may not be trivial. Bit do you have any updates on this?

Hi Damian, thanks for the update.

Several of us have been travelling to various conferences during the past weeks, devconf.cz, fosdem.org and others, so I'm sorry for the delay.

I've just asked Bob for review, whom I had contacted in advance by email. I hope we'll be able to move forward on this issue soon.

Comment 19

[Damian Poddebniak]

Thanks! We evaluated signature-handling/signature-presentation from different directions and wrote a paper on that topic. This is why I can not say for sure when will be the best moment for us to put our test cases online for example.

Anyway, if you are interested in the unsuccesful tests, I can provide a link to our repository. I think some of the tests could be useful as unit tests to avoid regression in future.

Do you mind if I incorporate the test you provided into our test set? (testcase2.eml)

Comment 20

[Kai Engert [:KaiE:]]

(In reply to Damian Poddebniak from comment #19)

Do you mind if I incorporate the test you provided into our test set? (testcase2.eml)

Sure, go ahead. Might be useful to slightly modify that message, for example use a different subject, to distinguish it more easily.

Comment 21

[Al Billings [:abillings - ex-MoCo]]

CVE-2018-18509 assigned.

Comment 22

[Robert Relyea]

Comment on attachment 9039106 [details] [diff] [review]
1507218-v1.patch (don't use yet)

Review of attachment 9039106 [details] [diff] [review]:
-----------------------------------------------------------------

basic comments. Nothing that needs to actually change.
r+
bob

::: mailnews/mime/src/nsCMS.cpp
@@ +213,5 @@
> +      case SEC_OID_PKCS7_DIGESTED_DATA:
> +      default: {
> +        MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("nsCMSMessage::CommonVerifySignature - unexpected ContentTypeTag\n"));
> +        rv = NS_ERROR_CMS_VERIFY_NO_CONTENT_INFO;
> +        goto loser;

style: you could simply  break here and let the if below handle it. That would be more 'efficient'. keeping the current code is probably better for future maintanence (a break only works as long as sigd is null coming in and the if below doesn't move). Also the current code is superior for debug/logging as it gives a unique message for failure because of incorrect content type as opposed to failure in NSS_CMSContentInfo_GetContent().

upshot: you could 'optimize' this with a break if you want to, but there are good reasons to keep the code as written.

@@ +656,5 @@
> +      aOidTag = SEC_OID_SHA512;
> +      break;
> +    default:
> +      rv = false;
> +  }

It's unfortunate you are using yet another way of specifying hashes separate from NSS. NSS already has at least 2 ways (SECOidTag and HASH_HashType). There are already ways of converting one to another. This code means that you'll have to update you code by hand in order to handle new hashes.

Comment 23

[Kai Engert [:KaiE:]]

Bob, thanks for the review.

(In reply to Robert Relyea from comment #22)

upshot: you could 'optimize' this with a break if you want to, but there are
good reasons to keep the code as written.

I agree. Having the more detailed error logging is good, and making it explicit this is a failure scenario is good, too.

It's unfortunate you are using yet another way of specifying hashes separate
from NSS. NSS already has at least 2 ways (SECOidTag and HASH_HashType).
There are already ways of converting one to another. This code means that
you'll have to update you code by hand in order to handle new hashes.

The constant symbols we're using here are from a PSM IDL, but they're expected to be in sync with the values from HASH_HashType. If there's an existing NSS conversion function from HASH_HashType to SECOidTag then we could reuse it.

However, I couldn't find an exported function for that, other places that need this conversion have also defined it locally, for example cmd/digest has an incomplete HashTypeToOID, and the new nss-tool for testing also has something that doesn't help here.

Given that we're likely to transplant this patch on older branches, I think it's best to potentially optimize your suggestion in a separate bug.

Comment 24

[Kai Engert [:KaiE:]]

I've filed bug 1526302 and bug 1526336 so we can improve the mapping in the future.

Comment 25

[Kai Engert [:KaiE:]]

I wonder how widespread this kind of signature is. Damian, do you know if any common S/MIME sending software creates signatures with embedded content?

If Charlie wants to use this bug to send a spoofed message, claiming to be sent by Alice, then Charlie needs to obtain any signed message from Alice that contains a signature with embedded content.

If Charlie uses a signature which wasn't created by Alice (like Damian's signature in the example), as a consequence the signature's email address doesn't match Alice's address, then Thunderbird displays a "questionable signature", not the expected "broken signature".

Comment 26

[Kai Engert [:KaiE:]]

I think the patch is ready to land, but the Thunderbird drivers need to discuss when the fix can be released. I'll email them, and give an update once it's decided.

The patch contains a comment, which makes it pretty obvious what the bug is about:

• // Potentially there was embedded content in CMS signed data,
• // and NSS has already calculated a digest for it.
• // Because we don't display that embedded content, we set all
• // preexisting digests to NULL, to ensure NSS won't use them.

When landing before an embargo date, it might be better to land without that comment.

The checkin comment could be
"don't use a hardcoded SEC_OID_SHA1 parameter",
which could serve as a valid description for most of the changes in the patch.

Comment 27

[Kai Engert [:KaiE:]]

Attachment: Patch without comment

Comment 28

[Damian Poddebniak]

I wonder how widespread this kind of signature is. Damian, do you know if any common S/MIME sending software creates signatures with embedded content?

Yes. At least Outlook 2013 and 2016 use opaque-signed as default.

If Charlie wants to use this bug to send a spoofed message, claiming to be sent by Alice, then Charlie needs to obtain any signed message from Alice that contains a signature with embedded content.

Charlie can also obtain a multipart/signed message and transform it to an opaque-signed one. detached --> opaque and opaque --> detached can easily be converted.

If Charlie uses a signature which wasn't created by Alice (like Damian's signature in the example), as a consequence the signature's email address doesn't match Alice's address, then Thunderbird displays a "questionable signature", not the expected "broken signature".

Yes. In order to spoof a signed email from Alice, Charlie must obtain a signed email from her first. But this is easy: just ask Alice something and get a signed answer. Then use this signature to spoof arbitrary new messages of Alice.

Comment 29

[Jorg K (CEST = GMT+2)]

I'm here to land whichever version you want with whichever commit message :-)

Comment 30

[Jorg K (CEST = GMT+2)]

Umm, that patch produces link errors :-(

0:15.60 lld-link.exe: error: undefined symbol: NSS_CMSSignedData_HasDigests
0:15.60 >>> referenced by c:\mozilla-source\comm-central\comm\mailnews\mime\src\nsCMS.cpp:228
0:15.60 >>> ....\comm\mailnews\mime\src\nsCMS.obj:(?CommonVerifySignature@nsCMSMessage@@AEAA?AW4nsresult@@PEAEIF@Z)
0:15.60 lld-link.exe: error: undefined symbol: NSS_CMSSignedData_GetDigestAlgs
0:15.60 >>> referenced by c:\mozilla-source\comm-central\comm\mailnews\mime\src\nsCMS.cpp:229
0:15.60 >>> ....\comm\mailnews\mime\src\nsCMS.obj:(?CommonVerifySignature@nsCMSMessage@@AEAA?AW4nsresult@@PEAEIF@Z)

Comment 31

[Kai Engert [:KaiE:]]

I had only tested on Linux so far. Windows folds all NSS symbols into a single DLL, and it uses a whitelist of symbols that are used. The patch uses two functions from NSS that were previously unused.

This means we need a change to Gecko that adds the new symbols. I filed bug 1526473, attached the necessary change and asked for review.

It also means, when uplifting to older branches, we need a Gecko branch that uplifts that change (unless we can convince Firefox people to also uplift that). I can try to ask for Beta.

Comment 32

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-beta/rev/77a16669b9c81f4132378effa8a1201953b7eee7

Bug 1526473 landed on C-B already, so I'm doing this in an unorthodox order ;-)

Comment 33

[Kai Engert [:KaiE:]]

I discovered that it was unnecessary to change the Gecko branches...

Because of the hassle of getting Gecko changed, I filed bug 1526519 to figure out how to override the list of exported symbols from within an Thunderbird build, and then I discovered it's already possible.

Variable NSS_EXTRA_SYMBOLS_FILE can be defined at build time. It can point to a filename with extra symbols to export. If defined, Gecko's nss.symbols file will include the file defined by the variable. It needs to be a relative path, relative to the security directory.

For example:
NSS_EXTRA_SYMBOLS_FILE=../comm/comm-extra-nss.symbols

It works to define that variable in comm/mail/confvars.sh

We could try that approach for comm-esr60.

Comment 34

[Kai Engert [:KaiE:]]

Attachment: Patch without comment - merged to esr-60, uses local config for extra NSS symbols

This might work.

Comment 35

[Jorg K (CEST = GMT+2)]

Comment on attachment 9042758 [details] [diff] [review]
Patch without comment - merged to esr-60, uses local config for extra NSS symbols

Review of attachment 9042758 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/confvars.sh
@@ +45,5 @@
>  MOZ_ENABLE_SIGNMAR=1
>  
>  MOZ_DEVTOOLS=all
> +
> +NSS_EXTRA_SYMBOLS_FILE=../comm/comm-extra-nss.symbols

How about $commreltopsrcdir/comm-extra-nss.symbols ?

Comment 36

[Kai Engert [:KaiE:]]

(In reply to Jorg K (GMT+1) from comment #35)

How about $commreltopsrcdir/comm-extra-nss.symbols ?

Thanks for the suggestion, sounds good. (I'm not up to date on the build time variables.)
If you prefer, we could also use a shorter filename, extra-nss.symbols

Comment 37

[Jorg K (CEST = GMT+2)]

I'm not familiar with the build variables either, but I looked for examples in confvars.sh.

Comment 38

[Kai Engert [:KaiE:]]

It might not work.
The other examples might assume PWD is the top level directory.

At the time the variable is used for loading, the PWD is mozilla/security/
So, we either require an absolute path, or a relative path that will navigate out of the security subdirectory.

My objdir/mozilla-config.h defines it simply as "comm". I think we need the ../ part in my suggestion.

Comment 39

[Jorg K (CEST = GMT+2)]

Umm, what is mozilla/security/? Looks like the old setup where M-C was stored in mozilla/.

Comment 40

[Kai Engert [:KaiE:]]

I use "mozilla" as a synonym for the toplevel directory of any mozilla tree.

The existing file nss.symbols is located in TOPLEVEL/security/nss.symbols

The comm tree is at TOPLEVEL/comm

We need a relative path from TOPLEVEL/security/nss.symbols to TOPLEVEL/comm,
which is why I used ../comm/ as the prefix for an extra NSS symbols file inside the comm tree.

Comment 41

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=eb0b62b3628d4fd47a465b84c065bd54dac46a1f

With https://hg.mozilla.org/try-comm-central/rev/1dd1572e6571a33dd8de173b369198c9210e2077#l2.13

Let's see.

Comment 42

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/32b5054643ac7bfd76a1b93883718f1e86dc55cb
Don't use a hardcoded SEC_OID_SHA1 parameter. r=rrelyea

Comment 43

[Jorg K (CEST = GMT+2)]

BTW, my ESR 60 suggestion didn't work, so here's a try with Kai's original suggestion:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bec21e91a896bec6f5dd515ed0a4634b94262012

Comment 44

[Jorg K (CEST = GMT+2)]

That worked, so I'm putting together a TB 60.5.1 later today.

Comment 45

[Al Billings [:abillings - ex-MoCo]]

(In reply to Jorg K (GMT+1) from comment #44)

That worked, so I'm putting together a TB 60.5.1 later today.

Is someone writing an advisory for this bug?

Comment 46

[Wayne Mery (:wsmwk)]

redirecting

Comment 47

[Jorg K (CEST = GMT+2)]

TB 60.5.1:
https://hg.mozilla.org/releases/comm-esr60/rev/a251d4246dcb54e4a06c94f63171dbceedd4c82e

Comment 48

[Frank-Rainer Grahl (:frg)]

Attachment: Fix for SeaMonkey link error

Comment 49

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9043408 [details] [diff] [review]
Fix for SeaMonkey link error

https://hg.mozilla.org/releases/comm-esr60/rev/66fb1b1fc5059bc6069693ca9a2886a10835f51b

Comment 50

[Kai Engert [:KaiE:]]

Damian, when crediting you as the reporter of the bug, is it sufficient to mention your full name?

Comment 51

[Kai Engert [:KaiE:]]

(In reply to Al Billings [:abillings] from comment #45)

Is someone writing an advisory for this bug?

Draft advisory sent to security@m.o

Comment 52

[Damian Poddebniak]

(In reply to Kai Engert (:kaie:) from comment #50)

Damian, when crediting you as the reporter of the bug, is it sufficient to mention your full name?

Yes :-)

Comment 53

[Kai Engert [:KaiE:]]

Damian, FYI, it seems like we'll release a TB update today with the fix from the bug, with a security advisory that discloses the issue.

Comment 54

[Damian Poddebniak]

Thank you very much! (Also for discussing this so openly and keeping me up to date.)

Comment 55

[Kai Engert [:KaiE:]]

The fix introduced regression bug 1528615.