@matheusbsbs / matheus-bagliano / commits / 59a255aef0

sideband: do not read beyond the end of input

The caller of maybe_colorize_sideband() gives a counted buffer <src, n>, but the callee checked src[] as if it were a NUL terminated buffer. If src[] had all isspace() bytes in it, we would have made n negative, and then (1) made number of strncasecmp() calls to see if the remaining bytes in src[] matched keywords, reading beyond the end of the array (this actually happens even if n does not go negative), and/or (2) called strbuf_add() with negative count, most likely triggering the "you want to use way too much memory" error due to unsigned integer overflow. Fix both issues by making sure we do not go beyond &src[n]. In the longer term we may want to accept size_t as parameter for clarity (even though we know that a sideband message we are painting typically would fit on a line on a terminal and int is sufficient). Write it down as a NEEDSWORK comment. Helped-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Junio C Hamano committed Aug 18, 2018 at 09:16 UTC 59a255aef05633c45c780987fa0c861cda9006f2

2 files changed +20 -2

sideband.c

+6 -2

@@ -65,6 +65,8 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref

65 * Optionally highlight one keyword in remote output if it appears at the start

66 * of the line. This should be called for a single line only, which is

67 * passed as the first N characters of the SRC array.

68 + *

69 + * NEEDSWORK: use "size_t n" instead for clarity.

70 */

71 static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)

72 {

@@ -75,7 +77,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)

77 return;

78 }

78 - while (isspace(*src)) {

80 + while (0 < n && isspace(*src)) {

81 strbuf_addch(dest, *src);

82 src++;

83 n--;

@@ -84,6 +86,9 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)

86 for (i = 0; i < ARRAY_SIZE(keywords); i++) {

87 struct keyword_entry *p = keywords + i;

88 int len = strlen(p->keyword);

89 +

90 + if (n <= len)

91 + continue;

92 /*

93 * Match case insensitively, so we colorize output from existing

94 * servers regardless of the case that they use for their

@@ -101,7 +106,6 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)

106 }

107

108 strbuf_add(dest, src, n);

104 -

109 }

110

111

t/t5409-colorize-remote-messages.sh

+14

@@ -15,6 +15,8 @@ test_expect_success 'setup' '

15 echo warning: warning

16 echo prefixerror: error

17 echo " " "error: leading space"

18 + echo " "

19 + echo Err

20 exit 0

21 EOF

22 echo 1 >file &&

@@ -44,6 +46,12 @@ test_expect_success 'whole words at line start' '

46 grep "prefixerror: error" decoded

47 '

49 +test_expect_success 'short line' '

50 + git -C child -c color.remote=always push -f origin HEAD:short-line 2>output &&

51 + test_decode_color <output >decoded &&

52 + grep "remote: Err" decoded

53 +'

54 +

55 test_expect_success 'case-insensitive' '

56 git --git-dir child/.git -c color.remote=always push -f origin HEAD:refs/heads/case-insensitive 2>output &&

57 cat output &&

@@ -58,6 +66,12 @@ test_expect_success 'leading space' '

66 grep " <BOLD;RED>error<RESET>: leading space" decoded

67 '

69 +test_expect_success 'spaces only' '

70 + git -C child -c color.remote=always push -f origin HEAD:only-space 2>output &&

71 + test_decode_color <output >decoded &&

72 + grep "remote: " decoded

73 +'

74 +

75 test_expect_success 'no coloring for redirected output' '

76 git --git-dir child/.git push -f origin HEAD:refs/heads/redirected-output 2>output &&

77 test_decode_color <output >decoded &&