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

From: Andrzej Hunt <[email protected]>

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 <[email protected]>

---
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(-)

fsmonitor.c +3 -3

index 23f8a0c97ebb..ab9bfc60b34e 100644
@@ -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

From: Andrzej Hunt <[email protected]>

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

Quoted 11 lines · Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>
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.