Leak in PK11_UnwrapPrivKey
Categories
(NSS :: Libraries, defect, P2)
Tracking
(Not tracked)
People
(Reporter: alexander.m.scheel, Assigned: kjacobs)
Details
Attachments
(1 file)
User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:83.0) Gecko/20100101 Firefox/83.0
Steps to reproduce:
When PK11_UnwrapPrivKey(...) is called with param=NULL, param_free is set but never freed.
See PK11_UnwrapPrivKey in pk11obj.c line 1256:
if (!param)
param = param_free = PK11_ParamFromIV(wrapType, NULL);
(note that param_free isn't freed after the PK11_UnwrapPrivKey call).
Actual results:
param wasn't freed.
Expected results:
param should've been freed.
(I likely won't file a Phab for this).
Reporter | ||
Comment 1•4 years ago
|
||
See also https://github.com/dogtagpki/jss/issues/667 for a valgrind trace.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 2•4 years ago
|
||
Reporter | ||
Comment 3•4 years ago
|
||
I tried adding a comment to the Phabricator, but it failed with:
You do not have permission to edit this object.
Users with the "Can Edit" capability:
Members of the project "Restricted Project" can take this action.
The owner of a revision can always view and edit it.
My comment was attached to new line 1331 (return NULL
inside the branch where we attempt to use the internal token):
@cipherboy - unsubmitted:
In the unlikely event new line 1309 triggers but the conditional on new line 1314 (i.e.,
!int_slot
orslot == int_slot
), don't we still leakparam_free
because wereturn NULL
here rather thangoto loser
?(e.g., wouldn't this happen when say, we attempt to use a random mechanism that NSS doesn't support while specifying the internal slot originally, like say, the made-up mech
CKM_DUAL_AES_CCM_OVER_ECB_WRAP
? The earlierC_UnwrapKey
will fail, we continue into this block, andslot == int_slot
).
I also wrote in the action box below:
Not sure if this analysis is right, but I think it warrants checking what else we need to free (from
loser
label) in that condition as well.
Assignee | ||
Comment 4•4 years ago
|
||
Thanks for looking this over. See bug 1491405 for a similar Phabricator issue. I'm not sure if simply commenting triggers this, or if approving/requesting changes is what requires permissions...
(In reply to Alexander Scheel from comment #3)
I tried adding a comment to the Phabricator, but it failed with:
You do not have permission to edit this object. Users with the "Can Edit" capability: Members of the project "Restricted Project" can take this action. The owner of a revision can always view and edit it.
My comment was attached to new line 1331 (
return NULL
inside the branch where we attempt to use the internal token):
@cipherboy - unsubmitted:
In the unlikely event new line 1309 triggers but the conditional on new line 1314 (i.e.,
!int_slot
orslot == int_slot
), don't we still leakparam_free
because wereturn NULL
here rather thangoto loser
?(e.g., wouldn't this happen when say, we attempt to use a random mechanism that NSS doesn't support while specifying the internal slot originally, like say, the made-up mech
CKM_DUAL_AES_CCM_OVER_ECB_WRAP
? The earlierC_UnwrapKey
will fail, we continue into this block, andslot == int_slot
).
Ah, good catch. Will fix.
I also wrote in the action box below:
Not sure if this analysis is right, but I think it warrants checking what else we need to free (from
loser
label) in that condition as well.
I don't see anything else that leaks, do you?
Reporter | ||
Comment 5•4 years ago
|
||
Ah I see:
new_key
is freed just afterC_UnwrapKey
if we take that branch otherwise it hasn't been allocated at all.ck_id
has been freed between the the two conditionals.
No nothing else leaks, it just requires more careful attention than I gave on the first pass :-)
Assignee | ||
Comment 6•4 years ago
|
||
https://hg.mozilla.org/projects/nss/rev/f84fb229842ab7bba938f943eed9d93c5b1c9189
Thanks for the report.
Description
•