347106 - Need function to clear out PLArenas in PLArenaPools for security

Status: RESOLVED FIXED

(NSPR :: NSPR, enhancement, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

NSPR's libplds needs a function that will clear out all the arenas in a 
PLArenaPool.  NSS would use this if it existed.  See bug 53427.
Patch forthcoming.

I'm guessing here about the target milestone.  Feel free to correct.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

12345678901234567890123456789012345678901234567890123456789012345678901234567890
This patch builds and, when tested together with the patch for bug 53427,
all.sh passes.  

There is no test program for this new function yet.
I added a new section to the .def file for this new function.
I guessed at the version number to put into the new section, and 
hope to receive advice on that number if my guess was not right.

Comment 2

[Wan-Teh Chang]

Comment on attachment 231891 [details] [diff] [review]
patch v1

Nelson,

This patch looks good.  Thanks for writing it.  I'd like to
suggest some changes.

1. Although it doesn't really matter, it is strange that an
argument named "zero" to PORT_FreeArena (the intended user of
the PL_ClearArenaPool function) causes the arena to be written
with the 0xDA pattern rather than being zeroed.

The 0xDA pattern helps debugging.  So perhaps we should add
a "pattern" argument to PL_ClearArenaPool:
  PR_IMPLEMENT(void) PL_ClearArenaPool(PLArenaPool *pool, int pattern);

2. The static function FreeArenaList has the following code:

#ifdef DEBUG
    do {
        PR_ASSERT(a->base <= a->avail && a->avail <= a->limit);
        a->avail = a->base;
        PL_CLEAR_UNUSED(a);
    } while ((a = a->next) != 0);
    a = *ap;
#endif

The do-while loop differs from the PL_ClearArenaPool function only
in the initial value of 'a'.  So perhaps you can add a new static
function for the common code.

static void ClearArenaList(PLArenaPool *pool, PLArena *head, int pattern)
{
    PLArena *a;

    for (a = head->next; a; a = a->next) {
        PR_ASSERT(a->base <= a->avail && a->avail <= a->limit);
        a->avail = a->base;
        PL_CLEAR_UNUSED2(a, pattern);
    }
}

Note the new macro PL_CLEAR_UNUSED2 if we want to support the "pattern"
argument.

#define PL_CLEAR_UNUSED2(a, pattern) (PR_ASSERT((a)->avail <= (a)->limit), \
                           memset((void*)(a)->avail, pattern, \
                           (a)->limit - (a)->avail))

#define PL_CLEAR_UNUSED(a) PL_CLEAR_UNUSED2(a, PL_FREE_PATTERN)

3. In lib/ds/plds.def, add the new function to the NSPR_4.6.3 section.
If you have time, you may want to update lib/ds/plds_symvec.opt, which
is only used for OpenVMS. (Not sure if Mozilla still supports OpenVMS.)

Comment 3

[Wan-Teh Chang]

Added Brendan, the original author of NSPR arena, to the cc list.
Brendan, we are going to add a new function PL_ClearArenaPool
that can be used to clear the memory in the arena pool before
we free the arena pool.

Comment 4

[Brendan Eich [:brendan]]

Sounds like a fine addition.

jsarena.c forked long ago (before NSPR 2, really) so it need not track this change, and won't without more motivation.

Cc'ing graydon, who is instrumenting SpiderMonkey with valgrind and annotating its arena-like data structures so valgrind can see individual allocations from pools as distinct objects.  I'm hoping SpiderMonkey has no malign (or benign) UMRs, but early word was that it seems to, given the annotations graydon has made.  Could be that plarena.c has bugs too, if jsarena.c or higher layers built on it have bugs.

/be

Comment 5

[Wan-Teh Chang]

Brendan,

The motivation for the NSS team to request this new NSPR function
is that NSS zeroes arena pools that contain sensitive information
such as cryptographic keys.  Although the definitions of the PLArena
and PLArenaPool structures are in the public header plarena.h, NSS
would like to treat PLArena and PLArenaPool as opaque types.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2 - with WTC's suggestions

Comment 7

[Wan-Teh Chang]

Comment on attachment 232309 [details] [diff] [review]
patch v2 - with WTC's suggestions

r=wtc. Please make the following changes.

1. Declare all the 'pattern' arguments with the type 'int', because
'pattern' is eventually passed as the 'int c' argument to memset.

2. In nsprpub/lib/ds/plarena.c, function FreeArenaList, we have:

> #ifdef DEBUG
>-    do {
>-        PR_ASSERT(a->base <= a->avail && a->avail <= a->limit);
>-        a->avail = a->base;
>-        PL_CLEAR_UNUSED(a);
>-    } while ((a = a->next) != 0);
>-    a = *ap;
>+    ClearArenaList(a, PL_FREE_PATTERN);

The "a = *ap;" statement after the do-while loop needs to be restored.

3. In nsprpub/lib/ds/plarenas.h, we have:

>+/*
>+** memset contents of all arenas in pool to pattern
>+*/
>+PR_EXTERN(void) PL_ClearArenaPool(PLArenaPool *pool, PRInt32 pattern);

We should also document that this function does this to all
arenas in pool:
    a->avail = a->base;

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #7)
> The "a = *ap;" statement after the do-while loop needs to be restored.

Why?  Unlike the loop that it replaced, the new call to ClearArenaList does 
not alter the caller's value of a.  The value of *ap is not altered, either.
So a == *ap before and after this call.  No point is setting a to the 
value it already contains.  Do you disagree?

Comment 9

[Wan-Teh Chang]

Nelson, you're right.  I missed that.  Please disregard that
suggested change (suggestion #2 in comment 7).

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #5)  Wan-Teh, you wrote:
> Although the definitions of the PLArena
> and PLArenaPool structures are in the public header plarena.h, NSS
> would like to treat PLArena and PLArenaPool as opaque types.

Wan-Teh, IIRC, when this issue first arose (bug 53427) 6 years ago, 
Larry opined that these structures were private NSPR structures.  
That's the only reason that I'm trying to implement this function in NSPR,
rather than in NSS.  I'm trying to honor the claim that these are private.
I think it would be easier for me to just implement this code in NSS.
That is, doing the work in NSS would obviate anotherr NSPR release for FIPS.

Now, you appear to be saying the relevant structures are public.  
If you're OK with NSS directly manipulating the PLArena and PLArenaPool 
structures, then I'll attach a patch to bug 53427 to do just that,
at least for the NSS 3.11 branch.  

So, please make an "official" declaration here about whether these structs
are public or private.  Thanks.

Comment 11

[Wan-Teh Chang]

We want NSPR users to use those structures as if they
were opaque types, but because of our strict backward
compatiblity requirements, we will not change those
structures.  So it is actually safe for NSPR users to
use the fields of those structures.

Perhaps for the NSS_3_11_BRANCH you can implement a fix
entirely in NSS, and for the NSS trunk you can call the
new NSPR function.  This will avoid the work to check in
the NSPR patch on the NSPR_4_6_BRANCH and MOZILLA_1_8_BRANCH.

We will release NSPR 4.6.3 for Firefox 2.0 because it
needs the fixes for bug 322956 and bug 340956.

Comment 12

[Wan-Teh Chang]

The NSS bug 53427 has been fixed by manipulating the PLArenaPool
and PLArena structures directly.  We should move that code to NSPR.
It should become two new NSPR functions.

One is the PL_ClearArenaPool function that Nelson already implemented.
It zeroizes the arena memory from a->base to a->avail.

The other is the port_ArenaZeroAfterMark function in
mozilla/security/nss/lib/util/secport.c.  It zeroizes the arena memory
from 'mark' to a->avail, before the PL_ARENA_RELEASE macro moves the
a->avail pointer back to 'mark'.  We may want to combine it with a new
variant of the PL_ARENA_RELEASE macro.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Checking in nsprpub/lib/ds/plarena.c;        new revision: 3.17; previous: 3.16
Checking in nsprpub/lib/ds/plarena.h;        new revision: 3.7;  previous: 3.6
Checking in nsprpub/lib/ds/plarenas.h;       new revision: 3.7;  previous: 3.6
Checking in nsprpub/lib/ds/plds.def;         new revision: 1.6;  previous: 1.5
Checking in security/nss/lib/util/secport.c; new revision: 1.29; previous: 1.28

Comment 14

[Wan-Teh Chang]

This comment merely states a fact important to NSPR
and NSS package maintainers.

The patch in this bug makes NSS 3.12.7 require NSPR
4.8.6 because of the new PL_ClearArenaPool call in
mozilla/security/nss/lib/util/secport.c.  Note: NSPR
4.8.5 was canceled, so the next release after NSPR
4.8.4 is NSPR 4.8.6.