Closed Bug 844513 Opened 7 years ago Closed 6 years ago

Add AddressSanitizer (ASan) memory check annotations to PLArena

Categories

(NSPR :: NSPR, enhancement, P2)

enhancement

Tracking

(firefox-esr17 wontfix, b2g18 wontfix)

RESOLVED FIXED
Tracking Status
firefox-esr17 --- wontfix
b2g18 --- wontfix

People

(Reporter: mats, Assigned: wtc)

References

Details

(Keywords: sec-other, sec-want)

Attachments

(5 files, 7 obsolete files)

9.15 KB, patch
mats
: review+
decoder
: superreview+
Details | Diff | Splinter Review
3.05 KB, patch
Details | Diff | Splinter Review
4.69 KB, patch
mats
: review-
Details | Diff | Splinter Review
1.45 KB, patch
mats
: review+
Details | Diff | Splinter Review
3.97 KB, patch
Details | Diff | Splinter Review
Add memory check annotations to PLArena so that Firefox ASan builds
can detect illegal use of the part of the arena that hasn't been
PL_ARENA_ALLOCATE'ed yet.
Attached patch fix (obsolete) — Splinter Review
This change is intended to only affect MOZILLA_CLIENT code.
The idea is to initially mark the malloc'ed memory as NOACCESS and then mark
each chunk that is allocated from the arena as UNDEFINED.
This enables ASan to detect any access to the "avail .. limit" area.
Attachment #717550 - Flags: review?(wtc)
Comment on attachment 717550 [details] [diff] [review]
fix

Review of attachment 717550 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch.

::: nsprpub/lib/ds/plarena.h
@@ +16,5 @@
>  #include "plarenas.h"
> +#ifdef MOZILLA_CLIENT
> +#include "../../../mozilla-config.h" /* for MOZ_ASAN and MOZ_VALGRIND defines */
> +#include "../mozilla/MemoryChecking.h" /* for MOZ_MAKE_MEM_* macros */
> +#endif

We need a better solution. We'll need to duplicate the MOZ_MAKE_MEM_*
macros in NSPR.
Attachment #717550 - Flags: review?(wtc) → review-
(In reply to Wan-Teh Chang from comment #2)
> We need a better solution. We'll need to duplicate the MOZ_MAKE_MEM_*
> macros in NSPR.

It's not clear to me what you're suggesting.  I guess I should copy
mfbt/MemoryChecking.h to nsprpub/lib/ds/MemoryChecking.h and then use:

> +#ifdef MOZILLA_CLIENT
> +#include "../../../mozilla-config.h" /* for MOZ_ASAN and MOZ_VALGRIND defines */
> +#endif
> +#include "MemoryChecking.h" /* for MOZ_MAKE_MEM_* macros */

If not, how should I get MOZ_ASAN and MOZ_VALGRIND that comes from
--enable-valgrind / --enable-address-sanitizer configure arguments?
Flags: needinfo?(wtc)
Attached patch fix, v2 (obsolete) — Splinter Review
No response so I'm assuming that's OK ;-)

https://tbpl.mozilla.org/?tree=Try&rev=479a10ce10c3
Attachment #717550 - Attachment is obsolete: true
Attachment #730749 - Flags: review?(wtc)
Flags: needinfo?(wtc)
Comment on attachment 730749 [details] [diff] [review]
fix, v2

Review of attachment 730749 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch and sorry for the late review.  I reviewed the patch
carefully today.  I suggest some changes.

Why is this bug marked as security-sensitive and labeled with the sec-other
keyword?

::: mfbt/MemoryChecking.h
@@ +1,1 @@
> +/* -*- Mode: C; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

When this file is copied to NSPR, it should be renamed.
I think we can rename it plmemcheck.h or plmarkmem.h.
Update the header include guard macro mozilla_MemoryChecking_h_
to match the new file name.

The macros defined in NSPR's copy of this header should
use the PL_ prefix instead of MOZ_.  So they should be
named PL_MAKE_MEM_NOACCESS, PL_MAKE_MEM_UNDEFINED, etc.

@@ +28,4 @@
>  
>  #if defined(MOZ_ASAN) || defined(MOZ_VALGRIND)
>  #define MOZ_HAVE_MEM_CHECKS 1
>  #endif

Delete these three lines from NSPR's copy of this header.

We can add PL_HAVE_MEM_CHECKS when we need it.

::: nsprpub/lib/ds/plarena.h
@@ +15,5 @@
>  #include "prtypes.h"
>  #include "plarenas.h"
> +#ifdef MOZILLA_CLIENT
> +#include "../mozilla-config.h" /* for MOZ_ASAN and MOZ_VALGRIND defines */
> +#endif

We need a better way to do this.  We should add --enable-address-sanitizer
and --enable-valgrind to nsprpub/configure.in, similar to how they were
added to js/src/configure.in:
http://mxr.mozilla.org/mozilla-central/source/js/src/configure.in

http://hg.mozilla.org/mozilla-central/rev/ad79ffdf94a3

I also wonder if NSPR should test its own versions of the MOZ_ASAN
and MOZ_VALGRIND macros: PL_ASAN and PL_VALGRIND.  It seems convenient
to just test MOZ_ASAN and MOZ_VALGRIND.  Otherwise any Mozilla code that
uses PLArena will need to be compiled with -DPL_ASAN -DPL_VALGRIND in
addition to -DMOZ_ASAN -DMOZ_VALGRIND.

::: security/coreconf/config.mk
@@ +176,5 @@
>  USE_UTIL_DIRECTLY = 1
>  
> +ifdef MOZILLA_CLIENT
> +DEFINES += -DMOZILLA_CLIENT=1
> +endif

We are trying to use "feature" make variables/C-preprocessor
macros.  So this should be:

# Define MOZ_ASAN or MOZ_VALGRIND for PLArena.
ifdef MOZ_ASAN
DEFINES += -DMOZ_ASAN
endif
ifdef MOZ_VALGRIND
DEFINES += -DMOZ_VALGRIND
endif

There is a way to pass MOZ_ASAN=1 or MOZ_VALGRIND=1 to the
NSS makefiles.  It used to be done in security/manager/Makefile.in.
But that makefile is no longer there. I don't know where it resides
now.
Attachment #730749 - Flags: review?(wtc) → review-
> Why is this bug marked as security-sensitive and labeled with the sec-other
> keyword?

It's security-sensitive as a precaution because the patch may reveal exploitable
bugs in shipped products.  We shouldn't make it public until we let our fuzz
team run with the patch for a while to confirm that's not the case.

It's sec-other just to give it a security rating - the bug in itself isn't
a security problem, since it's a tool.
Sorry, but I've lost interest in pursuing this.
I'm working on improving the pres shell arena and I think it's better
use of my time to finish that work (a side-effect is that it will
stop using plarena).
Feel free to use the patch as you see fit.
Assignee: matspal → wtc
Mats: no problem.

I researched AddressSanitizer and valgrind last night and have a better
understanding of this enhancement request.

If you are mainly interested in AddressSanitizer, the required changes
are small because AddressSanitizer builds can be detected using compiler
pre-defined macros (__has_feature(address_sanitizer) or __SANITIZE_ADDRESS__).
I can adapt your patch easily.  I also know how to add support for valgrind
and I can take care of that.
(In reply to Wan-Teh Chang from comment #8)

> If you are mainly interested in AddressSanitizer, the required changes
> are small because AddressSanitizer builds can be detected using compiler
> pre-defined macros (__has_feature(address_sanitizer) or
> __SANITIZE_ADDRESS__).

Yep, that's right. I think we didn't use this because when I implemented MOZ_ASAN, this way of checking for ASan didn't exist yet, or it wasn't documented/well-known yet. Now all the sanitizers have such a way of detecting at compile-time which could make the --enable-address-sanitizer flag we got, obsolete.
This adds ASan memory check annotations.  This does not require any
build system changes, so I suggest we do this first.

This patch is based on Mats's patch.
Attachment #730749 - Attachment is obsolete: true
Attachment #733658 - Flags: review?(matspal)
Attachment #733658 - Flags: review?(choller)
Comment on attachment 733658 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, by Mats Palmgren

In the overview comment in plarena.h:
"... the following three macros are provided:
MOZ_MAKE_MEM_NOACCESS ..."

You probably want to change those three to use PL_

Other than that, I'll leave the actual code review for choller@
Attachment #733658 - Flags: review?(matspal)
And thanks for picking this up Wan-Teh!
ASan support is indeed what I was looking for.
Mats: you're right. The comment should say PL_MAKE_MEM_. Thanks.
Attachment #733658 - Attachment is obsolete: true
Attachment #733658 - Flags: review?(choller)
Attachment #733664 - Flags: review?(choller)
Keywords: sec-want
Comment on attachment 733664 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, v2, by Mats Palmgren

Review of attachment 733664 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, r+ for the ASan stuff. Note that I'm not a peer for NSPR :)

::: lib/ds/plarena.h
@@ +180,5 @@
> +#define PL_CLEAR_ARENA(a) \
> +    PR_BEGIN_MACRO \
> +        PL_MAKE_MEM_UNDEFINED((void*)(a), (a)->limit - (PRUword)(a)); \
> +        memset((void*)(a), PL_FREE_PATTERN, (a)->limit - (PRUword)(a)); \
> +    PR_END_MACRO

Is the MAKE_MEM_UNDEFINED call here because the memory could have been marked NOACCESS before?
Attachment #733664 - Flags: review?(choller) → review+
> Is the MAKE_MEM_UNDEFINED call here because the memory could have been
> marked NOACCESS before?

Correct.  See FreeArenaList and PL_ClearArenaPool for example.
The whole arena is marked NOACCESS when in arena_freelist, other in-use
arenas are partly NOACCESS, so when freeing/clearing those things, which
a client can do at will afaict, we need to temporarily mark the arena
UNDEFINED to allow for the memset.  In the case of PL_ClearArenaPool,
it is then promptly marked NOACCESS again, and in the case of FreeArenaList
the memory is free'd which also marks it NOACCESS of course.
Gary, Jesse, can you take this patch for a spin in the fuzzers before it lands?
To find any mistakes in the code, and to avoid zero-daying ourselves.  Thanks.
decoder,mats: thank you for the review.

I will wait for the fuzzing report from Gary or Jesse before checking
this in.
I haven't been working much on Firefox Asan builds recently, so I'll have to differ here.
Comments in the patch mention Valgrind, but the code only seems to support ASan?
Jesse: yes, I will add Valgrind support later.
For Valgrind support, just extend the macros as in mfbt/MemoryChecking.h.
I'll give this patch a spin and report back on Monday.
Group: crypto-core-security
ASan DOM fuzzing with the patch didn't uncover any problems over the weekend.
(Please consider the "superreview" request as a second review request.)

I found a bug in the change to PL_ARENA_GROW. You can see my fix by
diff'ing this patch against the v2 patch.

This MXR query shows PL_ARENA_GROW is only used by NSS:
http://mxr.mozilla.org/mozilla-central/ident?i=PL_ARENA_GROW

So this bug did not invalidate the ASan DOM fuzzing that Jesse did
last weekend.
Attachment #733664 - Attachment is obsolete: true
Attachment #741561 - Flags: superreview?(choller)
Attachment #741561 - Flags: review?(matspal)
Comment on attachment 741561 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, v3, by Mats Palmgren

Review of attachment 741561 [details] [diff] [review]:
-----------------------------------------------------------------

I describe the bug in PL_ARENA_GROW below.

::: lib/ds/plarena.h
@@ +157,5 @@
>          PRUword _p = _a->avail; \
>          PRUword _q = _p + _incr; \
>          if (_p == (PRUword)(p) + PL_ARENA_ALIGN(pool, size) && \
>              _q <= _a->limit) { \
> +            PL_MAKE_MEM_UNDEFINED((void *)((PRUword)(p) + size), incr); \

In the v2 patch, this line is
    PL_MAKE_MEM_UNDEFINED((void *)_p, _incr); \

_p is _a->avail, which is the next aligned address after the last allocation
from the arena.  There may be a hole between the last allocated block and _p.
Therefore, PL_MAKE_MEM_UNDEFINED((void *)_p, _incr) will leave that hole with
the NOACCESS.
With this NSS patch, I can run the NSS test suite with no ASan errors.

The sqlite change is described in this Chromium bug report:
http://code.google.com/p/chromium/issues/detail?id=178677

The NSS secport.c change is necessary because that NSS function is
essentially part of PLArena.

Instructions for running NSS test suite under AddressSanitizer:

1. Install clang 3.3 or later on Linux.

2. Build NSS with this make command line:

make CC="clang -g -fsanitize=address" OPTIMIZER="-O1 -fno-omit-frame-pointer" DEFAULT_COMPILER=clang ZDEFS_FLAG= nss_build_all

3. Run NSS test suite (all.sh) as usual.

4. If there are any ASan errors, the call stacks in the output log do not
have function names. You need to use the asan_symbolize.py script to symbolize
the output log.
Comment on attachment 741561 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, v3, by Mats Palmgren

Review of attachment 741561 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds good to me :)
Attachment #741561 - Flags: superreview?(choller) → superreview+
Comment on attachment 741561 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, v3, by Mats Palmgren

Pushed to the NSPR hg repository (NSPR 4.10):
https://hg.mozilla.org/projects/nspr/rev/608d1530807d
Attachment #741561 - Flags: checked-in+
(In reply to Wan-Teh Chang from comment #25)
> There may be a hole between the last allocated block and _p.

Good catch!

I think the fix is wrong though.  It should work in ASan since it
doesn't detect a read of an UNDEFINED location that has never
been written to (it seems they are working on it though[1]), but it
will cause UMRs in Valgrind if/when you add support for it.

For the three areas - to old allocated area (A), the alignment gap (B),
the new area at the end (C).  These needs to be treated as such:

A: do not change!
B: mark as UNDEFINED
C: mark as UNDEFINED


[1] http://clang.llvm.org/docs/MemorySanitizer.html
Comment on attachment 741561 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, v3, by Mats Palmgren

r+ since it should work for ASan, but please add a XXX comment
in the code and file a bug about it so it's not forgotten!
Attachment #741561 - Flags: review?(matspal) → review+
(In reply to Mats Palmgren [:mats] from comment #29)
>
> For the three areas - to old allocated area (A), the alignment gap (B),
> the new area at the end (C).  These needs to be treated as such:
> 
> A: do not change!
> B: mark as UNDEFINED
> C: mark as UNDEFINED

Mats: thank you for your comment. I believe my fix does exactly what you
specified.

Here is my fix. I omitted the type casts for brevity:

 #define PL_ARENA_GROW(p, pool, size, incr) \
     PR_BEGIN_MACRO \
         ...
+            PL_MAKE_MEM_UNDEFINED(p + size, incr); \

A (old allocated data) is the byte range [p, p + size), which we do not
change.

B (alignment gap) may or may not exist. But if it exists, it will become
part of C (new area at the end) and marked as UNDEFINED below.

C (new area at the end) is the byte range [p + size, p + size + incr),
which we mark as UNDEFINED.

Is my analysis correct?
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → 4.10
Comment on attachment 741561 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, v3, by Mats Palmgren

Review of attachment 741561 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ds/plarena.h
@@ +157,5 @@
>          PRUword _p = _a->avail; \
>          PRUword _q = _p + _incr; \
>          if (_p == (PRUword)(p) + PL_ARENA_ALIGN(pool, size) && \
>              _q <= _a->limit) { \
> +            PL_MAKE_MEM_UNDEFINED((void *)((PRUword)(p) + size), incr); \

|p| is typically a void* pointer, so we can't perform pointer
arithmetic on |p| without typecasting.

Would it be clearer if I cast |p| to an unsigned char* pointer
(or even a char* pointer) instead?

            PL_MAKE_MEM_UNDEFINED((unsigned char *)(p) + size, incr); \
(In reply to Wan-Teh Chang from comment #31)
> +            PL_MAKE_MEM_UNDEFINED(p + size, incr); \

Ah, I misread the patch the first time, sorry.
Your analysis is correct and this is indeed the right fix.
(In reply to Wan-Teh Chang from comment #32)
Personally, I think "unsigned char*" would make the code clearer.
Attachment #741968 - Flags: review?(matspal)
Looking at PL_ARENA_ALLOCATE and PL_ArenaAllocate closer, I
noticed that the |nb| argument that PL_ARENA_ALLOCATE to
PL_ArenaAllocate is the aligned value |_nb|, which may be
larger than the original |nb|.  So when PL_ArenaAllocate
unpoisons the returned memory, it may unpoison more bytes
(the alignment gap) than it should.

This can be fixed by passing the original |nb| to
PL_ArenaAllocate. But I fix this by always unpoisoning
the returned memory in PL_ARENA_ALLOCATE.
Attachment #741968 - Attachment is obsolete: true
Attachment #741968 - Flags: review?(matspal)
Attachment #742137 - Flags: superreview?
Attachment #742137 - Flags: review?
Attachment #741968 - Flags: superreview?(choller)
Attachment #741968 - Flags: review?(matspal)
Attachment #741968 - Flags: superreview?(choller)
Attachment #741968 - Flags: review?(matspal)
Attachment #742137 - Flags: superreview?(choller)
Attachment #742137 - Flags: superreview?
Attachment #742137 - Flags: review?(matspal)
Attachment #742137 - Flags: review?
Comment on attachment 742137 [details] [diff] [review]
PL_ARENA_ALLOCATE should not let PL_ArenaAllocate unpoison the alignment gap. Use an unsigned char* typecast for pointer arithmetic.

Another good catch!

But, I don't like removing the annotations for direct calls to
PL_ArenaAllocate.

Can we mark the gap (if any) as NOACCESS in PL_ARENA_ALLOCATE instead?
(directly after the call to PL_ArenaAllocate)
Mats: these MXR queries show there are no direct calls to PL_ArenaAllocate
and its deprecated name PR_ArenaAllocate:
http://mxr.mozilla.org/mozilla-central/ident?i=PL_ArenaAllocate
http://mxr.mozilla.org/mozilla-central/ident?i=PR_ArenaAllocate

I inspected PL_ARENA_ALLOCATE and PL_ArenaAllocate, and convinced myself
that it doesn't make sense to call PL_ArenaAllocate directly. This is why
I think it's fine to always mark the allocated memory as UNDEFINED in
PL_ARENA_ALLOCATE.

This patch implements your proposed approach.  I am testing it with NSS
now.

I will attach another alternative patch later today.
Attachment #742472 - Flags: review?(matspal)
My "alternative fix 1" patch is broken. I can't spot where the bug is.

This is my alternative fix 2. The NSS test suite passed with this patch.
Attachment #742647 - Flags: review?(matspal)
The bug seems to be that the macro argument |nb| needs to be
parenthesized in the expression |_nb - nb|. I found a
PL_ARENA_ALLOCATE call where |nb| is an expression:

    PL_ARENA_ALLOCATE(newp, pool, size + incr);
Attachment #742472 - Attachment is obsolete: true
Attachment #742472 - Flags: review?(matspal)
Attachment #742652 - Flags: review?(matspal)
Comment on attachment 742652 [details] [diff] [review]
Alternative fix 1, v2, for PL_ARENA_ALLOCATE: mark alignment gap as NOACCESS in PL_ARENA_ALLOCATE

r=mats with a couple of nits below.
I slightly prefer this fix because it allows direct calls to
PL_ArenaAllocate just in case someone other than Mozilla does that.


>+            PL_MAKE_MEM_NOACCESS((void *)(_p + nb), _nb - (nb)); \

Please wrap the first "nb" in () too.

>+            PL_MAKE_MEM_UNDEFINED((unsigned char *)(p) + size, incr); \

Wrap "size".
Attachment #742652 - Flags: review?(matspal) → review+
Comment on attachment 742137 [details] [diff] [review]
PL_ARENA_ALLOCATE should not let PL_ArenaAllocate unpoison the alignment gap. Use an unsigned char* typecast for pointer arithmetic.

r=mats, but please wrap "nb" and "size" in ().

This looks fine too.
Attachment #742137 - Flags: review?(matspal) → review+
Comment on attachment 742647 [details] [diff] [review]
Alternative fix 2 for PL_ARENA_ALLOCATE: pass the original |nb| argument to PL_ArenaAllocate

This affects code that isn't memory annotations so I think it's
too invasive for this bug.  I think you should file a separate bug
if you want to do this cleanup (and ask a NSPR peer for review
which I'm not).
Attachment #742647 - Flags: review?(matspal) → review-
Depends on: 866525
Attachment #742137 - Flags: superreview?(choller) → superreview+
In the hg commit message for
https://hg.mozilla.org/integration/mozilla-inbound/rev/0314d200873a
I referenced this bug by mistake. I meant to reference the NSS bug 866525.

If that changeset is pushed to mozilla-central, please update bug 866525
instead. Thanks.
I found that none of the PL_Arena* friend functions should be
used directly, so I generalized the warning comment to apply
to all of them.

https://hg.mozilla.org/projects/nspr/rev/119d4ef3679c
Attachment #742137 - Attachment is obsolete: true
Attachment #752544 - Flags: checked-in+
Marked the bug fixed.  Changed the summary to reflect what was actually
added.  Valgrind support will be added in a future NSPR release.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Summary: Add memory check annotations to PLArena → Add AddressSanitizer (ASan) memory check annotations to PLArena
Group: crypto-core-security
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.