From 7f2e61f3ee5a15b7232a898f2464fbd7cc23aef2 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Sun, 19 Apr 2015 23:55:02 -0400 Subject: [PATCH 1/2] config_file: parse multilines generously Combine unquoting and multiline detection to avoid ambiguity when parsing. --- src/config_file.c | 92 +++++++++++++++++++-------------------------- tests/config/read.c | 40 +++++++++++++++++--- 2 files changed, 72 insertions(+), 60 deletions(-) diff --git a/src/config_file.c b/src/config_file.c index 8a2c88cd2..533a92bdd 100644 --- a/src/config_file.c +++ b/src/config_file.c @@ -1619,76 +1619,69 @@ static char *escape_value(const char *ptr) } /* '\"' -> '"' etc */ -static char *fixup_line(const char *ptr, int quote_count) +static int unescape_line( + char **out, bool *is_multi, const char *ptr, int quote_count) { - char *str, *out, *esc; + char *str, *fixed, *esc; size_t ptr_len = strlen(ptr), alloc_len; + *is_multi = false; + if (GIT_ADD_SIZET_OVERFLOW(&alloc_len, ptr_len, 1) || (str = git__malloc(alloc_len)) == NULL) { - return NULL; + return -1; } - out = str; + fixed = str; while (*ptr != '\0') { if (*ptr == '"') { quote_count++; } else if (*ptr != '\\') { - *out++ = *ptr; + *fixed++ = *ptr; } else { /* backslash, check the next char */ ptr++; /* if we're at the end, it's a multiline, so keep the backslash */ if (*ptr == '\0') { - *out++ = '\\'; - goto out; + *is_multi = true; + goto done; } if ((esc = strchr(escapes, *ptr)) != NULL) { - *out++ = escaped[esc - escapes]; + *fixed++ = escaped[esc - escapes]; } else { git__free(str); giterr_set(GITERR_CONFIG, "Invalid escape at %s", ptr); - return NULL; + return -1; } } ptr++; } -out: - *out = '\0'; +done: + *fixed = '\0'; + *out = str; - return str; -} - -static int is_multiline_var(const char *str) -{ - int count = 0; - const char *end = str + strlen(str); - while (end > str && end[-1] == '\\') { - count++; - end--; - } - - /* An odd number means last backslash wasn't escaped, so it's multiline */ - return count & 1; + return 0; } static int parse_multiline_variable(struct reader *reader, git_buf *value, int in_quotes) { char *line = NULL, *proc_line = NULL; int quote_count; + bool multiline; /* Check that the next line exists */ line = reader_readline(reader, false); if (line == NULL) return -1; - /* We've reached the end of the file, there is input missing */ + /* We've reached the end of the file, there is no continuation. + * (this is not an error). + */ if (line[0] == '\0') { - set_parse_error(reader, 0, "Unexpected end of file while parsing multine var"); git__free(line); - return -1; + return 0; } quote_count = strip_comments(line, !!in_quotes); @@ -1700,14 +1693,7 @@ static int parse_multiline_variable(struct reader *reader, git_buf *value, int i /* TODO: unbounded recursion. This **could** be exploitable */ } - /* Drop the continuation character '\': to closely follow the UNIX - * standard, this character **has** to be last one in the buf, with - * no whitespace after it */ - assert(is_multiline_var(value->ptr)); - git_buf_shorten(value, 1); - - proc_line = fixup_line(line, in_quotes); - if (proc_line == NULL) { + if (unescape_line(&proc_line, &multiline, line, in_quotes) < 0) { git__free(line); return -1; } @@ -1720,7 +1706,7 @@ static int parse_multiline_variable(struct reader *reader, git_buf *value, int i * If we need to continue reading the next line, let's just * keep putting stuff in the buffer */ - if (is_multiline_var(value->ptr)) + if (multiline) return parse_multiline_variable(reader, value, quote_count); return 0; @@ -1732,6 +1718,7 @@ static int parse_variable(struct reader *reader, char **var_name, char **var_val const char *value_start = NULL; char *line; int quote_count; + bool multiline; line = reader_readline(reader, true); if (line == NULL) @@ -1762,31 +1749,28 @@ static int parse_variable(struct reader *reader, char **var_name, char **var_val while (git__isspace(value_start[0])) value_start++; - if (is_multiline_var(value_start)) { + if (unescape_line(var_value, &multiline, value_start, 0) < 0) + goto on_error; + + if (multiline) { git_buf multi_value = GIT_BUF_INIT; - char *proc_line = fixup_line(value_start, 0); - GITERR_CHECK_ALLOC(proc_line); - git_buf_puts(&multi_value, proc_line); - git__free(proc_line); - if (parse_multiline_variable(reader, &multi_value, quote_count) < 0 || git_buf_oom(&multi_value)) { - git__free(*var_name); - git__free(line); + git_buf_attach(&multi_value, *var_value, 0); + + if (parse_multiline_variable(reader, &multi_value, quote_count) < 0 || + git_buf_oom(&multi_value)) { git_buf_free(&multi_value); - return -1; + goto on_error; } *var_value = git_buf_detach(&multi_value); - - } - else if (value_start[0] != '\0') { - *var_value = fixup_line(value_start, 0); - GITERR_CHECK_ALLOC(*var_value); - } else { /* equals sign but missing rhs */ - *var_value = git__strdup(""); - GITERR_CHECK_ALLOC(*var_value); } } git__free(line); return 0; + +on_error: + git__free(*var_name); + git__free(line); + return -1; } diff --git a/tests/config/read.c b/tests/config/read.c index 6512fcbfa..f20a3769f 100644 --- a/tests/config/read.c +++ b/tests/config/read.c @@ -69,6 +69,40 @@ void test_config_read__multiline_value(void) git_config_free(cfg); } +static void clean_test_config(void *unused) +{ + GIT_UNUSED(unused); + cl_fixture_cleanup("./testconfig"); +} + +void test_config_read__multiline_value_and_eof(void) +{ + git_config *cfg; + + cl_set_cleanup(&clean_test_config, NULL); + cl_git_mkfile("./testconfig", "[header]\n key1 = foo\\\n"); + cl_git_pass(git_config_open_ondisk(&cfg, "./testconfig")); + + cl_git_pass(git_config_get_string_buf(&buf, cfg, "header.key1")); + cl_assert_equal_s("foo", git_buf_cstr(&buf)); + + git_config_free(cfg); +} + +void test_config_read__multiline_eof(void) +{ + git_config *cfg; + + cl_set_cleanup(&clean_test_config, NULL); + cl_git_mkfile("./testconfig", "[header]\n key1 = \\\n"); + cl_git_pass(git_config_open_ondisk(&cfg, "./testconfig")); + + cl_git_pass(git_config_get_string_buf(&buf, cfg, "header.key1")); + cl_assert_equal_s("", git_buf_cstr(&buf)); + + git_config_free(cfg); +} + /* * This kind of subsection declaration is case-insensitive */ @@ -520,12 +554,6 @@ void test_config_read__simple_read_from_specific_level(void) git_config_free(cfg); } -static void clean_test_config(void *unused) -{ - GIT_UNUSED(unused); - cl_fixture_cleanup("./testconfig"); -} - void test_config_read__can_load_and_parse_an_empty_config_file(void) { git_config *cfg; From e009a7059d69173113122eb2d28de8a03083b201 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Mon, 20 Apr 2015 00:22:20 -0400 Subject: [PATCH 2/2] config_file: comment char can be invalid escape Don't assume that comment chars are comment chars, they may be (an attempt to be escaped). If so, \; is not a valid escape sequence, complain. --- src/config_file.c | 12 ++++++++++-- tests/config/read.c | 11 +++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/config_file.c b/src/config_file.c index 533a92bdd..350473434 100644 --- a/src/config_file.c +++ b/src/config_file.c @@ -1162,17 +1162,24 @@ static int skip_bom(struct reader *reader) static int strip_comments(char *line, int in_quotes) { - int quote_count = in_quotes; + int quote_count = in_quotes, backslash_count = 0; char *ptr; for (ptr = line; *ptr; ++ptr) { if (ptr[0] == '"' && ptr > line && ptr[-1] != '\\') quote_count++; - if ((ptr[0] == ';' || ptr[0] == '#') && (quote_count % 2) == 0) { + if ((ptr[0] == ';' || ptr[0] == '#') && + (quote_count % 2) == 0 && + (backslash_count % 2) == 0) { ptr[0] = '\0'; break; } + + if (ptr[0] == '\\') + backslash_count++; + else + backslash_count = 0; } /* skip any space at the end */ @@ -1698,6 +1705,7 @@ static int parse_multiline_variable(struct reader *reader, git_buf *value, int i return -1; } /* add this line to the multiline var */ + git_buf_puts(value, proc_line); git__free(line); git__free(proc_line); diff --git a/tests/config/read.c b/tests/config/read.c index f20a3769f..a19bc7dcb 100644 --- a/tests/config/read.c +++ b/tests/config/read.c @@ -247,6 +247,17 @@ void test_config_read__escaping_quotes(void) git_config_free(cfg); } +void test_config_read__invalid_escape_sequence(void) +{ + git_config *cfg; + + cl_set_cleanup(&clean_test_config, NULL); + cl_git_mkfile("./testconfig", "[header]\n key1 = \\\\\\;\n key2 = value2\n"); + cl_git_fail(git_config_open_ondisk(&cfg, "./testconfig")); + + git_config_free(cfg); +} + static int count_cfg_entries_and_compare_levels( const git_config_entry *entry, void *payload) {