Closed Bug 175167 Opened 22 years ago Closed 21 years ago

SEC_QuickDERDecodeItem should free memory upon failure

Categories

(NSS :: Libraries, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: julien.pierre, Assigned: wtc)

Details

Attachments

(1 file, 2 obsolete files)

When decoding fails, this function may have allocated memory in the arena. The
memory is never leaked since eventually the arena will be freed.
However, it is still allocated until that happens.

I have noticed that we have a PORT_ArenaMark and PORT_ArenaRelease which can be
used by this function to immediately release the memory that was allocated
within it.
Comment on attachment 103280 [details] [diff] [review]
use PORT_ArenaMark and PORT_ArenaRelease to free memory as needed

>+    if (SECSuccess != rv)
>+    {
>+        PORT_ArenaRelease(arena, savpos);
>     }

Just a nit: one reason for failure is that arena,
templateEntry, or src is NULL.	In that case,
savpos is NULL (the initial value).  Passing
a NULL savpos to PORT_ArenaRelease causes it to
malfunction.  Passing a NULL arena to PORT_ArenaRelease
causes it to dereference a NULL pointer.

These (unlikely) problems can be avoided if you
return from the function with SECFailure right
away on bad input arguments.  By the way, if src
is NULL, you still dereference it in "newsrc = *src;".
Comment on attachment 103280 [details] [diff] [review]
use PORT_ArenaMark and PORT_ArenaRelease to free memory as needed

>+    if (SECSuccess != rv)
>+    {
>+        PORT_ArenaRelease(arena, savpos);
>     }

Please ignore my previous comments on savpos being
NULL because by the time we get here, the line
    savpos = PORT_ArenaMark(arena);
has been executed.

In any case, this patch (or rather the
SEC_QuickDERDecodeItem function) needs work to
be bullet-proof against NULL input arguments.
Attachment #103280 - Flags: needs-work+
Comment on attachment 103369 [details] [diff] [review]
patch to make SEC_QuickDERDecodeItem bullet-proof against NULL input arguments

r=wtc.

I would return with SECFailure right away on
NULL input arguments to avoid the nested if
statements.  But I know this is the signature
of your code :-)
Attachment #103369 - Flags: review+
I checked in the fix to the NSS tip but forgot to mark this resolved.
Status: NEW → RESOLVED
Closed: 22 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → 3.7
I'm reopening this bug.
The approach chosen uses PORT_ArenaMark and PORT_ArenaRelease / PORT_ArenaUnmark
. This method is is fact not thread-safe in many cases. It wil only work safely
if the arena is known to only one thread. Therefore, we cannot use it in
something as generic as the DER decoder.
I'm going to attach a patch to undo the changes. Once reviewed, the bug should
be either LATER or WONTFIX.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #103280 - Attachment is obsolete: true
Attachment #103369 - Attachment is obsolete: true
Attachment #116378 - Flags: superreview?(relyea)
Attachment #116378 - Flags: review?(wtc)
Comment on attachment 116378 [details] [diff] [review]
Don't use mark & release in QuickDER

r=wtc.

The change to secdig.c is unrelated but is correct.
Attachment #116378 - Flags: review?(wtc) → review+
The change to secdig made it inadvertedly because I did a cvs diff in the
lib/util directory. I'll check it separately in its appropriate bug.
Resolving this one as WONTFIX .

Checking in quickder.c;
/cvsroot/mozilla/security/nss/lib/util/quickder.c,v  <--  quickder.c
new revision: 1.18; previous revision: 1.17
done
Status: REOPENED → RESOLVED
Closed: 22 years ago21 years ago
Resolution: --- → WONTFIX
Attachment #116378 - Flags: superreview?(rrelyea0264) → superreview+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: