From: "Andrzej Hunt via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Jeff Hostetler <jeffhost@microsoft.com>,
Andrzej Hunt <andrzej@ahunt.org>,
Andrzej Hunt <ajrhunt@google.com>
Subject: [PATCH] fsmonitor: avoid global-buffer-overflow READ when checking trivial response
Date: Mon, 15 Mar 2021 16:39:23 +0000 [thread overview]
Message-ID: <pull.904.git.1615826363431.gitgitgadget@gmail.com> (raw)
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
next reply other threads:[~2021-03-15 16:40 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-15 16:39 Andrzej Hunt via GitGitGadget [this message]
2021-03-16 10:55 ` [PATCH] fsmonitor: avoid global-buffer-overflow READ when checking trivial response Bagas Sanjaya
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=pull.904.git.1615826363431.gitgitgadget@gmail.com \
--to=gitgitgadget@gmail.com \
--cc=ajrhunt@google.com \
--cc=andrzej@ahunt.org \
--cc=git@vger.kernel.org \
--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).