* [PATCH] fsmonitor: avoid global-buffer-overflow READ when checking trivial response
@ 2021-03-15 16:39 Andrzej Hunt via GitGitGadget
2021-03-16 10:55 ` Bagas Sanjaya
2021-03-16 14:20 ` Jeff Hostetler
0 siblings, 2 replies; 4+ messages in thread
From: Andrzej Hunt via GitGitGadget @ 2021-03-15 16:39 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Andrzej Hunt, Andrzej Hunt
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] fsmonitor: avoid global-buffer-overflow READ when checking trivial response
2021-03-15 16:39 [PATCH] fsmonitor: avoid global-buffer-overflow READ when checking trivial response Andrzej Hunt via GitGitGadget
@ 2021-03-16 10:55 ` Bagas Sanjaya
2021-03-16 14:20 ` Jeff Hostetler
1 sibling, 0 replies; 4+ messages in thread
From: Bagas Sanjaya @ 2021-03-16 10:55 UTC (permalink / raw)
To: Andrzej Hunt via GitGitGadget
Cc: Jeff Hostetler, Andrzej Hunt, Andrzej Hunt, git
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] fsmonitor: avoid global-buffer-overflow READ when checking trivial response
2021-03-15 16:39 [PATCH] fsmonitor: avoid global-buffer-overflow READ when checking trivial response Andrzej Hunt via GitGitGadget
2021-03-16 10:55 ` Bagas Sanjaya
@ 2021-03-16 14:20 ` Jeff Hostetler
2021-03-17 17:10 ` Junio C Hamano
1 sibling, 1 reply; 4+ messages in thread
From: Jeff Hostetler @ 2021-03-16 14:20 UTC (permalink / raw)
To: Andrzej Hunt via GitGitGadget, git
Cc: Jeff Hostetler, Andrzej Hunt, Andrzej Hunt
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] fsmonitor: avoid global-buffer-overflow READ when checking trivial response
2021-03-16 14:20 ` Jeff Hostetler
@ 2021-03-17 17:10 ` Junio C Hamano
0 siblings, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2021-03-17 17:10 UTC (permalink / raw)
To: Jeff Hostetler
Cc: Andrzej Hunt via GitGitGadget, git, Jeff Hostetler, Andrzej Hunt,
Andrzej Hunt
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-03-17 17:11 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-15 16:39 [PATCH] fsmonitor: avoid global-buffer-overflow READ when checking trivial response Andrzej Hunt via GitGitGadget
2021-03-16 10:55 ` Bagas Sanjaya
2021-03-16 14:20 ` Jeff Hostetler
2021-03-17 17:10 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).