[Bug libc/18292] New: Invalid pointer dereference in nsswitch.c:nss_new_service()

[Bug libc/18292] New: Invalid pointer dereference in nsswitch.c:nss_new_service()

[jf at ownco dot net]

https://sourceware.org/bugzilla/show_bug.cgi?id=18292

            Bug ID: 18292
           Summary: Invalid pointer dereference in
                    nsswitch.c:nss_new_service()
           Product: glibc
           Version: 2.21
            Status: NEW
          Severity: normal
          Priority: P2
         Component: libc
          Assignee: unassigned at sourceware dot org
          Reporter: jf at ownco dot net
                CC: drepper.fsp at gmail dot com

While making local modifications to the allocator, specifically one that
modifies the prev_size member for fastbin chunks and debugging the
modifications I noted that nss_new_service() increments a pointer to a pointer
incorrectly, which ends up causing the pointer to utilize a member of the
malloc_chunk structure, specifically the prev_size member (Why I am doing this
to the allocator will be the subject of another bug report later).

It appears that because this occurs in a loop that is bounded by NULL, this is
usually a non-issue, however, when its a non-NULL value, it causes a
dereference of that value. 

Specifically the loop in question is:

800 while (*currentp != NULL)
801 {
802 if (strcmp ((*currentp)->name, name) == 0)
803 return *currentp;
804 currentp = &(*currentp)->next;
805 }

With the errant line occurring on line 804.

804           currentp = &(*currentp)->next;
(gdb) x/i $rip
=> 0x45c160 <__nss_lookup_function+480>:        mov    rax,QWORD PTR [rbx+0x10]
(gdb) x/x $rbx
0x6f9ce0:       0x006f9c20
(gdb) x/x $rbx+0x10
0x6f9cf0:       0x00000001
(gdb) x/x *($rbx)+0x10
0x6f9c30:       0x00000000
(gdb) p *(mchunkptr)0x6f9cf0 
$43 = {prev_size = 1, size = 49, fd = 0x6f9d70, bk = 0x0, fd_nextsize = 0x0,
bk_nextsize = 0x322e6f732e75}
(gdb) n
800       while (*currentp != NULL)
(gdb) 
802           if (strcmp ((*currentp)->name, name) == 0)
(gdb) x/i $rip
=> 0x45c170 <__nss_lookup_function+496>:        mov    rsi,QWORD PTR [rbp-0x40]
(gdb)  
   0x45c174 <__nss_lookup_function+500>:        mov    rdi,QWORD PTR [rbx]
(gdb) stepi
0x000000000045c174      802           if (strcmp ((*currentp)->name, name) ==
0)
(gdb) x/i $rip
=> 0x45c174 <__nss_lookup_function+500>:        mov    rdi,QWORD PTR [rbx]
(gdb) x/x $rbx
0x1:    Cannot access memory at address 0x1
(gdb) stepi

Program received signal SIGSEGV, Segmentation fault.
0x000000000045c174 in nss_new_service (name=0x6f9c60 "giles",
database=<optimized out>) at nsswitch.c:802
802           if (strcmp ((*currentp)->name, name) == 0)

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18292] Invalid pointer dereference in nsswitch.c:nss_new_service()

[jf at ownco dot net]

https://sourceware.org/bugzilla/show_bug.cgi?id=18292

--- Comment #1 from Justin N. Ferguson <jf at ownco dot net> ---
repro:
set prev_size member of chunk to non-zero value that doesn't properly
dereference. execute "ls -al".

I suppose why I have a non-stdandard prev_size is worth explanation, even
though this is clearly a bug irrelevant of allocator modifications:

I am in the process of developing a patch for the allocator that fixes a
particularly nasty set of double free related issues that can occur from
fastbin's and can be leveraged to bypass all memory corruption related
mitigations (ASLR/NX/ ...vtable verification); the short of it is that you can
get the same block of memory stored in the fastbin free list twice, which in
turn can cause it to be reallocated twice, which in some conditions can result
in conditions wherein a double free behaves more like a logic error and you can
manipulate two different objects/classes that occupy the same block of memory.

prev_size seemed like a good choice given the fastbin blocks are singly linked
and otherwise mostly unused.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18292] Invalid pointer dereference in nsswitch.c:nss_new_service()

[jf at ownco dot net]

https://sourceware.org/bugzilla/show_bug.cgi?id=18292

Justin N. Ferguson <jf at ownco dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jf at ownco dot net

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18292] Invalid pointer dereference in nsswitch.c:nss_new_service()

[schwab@linux-m68k.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=18292

Andreas Schwab <schwab@linux-m68k.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |INVALID

--- Comment #2 from Andreas Schwab <schwab@linux-m68k.org> ---
prev_size is only usable if PREV_INUSE is cleared.  Otherwise it is either
overlayed with the previous (allocated) chunk, or before the start of the area.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18292] Invalid pointer dereference in nsswitch.c:nss_new_service()

[jf at ownco dot net]

https://sourceware.org/bugzilla/show_bug.cgi?id=18292

Justin N. Ferguson <jf at ownco dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|INVALID                     |WONTFIX

--- Comment #3 from Justin N. Ferguson <jf at ownco dot net> ---
If I was filing a bug against the allocator, then this would be a valid
response. I filed a bug against nsswitch.c, which is operating off of heap
metadata and only works because the prev_size field is generally NULL.

You could just fix it so the function operates on the correct pointer.

I switched the status to 'WONTFIX', because that is correct, not 'INVALID'.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18292] Invalid pointer dereference in nsswitch.c:nss_new_service()

[schwab@linux-m68k.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=18292

Andreas Schwab <schwab@linux-m68k.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|WONTFIX                     |INVALID

--- Comment #4 from Andreas Schwab <schwab@linux-m68k.org> ---
There is nothing to fix here.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18292] Invalid pointer dereference in nsswitch.c:nss_new_service()

[jf at ownco dot net]

https://sourceware.org/bugzilla/show_bug.cgi?id=18292

Justin N. Ferguson <jf at ownco dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|INVALID                     |---

--- Comment #5 from Justin N. Ferguson <jf at ownco dot net> ---
In what way can you even begin to believe 'there is nothing to fix here'?

Putting aside that we shouldn't even be debating internal heap states in
reference to a bug that is triggered in another part of the library dependent
on the heap state, the exact condition requisite can happen when certain code
paths in realloc() happen with blocks immediately preceeding the fastbin chunk
in the contiguous memory.

That said, your code is operating off of heap metadata when it thinks its
operating off of application data. This is plainly incorrect.

This sort of behavior is probably why people are migrating to alternative
libcs.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18292] Invalid pointer dereference in nsswitch.c:nss_new_service()

[schwab@linux-m68k.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=18292

Andreas Schwab <schwab@linux-m68k.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|---                         |INVALID

--- Comment #6 from Andreas Schwab <schwab@linux-m68k.org> ---
There is nothing wrong with that reference.  Please read the comments in
malloc.c about how the allocator manages its meta data.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18292] Invalid pointer dereference in nsswitch.c:nss_new_service()

[jf at ownco dot net]

https://sourceware.org/bugzilla/show_bug.cgi?id=18292

Justin N. Ferguson <jf at ownco dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|INVALID                     |WONTFIX

--- Comment #7 from Justin N. Ferguson <jf at ownco dot net> ---
(In reply to Andreas Schwab from comment #6)
> There is nothing wrong with that reference.  Please read the comments in
> malloc.c about how the allocator manages its meta data.

Okay. This isn't relevant. There are code paths in realloc() that can trigger
this bug, but even if there were not, it has no bearing on the code in
nsswitch.c, which is where the bug in question is.

Please try to focus on the matter at hand, which is the bug in nsswitch.c, not
malloc.c.

800 while (*currentp != NULL)
801 {
802 if (strcmp ((*currentp)->name, name) == 0)
803 return *currentp;
804 currentp = &(*currentp)->next;
805 }

This code, in nsswitch.c, not malloc.c, increments the currentp ** incorrectly
at line 804. This causes it to point to incorrectly into heap metadata, which
may or may not be set to NULL, depending on the state of the heap-- which in
some instances, particularly edge cases where blocks are being resized and
memory is split from the top chunk IIRC, and the prior block has been
deallocated but not consolidated-- im hesitant to get overly into the semantics
of the heap, because its not relevant-- what is relevant is the code I actually
filed the bug against, which I've copy/pasted again above.

The net result is that you end up calling strcmp() on heap metadata,
specifically (mchunkptr)(x)->prev_size, which is NEVER VALID for you to be
doing, It just works because most of the time, the heap is in a state wherein
(mchunkptr)(x)->prev_size = 0 which == ((void*)0) or NULL.

The bug itself seems benign, and it probably is, but given that its in NSS
(which is not malloc.c), you probably shouldn't take the chance given that
there are any number of unexpected security related issues and environments
that could potentially encounter it.

to reiterate:
x = malloc(y);
x -= 0x10;
strcmp(x, "this is always invalid, period. it doesnt matter what the value of
prev_size is or is not.");

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18292] Invalid pointer dereference in nsswitch.c:nss_new_service()

[jf at ownco dot net]

https://sourceware.org/bugzilla/show_bug.cgi?id=18292

Justin N. Ferguson <jf at ownco dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|WONTFIX                     |INVALID

--- Comment #8 from Justin N. Ferguson <jf at ownco dot net> ---
For the record; rewriting the affected function showed that the bug is
occurring elsewhere and just exhibiting itself in that function. I am in the
process of trying to discern where it actually is occurring.

No matter what, the code should not be dereferencing heap metadata.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18292] Invalid pointer dereference in nsswitch.c:nss_new_service()

[jf at ownco dot net]

https://sourceware.org/bugzilla/show_bug.cgi?id=18292

--- Comment #9 from Justin N. Ferguson <jf at ownco dot net> ---
Just as a follow-up; what a weird set of circumstances. There is definitely a
bug, but I'm not 100% who to blame it on.

In the allocator there is conflicting ways of handling the block size, in one:
request2size() on amd64 we treat the minimum structure size as 0x17, and in
another we treat it as 0x10.

When we enter _int_malloc(), we bitwise and the block size with
0xFFFFFFFFFFFFFFF0, which is apparently the proper alignment even though
~SIZE_BITS is 0xFFFFFFFFFFFFFFF8. This often results in a smaller than expected
block size that is variable between 3 and 8 bytes in length, but that is always
within the width of prev_size, which makes it mostly a non-issue from libc's
perspective.

Then when two chunks are split off from the top chunk, this can result in the
second block that is allocated to start inside of the end of the prior block--
which is as you say, the prev_size field and its overlayed-- I don't think you
all mean to overlay it however as evidenced by the fact that the amount of the
overlay is variable, and even if that is what's intended, then why on earth
would you ever return a pointer to an application wherein the tail end of one
points to the metadata of another, even if said variable is always (almost
always?) written to before its read from.

In the specific service_user or whatever structure in NSCD, these two
allocations happen in short succession so that the next pointer actually points
into the name field. This is generally not a problem, because prev_size will be
zero/NULL (or rather it will be *uninitialized* until the correct block is
deallocated-- which is in itself a problem if you consider buffer semantics of
libraries like openssl, which tend to believe user-data too much and
overallocate buffers to be safe, but then put a hard limit on the data to be
written so that you cannot overflow the buffer at a lower layer abstraction;
meaning the potential for an uninitialized pointer sized leak of address space
information cannot be discounted).

You all are going to mark this notation as 'INVALID' and say its expected
behavior and at some point in the future enough use-after-free bugs are going
to exploit it that we'll end up fixing it, and so this note is a message in a
bottle from the past to whomever later discerns this is useful. I, I'm probably
just going to patch chunksize() so that it is using the same size field as
everything else, incidentally, this opens up a bit in the size field which I
can use to mark the block as in use, which makes double free detection far far
easier; at least on amd64 anyway.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18292] Invalid pointer dereference in nsswitch.c:nss_new_service()

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=18292

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fweimer at redhat dot com
              Flags|                            |security-

--- Comment #10 from Florian Weimer <fweimer at redhat dot com> ---
(In reply to Justin N. Ferguson from comment #1)
> repro:
> set prev_size member of chunk to non-zero value that doesn't properly
> dereference. execute "ls -al".

I assume that this is about the prev_size member of the following chunk.  Have
you increased the chunk size accordingly (by the size of a pointer)?  If not,
your malloc invalid because prev_size is only available if the preceding chunk
is free, as explained in comment #2.

The fundamental idea behind the free space management in dlmalloc and similar
allocators is to use the trailing end of a free chunk to store a pointer (or
offset) to its beginning, so that when the subsequent chunk is freed, it can be
merged with the preceding one.  Without the information prev_size provides, it
would not be possible to find the start of the chunk to merge with.  This is
explained on page 28 of <ftp://ftp.cs.utexas.edu/pub/garbage/allocsrv.ps> and
sometimes called a “boundary tag“ mechanism.

valgrind also does not show any out-of-bounds references, and it is usually
very precise for such issues.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
>From glibc-bugs-return-28128-listarch-glibc-bugs=sources.redhat.com@sourceware.org Wed Apr 29 13:53:43 2015
Return-Path: <glibc-bugs-return-28128-listarch-glibc-bugs=sources.redhat.com@sourceware.org>
Delivered-To: listarch-glibc-bugs@sources.redhat.com
Received: (qmail 47850 invoked by alias); 29 Apr 2015 13:53:43 -0000
Mailing-List: contact glibc-bugs-help@sourceware.org; run by ezmlm
Precedence: bulk
List-Id: <glibc-bugs.sourceware.org>
List-Subscribe: <mailto:glibc-bugs-subscribe@sourceware.org>
List-Post: <mailto:glibc-bugs@sourceware.org>
List-Help: <mailto:glibc-bugs-help@sourceware.org>, <http://sourceware.org/lists.html#faqs>
Sender: glibc-bugs-owner@sourceware.org
Delivered-To: mailing list glibc-bugs@sourceware.org
Received: (qmail 47802 invoked by uid 48); 29 Apr 2015 13:53:39 -0000
From: "fweimer at redhat dot com" <sourceware-bugzilla@sourceware.org>
To: glibc-bugs@sourceware.org
Subject: [Bug libc/18286] Please incorporate libffcall
Date: Wed, 29 Apr 2015 13:53:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: glibc
X-Bugzilla-Component: libc
X-Bugzilla-Version: unspecified
X-Bugzilla-Keywords:
X-Bugzilla-Severity: enhancement
X-Bugzilla-Who: fweimer at redhat dot com
X-Bugzilla-Status: SUSPENDED
X-Bugzilla-Resolution:
X-Bugzilla-Priority: P2
X-Bugzilla-Assigned-To: unassigned at sourceware dot org
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Flags: security-
X-Bugzilla-Changed-Fields: cc flagtypes.name
Message-ID: <bug-18286-131-F2bykl5OMF@http.sourceware.org/bugzilla/>
In-Reply-To: <bug-18286-131@http.sourceware.org/bugzilla/>
References: <bug-18286-131@http.sourceware.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://sourceware.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2015-04/txt/msg00186.txt.bz2
Content-length: 453

https://sourceware.org/bugzilla/show_bug.cgi?id\x18286

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fweimer at redhat dot com
              Flags|                            |security-

--
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18292] Invalid pointer dereference in nsswitch.c:nss_new_service()

[jf at ownco dot net]

https://sourceware.org/bugzilla/show_bug.cgi?id=18292

--- Comment #11 from Justin N. Ferguson <jf at ownco dot net> ---
> I assume that this is about the prev_size member of the following chunk. 
> Have you increased the chunk size accordingly (by the size of a pointer)? 
> If not, your malloc invalid because prev_size is only available if the
> preceding chunk is free, as explained in comment #2.

Well, we are debating semantics here in terms of where prev_size exists, its
quite obviously not being stored in the prior block in my reading as discerned
that an allocation will be at least 16-bytes/32-bytes on a 64-bit platform; The
pointer returned to the user is 16-bytes past the start of the allocation, with
the prev_size and size members being the header and the 16 remaining bytes
being utilized after free for forward and back pointers (assuming its not a
fastbin chunk).

So, the prev_size member is always available in the header of the allocation,
it's just often not initialized or similar. Furthermore, it would seem
self-evident to me that this is obviously not the case when we consider the top
chunk; but again, its semantics. It appears to be a non-issue because the field
is almost always written to before its read, but I am in the process of trying
to confirm all edge cases of this for myself.

A block that is mmap'd uses the prev_size member to store metadata when it is
allocated, because mmap'd chunks do not have a previous chunk next to them in
terms of the logic of the allocator, however this is not entirely true because
you can do subsequent allocations and force two mmap'd chunks to be next to
each other.

However, the issue itself appears when (only when?) we split a new block off of
the top block, where the arithmetic is treated as 16-bytes as opposed to the
more typical 32-bytes, leaving the next allocation from top overlapping with
the last-- which doesn't *appear* to be usable in terms of mmap'd memory, but
I'm still doofing around trying to make sure there is no clear way to handle
this.

> valgrind also does not show any out-of-bounds references, and it is 
> usually very precise for such issues.

Yeah well there are a lot of things automated tools don't detect, and they
often revolve around sections of code that cross multiple boundaries or where a
particular issue depends on how an OS handles things (id est mmap placements),
or other problem specific semantics that do not appear in many many use cases.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18292] Invalid pointer dereference in nsswitch.c:nss_new_service()

[jf at ownco dot net]

https://sourceware.org/bugzilla/show_bug.cgi?id=18292

--- Comment #12 from Justin N. Ferguson <jf at ownco dot net> ---
I haven't tried this out or worked it all the way through in my head, so YMMV,
it just occurred to me last night, but what if a block is in the process of
being written to in one thread, and in another the following block is being
consolidated or similar? There's a race; the returned allocations really
shouldn't be overlapping like this. 

Consider the code path in __libc_realloc() where _int_realloc() fails, but the
subsequent call to __libc_malloc() succeeds, and during the memcpy() the block
following the returned one is free'd? 

I would need to visualize this to make sure its correct; I've moved on from the
issue, I'm just weary of what is actually occurring in the allocator.

Additionally, it seems like creating a "BLOCK_INUSE" style flag could solve a
lot of problems, and the code path leading into mremap() can be abused by an
attacker to create a "write what where" primitive, which is not a huge ordeal
because corruption has to have already occurred, but then again we did harden
unlink() to prevent those exact sorts of exploitation scenarios.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18292] Invalid pointer dereference in nsswitch.c:nss_new_service()

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=18292

--- Comment #13 from Florian Weimer <fweimer at redhat dot com> ---
(In reply to Justin N. Ferguson from comment #12)
> I haven't tried this out or worked it all the way through in my head, so
> YMMV, it just occurred to me last night, but what if a block is in the
> process of being written to in one thread, and in another the following
> block is being consolidated or similar? There's a race; the returned
> allocations really shouldn't be overlapping like this. 

Consolidation only happens between chunks which are unused.  The arena lock
prevents a chunks from switching from unused to used during this processing. 
At least this is my understanding.

> Consider the code path in __libc_realloc() where _int_realloc() fails, but
> the subsequent call to __libc_malloc() succeeds, and during the memcpy() the
> block following the returned one is free'd? 

That's a race in the calling application, I think.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18292] Invalid pointer dereference in nsswitch.c:nss_new_service()

[jf at ownco dot net]

https://sourceware.org/bugzilla/show_bug.cgi?id=18292

--- Comment #14 from Justin N. Ferguson <jf at ownco dot net> ---

> Consolidation only happens between chunks which are unused.  The arena lock
> prevents a chunks from switching from unused to used during this processing.
> At least this is my understanding.

Well in the example I gave, there is no lock being held, but thinking about the
code a little more, you're okay in those circumstances because the only way
you're not is if _int_realloc() fails, which means the resize must be larger.


> That's a race in the calling application, I think.

We will agree to disagree here; all apologies for using bugzilla in this
manner.

In all earnest, there's a lot of things I'd like to do to improve the security
posture of the allocator, but I'm apprehensive about touching it because it's
sort of just waiting for a modification for things to blow up all over the
place. 

Some areas that could be improved:

- Introduction of heap cookies in chunks or at least arenas
- Introduction of a bit to discern whether a block is use to quash issues with
fastbin's and free(a); free(b); free(a); double free patterns
- Improved randomization; currently ASLR is easy to break because it looks more
random than it actually is 
- Patch the mremap code path and other mmap related code paths to prevent abuse
of unchecked prev_size in the event that size gets corrupted or similar 
- While not strictly allocator related; the load order of modules should
probably be randomized.
- et cetera

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18292] Invalid pointer dereference in nsswitch.c:nss_new_service()

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=18292

Carlos O'Donell <carlos at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |carlos at redhat dot com

--- Comment #15 from Carlos O'Donell <carlos at redhat dot com> ---
(In reply to Justin N. Ferguson from comment #14)
> In all earnest, there's a lot of things I'd like to do to improve the
> security posture of the allocator, but I'm apprehensive about touching it
> because it's sort of just waiting for a modification for things to blow up
> all over the place. 

I'm sorry to hear that. Hopefully we can convince you that even incremental
progress is a good step forward.

-- 
You are receiving this mail because:
You are on the CC list for the bug.