86 lines
2.9 KiB
Diff
86 lines
2.9 KiB
Diff
From 0ca6ead81edd4fb1984b69aae87c1189e3025530 Mon Sep 17 00:00:00 2001
|
|
From: Kevin Backhouse <kevinbackhouse@github.com>
|
|
Date: Wed, 28 Sep 2022 18:53:32 -0400
|
|
Subject: [PATCH] alias.c: reject too-long cmdline strings in split_cmdline()
|
|
|
|
This function improperly uses an int to represent the number of entries
|
|
in the resulting argument array. This allows a malicious actor to
|
|
intentionally overflow the return value, leading to arbitrary heap
|
|
writes.
|
|
|
|
Because the resulting argv array is typically passed to execv(), it may
|
|
be possible to leverage this attack to gain remote code execution on a
|
|
victim machine. This was almost certainly the case for certain
|
|
configurations of git-shell until the previous commit limited the size
|
|
of input it would accept. Other calls to split_cmdline() are typically
|
|
limited by the size of argv the OS is willing to hand us, so are
|
|
similarly protected.
|
|
|
|
So this is not strictly fixing a known vulnerability, but is a hardening
|
|
of the function that is worth doing to protect against possible unknown
|
|
vulnerabilities.
|
|
|
|
One approach to fixing this would be modifying the signature of
|
|
`split_cmdline()` to look something like:
|
|
|
|
int split_cmdline(char *cmdline, const char ***argv, size_t *argc);
|
|
|
|
Where the return value of `split_cmdline()` is negative for errors, and
|
|
zero otherwise. If non-NULL, the `*argc` pointer is modified to contain
|
|
the size of the `**argv` array.
|
|
|
|
But this implies an absurdly large `argv` array, which more than likely
|
|
larger than the system's argument limit. So even if split_cmdline()
|
|
allowed this, it would fail immediately afterwards when we called
|
|
execv(). So instead of converting all of `split_cmdline()`'s callers to
|
|
work with `size_t` types in this patch, instead pursue the minimal fix
|
|
here to prevent ever returning an array with more than INT_MAX entries
|
|
in it.
|
|
|
|
Signed-off-by: Kevin Backhouse <kevinbackhouse@github.com>
|
|
Signed-off-by: Taylor Blau <me@ttaylorr.com>
|
|
Signed-off-by: Jeff King <peff@peff.net>
|
|
Signed-off-by: Taylor Blau <me@ttaylorr.com>
|
|
---
|
|
alias.c | 11 +++++++++--
|
|
1 file changed, 9 insertions(+), 2 deletions(-)
|
|
|
|
diff --git a/alias.c b/alias.c
|
|
index c471538020..00abde0817 100644
|
|
--- a/alias.c
|
|
+++ b/alias.c
|
|
@@ -46,14 +46,16 @@ void list_aliases(struct string_list *list)
|
|
|
|
#define SPLIT_CMDLINE_BAD_ENDING 1
|
|
#define SPLIT_CMDLINE_UNCLOSED_QUOTE 2
|
|
+#define SPLIT_CMDLINE_ARGC_OVERFLOW 3
|
|
static const char *split_cmdline_errors[] = {
|
|
N_("cmdline ends with \\"),
|
|
- N_("unclosed quote")
|
|
+ N_("unclosed quote"),
|
|
+ N_("too many arguments"),
|
|
};
|
|
|
|
int split_cmdline(char *cmdline, const char ***argv)
|
|
{
|
|
- int src, dst, count = 0, size = 16;
|
|
+ size_t src, dst, count = 0, size = 16;
|
|
char quoted = 0;
|
|
|
|
ALLOC_ARRAY(*argv, size);
|
|
@@ -96,6 +98,11 @@ int split_cmdline(char *cmdline, const char ***argv)
|
|
return -SPLIT_CMDLINE_UNCLOSED_QUOTE;
|
|
}
|
|
|
|
+ if (count >= INT_MAX) {
|
|
+ FREE_AND_NULL(*argv);
|
|
+ return -SPLIT_CMDLINE_ARGC_OVERFLOW;
|
|
+ }
|
|
+
|
|
ALLOC_GROW(*argv, count + 1, size);
|
|
(*argv)[count] = NULL;
|
|
|
|
--
|
|
2.27.0
|
|
|