[PATCH] fsmonitor: avoid global-buffer-overflow READ when checking trivial response

[PATCH] fsmonitor: avoid global-buffer-overflow READ when checking trivial response

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

query_result can be be an empty strbuf (STRBUF_INIT) - in that case
trying to read 3 bytes triggers a buffer overflow read (as
query_result.buf = '\0').

Therefore we need to check query_result's length before trying to read 3
bytes.

This overflow was introduced in:
  940b94f35c (fsmonitor: log invocation of FSMonitor hook to trace2, 2021-02-03)
It was found when running the test-suite against ASAN, and can be most
easily reproduced with the following command:

make GIT_TEST_OPTS="-v" DEFAULT_TEST_TARGET="t7519-status-fsmonitor.sh" \
SANITIZE=address DEVELOPER=1 test

==2235==ERROR: AddressSanitizer: global-buffer-overflow on address 0x0000019e6e5e at pc 0x00000043745c bp 0x7fffd382c520 sp 0x7fffd382bcc8
READ of size 3 at 0x0000019e6e5e thread T0
    #0 0x43745b in MemcmpInterceptorCommon(void*, int (*)(void const*, void const*, unsigned long), void const*, void const*, unsigned long) /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:842:7
    #1 0x43786d in bcmp /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:887:10
    #2 0x80b146 in fsmonitor_is_trivial_response /home/ahunt/oss-fuzz/git/fsmonitor.c:192:10
    #3 0x80b146 in query_fsmonitor /home/ahunt/oss-fuzz/git/fsmonitor.c:175:7
    #4 0x80a749 in refresh_fsmonitor /home/ahunt/oss-fuzz/git/fsmonitor.c:267:21
    #5 0x80bad1 in tweak_fsmonitor /home/ahunt/oss-fuzz/git/fsmonitor.c:429:4
    #6 0x90f040 in read_index_from /home/ahunt/oss-fuzz/git/read-cache.c:2321:3
    #7 0x8e5d08 in repo_read_index_preload /home/ahunt/oss-fuzz/git/preload-index.c:164:15
    #8 0x52dd45 in prepare_index /home/ahunt/oss-fuzz/git/builtin/commit.c:363:6
    #9 0x52a188 in cmd_commit /home/ahunt/oss-fuzz/git/builtin/commit.c:1588:15
    #10 0x4ce77e in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #11 0x4ccb18 in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #12 0x4cb01c in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #13 0x4cb01c in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #14 0x6aca8d in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #15 0x7fb027bf5349 in __libc_start_main (/lib64/libc.so.6+0x24349)
    #16 0x4206b9 in _start /home/abuild/rpmbuild/BUILD/glibc-2.26/csu/../sysdeps/x86_64/start.S:120

0x0000019e6e5e is located 2 bytes to the left of global variable 'strbuf_slopbuf' defined in 'strbuf.c:51:6' (0x19e6e60) of size 1
  'strbuf_slopbuf' is ascii string ''
0x0000019e6e5e is located 126 bytes to the right of global variable 'signals' defined in 'sigchain.c:11:31' (0x19e6be0) of size 512
SUMMARY: AddressSanitizer: global-buffer-overflow /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:842:7 in MemcmpInterceptorCommon(void*, int (*)(void const*, void const*, unsigned long), void const*, void const*, unsigned long)
Shadow bytes around the buggy address:
  0x000080334d70: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 00 00 00 00
  0x000080334d80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x000080334d90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x000080334da0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x000080334db0: 00 00 00 00 00 00 00 00 00 00 00 00 f9 f9 f9 f9
=>0x000080334dc0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9[f9]01 f9 f9 f9
  0x000080334dd0: f9 f9 f9 f9 03 f9 f9 f9 f9 f9 f9 f9 02 f9 f9 f9
  0x000080334de0: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9
  0x000080334df0: f9 f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9 00 00 00 00
  0x000080334e00: f9 f9 f9 f9 00 00 00 00 f9 f9 f9 f9 01 f9 f9 f9
  0x000080334e10: f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9
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
  Shadow gap:              cc

Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
---
    fsmonitor: fix overflow read
    
    This patch fixes a buffer overflow read in
    fsmonitor_is_trivial_response().
    
    I'm not super familiar with fsmonitor, so I'm not 100% sure what the
    empty response actually means. Based on my reading of the docs below,
    this can happen with fsmonitor-watchman v1 when no files have changed.
    But it could also happen for v2 if the implementation is broken (in
    which case we also shouldn't overflow)? Either way, I'm guessing the
    empty response doesn't count as trivial:
    https://git-scm.com/docs/githooks#_fsmonitor_watchman
    
    The other question I had is: can watchman V1 return "/\0" as the trivial
    response (as it has no token header) - and should we be recognising that
    too?
    
    ATB,
    
    Andrzej

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-904%2Fahunt%2Fasan-fsmonitor-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-904/ahunt/asan-fsmonitor-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/904

 fsmonitor.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fsmonitor.c b/fsmonitor.c
index 23f8a0c97ebb..ab9bfc60b34e 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -185,10 +185,10 @@ static int query_fsmonitor(int version, const char *last_update, struct strbuf *
 int fsmonitor_is_trivial_response(const struct strbuf *query_result)
 {
 	static char trivial_response[3] = { '\0', '/', '\0' };
-	int is_trivial = !memcmp(trivial_response,
-				 &query_result->buf[query_result->len - 3], 3);
 
-	return is_trivial;
+	return query_result->len >= 3 &&
+		!memcmp(trivial_response,
+			&query_result->buf[query_result->len - 3], 3);
 }
 
 static void fsmonitor_refresh_callback(struct index_state *istate, char *name)

base-commit: 13d7ab6b5d7929825b626f050b62a11241ea4945
-- 
gitgitgadget

Re: [PATCH] fsmonitor: avoid global-buffer-overflow READ when checking trivial response

[Bagas Sanjaya]

Applied the patch and worked as expected (no overflow  and passed the specified test).

Don't forget to add Tested-by trailer:

Tested-by: Bagas Sanjaya <bagasdotme@gmail.com>

On 15/03/21 23.39, Andrzej Hunt via GitGitGadget wrote:
> From: Andrzej Hunt <ajrhunt@google.com>
> 
> query_result can be be an empty strbuf (STRBUF_INIT) - in that case
> trying to read 3 bytes triggers a buffer overflow read (as
> query_result.buf = '\0').
> 
> Therefore we need to check query_result's length before trying to read 3
> bytes.
> 
> This overflow was introduced in:
>    940b94f35c (fsmonitor: log invocation of FSMonitor hook to trace2, 2021-02-03)
> It was found when running the test-suite against ASAN, and can be most
> easily reproduced with the following command:
> 
> make GIT_TEST_OPTS="-v" DEFAULT_TEST_TARGET="t7519-status-fsmonitor.sh" \
> SANITIZE=address DEVELOPER=1 test
> 
> ==2235==ERROR: AddressSanitizer: global-buffer-overflow on address 0x0000019e6e5e at pc 0x00000043745c bp 0x7fffd382c520 sp 0x7fffd382bcc8
> READ of size 3 at 0x0000019e6e5e thread T0
>      #0 0x43745b in MemcmpInterceptorCommon(void*, int (*)(void const*, void const*, unsigned long), void const*, void const*, unsigned long) /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:842:7
>      #1 0x43786d in bcmp /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:887:10
>      #2 0x80b146 in fsmonitor_is_trivial_response /home/ahunt/oss-fuzz/git/fsmonitor.c:192:10
>      #3 0x80b146 in query_fsmonitor /home/ahunt/oss-fuzz/git/fsmonitor.c:175:7
>      #4 0x80a749 in refresh_fsmonitor /home/ahunt/oss-fuzz/git/fsmonitor.c:267:21
>      #5 0x80bad1 in tweak_fsmonitor /home/ahunt/oss-fuzz/git/fsmonitor.c:429:4
>      #6 0x90f040 in read_index_from /home/ahunt/oss-fuzz/git/read-cache.c:2321:3
>      #7 0x8e5d08 in repo_read_index_preload /home/ahunt/oss-fuzz/git/preload-index.c:164:15
>      #8 0x52dd45 in prepare_index /home/ahunt/oss-fuzz/git/builtin/commit.c:363:6
>      #9 0x52a188 in cmd_commit /home/ahunt/oss-fuzz/git/builtin/commit.c:1588:15
>      #10 0x4ce77e in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
>      #11 0x4ccb18 in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
>      #12 0x4cb01c in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
>      #13 0x4cb01c in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
>      #14 0x6aca8d in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
>      #15 0x7fb027bf5349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>      #16 0x4206b9 in _start /home/abuild/rpmbuild/BUILD/glibc-2.26/csu/../sysdeps/x86_64/start.S:120
> 
> 0x0000019e6e5e is located 2 bytes to the left of global variable 'strbuf_slopbuf' defined in 'strbuf.c:51:6' (0x19e6e60) of size 1
>    'strbuf_slopbuf' is ascii string ''
> 0x0000019e6e5e is located 126 bytes to the right of global variable 'signals' defined in 'sigchain.c:11:31' (0x19e6be0) of size 512
> SUMMARY: AddressSanitizer: global-buffer-overflow /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:842:7 in MemcmpInterceptorCommon(void*, int (*)(void const*, void const*, unsigned long), void const*, void const*, unsigned long)
> Shadow bytes around the buggy address:
>    0x000080334d70: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 00 00 00 00
>    0x000080334d80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>    0x000080334d90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>    0x000080334da0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>    0x000080334db0: 00 00 00 00 00 00 00 00 00 00 00 00 f9 f9 f9 f9
> =>0x000080334dc0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9[f9]01 f9 f9 f9
>    0x000080334dd0: f9 f9 f9 f9 03 f9 f9 f9 f9 f9 f9 f9 02 f9 f9 f9
>    0x000080334de0: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9
>    0x000080334df0: f9 f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9 00 00 00 00
>    0x000080334e00: f9 f9 f9 f9 00 00 00 00 f9 f9 f9 f9 01 f9 f9 f9
>    0x000080334e10: f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9
> 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
>    Shadow gap:              cc
> 
> Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
> ---
>      fsmonitor: fix overflow read
>      
>      This patch fixes a buffer overflow read in
>      fsmonitor_is_trivial_response().
>      
>      I'm not super familiar with fsmonitor, so I'm not 100% sure what the
>      empty response actually means. Based on my reading of the docs below,
>      this can happen with fsmonitor-watchman v1 when no files have changed.
>      But it could also happen for v2 if the implementation is broken (in
>      which case we also shouldn't overflow)? Either way, I'm guessing the
>      empty response doesn't count as trivial:
>      https://git-scm.com/docs/githooks#_fsmonitor_watchman
>      
>      The other question I had is: can watchman V1 return "/\0" as the trivial
>      response (as it has no token header) - and should we be recognising that
>      too?
>      
>      ATB,
>      
>      Andrzej
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-904%2Fahunt%2Fasan-fsmonitor-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-904/ahunt/asan-fsmonitor-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/904
> 
>   fsmonitor.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fsmonitor.c b/fsmonitor.c
> index 23f8a0c97ebb..ab9bfc60b34e 100644
> --- a/fsmonitor.c
> +++ b/fsmonitor.c
> @@ -185,10 +185,10 @@ static int query_fsmonitor(int version, const char *last_update, struct strbuf *
>   int fsmonitor_is_trivial_response(const struct strbuf *query_result)
>   {
>   	static char trivial_response[3] = { '\0', '/', '\0' };
> -	int is_trivial = !memcmp(trivial_response,
> -				 &query_result->buf[query_result->len - 3], 3);
>   
> -	return is_trivial;
> +	return query_result->len >= 3 &&
> +		!memcmp(trivial_response,
> +			&query_result->buf[query_result->len - 3], 3);
>   }
>   
>   static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
> 
> base-commit: 13d7ab6b5d7929825b626f050b62a11241ea4945
> 

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH] fsmonitor: avoid global-buffer-overflow READ when checking trivial response

[Jeff Hostetler]

On 3/15/21 12:39 PM, Andrzej Hunt via GitGitGadget wrote:
> From: Andrzej Hunt <ajrhunt@google.com>
> 
> query_result can be be an empty strbuf (STRBUF_INIT) - in that case
> trying to read 3 bytes triggers a buffer overflow read (as
> query_result.buf = '\0').
> 
> Therefore we need to check query_result's length before trying to read 3
> bytes.
> 
> This overflow was introduced in:
>    940b94f35c (fsmonitor: log invocation of FSMonitor hook to trace2, 2021-02-03)
> It was found when running the test-suite against ASAN, and can be most
> easily reproduced with the following command:

[...]

>      fsmonitor: fix overflow read
>      
>      This patch fixes a buffer overflow read in
>      fsmonitor_is_trivial_response().
>      
>      I'm not super familiar with fsmonitor, so I'm not 100% sure what the
>      empty response actually means. Based on my reading of the docs below,
>      this can happen with fsmonitor-watchman v1 when no files have changed.
>      But it could also happen for v2 if the implementation is broken (in
>      which case we also shouldn't overflow)? Either way, I'm guessing the
>      empty response doesn't count as trivial:
>      https://git-scm.com/docs/githooks#_fsmonitor_watchman
>      
>      The other question I had is: can watchman V1 return "/\0" as the trivial
>      response (as it has no token header) - and should we be recognising that
>      too?
>      
>      ATB,
>      
>      Andrzej

[...]

Looks good to me.  And thanks for catching this.

WRT your questions:

An empty response means no files have changed since the last query.
The client can assume all cache-entries are valid and doesn't need
to scan.

A "trivial" response means that the monitor doesn't have enough
information to answer the question. The client should assume that
everything is invalid and do a full scan (as if no monitor were
present).

I added the `fsmonitor_is_trivial_response()` function with the
tracing that I added in [1] in preparation for adding a builtin
fsmonitor service (and currently only my tracing uses that function),
but the concept of a trivial "/" response line has been present since
the initial fsmonitor implementation [2].   See [3] and [4].

[1] 940b94f35c (fsmonitor: log invocation of FSMonitor hook to trace2, 
2021-02-03)
[2] 883e248b8a (fsmonitor: teach git to optionally utilize a file system 
monitor to speed up detecting new or changed files., 2017-09-22)
[3] 
https://github.com/git/git/blob/a5828ae6b52137b913b978e16cd2334482eb4c1f/fsmonitor.c#L304
[4] 
https://github.com/git/git/blob/a5828ae6b52137b913b978e16cd2334482eb4c1f/fsmonitor.c#L320

Thanks again for the review.
Jeff

Re: [PATCH] fsmonitor: avoid global-buffer-overflow READ when checking trivial response

[Junio C Hamano]

Jeff Hostetler <git@jeffhostetler.com> writes:

> On 3/15/21 12:39 PM, Andrzej Hunt via GitGitGadget wrote:
>> From: Andrzej Hunt <ajrhunt@google.com>
>> query_result can be be an empty strbuf (STRBUF_INIT) - in that case
>> trying to read 3 bytes triggers a buffer overflow read (as
>> query_result.buf = '\0').
>> Therefore we need to check query_result's length before trying to
>> read 3
>> bytes.
>> This overflow was introduced in:
>>    940b94f35c (fsmonitor: log invocation of FSMonitor hook to trace2, 2021-02-03)
>> It was found when running the test-suite against ASAN, and can be most
>> easily reproduced with the following command:
> ...
> Looks good to me.  And thanks for catching this.

Thanks, will queue on jh/fsmonitor-prework as a maint-2.31 candidate.