650272 - seckey functions call memset with sizeof(pointer) instead of sizeof(*pointer)

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[timeless]

http://mxr.mozilla.org/security/source/security/nss/lib/cryptohi/seckey.c?mark=2257,2267-2267#2245

2245 SECKEY_DestroyPrivateKeyInfo(SECKEYPrivateKeyInfo *pvk,

this is correct:
2257             PORT_Memset((char *)pvk, 0, sizeof(*pvk));

this is wrong:
2267             PORT_Memset((char *)pvk, 0, sizeof(pvk));

accounting note: COV5.3/BAD_SIZEOF/OU/19137

Comment 1

[timeless]

2276 SECKEY_DestroyEncryptedPrivateKeyInfo(SECKEYEncryptedPrivateKeyInfo *epki,

looks ok:
2288             PORT_Memset((char *)epki, 0, sizeof(*epki));

looks wrong:
2297             PORT_Memset((char *)epki, 0, sizeof(epki));

accounting note: COV5.3/BAD_SIZEOF/OU/19135

Comment 2

[Ludovic Hirlimann [:Usul]]

Attachment: One liner fixing the reported issue

Patch as described in comment #0 and #1

Comment 3

[Ludovic Hirlimann [:Usul]]

Adding kai so he can at least read the bug - I didn't see him on the seclist group.

Comment 4

[timeless]

comment 0 and comment 1 are distinct instances of this bug

Comment 5

[Ludovic Hirlimann [:Usul]]

Attachment: Complete patch this time - two lines

Taking into account timeless comment. sorry for the spam

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 526575 [details] [diff] [review]
Complete patch this time - two lines

r=nelson

Comment 7

[Ludovic Hirlimann [:Usul]]

(In reply to comment #6)
> Comment on attachment 526575 [details] [diff] [review]
> Complete patch this time - two lines
> 
> r=nelson

Do I need sr ? if not can I just ask for checking needed ?

Comment 8

[Kai Engert (:KaiE:)]

I think Nelson's review is sufficient.
We should land this into both NSS trunk and NSS 3.12 branch.

However:

This bug is currently marked as security-sensitive.
Checking in the patch and referring to this restricted bug will raise attention.

Question:
How should we proceed?
Are you worried this can be exploited?
If not, can we open the bug?

I can check in the patch, once we agree what to do.

Comment 9

[Kai Engert (:KaiE:)]

If I understand correctly, because of the bug, we fail to erase algorithm info and private key attributes, but the private key itself gets successfully erased.

Knowledge about this bug seems to be of little value to a potential attacker, which would have to have access to a victim's memory dump.

I'd conclude this bug can be opened, but I'd like a second opinion. Bob/Brian/Nelson, if you agree, please open this bug.


Some more details, to make it easier for you to decide:


Regarding the first occurrence:

struct SECKEYPrivateKeyInfoStr {
    PLArenaPool *arena;
    SECItem version;
    SECAlgorithmID algorithm;
    SECItem privateKey;
    SECKEYAttribute **attributes;
};
typedef struct SECKEYPrivateKeyInfoStr SECKEYPrivateKeyInfo;

The wrong call will erase the initial pointer, but will not erase the remaininig contents.

However, the private key itself has already been erased by an earlier call.


Regarding the second occurrence:

struct SECKEYEncryptedPrivateKeyInfoStr {
    PLArenaPool *arena;
    SECAlgorithmID algorithm;
    SECItem encryptedData;
};
typedef struct SECKEYEncryptedPrivateKeyInfoStr SECKEYEncryptedPrivateKeyInfo;

The wrong call will erase the initial pointer, but will not erase the remaininig contents.

However, the encrypted data referenced by the struct has already been erased with zero by an earlier call.


Both functions can be found, next to each other:
http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/cryptohi/seckey.c#2206

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I do not think this bug needs to be hidden as its nothing exploitable. These particular memsets are unnecessary and it should just be removed. Leaking information about the key metadata isn't a security problem.

Nelson or Bob, do you agree?

Comment 11

[Robert Relyea]

I agree that this is not security sensitive. I think the corrected memsets should stay simply because it helps review. 

It's clear the sensitive information is in the SECItem, which isn't cleared by the memset anyway (only the pointer in data structure is cleared.

bob

Comment 12

[Kai Engert (:KaiE:)]

trunk:

Checking in seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.61; previous revision: 1.60
done


3.12 branch:

Checking in seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.54.2.3; previous revision: 1.54.2.2
done