From 97bbe68363ccf2de0c07f67170ec64a8b4d62592 Mon Sep 17 00:00:00 2001 From: TJ Saunders Date: Sun, 6 Aug 2023 13:16:26 -0700 Subject: [PATCH] Issue #1683: Avoid an edge case when handling unexpectedly formatted input text from client, caused by quote/backslash semantics, by skipping those semantics. Upstream-Status: Backport [https://github.com/proftpd/proftpd/commit/97bbe68363ccf2de0c07f67170ec64a8b4d62592] CVE: CVE-2023-51713 Signed-off-by: Hitendra Prajapati --- include/str.h | 3 ++- src/main.c | 35 +++++++++++++++++++++++++++++----- src/str.c | 22 +++++++++++++--------- tests/api/str.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 94 insertions(+), 16 deletions(-) diff --git a/include/str.h b/include/str.h index 316a32a..049a1b2 100644 --- a/include/str.h +++ b/include/str.h @@ -1,6 +1,6 @@ /* * ProFTPD - FTP server daemon - * Copyright (c) 2008-2017 The ProFTPD Project team + * Copyright (c) 2008-2023 The ProFTPD Project team * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -121,6 +121,7 @@ const char *pr_gid2str(pool *, gid_t); #define PR_STR_FL_PRESERVE_COMMENTS 0x0001 #define PR_STR_FL_PRESERVE_WHITESPACE 0x0002 #define PR_STR_FL_IGNORE_CASE 0x0004 +#define PR_STR_FL_IGNORE_QUOTES 0x0008 char *pr_str_get_token(char **, char *); char *pr_str_get_token2(char **, char *, size_t *); diff --git a/src/main.c b/src/main.c index 1ead27f..01b1ef8 100644 --- a/src/main.c +++ b/src/main.c @@ -787,8 +787,24 @@ static cmd_rec *make_ftp_cmd(pool *p, char *buf, size_t buflen, int flags) { return NULL; } + /* By default, pr_str_get_word will handle quotes and backslashes for + * escaping characters. This can produce words which are shorter, use + * fewer bytes than the corresponding input buffer. + * + * In this particular situation, we use the length of this initial word + * for determining the length of the remaining buffer bytes, assumed to + * contain the FTP command arguments. If this initial word is thus + * unexpectedly "shorter", due to nonconformant FTP text, it can lead + * the subsequent buffer scan, looking for CRNUL sequencees, to access + * unexpected memory addresses (Issue #1683). + * + * Thus for this particular situation, we tell the function to ignore/skip + * such quote/backslash semantics, and treat them as any other character + * using the IGNORE_QUOTES flag. + */ + ptr = buf; - wrd = pr_str_get_word(&ptr, str_flags); + wrd = pr_str_get_word(&ptr, str_flags|PR_STR_FL_IGNORE_QUOTES); if (wrd == NULL) { /* Nothing there...bail out. */ pr_trace_msg("ctrl", 5, "command '%s' is empty, ignoring", buf); @@ -796,6 +812,11 @@ static cmd_rec *make_ftp_cmd(pool *p, char *buf, size_t buflen, int flags) { return NULL; } + /* Note that this first word is the FTP command. This is why we make + * use of the ptr buffer, which advances through the input buffer as + * we read words from the buffer. + */ + subpool = make_sub_pool(p); pr_pool_tag(subpool, "make_ftp_cmd pool"); cmd = pcalloc(subpool, sizeof(cmd_rec)); @@ -822,6 +843,7 @@ static cmd_rec *make_ftp_cmd(pool *p, char *buf, size_t buflen, int flags) { arg_len = buflen - strlen(wrd); arg = pcalloc(cmd->pool, arg_len + 1); + /* Remember that ptr here is advanced past the first word. */ for (i = 0, j = 0; i < arg_len; i++) { pr_signals_handle(); if (i > 1 && @@ -830,15 +852,13 @@ static cmd_rec *make_ftp_cmd(pool *p, char *buf, size_t buflen, int flags) { /* Strip out the NUL by simply not copying it into the new buffer. */ have_crnul = TRUE; - + } else { arg[j++] = ptr[i]; } } - cmd->arg = arg; - - if (have_crnul) { + if (have_crnul == TRUE) { char *dup_arg; /* Now make a copy of the stripped argument; this is what we need to @@ -848,6 +868,11 @@ static cmd_rec *make_ftp_cmd(pool *p, char *buf, size_t buflen, int flags) { ptr = dup_arg; } + cmd->arg = arg; + + /* Now we can read the remamining words, as command arguments, from the + * input buffer. + */ while ((wrd = pr_str_get_word(&ptr, str_flags)) != NULL) { pr_signals_handle(); *((char **) push_array(tarr)) = pstrdup(cmd->pool, wrd); diff --git a/src/str.c b/src/str.c index eeed096..04188ce 100644 --- a/src/str.c +++ b/src/str.c @@ -1,6 +1,6 @@ /* * ProFTPD - FTP server daemon - * Copyright (c) 2008-2017 The ProFTPD Project team + * Copyright (c) 2008-2023 The ProFTPD Project team * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -1209,7 +1209,7 @@ int pr_str_get_nbytes(const char *str, const char *units, off_t *nbytes) { char *pr_str_get_word(char **cp, int flags) { char *res, *dst; - char quote_mode = 0; + int quote_mode = FALSE; if (cp == NULL || !*cp || @@ -1238,24 +1238,28 @@ char *pr_str_get_word(char **cp, int flags) { } } - if (**cp == '\"') { - quote_mode++; - (*cp)++; + if (!(flags & PR_STR_FL_IGNORE_QUOTES)) { + if (**cp == '\"') { + quote_mode = TRUE; + (*cp)++; + } } while (**cp && (quote_mode ? (**cp != '\"') : !PR_ISSPACE(**cp))) { pr_signals_handle(); - if (**cp == '\\' && quote_mode) { - + if (**cp == '\\' && + quote_mode == TRUE) { /* Escaped char */ if (*((*cp)+1)) { - *dst = *(++(*cp)); + *dst++ = *(++(*cp)); + (*cp)++; + continue; } } *dst++ = **cp; - ++(*cp); + (*cp)++; } if (**cp) { diff --git a/tests/api/str.c b/tests/api/str.c index 7c6e110..77fda8f 100644 --- a/tests/api/str.c +++ b/tests/api/str.c @@ -1,6 +1,6 @@ /* * ProFTPD - FTP server testsuite - * Copyright (c) 2008-2017 The ProFTPD Project team + * Copyright (c) 2008-2023 The ProFTPD Project team * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -695,19 +695,23 @@ END_TEST START_TEST (get_word_test) { char *ok, *res, *str; + mark_point(); res = pr_str_get_word(NULL, 0); fail_unless(res == NULL, "Failed to handle null arguments"); fail_unless(errno == EINVAL, "Failed to set errno to EINVAL"); + mark_point(); str = NULL; res = pr_str_get_word(&str, 0); fail_unless(res == NULL, "Failed to handle null str argument"); fail_unless(errno == EINVAL, "Failed to set errno to EINVAL"); + mark_point(); str = pstrdup(p, " "); res = pr_str_get_word(&str, 0); fail_unless(res == NULL, "Failed to handle whitespace argument"); + mark_point(); str = pstrdup(p, " foo"); res = pr_str_get_word(&str, PR_STR_FL_PRESERVE_WHITESPACE); fail_unless(res != NULL, "Failed to handle whitespace argument: %s", @@ -723,6 +727,7 @@ START_TEST (get_word_test) { ok = "foo"; fail_unless(strcmp(res, ok) == 0, "Expected '%s', got '%s'", ok, res); + mark_point(); str = pstrdup(p, " # foo"); res = pr_str_get_word(&str, 0); fail_unless(res == NULL, "Failed to handle commented argument"); @@ -742,6 +747,8 @@ START_TEST (get_word_test) { fail_unless(strcmp(res, ok) == 0, "Expected '%s', got '%s'", ok, res); /* Test multiple embedded quotes. */ + + mark_point(); str = pstrdup(p, "foo \"bar baz\" qux \"quz norf\""); res = pr_str_get_word(&str, 0); fail_unless(res != NULL, "Failed to handle quoted argument: %s", @@ -770,6 +777,47 @@ START_TEST (get_word_test) { ok = "quz norf"; fail_unless(strcmp(res, ok) == 0, "Expected '%s', got '%s'", ok, res); + + + /* Test embedded quotes with backslashes (Issue #1683). */ + mark_point(); + + str = pstrdup(p, "\"\\\\SYST\""); + res = pr_str_get_word(&str, 0); + fail_unless(res != NULL, "Failed to handle quoted argument: %s", + strerror(errno)); + + ok = "\\SYST"; + fail_unless(strcmp(res, ok) == 0, "Expected '%s', got '%s'", ok, res); + + mark_point(); + str = pstrdup(p, "\"\"\\\\SYST"); + res = pr_str_get_word(&str, 0); + fail_unless(res != NULL, "Failed to handle quoted argument: %s", + strerror(errno)); + + /* Note that pr_str_get_word() is intended to be called multiple times + * on an advancing buffer, effectively tokenizing the buffer. This is + * why the function does NOT decrement its quote mode. + */ + ok = ""; + fail_unless(strcmp(res, ok) == 0, "Expected '%s', got '%s'", ok, res); + + /* Now do the same tests with the IGNORE_QUOTES flag */ + mark_point(); + + str = ok = pstrdup(p, "\"\\\\SYST\""); + res = pr_str_get_word(&str, PR_STR_FL_IGNORE_QUOTES); + fail_unless(res != NULL, "Failed to handle quoted argument: %s", + strerror(errno)); + fail_unless(strcmp(res, ok) == 0, "Expected '%s', got '%s'", ok, res); + + mark_point(); + str = ok = pstrdup(p, "\"\"\\\\SYST"); + res = pr_str_get_word(&str, PR_STR_FL_IGNORE_QUOTES); + fail_unless(res != NULL, "Failed to handle quoted argument: %s", + strerror(errno)); + fail_unless(strcmp(res, ok) == 0, "Expected '%s', got '%s'", ok, res); } END_TEST -- 2.25.1