Closed Bug 1680400 Opened 4 years ago Closed 4 years ago

Leak in PK11_UnwrapPrivKey

Categories

(NSS :: Libraries, defect, P2)

Tracking

(Not tracked)

RESOLVED FIXED

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).

See also https://github.com/dogtagpki/jss/issues/667 for a valgrind trace.

Assignee: nobody → kjacobs.bugzilla
Severity: -- → S4
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P2

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 or slot == int_slot), don't we still leak param_free because we return NULL here rather than goto 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 earlier C_UnwrapKey will fail, we continue into this block, and slot == 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.

Flags: needinfo?(kjacobs.bugzilla)

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 or slot == int_slot), don't we still leak param_free because we return NULL here rather than goto 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 earlier C_UnwrapKey will fail, we continue into this block, and slot == 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?

Flags: needinfo?(kjacobs.bugzilla)

Ah I see:

  • new_key is freed just after C_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 :-)

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: