From: Bagas Sanjaya <bagasdotme@gmail.com>
To: Andrzej Hunt via GitGitGadget <gitgitgadget@gmail.com>
Cc: Jeff Hostetler <jeffhost@microsoft.com>,
Andrzej Hunt <andrzej@ahunt.org>,
Andrzej Hunt <ajrhunt@google.com>,
git@vger.kernel.org
Subject: Re: [PATCH] fsmonitor: avoid global-buffer-overflow READ when checking trivial response
Date: Tue, 16 Mar 2021 17:55:19 +0700 [thread overview]
Message-ID: <27cc2d6b-834f-e34b-0949-561b575544da@gmail.com> (raw)
In-Reply-To: <pull.904.git.1615826363431.gitgitgadget@gmail.com>
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
next prev parent reply other threads:[~2021-03-16 10:56 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2021-03-16 14:20 ` Jeff Hostetler
2021-03-17 17:10 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=27cc2d6b-834f-e34b-0949-561b575544da@gmail.com \
--to=bagasdotme@gmail.com \
--cc=ajrhunt@google.com \
--cc=andrzej@ahunt.org \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=jeffhost@microsoft.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).