Re: security: potential out-of-bound read at ewah_io.c |ewah_read_mmap|

2018-06-19 Thread Jeff King
On Tue, Jun 19, 2018 at 07:00:48PM +, Dyer, Edwin wrote:

> Just curious if there was any additional comment on this potential
> OOB? I may have missed it and if so, apologies for the ask.

The fix is in master, and should be part of the upcoming v2.18. See
commit 9d2e330b17 (ewah_read_mmap: bounds-check mmap reads, 2018-06-14).

-Peff


RE: security: potential out-of-bound read at ewah_io.c |ewah_read_mmap|

2018-06-19 Thread Dyer, Edwin
Greetings, all:

Just curious if there was any additional comment on this potential OOB? I may 
have missed it and if so, apologies for the ask.

Cheers,

Ed


-Original Message-
From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On Behalf Of 
Luat Nguyen
Sent: Thursday, June 14, 2018 7:00 PM
To: git@vger.kernel.org
Subject: security: potential out-of-bound read at ewah_io.c |ewah_read_mmap|

Hi folks,

Recently, I’ve found a security issue related to out-of-bound read at function 
named `ewah_read_mmap`

Assume that, an attacker can put malicious `./git/index` into a repo by somehow.

Since there is lack of check whether the remaining size of `ptr`is equal to 
`buffer_size` or not.

So the code reads exceed the buffer of `ptr` and reach to higher page. In this 
case, it is `/lib/x86_64-linux-gnu/ld-2.23.so`.

Leads to infoleak. You can find more details and asan crash below.



# xxd .git/index
: 4449 5243  0002   4653 4d4e  DIRCFSMN
0010:  0024  0001 1538 2489 c8fc 3616  ...$.8$...6.
0020:  0014    2000 4141   .. .AA
^ evil size here = 0x2000


* SNIP CODE *

int ewah_read_mmap(struct ewah_bitmap *self, const void *map, size_t len) { … 
self->buffer_size = self->alloc_size = get_be32(ptr);
ptr += sizeof(uint32_t);
… 
memcpy(self->buffer, ptr, self->buffer_size * sizeof(eword_t));


[memory map]

0x7f990eca3000 0x7f990eca4000 r--p 1000 0  
/media/sf_Fuzz/vuln_repo/.git/index <— where `ptr` is placed
0x7f990eca4000 0x7f990eca5000 r--p 1000 25000  
/lib/x86_64-linux-gnu/ld-2.23.so <— memcpy will reach here
0x7f990eca5000 0x7f990eca6000 rw-p 1000 26000  
/lib/x86_64-linux-gnu/ld-2.23.so <— and here 


[ ASAN log ]

root@guest:/media/sf_SHARE/vuln_repo# /media/sf_SHARE/git-master-asan/git 
status =
==4324==ERROR: AddressSanitizer: unknown-crash on address 0x7f6f235b at pc 
0x004bba79 bp 0x7ffc75e68850 sp 0x7ffc75e68000 READ of size 65536 at 
0x7f6f235b thread T0
#0 0x4bba78 in __asan_memcpy 
/tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:23:3
#1 0x8c910e in ewah_read_mmap 
/media/sf_SHARE/git-master-asan/ewah/ewah_io.c:144:2
#2 0x8e2534 in read_fsmonitor_extension 
/media/sf_SHARE/git-master-asan/fsmonitor.c:46:8
#3 0xa05862 in read_index_extension 
/media/sf_SHARE/git-master-asan/read-cache.c:1615:3
#4 0xa046f3 in do_read_index 
/media/sf_SHARE/git-master-asan/read-cache.c:1872:7
#5 0xa03325 in read_index_from 
/media/sf_SHARE/git-master-asan/read-cache.c:1913:8
#6 0xa03231 in read_index 
/media/sf_SHARE/git-master-asan/read-cache.c:1634:9
#7 0x9de5e8 in read_index_preload 
/media/sf_SHARE/git-master-asan/preload-index.c:119:15
#8 0x566cc6 in cmd_status 
/media/sf_SHARE/git-master-asan/builtin/commit.c:1358:2
#9 0x4ede8c in run_builtin /media/sf_SHARE/git-master-asan/git.c:417:11
#10 0x4ea939 in handle_builtin /media/sf_SHARE/git-master-asan/git.c:632:8
#11 0x4ed655 in run_argv /media/sf_SHARE/git-master-asan/git.c:684:4
#12 0x4ea037 in cmd_main /media/sf_SHARE/git-master-asan/git.c:761:19
#13 0x759c8b in main /media/sf_SHARE/git-master-asan/common-main.c:45:9
#14 0x7f6f2243382f in __libc_start_main 
(/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
#15 0x41c268 in _start (/media/sf_SHARE/git-master-asan/git+0x41c268)

Address 0x7f6f235b is a wild pointer.
SUMMARY: AddressSanitizer: unknown-crash 
/tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:23:3
 in __asan_memcpy Shadow bytes around the buggy address:
  0x0fee646adfb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fee646adfc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fee646adfd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fee646adfe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fee646adff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
=>0x0fee646ae000:[fe]fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fee646ae010: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fee646ae020: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fee646ae030: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fee646ae040: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fee646ae050: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe Shadow byte 
legend (one shadow byte represents 8 application bytes):
  Addressable:   00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:   fa
  Freed heap region:   fd
  Stack left redzone:  f1
  Stack mid redzone:   f2
  Stack right redzone: f3
  Stack after return:  f5
  Stack use after scope:   f8
  Global redzone:  f9
  Global init order:   f6
  Poisoned by user:f7
  Container o

Re: security: potential out-of-bound read at ewah_io.c |ewah_read_mmap|

2018-06-15 Thread Junio C Hamano
Jeff King  writes:

> On Fri, Jun 15, 2018 at 06:59:43AM +0800, Luat Nguyen wrote:
>
>> Recently, I’ve found a security issue related to out-of-bound read at 
>> function named `ewah_read_mmap`
>
> Thanks, this is definitely a bug worth addressing. But note...
>
>> Assume that, an attacker can put malicious `./git/index` into a repo by 
>> somehow.
>
> We generally don't consider .git/index (or pack .bitmap files, which
> also use this implementation) to be a major part of Git's attack
> surface, since they are generated locally. And if you can write to
> somebody's .git directory, there are already much easier ways to execute
> arbitrary code.

Thanks for giving a fair assessment on the gravity of the issue, to
which I agree fully, and also fixes and clean-ups.



>
>> Since there is lack of check whether the remaining size of `ptr`is
>> equal to `buffer_size` or not.
>
> Yep. We also fail to check if we even have enough bytes to read the
> buffer_size in the first place.
>
> Here are some patches. The first one fixes the problem you found. The
> second one drops some dead code that has a related problem. And the
> third just drops some dead code that I noticed in the same file. :)
>
>   [1/3]: ewah_read_mmap: bounds-check mmap reads
>   [2/3]: ewah: drop ewah_deserialize function
>   [3/3]: ewah: drop ewah_serialize_native function
>
>  ewah/ewah_io.c  | 106 
>  ewah/ewok.h |   4 +-
>  t/t5310-pack-bitmaps.sh |  13 +
>  3 files changed, 35 insertions(+), 88 deletions(-)
>
> -Peff


Re: security: potential out-of-bound read at ewah_io.c |ewah_read_mmap|

2018-06-14 Thread Jeff King
On Fri, Jun 15, 2018 at 06:59:43AM +0800, Luat Nguyen wrote:

> Recently, I’ve found a security issue related to out-of-bound read at 
> function named `ewah_read_mmap`

Thanks, this is definitely a bug worth addressing. But note...

> Assume that, an attacker can put malicious `./git/index` into a repo by 
> somehow.

We generally don't consider .git/index (or pack .bitmap files, which
also use this implementation) to be a major part of Git's attack
surface, since they are generated locally. And if you can write to
somebody's .git directory, there are already much easier ways to execute
arbitrary code.

> Since there is lack of check whether the remaining size of `ptr`is
> equal to `buffer_size` or not.

Yep. We also fail to check if we even have enough bytes to read the
buffer_size in the first place.

Here are some patches. The first one fixes the problem you found. The
second one drops some dead code that has a related problem. And the
third just drops some dead code that I noticed in the same file. :)

  [1/3]: ewah_read_mmap: bounds-check mmap reads
  [2/3]: ewah: drop ewah_deserialize function
  [3/3]: ewah: drop ewah_serialize_native function

 ewah/ewah_io.c  | 106 
 ewah/ewok.h |   4 +-
 t/t5310-pack-bitmaps.sh |  13 +
 3 files changed, 35 insertions(+), 88 deletions(-)

-Peff


security: potential out-of-bound read at ewah_io.c |ewah_read_mmap|

2018-06-14 Thread Luat Nguyen
Hi folks,

Recently, I’ve found a security issue related to out-of-bound read at function 
named `ewah_read_mmap`

Assume that, an attacker can put malicious `./git/index` into a repo by somehow.

Since there is lack of check whether the remaining size of `ptr`is equal to 
`buffer_size` or not.

So the code reads exceed the buffer of `ptr` and reach to higher page. In this 
case, it is `/lib/x86_64-linux-gnu/ld-2.23.so`.

Leads to infoleak. You can find more details and asan crash below.



# xxd .git/index
: 4449 5243  0002   4653 4d4e  DIRCFSMN
0010:  0024  0001 1538 2489 c8fc 3616  ...$.8$...6.
0020:  0014    2000 4141   .. .AA
^ evil size here = 0x2000


* SNIP CODE *

int ewah_read_mmap(struct ewah_bitmap *self, const void *map, size_t len)
{
… 
self->buffer_size = self->alloc_size = get_be32(ptr);
ptr += sizeof(uint32_t);
… 
memcpy(self->buffer, ptr, self->buffer_size * sizeof(eword_t));


[memory map]

0x7f990eca3000 0x7f990eca4000 r--p 1000 0  
/media/sf_Fuzz/vuln_repo/.git/index <— where `ptr` is placed
0x7f990eca4000 0x7f990eca5000 r--p 1000 25000  
/lib/x86_64-linux-gnu/ld-2.23.so <— memcpy will reach here
0x7f990eca5000 0x7f990eca6000 rw-p 1000 26000  
/lib/x86_64-linux-gnu/ld-2.23.so <— and here 


[ ASAN log ]

root@guest:/media/sf_SHARE/vuln_repo# /media/sf_SHARE/git-master-asan/git status
=
==4324==ERROR: AddressSanitizer: unknown-crash on address 0x7f6f235b at pc 
0x004bba79 bp 0x7ffc75e68850 sp 0x7ffc75e68000
READ of size 65536 at 0x7f6f235b thread T0
#0 0x4bba78 in __asan_memcpy 
/tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:23:3
#1 0x8c910e in ewah_read_mmap 
/media/sf_SHARE/git-master-asan/ewah/ewah_io.c:144:2
#2 0x8e2534 in read_fsmonitor_extension 
/media/sf_SHARE/git-master-asan/fsmonitor.c:46:8
#3 0xa05862 in read_index_extension 
/media/sf_SHARE/git-master-asan/read-cache.c:1615:3
#4 0xa046f3 in do_read_index 
/media/sf_SHARE/git-master-asan/read-cache.c:1872:7
#5 0xa03325 in read_index_from 
/media/sf_SHARE/git-master-asan/read-cache.c:1913:8
#6 0xa03231 in read_index 
/media/sf_SHARE/git-master-asan/read-cache.c:1634:9
#7 0x9de5e8 in read_index_preload 
/media/sf_SHARE/git-master-asan/preload-index.c:119:15
#8 0x566cc6 in cmd_status 
/media/sf_SHARE/git-master-asan/builtin/commit.c:1358:2
#9 0x4ede8c in run_builtin /media/sf_SHARE/git-master-asan/git.c:417:11
#10 0x4ea939 in handle_builtin /media/sf_SHARE/git-master-asan/git.c:632:8
#11 0x4ed655 in run_argv /media/sf_SHARE/git-master-asan/git.c:684:4
#12 0x4ea037 in cmd_main /media/sf_SHARE/git-master-asan/git.c:761:19
#13 0x759c8b in main /media/sf_SHARE/git-master-asan/common-main.c:45:9
#14 0x7f6f2243382f in __libc_start_main 
(/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
#15 0x41c268 in _start (/media/sf_SHARE/git-master-asan/git+0x41c268)

Address 0x7f6f235b is a wild pointer.
SUMMARY: AddressSanitizer: unknown-crash 
/tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:23:3
 in __asan_memcpy
Shadow bytes around the buggy address:
  0x0fee646adfb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fee646adfc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fee646adfd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fee646adfe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fee646adff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0fee646ae000:[fe]fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fee646ae010: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fee646ae020: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fee646ae030: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fee646ae040: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fee646ae050: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:   00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:   fa
  Freed heap region:   fd
  Stack left redzone:  f1
  Stack mid redzone:   f2
  Stack right redzone: f3
  Stack after return:  f5
  Stack use after scope:   f8
  Global redzone:  f9
  Global init order:   f6
  Poisoned by user:f7
  Container overflow:  fc
  Array cookie:ac
  Intra object redzone:bb
  ASan internal:   fe
  Left alloca redzone: ca
  Right alloca redzone:cb
==4324==ABORTING
root@guest:/media/sf_SHARE/vuln_repo#


Regards,
Luat Nguyen.