103011 - nsNSSComponent::GetPK11String leaks

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm seeing leaks of the PR_Malloc in nsNSSComponent::GetPK11String.  I'm not
sure if it's the caller's fault or that function, but that function could use
some cleanup.  The leak stack is:
PR_Malloc[/builds/trunk/obj/debug/dist/bin/libnspr4.so +0x1742D]
nsNSSComponent::GetPK11String(unsigned short const*,
unsigned)[/builds/trunk/obj/debug/dist/bin/components/libpipnss.so +0x5DEC0]
nsNSSComponent::ConfigureInternalPKCS11Token()[/builds/trunk/obj/debug/dist/bin/components/libpipnss.so
+0x5E1C9]
nsNSSComponent::Init()[/builds/trunk/obj/debug/dist/bin/components/libpipnss.so
+0x5ED1D]
nsSSLIOLayerAddToSocket(char const*, int, char const*, int, PRFileDesc*,
nsISupports**, int)[/builds/trunk/obj/debug/dist/bin/components/libpipnss.so
+0x67B83]
nsGenericFactory::CreateInstance(nsISupports*, nsID const&,
void**)[/builds/trunk/obj/debug/dist/bin/libxpcom.so +0xEA529]
...

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Well, I wish NSS used |const char*| when appropriate.  I did find one bug in NSS
that this code was working around and becoming very complicated, so I was able
to simplify things quite a bit.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: untested patch

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: above, but correct const-ness in NSS (with fan-out)

Comment 4

[Stephane Saux]

cc PSM team.
David, can you review.

Thanks to David Baron

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: previous patch with NS_CONST_CASTs removed

Comment 6

[David P. Drinan]

r=ddrinan.

Comment 7

[Javier Delgadillo]

there are a bunch of changes to NSS in here.  Have those changes been run
by/approved by the NSS team?

Comment 8

[David P. Drinan]

Adding relyea to the cc-list. Bob, please review the NSS changes in this patch.

Thanks.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that attachment 51974 [details] [diff] [review] makes only the minimal changes to NSS (correcting an
obvious mistake to avoid lots of complexity in the PSM code), whereas attachment
52597 [details] [diff] [review] fixes a tiny part of bug 103736 to avoid NS_CONST_CAST all over the PSM
changes.

Comment 10

[Robert Relyea]

The NSS changes (mozilla/security/nss/lib/*) attachment (id=52597)looks fine.
r=relyea

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 52597 [details] [diff] [review]
previous patch with NS_CONST_CASTs removed

 /* The next two strings must be exactly 64 characters long, with the
    first 32 characters meaningful  */
-static char *slotDescription     = 
+static const char *slotDescription     = 
 	"Netscape Internal FIPS-140-1 Cryptographic Services             ";
-static char *privSlotDescription = 
+static const char *privSlotDescription = 
 	"Netscape FIPS-140-1 User Private Key Services                   ";


These should be const char arrays ([] after the declarator name),
not pointers to const chars (* before the declarator).
Fix that and sr=[e-mail].

/be

Comment 12

[Wan-Teh Chang]

Brendan,

slotDescription and privSlotDescription must be
pointers to const chars so that they can point
to other strings.  

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So could someone check the NSS part of the patch in?

Comment 14

[Kai Engert [:KaiE:]]

Regarding comment #11, just an idea.

Instead of:

static const char *slotDescription     = 
 	"Netscape Internal FIPS-140-1 Cryptographic Services             ";

Could we use the following?

static const char slotDescriptionDefault[] = {
 	"Netscape Internal FIPS-140-1 Cryptographic Services             " };

static const char *slotDescription     = slotDescriptionDefault;

Comment 15

[Robert Relyea]

No, because the string can be changes.

It's actually Moot for most of the strings. In 3.4 they only static strings left
are the ManufactureID and the Library description.

The token and slot descriptions are now stored in the PK11Slot structure.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Giving this to default PSM owner because I'm not going to be able to check in
the NSS changes.

Comment 17

[Stephane Saux]

and neither can I
so over to kaie

Comment 18

[Wan-Teh Chang]

I checked in the NSS part of the patch on the
NSS_3_3_BRANCH.  However, I can't move the
NSS_CLIENT_TAG until I have verified the other
changes that have been checked in on the
NSS_3_3_BRANCH.

Bob, could you merge the NSS part of the patch
into the tip of NSS?  The nss/lib/softoken
directory has changed so much that the patch
fails to apply to the tip of NSS.

Comment 19

[Robert Relyea]

Yes, I've made them in my tree now and I'll check them in as soon as I have them
tested.

I think now would also be a good time to stop checking in NSS fixes to the 3.3
branch. NSS 3.4 has been integrated with PSM, so at this point I think the PSM
team should start using 3.4. Once we are happy with this we can create a tag
from the 3.4 branch and land that.

bob

Comment 20

[Wan-Teh Chang]

Assigned the bug back to Kai.

Kai, please work with Bob Relyea and me to get the
NSS changes in NSS_CLIENT_TAG.  Depending on the PSM
2.2 schedule, we can either do this in the NSS 3.3
branch or in NSS 3.4.  It seems that Bob prefers to
do this in NSS 3.4.

Comment 21

[Kai Engert [:KaiE:]]

Wan-Teh, I see that you meanwhile have moved the NSS_CLIENT_TAG, so the current
Mozilla trunk has the NSS changes from the patch.

That means we can now check in the PSM portion of the patch, I will do it.

Comment 22

[Wan-Teh Chang]

Kai,

Yes, the fix is in NSS_CLIENT_TAG.  I forgot to notify you.
Sorry about that.

Comment 23

[Kai Engert [:KaiE:]]

checked in => fixed

Comment 24

[John Unruh]

Verified per kaie's comment.

Comment 25

[Kai Engert [:KaiE:]]

Triggered by researching a different bug, I by chance looked at this patch again.

Once thing I noted, but which has not been discussed here: Before this patch,
the strings given to PK11_ConfigurePKCS11 were truncated to a maximum length,
i.e. SHORT_PK11_STRING.

But now they can be of any length, whatever the locale string defines.

Is that a problem?

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

pk11_SetStringName just ignores the excess, right?

Comment 27

[Kai Engert [:KaiE:]]

> pk11_SetStringName just ignores the excess, right?

I realize that in the past the implementation of PK11_ConfigurePKCS11 called
pk11_SetStringName with the available storage space, so you are right, the patch
was correct.

Now with NSS 3.4, PK11_ConfigurePKCS11 builds a dynamic string. I suppose NSS
has no longer that size restriction.

Comment 28

[Robert Relyea]

No, there is still a size restriction. It's based on the PKCS #11 interface. The
truncation would happen inside the softoken now.

bob

Comment 29

[Kai Engert [:KaiE:]]

Just for tracking purposes, the truncation will be cared for in bug 125728.