better display for canary command

[dmur1]

#1704

[dmur1]

before:

after:

[codecov]

Codecov Report

Attention: Patch coverage is 46.87500% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 59.70%. Comparing base (6d12d2f) to head (5f7538d).

Files	Patch %	Lines
pwndbg/commands/canary.py	46.87%	14 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #2044      +/-   ##
==========================================
- Coverage   59.70%   59.70%   -0.01%     
==========================================
  Files         191      191              
  Lines       24407    24430      +23     
  Branches     2418     2424       +6     
==========================================
+ Hits        14572    14585      +13     
- Misses       9051     9060       +9     
- Partials      784      785       +1     

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

[dmur1]

before:

after:

[dmur1]

@disconnect3d is this roughly what you want? ^

[disconnect3d]

The way canary works now (before this PR) is that it looks for stack canaries everywhere.

With this PR change, canaries would be only searched for on thread stacks. I think that this is not great as this may lead to us missing a canary value in some memory (that we may want to leak somehow).

I think we should change the command so that:

1. canary searches everywhere and dumps the info as it did before with a better display. It should probably be 3 columns: Stack? | Memory page | Address. The first column should tell "Thread # 1" if its memory page for stack from thread 1. We probably don't need to use telescope here as the addresses contain the stack canary values anyway.

2. canary --stacks-only should limit the search/detection to memory pages from tread stacks.

I am not sure about --all (?) - if we do limit results, maybe we should allow to filter by memory pages or by number of results. But I think above is probably more useful than this.

[dmur1]

The way canary works now (before this PR) is that it looks for stack canaries everywhere.

With this PR change, canaries would be only searched for on thread stacks. I think that this is not great as this may lead to us missing a canary value in some memory (that we may want to leak somehow).

I don't quite follow this - https://github.com/pwndbg/pwndbg/pull/2044/files#diff-1aab26f35891063c5e0bf76bc9578b142c1aa57fd9f30c9ccc5c1ba27228f037L44 - isn't this just searching the current thread stack before?

[dmur1]

user@ctf:/tmp/canary-test$ cat main.c 
#include <stdlib.h>
#include <string.h>
int main() {
    for (int i = 0; i < 10; i++) {
        char* b = (char*)malloc(32);
        memcpy(b, &b, 32);
    }
    __builtin_trap();
    return 0;
}
user@ctf:/tmp/canary-test$ gcc -g -fstack-protector-all main.c
user@ctf:/tmp/canary-test$ gdb --quiet ./a.out
pwndbg> r
--- SNIP ---
pwndbg> vis_heap_chunks 

0x555555559000	0x0000000000000000	0x0000000000000291	................
--- SNIP ---
0x555555559360	0x0000555555559360	0x009bca6ba7fc8800	`.UUUU......k...
0x555555559370	0x00007fffffffda10	0x00007ffff7c2a1ca	................
0x555555559380	0x0000000000000000	0x0000000000000031	........1.......
0x555555559390	0x0000555555559390	0x009bca6ba7fc8800	..UUUU......k...
0x5555555593a0	0x00007fffffffda10	0x00007ffff7c2a1ca	................
0x5555555593b0	0x0000000000000000	0x0000000000000031	........1.......
0x5555555593c0	0x00005555555593c0	0x009bca6ba7fc8800	..UUUU......k...
0x5555555593d0	0x00007fffffffda10	0x00007ffff7c2a1ca	................
0x5555555593e0	0x0000000000000000	0x0000000000000031	........1.......
0x5555555593f0	0x00005555555593f0	0x009bca6ba7fc8800	..UUUU......k...
0x555555559400	0x00007fffffffda10	0x00007ffff7c2a1ca	................
0x555555559410	0x0000000000000000	0x0000000000000031	........1.......
0x555555559420	0x0000555555559420	0x009bca6ba7fc8800	 .UUUU......k...
0x555555559430	0x00007fffffffda10	0x00007ffff7c2a1ca	................
0x555555559440	0x0000000000000000
pwndbg> canary 
AT_RANDOM = 0x7fffffffde89 # points to (not masked) global canary value
Canary    = 0x9bca6ba7fc8800 (may be incorrect on != glibc)
Found valid canaries on the stacks:
00:0000│-308 0x7fffffffd668 ◂— 0x9bca6ba7fc8800
00:0000│-2c8 0x7fffffffd6a8 ◂— 0x9bca6ba7fc8800
00:0000│-0d8 0x7fffffffd898 ◂— 0x9bca6ba7fc8800
00:0000│-0a8 0x7fffffffd8c8 ◂— 0x9bca6ba7fc8800
00:0000│-008 0x7fffffffd968 ◂— 0x9bca6ba7fc8800
00:0000│+098 0x7fffffffda08 ◂— 0x9bca6ba7fc8800

i made a slightly janky example to show the current behaviour - i copy some stuff from the stack ( including the canary ) into some heap allocations - the canary command doesn't see them it only sees the ones on the stack

[disconnect3d]

Oh interesting, I thought it was searched everywhere... my bad...

Anyway, showing just stacks may be ok in the end, since one can fire a full search if they wanted to and doing a full search ourselves may be painful for the users (may take long time if there are looooots of mappings).