117148 - S/MIME tool fails to find certificate while signing.

Status: VERIFIED FIXED

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

Description

[Daniel Lanovaz]

I'm using the NSS subsystem and parts of Mozilla 0.9.6
(soon 0.9.7) as a stand-alone S/MIME engine.

When trying to create digitally signed messages I was
seeing failures to find a certificate given a nickname.
I traced it down to mozilla/security/manager/ssl/src/nsNSSCertificate.cpp,
in several member functions such as nsNSSCertificateDB::GetCertByNickname.
There is a style of using the NS_ConvertUCS2toUTF8 in a expression
of the form:

    char *asciiname = NS_CONST_CAST(char*, NS_ConvertUCS2toUTF8(aNickname).get
());

The problem seems to stem from the fact that the class
desctructor for NS_ConvertUCS2toUTF8 is executed after
the statement completes.

This means the char* pointer in asciiname is invalid
just after it's initialized.

I changed the lines to read:

    NS_ConvertUCS2toUTF8 aUtf8Nickname(aNickname);
    char *asciiname = NSC_ONST_CAST(char*, aUtf8Nickname.get());

So the NS_ConvertUCS2toUTF8 destructor doesn't get called
until after usage of asciiname for certificate lookups.

There are other locations in the file that have questionable
usage of NS_ConvertUCS2toUTF8 in printf() statements.

I've attached a proposed patch.

I haven't looked at the supposed lifetime of the char*
autos (asciiname) to see if there are other lurking problems.

When I downloaded the latest binary build of Mozilla
0.9.7 I seemed to be able to reproduce the problem.

1. Ensure there is a signing certificate in the
key database.
2. Ensure that the Mail & Newsgroups Account Settings
has a personal certificate selected for signing.
3. Create a fresh e-mail message and mark it for
signing.
4. Send the message. I receive a dialog, "Sending
of message failed. You requested to digitally sign
this message, but the application failed to find the
signing certificate you specified in you Mail/News
account preferences...."

This does seem like the same problem, although I haven't
yet run the distribution in the debugger.

Cheers,
Daniel

Comment 1

[Daniel Lanovaz]

Attachment: Proposed patch to nsNSSCertificate.cpp

The proposed patch to nsNSSCertificate.cpp.

Comment 2

[Keyser Sose]

Thanks for the patch you might want to contact the module owner for a r=/sr.
Marking new and adding keywords.

Comment 3

[Stephane Saux]

cc kai

Comment 4

[Kai Engert [:KaiE:]]

Again thanks for the patch, I confirm that this is a serious bug.

I will look over all of the security code and create a patch that changes all
locations, to make sure nobody can find a bad example to copy and paste from :)

(Just a nit-pick: In the future when creating patches, please use "diff -u" to
create the patch, and use the orig file as the first argument to diff.)

Comment 5

[Daniel Lanovaz]

Hand slapped, and felt. Next time I'll use "diff -u"
and the correct order.

Cheers,
Daniel

Comment 6

[Kai Engert [:KaiE:]]

Attachment: Extended patch

I at least found one additional place where the same error is being made.
In addition, I changed a couple of more locations, where intermediate typecasts
are used.
I did not change those locations, where the temporary string is solely used as
an inline function parameter.

Comment 7

[Kai Engert [:KaiE:]]

Javi, can you please review?

Comment 8

[Javier Delgadillo]

Comment on attachment 64880 [details] [diff] [review]
Extended patch

r=javi

Comment 9

[Alec Flett]

Comment on attachment 64880 [details] [diff] [review]
Extended patch

>Index: mozilla/security/manager/pki/src/nsASN1Outliner.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/security/manager/pki/src/nsASN1Outliner.cpp,v
>retrieving revision 1.5
>diff -u -d -r1.5 nsASN1Outliner.cpp
>--- mozilla/security/manager/pki/src/nsASN1Outliner.cpp	29 Dec 2001 22:05:13 -0000	1.5
>+++ mozilla/security/manager/pki/src/nsASN1Outliner.cpp	14 Jan 2002 21:44:06 -0000
>@@ -403,7 +403,8 @@
> {
>   nsCOMPtr<nsIASN1Object> object;
>   _retval.SetCapacity(0);
>-  char *col = NS_CONST_CAST(char *, NS_ConvertUCS2toUTF8(colID).get());
>+  NS_ConvertUCS2toUTF8 aUtf8ColID(colID);
>+  char *col = NS_CONST_CAST(char *, aUtf8ColID.get());

Ack! why NS_CONST_CAST? col should be a const char*
If you're modifying the string, you should make a 
copy of it with ToNewUTF8String()


> {
>   nsresult rv;
>-  char *col = NS_CONST_CAST(char *, NS_ConvertUCS2toUTF8(colID).get());
>+  NS_ConvertUCS2toUTF8 aUtf8ColID(colID);
>+  char *col = NS_CONST_CAST(char *, aUtf8ColID.get());

same here

\>   char *asciiname = NULL;
>-  asciiname = NS_CONST_CAST(char*, NS_ConvertUCS2toUTF8(nickname).get());
>+  NS_ConvertUCS2toUTF8 aUtf8Nickname(nickname);
>+  asciiname = NS_CONST_CAST(char*, aUtf8Nickname.get());

and here

>   char *asciiname = NULL;
>-  asciiname = NS_CONST_CAST(char*, NS_ConvertUCS2toUTF8(aNickname).get());
>+  NS_ConvertUCS2toUTF8 aUtf8Nickname(aNickname);
>+  asciiname = NS_CONST_CAST(char*, aUtf8Nickname.get());

and here

> 
>   *_retval = 0;
> 
>@@ -4335,7 +4337,8 @@
>   nsCOMPtr<nsIInterfaceRequestor> ctx = new PipUIContext();
>   nsNSSCertificate *nssCert = nsnull;
>   char *asciiname = NULL;
>-  asciiname = NS_CONST_CAST(char*, NS_ConvertUCS2toUTF8(aNickname).get());
>+  NS_ConvertUCS2toUTF8 aUtf8Nickname(aNickname);
>+  asciiname = NS_CONST_CAST(char*, aUtf8Nickname.get());

and here.

>   PRInt32 prerr;
>+  NS_ConvertUCS2toUTF8 aUtf8Password(password);
>   srv = PK11_CheckUserPassword(mSlot, 
>-                  NS_CONST_CAST(char *, NS_ConvertUCS2toUTF8(password).get()));
>+                  NS_CONST_CAST(char *, aUtf8Password.get()));


Not sure why this is necessary. dos PK11_CheckUserPassword keep a copy of the
const char* 
pointer hanging around? And if so, you should be making a copy of the string
internally

> 
>-    status = PK11_InitPin(mSlot, "", NS_CONST_CAST(char*, NS_ConvertUCS2toUTF8(initialPassword).get()));
>+    NS_ConvertUCS2toUTF8 aUtf8InitialPassword(initialPassword);
>+    status = PK11_InitPin(mSlot, "", NS_CONST_CAST(char*, aUtf8InitialPassword.get()));

same here... plus, you shouldn't be NS_CONST_CAST'ing... if PK11_InitPin takes 
a char*, then it must be modifying the string.. and if that's true, you should
be
making a copy of the string anyway. If it is NOT modifying the string, then it
should be
taking a const char*

>   SECStatus rv;
>+  NS_ConvertUCS2toUTF8 aUtf8OldPassword(oldPassword);
>+  NS_ConvertUCS2toUTF8 aUtf8NewPassword(newPassword);
>   rv = PK11_ChangePW(mSlot, 
>-               NS_CONST_CAST(char *, NS_ConvertUCS2toUTF8(oldPassword).get()), 
>-               NS_CONST_CAST(char *, NS_ConvertUCS2toUTF8(newPassword).get()));
>+               NS_CONST_CAST(char *, aUtf8OldPassword.get()), 
>+               NS_CONST_CAST(char *, aUtf8NewPassword.get()));
>   return (rv == SECSuccess) ? NS_OK : NS_ERROR_FAILURE;

same as above

> }
> 
>@@ -335,7 +339,8 @@
> {
>   nsresult rv = NS_OK;
>   PK11SlotInfo *slot = 0;
>-  slot = PK11_FindSlotByName(NS_CONST_CAST(char*, NS_ConvertUCS2toUTF8(tokenName).get()));
>+  NS_ConvertUCS2toUTF8 aUtf8TokenName(tokenName);
>+  slot = PK11_FindSlotByName(NS_CONST_CAST(char*, aUtf8TokenName.get()));

and again
>                                    nsIPKCS11Module **_retval)
> {
>+  NS_ConvertUCS2toUTF8 aUtf8Name(aName);
>   SECMODModule *mod =
>-    SECMOD_FindModule(NS_CONST_CAST(char *, NS_ConvertUCS2toUTF8(aName).get()));
>+    SECMOD_FindModule(NS_CONST_CAST(char *, aUtf8Name.get()));

and again

> {
>+  NS_ConvertUCS2toUTF8 aUtf8Name(aName);
>   PK11SlotInfo *slotinfo =
>-   PK11_FindSlotByName(NS_CONST_CAST(char*, NS_ConvertUCS2toUTF8(aName).get()));
>+   PK11_FindSlotByName(NS_CONST_CAST(char*, aUtf8Name.get()));

and again

>   for (int i=0; i<numCerts; i++) {
>-    const char *name = NS_ConvertUCS2toUTF8(certNames[i]).get();
>-    strcpy(namecpy, name);
>+    NS_ConvertUCS2toUTF8 aUtf8Name(certNames[i]);
>+    strcpy(namecpy, name.get());

You're just making a copy of the string - you should do this instead:
strcpy(namecpy, NS_ConvertUCS2toUTF8(certNames[i]).get());

Comment 10

[Kai Engert [:KaiE:]]

Alec, I agree with you, but I can't fulfil most of your requests. Can you please
read (the short) bug 103736?

To summarize, NSS uses "char*" when it does not modify parameters. If we wanted
to avoid const casts, we would have to use unnecessary string duplication,
whereever the interface offers the potential for modification.

Comment 11

[Kai Engert [:KaiE:]]

Attachment: Updated patch

> [your statements 1 and 2]

Ok, now using const char*


> [your statements 3, 4 and 5]

At all those locations, we must use const cast, because of NSS interfaces.


> Not sure why this is necessary. dos PK11_CheckUserPassword keep a copy of the

> const char* 
> pointer hanging around? And if so, you should be making a copy of the string
> internally

No, NSS should not keep a copy around.
I just did this to motivate people to use safer code.
As the string object needs to be created anyway,
it doesn't hurt to place it on the stack.

I thought, when other programmers see, that complicated one line expressions 
involving those helper string classes and ".get()" are avoided, 
but instead those temporary string classes are placed on the stack, 
the risk of this' bug problem is minimized.

Of course, it'd be best if every user of that string classes understood the
buffer lifetime issues.

> same here... plus, you shouldn't be NS_CONST_CAST'ing... if PK11_InitPin
takes 
> a char*, then it must be modifying the string.. and if that's true, you
should
> be
> making a copy of the string anyway. If it is NOT modifying the string, then
it
> should be
> taking a const char*

Unfortunately that's wrong. NSS uses char* but is not modifying it.
Const cast is necessary, or we have to create a temporary copy it.


> [your statements 8, 9, 10, 11]

Const cast is necessary.


> You're just making a copy of the string - you should do this instead:
> strcpy(namecpy, NS_ConvertUCS2toUTF8(certNames[i]).get());

Ok, changed.


Alef, do you agree with the patch, or do you think we should make real
temporary C buffer copies, that will only to exist for the lifetime of NSS
calls?

Comment 12

[Alec Flett]

Comment on attachment 64966 [details] [diff] [review]
Updated patch

ok, I'm satisfied then.
boy, the PK11_* API's suck :)

sr=alecf

Comment 13

[Kai Engert [:KaiE:]]

Patch checked in, fixed.

Comment 14

[John Unruh]

Verified per kaie's comment.