Closed Bug 1898074 Opened 2 months ago Closed 2 months ago

Memory leak in create_objects_from_handles()

Categories

(NSS :: Libraries, defect, P3)

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mozillabugs, Assigned: jschanck)

Details

Attachments

(1 file)

create_objects_from_handles() (security/nss/lib/dev/devtoken.c) invokes undefined behavior if the allocation of objects[0] (line 216, below) fails, and leaks memory if the allocation of objects[i] fails, where i >= 1.

The first issue is that if the allocation of objects[0] fails, the test on line 217 causes line 218 to be executed, which underflows the int i, which is undefined behavior according to C++23 (N4950) s.6.8.2 ("Overflow for signed arithmetic yields undefined behavior").

The second issue is that if the allocation of objects[i] fails, where i >= 1, the for loop on lines 218-20 doesn't release objects[0] because the condition on line 218 is > 0.

Code from FIREFOX_126_0_RELEASE:

204: static nssCryptokiObject **
205: create_objects_from_handles(
206:     NSSToken *tok,
207:     nssSession *session,
208:     CK_OBJECT_HANDLE *handles,
209:     PRUint32 numH)
210: {
211:     nssCryptokiObject **objects;
212:     objects = nss_ZNEWARRAY(NULL, nssCryptokiObject *, numH + 1);
213:     if (objects) {
214:         PRInt32 i;
215:         for (i = 0; i < (PRInt32)numH; i++) {
216:             objects[i] = nssCryptokiObject_Create(tok, session, handles[i]);
217:             if (!objects[i]) {
218:                 for (--i; i > 0; --i) {
219:                     nssCryptokiObject_Destroy(objects[i]);
220:                 }
221:                 nss_ZFreeIf(objects);
222:                 objects = NULL;
223:                 break;
224:             }
225:         }
226:     }
227:     return objects;
228: }

For the loop, I suggest:

    for (; i > 0; --i) {
        nssCryptokiObject_Destroy(objects[i-1]);
    }
Flags: sec-bounty?

The leak is an issue. I wouldn't consider it a security bug.

I don't see the undefined behavior. Decrementing a signed int equal to 0 is not an underflow (an underflow would be PRInt32 i = INT_MIN; --i;). Am I missing something?

Ugh. You're correct. This is just a leak. Need coffee. Shall I unhide the bug?

Flags: sec-bounty?
Assignee: nobody → jschanck
Group: crypto-core-security
Severity: -- → S4
Status: NEW → ASSIGNED
Priority: -- → P3
Summary: Undefined behavior and leak in create_objects_from_handles() → Memory leak in create_objects_from_handles()
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: