Re: [PATCH 3/3] git_parse_signed(): avoid integer overflow

["René Scharfe" <l.s.r@web.de>]

Am 21.10.22 um 20:31 schrieb Junio C Hamano:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> git_parse_signed() checks that the absolute value of the parsed string
>> is less than or equal to a caller supplied maximum value. When
>> calculating the absolute value there is a integer overflow if `val ==
>> INTMAX_MIN`.
>
> It is a problem that is worth looking into.
>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>  config.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/config.c b/config.c
>> index b7fb68026d8..aad3e00341d 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -1160,8 +1160,10 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
>>  	if (value && *value) {
>>  		char *end;
>>  		intmax_t val;
>> -		uintmax_t uval;
>> -		uintmax_t factor;
>> +		intmax_t factor;
>> +
>> +		if (max < 0)
>> +			BUG("max must be a positive integer");
>
> In parse_signed(), would we expect to accept end-user input that is
> a negative integer?  We must.  Otherwise we would not be calling a
> "signed" parser.  Now, are there cases where the valid value range
> is bounded by a negative integer at the top?  No current callers may
> pass such a value, but is it reasonable to add such a new constraints
> to an existing API function?

Hmm, if minimum and maximum are not symmetric, then we need to supply
both, don't we?

--- >8 ---
Subject: [PATCH] config: let git_parse_signed() check minimum

git_parse_signed() checks that the absolute value of the parsed string
is less than or equal to a caller supplied maximum value.  When
calculating the absolute value there is a integer overflow if `val ==
INTMAX_MIN`.

Avoid overflow during sign inversion by supplying the minimum value to
the function as well.  Make `factor` signed to avoid promoting the
division results in the limit check line to unsigned, but check whether
it's positive.

Add a new macro, minimum_signed_value_of_type, and use it in the callers
of git_parse_signed() to provide the newly required minimum value.  It
calculates the minimum for a given type using division instead of right
shift because the latter is implementation-defined for signed types,
and we need an arithmetic right shift here, not a logical one.

Original-patch-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
git_config_get_expiry_in_days() could use git_parse_int(), but that's
a different topic.

 config.c          | 28 +++++++++++++++++-----------
 git-compat-util.h |  2 ++
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/config.c b/config.c
index cbb5a3bab7..7cf196dc84 100644
--- a/config.c
+++ b/config.c
@@ -1155,26 +1155,24 @@ static uintmax_t get_unit_factor(const char *end)
 	return 0;
 }

-static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
+static int git_parse_signed(const char *value, intmax_t *ret,
+			    intmax_t min, intmax_t max)
 {
 	if (value && *value) {
 		char *end;
 		intmax_t val;
-		uintmax_t uval;
-		uintmax_t factor;
+		intmax_t factor;

 		errno = 0;
 		val = strtoimax(value, &end, 0);
 		if (errno == ERANGE)
 			return 0;
 		factor = get_unit_factor(end);
-		if (!factor) {
+		if (factor < 1) {
 			errno = EINVAL;
 			return 0;
 		}
-		uval = val < 0 ? -val : val;
-		if (unsigned_mult_overflows(factor, uval) ||
-		    factor * uval > max) {
+		if (val < min / factor || val > max / factor) {
 			errno = ERANGE;
 			return 0;
 		}
@@ -1218,7 +1216,9 @@ static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)
 static int git_parse_int(const char *value, int *ret)
 {
 	intmax_t tmp;
-	if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(int)))
+	if (!git_parse_signed(value, &tmp,
+			      minimum_signed_value_of_type(int),
+			      maximum_signed_value_of_type(int)))
 		return 0;
 	*ret = tmp;
 	return 1;
@@ -1227,7 +1227,9 @@ static int git_parse_int(const char *value, int *ret)
 static int git_parse_int64(const char *value, int64_t *ret)
 {
 	intmax_t tmp;
-	if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(int64_t)))
+	if (!git_parse_signed(value, &tmp,
+			      minimum_signed_value_of_type(int64_t),
+			      maximum_signed_value_of_type(int64_t)))
 		return 0;
 	*ret = tmp;
 	return 1;
@@ -1245,7 +1247,9 @@ int git_parse_ulong(const char *value, unsigned long *ret)
 int git_parse_ssize_t(const char *value, ssize_t *ret)
 {
 	intmax_t tmp;
-	if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(ssize_t)))
+	if (!git_parse_signed(value, &tmp,
+			      minimum_signed_value_of_type(ssize_t),
+			      maximum_signed_value_of_type(ssize_t)))
 		return 0;
 	*ret = tmp;
 	return 1;
@@ -2751,7 +2755,9 @@ int git_config_get_expiry_in_days(const char *key, timestamp_t *expiry, timestam
 	if (git_config_get_string_tmp(key, &expiry_string))
 		return 1; /* no such thing */

-	if (git_parse_signed(expiry_string, &days, maximum_signed_value_of_type(int))) {
+	if (git_parse_signed(expiry_string, &days,
+			     minimum_signed_value_of_type(int),
+			     maximum_signed_value_of_type(int))) {
 		const int scale = 86400;
 		*expiry = now - days * scale;
 		return 0;
diff --git a/git-compat-util.h b/git-compat-util.h
index ea53ea4a78..35425c373b 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -112,6 +112,8 @@ struct strbuf;

 #define bitsizeof(x)  (CHAR_BIT * sizeof(x))

+#define minimum_signed_value_of_type(a) \
+    (INTMAX_MIN / ((intmax_t)1 << (bitsizeof(intmax_t) - bitsizeof(a))))
 #define maximum_signed_value_of_type(a) \
     (INTMAX_MAX >> (bitsizeof(intmax_t) - bitsizeof(a)))

--
2.38.1