From 09340a25e6f303e7681fb9b8b56d1055eb194026 Mon Sep 17 00:00:00 2001 From: Ivan Lazarev Date: Thu, 16 Jun 2022 00:57:14 +0300 Subject: [PATCH 1/6] [PBCKP-165] get_control_value() int64 buffer vulnerability fix --- src/dir.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/dir.c b/src/dir.c index 4ebe0939b..afdc5c3dd 100644 --- a/src/dir.c +++ b/src/dir.c @@ -1550,11 +1550,6 @@ get_control_value(const char *str, const char *name, } else if (value_int64) { - /* Length of buf_uint64 should not be greater than 31 */ - if (buf_int64_ptr - buf_int64 >= 32) - elog(ERROR, "field \"%s\" is out of range in the line %s of the file %s", - name, str, DATABASE_FILE_LIST); - *buf_int64_ptr = '\0'; if (!parse_int64(buf_int64, value_int64, 0)) { @@ -1577,8 +1572,16 @@ get_control_value(const char *str, const char *name, } else { - *buf_int64_ptr = *buf; - buf_int64_ptr++; + /* verify if buf_int64 not exceeds size limits */ + if (buf_int64_ptr - buf_int64 < sizeof buf_int64) { + *buf_int64_ptr = *buf; + buf_int64_ptr++; + } + else + { + elog(ERROR, "field \"%s\" is out of range in the line %s of the file %s", + name, str, DATABASE_FILE_LIST); + } } } break; From 0c6e5e9865ac259fdbc72e2e8d5723d746435c10 Mon Sep 17 00:00:00 2001 From: Ivan Lazarev Date: Thu, 16 Jun 2022 20:06:39 +0300 Subject: [PATCH 2/6] [PBCKP-145] after-review fix --- src/dir.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/dir.c b/src/dir.c index afdc5c3dd..73f83c39a 100644 --- a/src/dir.c +++ b/src/dir.c @@ -1572,16 +1572,13 @@ get_control_value(const char *str, const char *name, } else { - /* verify if buf_int64 not exceeds size limits */ - if (buf_int64_ptr - buf_int64 < sizeof buf_int64) { - *buf_int64_ptr = *buf; - buf_int64_ptr++; - } - else - { - elog(ERROR, "field \"%s\" is out of range in the line %s of the file %s", - name, str, DATABASE_FILE_LIST); - } + /* verify if buf_int64 not exceeds size limits */ + if (buf_int64_ptr - buf_int64 >= sizeof buf_int64 - 1) { + elog(ERROR, "field \"%s\" is out of range in the line %s of the file %s", + name, str, DATABASE_FILE_LIST); + } + *buf_int64_ptr = *buf; + buf_int64_ptr++; } } break; From 9d2b827c5281cf6c7dac58002b9414030dc91fca Mon Sep 17 00:00:00 2001 From: Ivan Lazarev Date: Thu, 16 Jun 2022 20:13:12 +0300 Subject: [PATCH 3/6] [PBCKP-145] after-review fix --- src/dir.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dir.c b/src/dir.c index 73f83c39a..aadc4c113 100644 --- a/src/dir.c +++ b/src/dir.c @@ -1573,7 +1573,7 @@ get_control_value(const char *str, const char *name, else { /* verify if buf_int64 not exceeds size limits */ - if (buf_int64_ptr - buf_int64 >= sizeof buf_int64 - 1) { + if (buf_int64_ptr - buf_int64 >= sizeof(buf_int64) - 1) { elog(ERROR, "field \"%s\" is out of range in the line %s of the file %s", name, str, DATABASE_FILE_LIST); } From da987910e2480b3ed9136a14168b41d8f41907e7 Mon Sep 17 00:00:00 2001 From: Ivan Lazarev Date: Sat, 18 Jun 2022 04:12:29 +0300 Subject: [PATCH 4/6] [PBCKP-165] get_control_value() - added output buffer size limit check --- src/catalog.c | 32 +++++------ src/dir.c | 137 ++++++++++++++++++++++++++++----------------- src/pg_probackup.h | 2 +- 3 files changed, 103 insertions(+), 68 deletions(-) diff --git a/src/catalog.c b/src/catalog.c index b4ed8c189..2289211e7 100644 --- a/src/catalog.c +++ b/src/catalog.c @@ -1084,15 +1084,15 @@ get_backup_filelist(pgBackup *backup, bool strict) COMP_FILE_CRC32(true, content_crc, buf, strlen(buf)); - get_control_value(buf, "path", path, NULL, true); - get_control_value(buf, "size", NULL, &write_size, true); - get_control_value(buf, "mode", NULL, &mode, true); - get_control_value(buf, "is_datafile", NULL, &is_datafile, true); - get_control_value(buf, "is_cfs", NULL, &is_cfs, false); - get_control_value(buf, "crc", NULL, &crc, true); - get_control_value(buf, "compress_alg", compress_alg_string, NULL, false); - get_control_value(buf, "external_dir_num", NULL, &external_dir_num, false); - get_control_value(buf, "dbOid", NULL, &dbOid, false); + get_control_value(buf, "path", path, sizeof(path), NULL, true); + get_control_value(buf, "size", NULL, 0, &write_size, true); + get_control_value(buf, "mode", NULL, 0, &mode, true); + get_control_value(buf, "is_datafile", NULL, 0, &is_datafile, true); + get_control_value(buf, "is_cfs", NULL, 0, &is_cfs, false); + get_control_value(buf, "crc", NULL, 0, &crc, true); + get_control_value(buf, "compress_alg", compress_alg_string, sizeof(compress_alg_string), NULL, false); + get_control_value(buf, "external_dir_num", NULL, 0, &external_dir_num, false); + get_control_value(buf, "dbOid", NULL, 0, &dbOid, false); file = pgFileInit(path); file->write_size = (int64) write_size; @@ -1107,28 +1107,28 @@ get_backup_filelist(pgBackup *backup, bool strict) /* * Optional fields */ - if (get_control_value(buf, "linked", linked, NULL, false) && linked[0]) + if (get_control_value(buf, "linked", linked, sizeof(linked), NULL, false) && linked[0]) { file->linked = pgut_strdup(linked); canonicalize_path(file->linked); } - if (get_control_value(buf, "segno", NULL, &segno, false)) + if (get_control_value(buf, "segno", NULL, 0, &segno, false)) file->segno = (int) segno; - if (get_control_value(buf, "n_blocks", NULL, &n_blocks, false)) + if (get_control_value(buf, "n_blocks", NULL, 0, &n_blocks, false)) file->n_blocks = (int) n_blocks; - if (get_control_value(buf, "n_headers", NULL, &n_headers, false)) + if (get_control_value(buf, "n_headers", NULL, 0, &n_headers, false)) file->n_headers = (int) n_headers; - if (get_control_value(buf, "hdr_crc", NULL, &hdr_crc, false)) + if (get_control_value(buf, "hdr_crc", NULL, 0, &hdr_crc, false)) file->hdr_crc = (pg_crc32) hdr_crc; - if (get_control_value(buf, "hdr_off", NULL, &hdr_off, false)) + if (get_control_value(buf, "hdr_off", NULL, 0, &hdr_off, false)) file->hdr_off = hdr_off; - if (get_control_value(buf, "hdr_size", NULL, &hdr_size, false)) + if (get_control_value(buf, "hdr_size", NULL, 0, &hdr_size, false)) file->hdr_size = (int) hdr_size; parray_append(files, file); diff --git a/src/dir.c b/src/dir.c index aadc4c113..81851ebe6 100644 --- a/src/dir.c +++ b/src/dir.c @@ -130,6 +130,13 @@ static void opt_path_map(ConfigOption *opt, const char *arg, TablespaceList *list, const char *type); static void cleanup_tablespace(const char *path); +static void control_string_bad_format(const char* str); + +static bool get_control_string_value(const char *str, const char *name, + char *value_str, size_t value_str_size, bool is_mandatory); + + + /* Tablespace mapping */ static TablespaceList tablespace_dirs = {NULL, NULL}; /* Extra directories mapping */ @@ -1488,19 +1495,77 @@ get_external_remap(char *current_dir) */ bool get_control_value(const char *str, const char *name, - char *value_str, int64 *value_int64, bool is_mandatory) + char *value_str, size_t value_str_size, int64 *value_int64, bool is_mandatory) +{ + + char buf_int64[32]; + bool result; + + //TODO REVUEW XXX use asserts? + assert((value_str && value_str_size >0) || value_int64); + + if (!value_str) + { + value_str = buf_int64; + value_str_size = sizeof(buf_int64); + } + + *value_str = '\0'; + + result = get_control_string_value(str, name, value_str, value_str_size, is_mandatory); + + if (!result) + return false; + + if (value_str != buf_int64) + return true; + + if (!parse_int64(buf_int64, value_int64, 0)) + { + /* We assume that too big value is -1 */ + if (errno == ERANGE) + *value_int64 = BYTES_INVALID; + else + control_string_bad_format(str); + } + + return true; +} + +//TODO REVIEW XXX if it's needed and to be static, otherwise go back to goto bad_format: and custom message for parse_int64 +static void +control_string_bad_format(const char* str) +{ + elog(ERROR, "%s file has invalid format in line %s", + DATABASE_FILE_LIST, str); +} + +/* + * Get value from json-like line "str" of backup_content.control file. + * + * The line has the following format: + * {"name1":"value1", "name2":"value2"} + * + * The value will be returned to "value_str" as string. + * + * Returns true if the value was found in the line. + */ + +static bool get_control_string_value(const char *str, const char *name, + char *value_str, size_t value_str_size, bool is_mandatory) { int state = CONTROL_WAIT_NAME; char *name_ptr = (char *) name; char *buf = (char *) str; - char buf_int64[32], /* Buffer for "value_int64" */ - *buf_int64_ptr = buf_int64; + //TODO REVIEW XXX *const can contradict postgres coding rules + char *const value_str_start = value_str; + + //TODO REVIEW XXX asserts in production code? are there any other ways to validate inputs + assert(value_str); + assert(value_str_size > 0); - /* Set default values */ - if (value_str) - *value_str = '\0'; - else if (value_int64) - *value_int64 = 0; + /* Set default value */ + *value_str = '\0'; while (*buf) { @@ -1510,7 +1575,7 @@ get_control_value(const char *str, const char *name, if (*buf == '"') state = CONTROL_INNAME; else if (IsAlpha(*buf)) - goto bad_format; + control_string_bad_format(str); break; case CONTROL_INNAME: /* Found target field. Parse value. */ @@ -1529,57 +1594,32 @@ get_control_value(const char *str, const char *name, if (*buf == ':') state = CONTROL_WAIT_VALUE; else if (!IsSpace(*buf)) - goto bad_format; + control_string_bad_format(str); break; case CONTROL_WAIT_VALUE: if (*buf == '"') { state = CONTROL_INVALUE; - buf_int64_ptr = buf_int64; } else if (IsAlpha(*buf)) - goto bad_format; + control_string_bad_format(str); break; case CONTROL_INVALUE: /* Value was parsed, exit */ if (*buf == '"') { - if (value_str) - { - *value_str = '\0'; - } - else if (value_int64) - { - *buf_int64_ptr = '\0'; - if (!parse_int64(buf_int64, value_int64, 0)) - { - /* We assume that too big value is -1 */ - if (errno == ERANGE) - *value_int64 = BYTES_INVALID; - else - goto bad_format; - } - } - + *value_str = '\0'; return true; } else { - if (value_str) - { - *value_str = *buf; - value_str++; - } - else - { - /* verify if buf_int64 not exceeds size limits */ - if (buf_int64_ptr - buf_int64 >= sizeof(buf_int64) - 1) { - elog(ERROR, "field \"%s\" is out of range in the line %s of the file %s", - name, str, DATABASE_FILE_LIST); - } - *buf_int64_ptr = *buf; - buf_int64_ptr++; + /* verify if value_str not exceeds value_str_size limits */ + if (value_str - value_str_start >= value_str_size - 1) { + elog(ERROR, "field \"%s\" is out of range in the line %s of the file %s", + name, str, DATABASE_FILE_LIST); } + *value_str = *buf; + value_str++; } break; case CONTROL_WAIT_NEXT_NAME: @@ -1596,18 +1636,13 @@ get_control_value(const char *str, const char *name, /* There is no close quotes */ if (state == CONTROL_INNAME || state == CONTROL_INVALUE) - goto bad_format; + control_string_bad_format(str); /* Did not find target field */ if (is_mandatory) elog(ERROR, "field \"%s\" is not found in the line %s of the file %s", name, str, DATABASE_FILE_LIST); return false; - -bad_format: - elog(ERROR, "%s file has invalid format in line %s", - DATABASE_FILE_LIST, str); - return false; /* Make compiler happy */ } /* @@ -1841,8 +1876,8 @@ read_database_map(pgBackup *backup) db_map_entry *db_entry = (db_map_entry *) pgut_malloc(sizeof(db_map_entry)); - get_control_value(buf, "dbOid", NULL, &dbOid, true); - get_control_value(buf, "datname", datname, NULL, true); + get_control_value(buf, "dbOid", NULL, 0, &dbOid, true); + get_control_value(buf, "datname", datname, sizeof(datname), NULL, true); db_entry->dbOid = dbOid; db_entry->datname = pgut_strdup(datname); diff --git a/src/pg_probackup.h b/src/pg_probackup.h index 2c4c61036..d341722f8 100644 --- a/src/pg_probackup.h +++ b/src/pg_probackup.h @@ -1011,7 +1011,7 @@ extern const char* deparse_compress_alg(int alg); /* in dir.c */ extern bool get_control_value(const char *str, const char *name, - char *value_str, int64 *value_int64, bool is_mandatory); + char *value_str, size_t value_str_size, int64 *value_int64, bool is_mandatory); extern void dir_list_file(parray *files, const char *root, bool exclude, bool follow_symlink, bool add_root, bool backup_logs, bool skip_hidden, int external_dir_num, fio_location location); From bb914aa5bb180e205b9d9a8b74129b3806509d87 Mon Sep 17 00:00:00 2001 From: Ivan Lazarev Date: Tue, 21 Jun 2022 21:46:13 +0300 Subject: [PATCH 5/6] [PBCKP-165] get_control_velue() splitted to get_get_control_value_str() & get_control_value_int64() api --- src/catalog.c | 32 ++++++++++++------------ src/dir.c | 61 ++++++++++++++++------------------------------ src/pg_probackup.h | 5 ++-- 3 files changed, 40 insertions(+), 58 deletions(-) diff --git a/src/catalog.c b/src/catalog.c index 2289211e7..9d817913e 100644 --- a/src/catalog.c +++ b/src/catalog.c @@ -1084,15 +1084,15 @@ get_backup_filelist(pgBackup *backup, bool strict) COMP_FILE_CRC32(true, content_crc, buf, strlen(buf)); - get_control_value(buf, "path", path, sizeof(path), NULL, true); - get_control_value(buf, "size", NULL, 0, &write_size, true); - get_control_value(buf, "mode", NULL, 0, &mode, true); - get_control_value(buf, "is_datafile", NULL, 0, &is_datafile, true); - get_control_value(buf, "is_cfs", NULL, 0, &is_cfs, false); - get_control_value(buf, "crc", NULL, 0, &crc, true); - get_control_value(buf, "compress_alg", compress_alg_string, sizeof(compress_alg_string), NULL, false); - get_control_value(buf, "external_dir_num", NULL, 0, &external_dir_num, false); - get_control_value(buf, "dbOid", NULL, 0, &dbOid, false); + get_control_value_str(buf, "path", path, sizeof(path),true); + get_control_value_int64(buf, "size", &write_size, true); + get_control_value_int64(buf, "mode", &mode, true); + get_control_value_int64(buf, "is_datafile", &is_datafile, true); + get_control_value_int64(buf, "is_cfs", &is_cfs, false); + get_control_value_int64(buf, "crc", &crc, true); + get_control_value_str(buf, "compress_alg", compress_alg_string, sizeof(compress_alg_string), false); + get_control_value_int64(buf, "external_dir_num", &external_dir_num, false); + get_control_value_int64(buf, "dbOid", &dbOid, false); file = pgFileInit(path); file->write_size = (int64) write_size; @@ -1107,28 +1107,28 @@ get_backup_filelist(pgBackup *backup, bool strict) /* * Optional fields */ - if (get_control_value(buf, "linked", linked, sizeof(linked), NULL, false) && linked[0]) + if (get_control_value_str(buf, "linked", linked, sizeof(linked), false) && linked[0]) { file->linked = pgut_strdup(linked); canonicalize_path(file->linked); } - if (get_control_value(buf, "segno", NULL, 0, &segno, false)) + if (get_control_value_int64(buf, "segno", &segno, false)) file->segno = (int) segno; - if (get_control_value(buf, "n_blocks", NULL, 0, &n_blocks, false)) + if (get_control_value_int64(buf, "n_blocks", &n_blocks, false)) file->n_blocks = (int) n_blocks; - if (get_control_value(buf, "n_headers", NULL, 0, &n_headers, false)) + if (get_control_value_int64(buf, "n_headers", &n_headers, false)) file->n_headers = (int) n_headers; - if (get_control_value(buf, "hdr_crc", NULL, 0, &hdr_crc, false)) + if (get_control_value_int64(buf, "hdr_crc", &hdr_crc, false)) file->hdr_crc = (pg_crc32) hdr_crc; - if (get_control_value(buf, "hdr_off", NULL, 0, &hdr_off, false)) + if (get_control_value_int64(buf, "hdr_off", &hdr_off, false)) file->hdr_off = hdr_off; - if (get_control_value(buf, "hdr_size", NULL, 0, &hdr_size, false)) + if (get_control_value_int64(buf, "hdr_size", &hdr_size, false)) file->hdr_size = (int) hdr_size; parray_append(files, file); diff --git a/src/dir.c b/src/dir.c index 81851ebe6..375e22c68 100644 --- a/src/dir.c +++ b/src/dir.c @@ -132,10 +132,6 @@ static void cleanup_tablespace(const char *path); static void control_string_bad_format(const char* str); -static bool get_control_string_value(const char *str, const char *name, - char *value_str, size_t value_str_size, bool is_mandatory); - - /* Tablespace mapping */ static TablespaceList tablespace_dirs = {NULL, NULL}; @@ -1474,7 +1470,7 @@ get_external_remap(char *current_dir) return current_dir; } -/* Parsing states for get_control_value() */ +/* Parsing states for get_control_value_str() */ #define CONTROL_WAIT_NAME 1 #define CONTROL_INNAME 2 #define CONTROL_WAIT_COLON 3 @@ -1488,38 +1484,24 @@ get_external_remap(char *current_dir) * The line has the following format: * {"name1":"value1", "name2":"value2"} * - * The value will be returned to "value_str" as string if it is not NULL. If it - * is NULL the value will be returned to "value_int64" as int64. + * The value will be returned in "value_int64" as int64. * - * Returns true if the value was found in the line. + * Returns true if the value was found in the line and parsed. */ bool -get_control_value(const char *str, const char *name, - char *value_str, size_t value_str_size, int64 *value_int64, bool is_mandatory) +get_control_value_int64(const char *str, const char *name, int64 *value_int64, bool is_mandatory) { char buf_int64[32]; - bool result; - //TODO REVUEW XXX use asserts? - assert((value_str && value_str_size >0) || value_int64); + assert(value_int64); - if (!value_str) - { - value_str = buf_int64; - value_str_size = sizeof(buf_int64); - } + /* Set default value */ + *value_int64 = 0; - *value_str = '\0'; - - result = get_control_string_value(str, name, value_str, value_str_size, is_mandatory); - - if (!result) + if (!get_control_value_str(str, name, buf_int64, sizeof(buf_int64), is_mandatory)) return false; - if (value_str != buf_int64) - return true; - if (!parse_int64(buf_int64, value_int64, 0)) { /* We assume that too big value is -1 */ @@ -1527,19 +1509,12 @@ get_control_value(const char *str, const char *name, *value_int64 = BYTES_INVALID; else control_string_bad_format(str); + return false; } return true; } -//TODO REVIEW XXX if it's needed and to be static, otherwise go back to goto bad_format: and custom message for parse_int64 -static void -control_string_bad_format(const char* str) -{ - elog(ERROR, "%s file has invalid format in line %s", - DATABASE_FILE_LIST, str); -} - /* * Get value from json-like line "str" of backup_content.control file. * @@ -1551,16 +1526,15 @@ control_string_bad_format(const char* str) * Returns true if the value was found in the line. */ -static bool get_control_string_value(const char *str, const char *name, - char *value_str, size_t value_str_size, bool is_mandatory) +bool +get_control_value_str(const char *str, const char *name, + char *value_str, size_t value_str_size, bool is_mandatory) { int state = CONTROL_WAIT_NAME; char *name_ptr = (char *) name; char *buf = (char *) str; - //TODO REVIEW XXX *const can contradict postgres coding rules char *const value_str_start = value_str; - //TODO REVIEW XXX asserts in production code? are there any other ways to validate inputs assert(value_str); assert(value_str_size > 0); @@ -1645,6 +1619,13 @@ static bool get_control_string_value(const char *str, const char *name, return false; } +static void +control_string_bad_format(const char* str) +{ + elog(ERROR, "%s file has invalid format in line %s", + DATABASE_FILE_LIST, str); +} + /* * Check if directory empty. */ @@ -1876,8 +1857,8 @@ read_database_map(pgBackup *backup) db_map_entry *db_entry = (db_map_entry *) pgut_malloc(sizeof(db_map_entry)); - get_control_value(buf, "dbOid", NULL, 0, &dbOid, true); - get_control_value(buf, "datname", datname, sizeof(datname), NULL, true); + get_control_value_int64(buf, "dbOid", &dbOid, true); + get_control_value_str(buf, "datname", datname, sizeof(datname), true); db_entry->dbOid = dbOid; db_entry->datname = pgut_strdup(datname); diff --git a/src/pg_probackup.h b/src/pg_probackup.h index d341722f8..7eb62466f 100644 --- a/src/pg_probackup.h +++ b/src/pg_probackup.h @@ -1010,8 +1010,9 @@ extern CompressAlg parse_compress_alg(const char *arg); extern const char* deparse_compress_alg(int alg); /* in dir.c */ -extern bool get_control_value(const char *str, const char *name, - char *value_str, size_t value_str_size, int64 *value_int64, bool is_mandatory); +extern bool get_control_value_int64(const char *str, const char *name, int64 *value_int64, bool is_mandatory); +extern bool get_control_value_str(const char *str, const char *name, + char *value_str, size_t value_str_size, bool is_mandatory); extern void dir_list_file(parray *files, const char *root, bool exclude, bool follow_symlink, bool add_root, bool backup_logs, bool skip_hidden, int external_dir_num, fio_location location); From ba1485029244d4ffbcb3a139986654a065b89309 Mon Sep 17 00:00:00 2001 From: Ivan Lazarev Date: Wed, 22 Jun 2022 02:06:37 +0300 Subject: [PATCH 6/6] [PBCKP-165] included for windows build --- src/dir.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/dir.c b/src/dir.c index 375e22c68..e76122ae7 100644 --- a/src/dir.c +++ b/src/dir.c @@ -8,6 +8,7 @@ *------------------------------------------------------------------------- */ +#include #include "pg_probackup.h" #include "utils/file.h"