Improve bin corruption checks

[gsingh93]

• The existing bin corruption checks would mark some corrupted bins as empty instead of corrupted. There are now some additional checks, including checking if the fd and bk pointers are in the libc data section.
• Handle the case where the chain is less than 2 elements
• Check the full list integrity only if we've fully traversed the fd and bk lists (addresses some of Heap chain corruption check is incorrect for long free lists #1196)
• Make the list integrity check more clear
• In the case we can't traverse the entire lists, do a basic sanity check that chunk->bk->fd = chunk

[gsingh93]

I'm wondering instead of a boolean, this could be one of three values: corrupted, not corrupted, not sure.

[gsingh93]

I wonder if chunk.bk could return a Chunk instead of an int?

[CptGibbon]

You could add a previous_chunk() method for example, since this is meant to mimic the malloc_chunk struct's fields. If someone wanted the bk value itself, that is lost as soon as you get the previous chunk. That would also be in line with the current next_chunk() method, but I suppose it would always return None for singly-linked lists.

[gsingh93]

Sorry if I'm misunderstanding, but Chunk.next_chunk seems to return the next chunk in memory, whereas I'm wondering if Chunk.fd and Chunk.bk should just return a Chunk for convenience. If we'd like to keep this inline with the structs fields, maybe Chunk.fd_chunk and Chunk.bk_chunk could just be implemented as def fd_chunk(self): return Chunk(self.fd).

It's not a major difference, but I'd prefer if the Chunk class took care of this for me.

[CptGibbon]

I agree that Chunk should handle this, I can put together something like you suggest 👍

[CptGibbon]

On revisiting this, it's not as simple as it might seem.

Since tcache fd's are treated differently to other bin fd's (they point to user data rather than chunk metadata) the Chunk object needs to check if it represents a chunk that's in a tcachebin, so it can either do Chunk(chunk.fd) or Chunk(chunk.fd - (size_sz*2)).

This could involve checking the size field, and if it's in tcache range searching the appropriate tcachebin for this chunk's address. But assuming corruption requires searching every tcachebin for this chunk.

Since the bin_at() method only operates on the "normal" bins, there's no need to do this calculation. I think in this case using Chunk(chunk.bk) avoids unnecessary reads from gdb whilst still being clear as to what it's doing.

[disconnect3d]

What's the status here? Seems that some heap tests are failing

[gsingh93]

Will be very busy for a bit longer, I think I can complete this before the next release.

[gsingh93]

@CptGibbon tests are passing now, should be ready for review

[lebr0nli]

I do not very understand the logic behind this yet, so probably I misunderstood something:

I have some concerns about the check here:

1. What if vmmap does not work well(e.g. for the native qemu-user), will it make pwndbg think the empty bins are corrupted bins?
2. If we are debugging a binary with statically linked libc, will it make pwndbg think the empty bins are corrupted bins since "libc" in page.objfile is never be True?
3. IIRC, bins in non-main arena might not in someone's .data sectioin, does this check also true for the bins not in main_arena?
4. Seems like it might pass the check if the address is on the same page as the .data section, but not actually "in" the .data section? Maybe parsing info files will be better if we want to know if the address is in .data section or not?

[gsingh93]

What if vmmap does not work well(e.g. for the native qemu-user), will it make pwndbg think the empty bins are corrupted bins?

Hmm, I think you're right. Maybe we should have a function that returns whether the vmmap output can be trusted or not?

If we are debugging a binary with statically linked libc, will it make pwndbg think the empty bins are corrupted bins since "libc" in page.objfile is never be True?

I didn't think about this, you're right.

IIRC, bins in non-main arena might not in someone's .data sectioin, does this check also true for the bins not in main_arena?

Good point. I can add a check here to only do this logic if it's the main arena?

Seems like it might pass the check if the address is on the same page as the .data section, but not actually "in" the .data section? Maybe parsing info files will be better if we want to know if the address is in .data section or not?

Sure, I'm more worried though about a non-corrupted chain being marked as corrupted, not a corrupted chain being marked as not corrupted. The latter is just best effort on our part, while the former would be considered a bug.

[lebr0nli]

I can add a check here to only do this logic if it's the main arena?

Hmm, instead of still checking the fd_chain/bk_chain point to the .data section or not, could we check the front and back pointers instead of the fd_chain/bk_chain?
I mean, something like this:

--- a/pwndbg/heap/ptmalloc.py
+++ b/pwndbg/heap/ptmalloc.py
@@ -1200,13 +1200,7 @@ class GlibcMemoryAllocator(pwndbg.heap.heap.MemoryAllocator):
             is_corrupted = True
         elif len(chain_fd) == len(chain_bk) == 2 and chain_fd == chain_bk and chain_fd[-1] == 0:
             # Check if bin[index] points to itself (is empty)
-
-            # If the fd pointer is in libc data section, we can be more
-            # confident that this list is actually empty and not just
-            # corrupted. If we can't find any mapping for the pointer or it's in
-            # some other mapping, we assume the bin is corrupted
-            page = pwndbg.gdblib.vmmap.find(chain_fd[0])
-            if page and page.rw and "libc" in page.objfile:
+            if front == back and front == current_base:
                 chain_fd = [0]
                 chain_bk = [0]
             else:

Is this correct? (This is entirely from my observation, I'm not very sure :p)

[CptGibbon]

@CptGibbon tests are passing now, should be ready for review

I can take a thorough look next week old chap 👍

[CptGibbon]

I may be missing something, but I don't see this variable being used anywhere.
If that is the case, then a test for this scenario could be useful.

[gsingh93]

Sorry, I meant is_chain_corrupted here.

[CptGibbon]

Yeah I guess there must be a gap in the current tests since this still passed even with an unused variable.
Probably worth a adding a test that checks for this case wouldn't you say?

[disconnect3d]

Ping @CptGibbon :)

[disconnect3d]

What's the status of this? Can we re-trigger CI here? For some reason I think I can't? Maybe we should just push whatever here

[gsingh93]

Was just planning on adding a test but that can be done in a future PR.

[codecov]

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (758fb9c) 59.18% compared to head (c36e1ea) 60.55%.
Report is 219 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1564      +/-   ##
==========================================
+ Coverage   59.18%   60.55%   +1.37%     
==========================================
  Files         175      180       +5     
  Lines       21019    22236    +1217     
  Branches     1859     2088     +229     
==========================================
+ Hits        12441    13466    +1025     
- Misses       7977     8062      +85     
- Partials      601      708     +107     

Files	Coverage Δ
pwndbg/heap/ptmalloc.py	71.07% <57.14%> (-0.47%)	⬇️

... and 180 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.