298538 - S/MIME message verification fails if cert is signing-only

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

Using the attached message and signature files, and a secmod.db containing the
root cert module, the following command fails :

cmsutil -D -d . -h 1 -c message -i signature

                signer0.id="Iain MacDonnell (50766)";
signer0.status=SigningCertNotTrusted;

The problem is actually that CERT_FindCertIssuer returned NULL . This is despite
the fact that the certs were previously imported as temporary.

If you rerun this command with the additional -k argument, which causes the
certs to be imported to the permanent DB, then the verification passes.

There is no reason why our chain building and verification algorithm should
behave any different whether the certs are temp certs or a permanent certs. I
believe the certs are fine, and the verification should succeed in both cases.

Comment 1

[Julien Pierre]

Attachment: message file for the test

Comment 2

[Julien Pierre]

Attachment: signature file for the test

Comment 3

[Julien Pierre]

A clarification here - I don't expect the message (as attached) to pass the
signature verification . There will be a DigestMismatch error when -k is used.
But that's farther along than without -k - which dies because of
CERT_VerifyCertificate .

The problem was diagnosed with a real cert chain, which is 4 certs long - 1 EE,
2 intermediates and one root. The message signature includes 3 - all but the
root, which is valid. The root is GTE, and is available in the built-ins. I will
attach the DER for the individual certs in a moment.

Comment 4

[Julien Pierre]

Looks like the certs are all being imported with CERT_ImportCerts, and
immediately destroyed in NSS_CMSSignedData_ImportCerts . This causes cmsutil -D
to always fail without the -k option, as best as I can tell, even with my own
cert chain .

Comment 5

[Julien Pierre]

It turns out the problem is related to the fact that the cert is only a signing
cert. Mozilla actually tries to import the certs permanently, but only with the
certUsageEmailRecipient usage. With Iain's certs, this fails. NSS_CMSSignedData
should do a temporary import of the certs, but instead, it just discards them. I
believe I can add a CERTCertList to the NSS_CMSSignedData structure for the
purpose of holding the temp certs.

Comment 6

[Julien Pierre]

Attachment: Fix

The fix is to save the temp certs within the NSSCMSSignedData structure, so
they are always available when the signature on this data needs to be verified
later .

Comment 7

[Julien Pierre]

I verified that this fix works both for my cmsutil test case and with mozilla
mail . In mozilla mail, with the updated libsmime3.so, I can now verify the
original message signed with a signing-only SunPKI cert. This operation
previously failed because the issuer was not found.

IMO, this fix should be urgently go into Mozilla Mail and Thunderbird as well as
in NSS 3.10.1 .

Comment 8

[Iain MacDonnell]

I applied the patch to thunderbird 1.0.2. My signed messages are still not
verifying - it says that the certificate used to sign the message was issued by
a CA that I do not trust. When I view the certificate, I do see the complete
chain now, though, which I did not before (only say the signing cert and CA cert
that signed it). Is there something more that needs to be done to integrate the
fix into thunderbird?

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 187256 [details] [diff] [review]
Fix

I'm not sure that the new list of certs, named tempCerts, is needed.  I think
that perhaps the existing list of certs, named simply certs, could be used for
this purpose.  
 Even if it's not strictly needed, it may be that having two lists will be
cleaner, avoiding any interaction between two sets of certs combined into a
single list.  But the code in this patch appears to be correct, so r=nelson. 
I agree that this should go into the email clients ASAP.

Comment 10

[Iain MacDonnell]

Sorry, scratch my previous message - I had a bad (expired) copy of one of the
higher-up CA certs stored in my software security device. I cleaned that out,
and now my messages verify OK. I went back to my previous (unpatched)
thunderbird build (same profile) and confirmed that they do NOT verify OK there,
then back to the patched version, and all looks good.

Comment 11

[Wan-Teh Chang]

Comment on attachment 187256 [details] [diff] [review]
Fix

You can avoid adding the NSS_CMSSignedData_AddTempCertificate function
by passing &sigd->tempCerts instead of &certArray to
CERT_ImportCerts in NSS_CMSSignedData_ImportCerts.

The purpose of "tempCerts" should be documented in cmst.h.

It seems that the existing "certs" field is only used for *encoding*
a signed message, so it should be possible to use "certs" for a
different purpose when we *decode* a signed message.  But I agree that
it may not be worthwhile to do it.

Comment 12

[Wan-Teh Chang]

Another reason to not add NSS_CMSSignedData_AddTempCertificate
is that unlike NSS_CMSSignedData_AddCertificateit, it is not a
generally useful function.  At least, it should be made a static
function or a module private function (declared in cmslocal.h).

Comment 13

[Julien Pierre]

Wan-Teh,

certArray is a local variable . I can't just use CERT_ImportCerts on it. I need
to keep track of it, so I can destroy it with the NSSCMSSignedData . That's why
I added an "AddTempCertificate" function . I could add an "AddTempCertArray"
function instead, but I think the later is more complicated as it involves
merging arrays. It is also less flexible, in case the future source of temp
certs isn't an array . I don't agree the new function may not have other uses -
I think it could - but I agree it should be made static and moved to cmslocal.h
. I will provide a new patch to that effect .

Do you agree the fix should go into both 3.10.1 and 3.11 ?

Nelson,

I added a separate temp cert list because I didn't want to interfere with the
use of the existing cert list.

Comment 14

[Julien Pierre]

Attachment: update CMS header files

I didn't make the new function NSS_CMSSignedData_AddTempCertificate static,
because nearly all of the functions in cmslocal.h are extern . I think it's OK
for other source files within libsmime to call that function. It just shouldn't
be exported in the shared library.

Comment 15

[Wan-Teh Chang]

Attachment: Patch that describes wtc's suggested changes

This patch is the best way to describe the changes
I suggested.

Comment 16

[Wan-Teh Chang]

Comment on attachment 187434 [details] [diff] [review]
update CMS header files

I don't think the NSS_CMSSignedData_AddTempCertificate function
is useful in any other circumstances, but it's okay to add it.

Comment 17

[Julien Pierre]

Comment on attachment 187435 [details] [diff] [review]
Patch that describes wtc's suggested changes

Wan-Teh,

I understand how your patch works and I believe it is correct. However, I think
the original author of the S/MIME library tried to maintain a relatively high
degree of data abstraction, and thus created setter and/or getter functions for
many of the structure fields, and my patch continues that.

Comment 18

[Julien Pierre]

Attachment: Patch as checked in

Thanks for the review, Wan-Teh.
I checked in the patch to NSS_3_10_BRANCH :

Checking in cmslocal.h;
/cvsroot/mozilla/security/nss/lib/smime/cmslocal.h,v  <--  cmslocal.h
new revision: 1.4.8.1; previous revision: 1.4
done
Checking in cmssigdata.c;
/cvsroot/mozilla/security/nss/lib/smime/cmssigdata.c,v	<--  cmssigdata.c
new revision: 1.28.8.1; previous revision: 1.28
done
Checking in cmst.h;
/cvsroot/mozilla/security/nss/lib/smime/cmst.h,v  <--  cmst.h
new revision: 1.9.8.1; previous revision: 1.9
done

and to the tip

Checking in cmslocal.h;
/cvsroot/mozilla/security/nss/lib/smime/cmslocal.h,v  <--  cmslocal.h
new revision: 1.5; previous revision: 1.4
done
Checking in cmssigdata.c;
/cvsroot/mozilla/security/nss/lib/smime/cmssigdata.c,v	<--  cmssigdata.c
new revision: 1.29; previous revision: 1.28
done
Checking in cmst.h;
/cvsroot/mozilla/security/nss/lib/smime/cmst.h,v  <--  cmst.h
new revision: 1.10; previous revision: 1.9
done

Comment 19

[Wan-Teh Chang]

There is one thing about this bug that I don't understand.

It is common for a user to have two certs: one signing-only
cert, and one encryption-only cert.

If a signed email message has both of these certs, do we
import both of them or just the encryption-only cert into
the permanent database?

If we only import the encryption-only cert into the permanent
database, how were we able to verify the signature before?

Comment 20

[Iain MacDonnell]

(In reply to comment #19)
> There is one thing about this bug that I don't understand.
> 
> It is common for a user to have two certs: one signing-only
> cert, and one encryption-only cert.

The reason we do this is so we can escrow the private key that's used for
encryption (so we can recover encrypted data if it gets lost) whereas we
don't want to escrow the signing key.


> If a signed email message has both of these certs, do we
> import both of them or just the encryption-only cert into
> the permanent database?

I'd say just the encryption one, as that's the one you'll need if you
want to send an encrypted response.


> If we only import the encryption-only cert into the permanent
> database, how were we able to verify the signature before?

We weren't - hence the bug :) It only worked for certs that were for both
signing and encryption... Right?

Comment 21

[Julien Pierre]

Wan-Teh,

In the early releases of Mozilla - 1.0 through 1.3, the PSM S/MIME code was
importing all the e-mail certs permanently, setting the keepCerts argument to 1.
This was happening regardless of whether the certs were for signing, encryption,
or both .

In Mozilla 1.4, PSM got changed to no longer import the signing certs
permanently. Only the encryption certs were imported permanently. The other
certs were supposed to be imported temporarily - except they weren't, due to
this NSS bug.

Thus, Mozilla hasn't been able to verify the signing-only certs since version 1.4.

Comment 22

[Wan-Teh Chang]

Does the signed message come with both the signing-only
and encryption-only certs, or just the signing-only cert?

If it is the former, this is a major regression and I'm
surprised that I haven't run into it.

Comment 23

[Iain MacDonnell]

(In reply to comment #22)
> Does the signed message come with both the signing-only
> and encryption-only certs, or just the signing-only cert?

Depends how the sending client is configured, I guess. In my tests, I was
sending the signing cert only - hadn't bothered with the encryption cert yet.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Yes, this is a major regression that happened long ago.  
It shows that our SMIME QA coverage is woefully inadequate.  
In particular, it seems we don't cover separate signing/encryption certs.
Additional QA is greatly needed.  Seems like this is just a matter of 
scripting.  I doubt that the cmsutil program needs to be changed.  Although 
I wish it were otherwise, SMIME QA is not going to be a priority at Sun.

Comment 25

[Julien Pierre]

I think what happens in the case where you attach the encryption cert too is
that the CA cert gets imported permanently as part of the encryption cert
permanent import. Thus, when the signature check happens, it finds the CA cert,
because it is the same for both the encryption and signing certs. I think it
wouldn't work if the two certs (signing and encryption) had different issuers,
because in that case the signing cert's issuer would not be imported permanently.

Comment 26

[Wan-Teh Chang]

So it's the CA cert that needs to be imported.  I thought
it's the leaf signing-only cert that needs to be imported
as temporary.

This explains why I haven't run into this problem.  Our
test CA issues signing-only and encryption-only certs to
each person, but our email application always sends the
encryption-only cert with signed messages, and our
encryption-only cert is issued by the same CA as the
signing-only cert.

I fully understand the bug now.  Thanks for explaining.

Comment 27

[Julien Pierre]

Wan-Teh,

Technically, both the signing cert and the CA cert need to be imported for the
e-mail signature to verify. But the failure that was seen here was resulting in
the issuer cert not being found in CERT_VerifyCert - and this is what the patch
fixed. In order for there to even be a CERT_VerifyCert call, the signing cert
itself had to have been decoded, outside of NSS_CMSSignedData_ImportCerts. I
didn't check where exactly where that was happening .

On a related note, for a long time, Mozilla mail required an encryption cert,
even if one only wanted to sign a message. The requirement was dropped recently
in PSM, so that signed emails could be sent without encryption certs in them. 
Iain apparently took advantage of that feature, and discovered it was broken.

Comment 28

[Wan-Teh Chang]

Julien,

I believe PSM verified a signed message here:
http://lxr.mozilla.org/seamonkey/source/security/manager/ssl/src/nsCMS.cpp#289

289   // We verify the first signer info,  only //
290   if (NSS_CMSSignedData_VerifySignerInfo(sigd, 0, CERT_GetDefaultCertDB(),
certUsageEmailSigner) != SECSuccess) {

If you trace the code, you will see that sigd->signerInfos[i]->cert
is a reference to the signing cert.  That cert reference ensures
that the signing cert is in at least the temp cert database.

Iain, could you explain how to configure Mozilla mail to not require
an encryption cert?

Comment 29

[Julien Pierre]

Wan-Teh,

I'm not sure how to do the configuration, but I can tell you that the test case
I have received from Iain was signed with Thunderbird 1.0.2 . Maybe Mozilla Mail
didn't get the fix to allow signing only .

Comment 30

[Iain MacDonnell]

(In reply to comment #28)

> Iain, could you explain how to configure Mozilla mail to not require
> an encryption cert?

I've been using thunderbird, but I believe it's the same in mozilla 1.7. When
you select a signing certificate for an email account, the preferences dialog
asks you if you want to use the same certificate as your encryption one. I
always hit cancel at that point, which results in the encryption cert being
left blank.

Comment 31

[Julien Pierre]

Comment on attachment 187446 [details] [diff] [review]
Patch as checked in

Requesting approval to have this fix for a serious S/MIME regression integrated
.

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

Elsewhere, it has been suggested that, ever since libSMIME was changed to 
only import encryption certs, that it was not permanently importing 
signing-only CA certs that are part of the encryption cert chain, 
leading to inability to verify encryption certs, when attempting to send
encrypted email at a later time.

If true, that is also a part of this same regression, and IMO should be
fixed at the same time.  Are we sure that problem is actually fixed here?

Comment 33

[Iain MacDonnell]

(In reply to comment #32)
> Elsewhere, it has been suggested that, ever since libSMIME was changed to 
> only import encryption certs, that it was not permanently importing 
> signing-only CA certs that are part of the encryption cert chain, 
> leading to inability to verify encryption certs, when attempting to send
> encrypted email at a later time.

Is there any such thing as a "signing-only CA cert"? Trust attributes are
usually assigned to CA certs by the user on (manual) import, but I don't
think they are properties of the CA cert itself?

Julien and I did test messages signed with my signing cert AND also
carrying my encryption cert. In that case, the encryption cert and the CA
chain do get stored (and consequently the signature verifies successfully).

Comment 34

[Wan-Teh Chang]

So now we are relying on a new (undocumented) side effect
of NSS_CMSSignedData_ImportCerts, that the function will
decode sigd->rawCerts and save references to the temp certs.
This fixes the bug for apps such as Mozilla and Thunderbird
that call NSS_CMSSignedData_ImportCerts.

However, it seems reasonable for someone to just verify a
signed message without calling NSS_CMSSignedData_ImportCerts.
So it seems that it is best for the decoding of sigd->rawCerts
to occur in NSS_CMSSignedData_VerifySignerInfo, if
sigd->rawCerts haven't been decoded yet.

Comment 35

[Julien Pierre]

Wan-Teh,

I think NSS_CMSSignedData_ImportCerts is the right function for the applications
to use . Even our examples (cmsutil) rely on it to import the certs for
signature verification. It takes a keepCerts and a usage argument, so that the
application can specify if it wants to import the certs permanently or
temporarily. What would be the purpose of having a keepCerts argument at all if
the function was always a no-op when the value is PR_FALSE ? This is the
behavior my patch fixed .
You are right that it would be cleaner to just be able to call
NSS_CMSSignedData_VerifySignerInfo without calling
NSS_CMSSignedData_ImportCerts. However, I believe that would constitute a change
of API, because the requirement to call NSS_CMSSignedData_ImportCerts always
existed. No signature could ever have been verified without that call in the
past regardless of the type of cert; nor could any encrypted e-mails have been
sent to the recipient because Mozilla verifies the recipient's cert chain before
allowing an e-mail to be sent.

Nelson,

If you look at the body of code in NSS_CMSSignedData_ImportCerts, it filters the
certs and tries to permanently import (if keepCerts is set) only the ones that
validate with the usage passed . But then, it also reconstructs the cert chain
from those certs, and imports all the certs in the chain permanently. So,
effectively, in most cases, all the certs get imported permanently, unless there
were some extraneous certs unrelated to the encryption or signing cert, or some
expired certs in the signature where another newer one exists in the user's DB.
So, I don't think the problem you mention actually ever existed. If the
encryption cert was imported, then its whole chain was imported as well.

The decision to import signing vs encryption certs isn't made by libsmime. It is
made by PSM, in the arguments it chooses to pass to
NSS_CMSSignedData_ImportCerts . It used to import certs for both encryption and
signing permanently, by making multiple calls to NSS_CMSSignedData_ImportCerts,
with the different usages, but both with keepCerts = PR_TRUE . Now, it still
makes two ImportCerts calls, but keepCerts is set only for the encryption usage,
not the signing usage. My patch fixes the problem becauses it causes all the
certs to always be imported temporarily, regardless of their usage, which I
believe was the intent of setting keepCerts to PR_FALSE. The logic to decide
which certs to make permanent is unchanged.

Comment 36

[Julien Pierre]

*** Bug 54014 has been marked as a duplicate of this bug. ***

Comment 37

[Robert Relyea]

Before we settle on this patch, we should discuss one issue:
   poison cert corruption.

The problem is if we just take the list of certs as is and import them into
either the temp or the perm db long term, we can open ourselves up to a
difficult to detect denial of service attack. An attacker creates a certificate
with the same subject and key id of an existing CA we trust or a popular
intermediate CA. He includes that CA in his email chain. If that cert has
'appropriate' expiration times and cert extensions, NSS could choose this cert
over the trusted CA cert, and any validation against that chain could fail.

Since the patch involves temp certs, the effect is much less than perm certs
(older versions of NSS blindly imported all certs from the email message, rather
than validating each imported cert in turn because of this potential attack).
Also this is a Denial of Service attack, not a spoofing attack, and it's effect
would only be while the certs are held in the temp DB (that is while the email
message was open). I don't see it as an issue for thunderbird (more of an NSS
correctness issue).

Anyway the solutions could be as simple as leaving the code 'as is' to using the
equivalent logic we have for permanently importing the encryption cert and its
chain for tempararily importing the signing cert and it's chain.

bob.

Comment 38

[Julien Pierre]

Bob,

I thought of this poison cert problem when I wrote the patch. However, my take
on is that the application should have the whole cert chain available when
verifying the message signature - in order to be able to display it, for example
. Once the message goes away, the certs can no longer poison anything.

Also, you need to import the certs anyway, even for a brief period of time, to
find out they don't validate - and during that period of time, they are still
poisonous . This isn't much of a problem in a client app, but it is in a server.

I believe the same problem exists with SSL as well. We can receive a chain from
a server, import the certs temporarily, and this may cause long-term problems. I
have filed bugs on that issue before. The only true fix would be to have real
compartmentalized contexts in which we could import the certs safely, without
affecting anything else going in the NSS application, in particular the other
threads. I think Stan was supposed to solve this with the cryto contexts ... :-(

Comment 39

[Robert Relyea]

The thing I worry about is how long we keep the certs.

The question is do we need to keep the certs if the verification fails. The
answer is 'it depends'. If the verify certificate fails on the signing cert in
the import, it won't matter to the next stage the the cert chain is imported or
not (the signature will fail in either case). However, if you have code that
displays the chain to the user so the user can see why the signature failed, you
would want to keep the chain around even if verification fails.

I believe, in the SSL case, the chain is kept around as long as the page is
displayed (in Firefox anyway. PSM extracts the chain before SSL frees the cert).
Note that spurious poison certs won't live longer than the SSL verification in
SSL, though poison certs in the actual chain my live through the life of the page.

The question is how long does the NSSCMSSignedData structure that we are now
storing the temp certs in stay around. If it's only over the decode, the current
code is absolutely no problem. If it's over the life of email, then it would be
better to extract only the signing cert chain. That way we don't have a problem
with spurious poison certs, only those poison certs that are part of the signing
chain.

Anyway I suspect that we have the former case (NSSCMSSignedData data only lives
as long as the life of the decode), I just want to verify that.

bob

Comment 40

[Julien Pierre]

Bob,

NSS_CMSSignedData_Destroy is called as part of NSS_CMSMessage_Destroy, which is
called by PSM immediately after the cert verification happens.

I found that PSM makes a copy of the cert chain with CERT_DupCertificate before
it calls NSS_CMSMessage_Destroy. This is what allows you to click on the
signature in the UI in Mozilla after the verification has been done and get the
whole cert chain, after the NSS_CMSMessage object has been destroyed.

I couldn't see exactly what PSM was doing since I had an optimized build of
mozilla, I didn't build Mozilla myself. Only my NSS bits were debug.

Comment 41

[Robert Relyea]

OK, I don't think this patch is a risk for any NSS client then. (including mozilla).

bob

Comment 42

[Julien Pierre]

BTW, I believe with libpkix, the whole concept of "poison certs" will no longer
exist, because the algorithm backtracks if it finds a cert chain it can't
validate, and finds another possible issuer cert. So, adding bogus untrusted
certs should not have a negative effect functionally - it will only slightly
increase validation time.

Comment 43

[Asa Dotzler [:asa]]

Comment on attachment 187446 [details] [diff] [review]
Patch as checked in

1.0.5 and 1.7.9 have already shipped; unsetting approval requests.