Skip to content

Commit 7e16642

Browse files
avanessIvan Lazarev
avaness
and
Ivan Lazarev
authored
[PBCKP-165] get_control_value() int64 buffer vulnerability fix (#496)
* [PBCKP-165] get_control_value() int64 buffer vulnerability fix - added output buffer size limit check - splitted to get_get_control_value_str() & get_control_value_int64() api - included <assert.h> for windows build Co-authored-by: Ivan Lazarev <[email protected]>
1 parent acc8edc commit 7e16642

File tree

3 files changed

+90
-72
lines changed

3 files changed

+90
-72
lines changed

‎src/catalog.c‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1084,15 +1084,15 @@ get_backup_filelist(pgBackup *backup, bool strict)
10841084

10851085
COMP_FILE_CRC32(true, content_crc, buf, strlen(buf));
10861086

1087-
get_control_value(buf, "path", path, NULL, true);
1088-
get_control_value(buf, "size", NULL, &write_size, true);
1089-
get_control_value(buf, "mode", NULL, &mode, true);
1090-
get_control_value(buf, "is_datafile", NULL, &is_datafile, true);
1091-
get_control_value(buf, "is_cfs", NULL, &is_cfs, false);
1092-
get_control_value(buf, "crc", NULL, &crc, true);
1093-
get_control_value(buf, "compress_alg", compress_alg_string, NULL, false);
1094-
get_control_value(buf, "external_dir_num", NULL, &external_dir_num, false);
1095-
get_control_value(buf, "dbOid", NULL, &dbOid, false);
1087+
get_control_value_str(buf, "path", path, sizeof(path),true);
1088+
get_control_value_int64(buf, "size", &write_size, true);
1089+
get_control_value_int64(buf, "mode", &mode, true);
1090+
get_control_value_int64(buf, "is_datafile", &is_datafile, true);
1091+
get_control_value_int64(buf, "is_cfs", &is_cfs, false);
1092+
get_control_value_int64(buf, "crc", &crc, true);
1093+
get_control_value_str(buf, "compress_alg", compress_alg_string, sizeof(compress_alg_string), false);
1094+
get_control_value_int64(buf, "external_dir_num", &external_dir_num, false);
1095+
get_control_value_int64(buf, "dbOid", &dbOid, false);
10961096

10971097
file = pgFileInit(path);
10981098
file->write_size = (int64) write_size;
@@ -1107,28 +1107,28 @@ get_backup_filelist(pgBackup *backup, bool strict)
11071107
/*
11081108
* Optional fields
11091109
*/
1110-
if (get_control_value(buf, "linked", linked, NULL, false) && linked[0])
1110+
if (get_control_value_str(buf, "linked", linked, sizeof(linked), false) && linked[0])
11111111
{
11121112
file->linked = pgut_strdup(linked);
11131113
canonicalize_path(file->linked);
11141114
}
11151115

1116-
if (get_control_value(buf, "segno", NULL, &segno, false))
1116+
if (get_control_value_int64(buf, "segno", &segno, false))
11171117
file->segno = (int) segno;
11181118

1119-
if (get_control_value(buf, "n_blocks", NULL, &n_blocks, false))
1119+
if (get_control_value_int64(buf, "n_blocks", &n_blocks, false))
11201120
file->n_blocks = (int) n_blocks;
11211121

1122-
if (get_control_value(buf, "n_headers", NULL, &n_headers, false))
1122+
if (get_control_value_int64(buf, "n_headers", &n_headers, false))
11231123
file->n_headers = (int) n_headers;
11241124

1125-
if (get_control_value(buf, "hdr_crc", NULL, &hdr_crc, false))
1125+
if (get_control_value_int64(buf, "hdr_crc", &hdr_crc, false))
11261126
file->hdr_crc = (pg_crc32) hdr_crc;
11271127

1128-
if (get_control_value(buf, "hdr_off", NULL, &hdr_off, false))
1128+
if (get_control_value_int64(buf, "hdr_off", &hdr_off, false))
11291129
file->hdr_off = hdr_off;
11301130

1131-
if (get_control_value(buf, "hdr_size", NULL, &hdr_size, false))
1131+
if (get_control_value_int64(buf, "hdr_size", &hdr_size, false))
11321132
file->hdr_size = (int) hdr_size;
11331133

11341134
parray_append(files, file);

‎src/dir.c‎

Lines changed: 71 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
*-------------------------------------------------------------------------
99
*/
1010

11+
#include <assert.h>
1112
#include "pg_probackup.h"
1213
#include "utils/file.h"
1314

@@ -130,6 +131,9 @@ static void opt_path_map(ConfigOption *opt, const char *arg,
130131
TablespaceList *list, const char *type);
131132
static void cleanup_tablespace(const char *path);
132133

134+
static void control_string_bad_format(const char* str);
135+
136+
133137
/* Tablespace mapping */
134138
static TablespaceList tablespace_dirs = {NULL, NULL};
135139
/* Extra directories mapping */
@@ -1467,7 +1471,7 @@ get_external_remap(char *current_dir)
14671471
return current_dir;
14681472
}
14691473

1470-
/* Parsing states for get_control_value() */
1474+
/* Parsing states for get_control_value_str() */
14711475
#define CONTROL_WAIT_NAME 1
14721476
#define CONTROL_INNAME 2
14731477
#define CONTROL_WAIT_COLON 3
@@ -1481,26 +1485,62 @@ get_external_remap(char *current_dir)
14811485
* The line has the following format:
14821486
* {"name1":"value1", "name2":"value2"}
14831487
*
1484-
* The value will be returned to "value_str" as string if it is not NULL. If it
1485-
* is NULL the value will be returned to "value_int64" as int64.
1488+
* The value will be returned in "value_int64" as int64.
1489+
*
1490+
* Returns true if the value was found in the line and parsed.
1491+
*/
1492+
bool
1493+
get_control_value_int64(const char *str, const char *name, int64 *value_int64, bool is_mandatory)
1494+
{
1495+
1496+
char buf_int64[32];
1497+
1498+
assert(value_int64);
1499+
1500+
/* Set default value */
1501+
*value_int64 = 0;
1502+
1503+
if (!get_control_value_str(str, name, buf_int64, sizeof(buf_int64), is_mandatory))
1504+
return false;
1505+
1506+
if (!parse_int64(buf_int64, value_int64, 0))
1507+
{
1508+
/* We assume that too big value is -1 */
1509+
if (errno == ERANGE)
1510+
*value_int64 = BYTES_INVALID;
1511+
else
1512+
control_string_bad_format(str);
1513+
return false;
1514+
}
1515+
1516+
return true;
1517+
}
1518+
1519+
/*
1520+
* Get value from json-like line "str" of backup_content.control file.
1521+
*
1522+
* The line has the following format:
1523+
* {"name1":"value1", "name2":"value2"}
1524+
*
1525+
* The value will be returned to "value_str" as string.
14861526
*
14871527
* Returns true if the value was found in the line.
14881528
*/
1529+
14891530
bool
1490-
get_control_value(const char *str, const char *name,
1491-
char *value_str, int64 *value_int64, bool is_mandatory)
1531+
get_control_value_str(const char *str, const char *name,
1532+
char *value_str, size_t value_str_size, bool is_mandatory)
14921533
{
14931534
int state = CONTROL_WAIT_NAME;
14941535
char *name_ptr = (char *) name;
14951536
char *buf = (char *) str;
1496-
char buf_int64[32], /* Buffer for "value_int64" */
1497-
*buf_int64_ptr = buf_int64;
1537+
char *const value_str_start = value_str;
14981538

1499-
/* Set default values */
1500-
if (value_str)
1501-
*value_str = '\0';
1502-
else if (value_int64)
1503-
*value_int64 = 0;
1539+
assert(value_str);
1540+
assert(value_str_size > 0);
1541+
1542+
/* Set default value */
1543+
*value_str = '\0';
15041544

15051545
while (*buf)
15061546
{
@@ -1510,7 +1550,7 @@ get_control_value(const char *str, const char *name,
15101550
if (*buf == '"')
15111551
state = CONTROL_INNAME;
15121552
else if (IsAlpha(*buf))
1513-
goto bad_format;
1553+
control_string_bad_format(str);
15141554
break;
15151555
case CONTROL_INNAME:
15161556
/* Found target field. Parse value. */
@@ -1529,57 +1569,32 @@ get_control_value(const char *str, const char *name,
15291569
if (*buf == ':')
15301570
state = CONTROL_WAIT_VALUE;
15311571
else if (!IsSpace(*buf))
1532-
goto bad_format;
1572+
control_string_bad_format(str);
15331573
break;
15341574
case CONTROL_WAIT_VALUE:
15351575
if (*buf == '"')
15361576
{
15371577
state = CONTROL_INVALUE;
1538-
buf_int64_ptr = buf_int64;
15391578
}
15401579
else if (IsAlpha(*buf))
1541-
goto bad_format;
1580+
control_string_bad_format(str);
15421581
break;
15431582
case CONTROL_INVALUE:
15441583
/* Value was parsed, exit */
15451584
if (*buf == '"')
15461585
{
1547-
if (value_str)
1548-
{
1549-
*value_str = '\0';
1550-
}
1551-
else if (value_int64)
1552-
{
1553-
/* Length of buf_uint64 should not be greater than 31 */
1554-
if (buf_int64_ptr - buf_int64 >= 32)
1555-
elog(ERROR, "field \"%s\" is out of range in the line %s of the file %s",
1556-
name, str, DATABASE_FILE_LIST);
1557-
1558-
*buf_int64_ptr = '\0';
1559-
if (!parse_int64(buf_int64, value_int64, 0))
1560-
{
1561-
/* We assume that too big value is -1 */
1562-
if (errno == ERANGE)
1563-
*value_int64 = BYTES_INVALID;
1564-
else
1565-
goto bad_format;
1566-
}
1567-
}
1568-
1586+
*value_str = '\0';
15691587
return true;
15701588
}
15711589
else
15721590
{
1573-
if (value_str)
1574-
{
1575-
*value_str = *buf;
1576-
value_str++;
1577-
}
1578-
else
1579-
{
1580-
*buf_int64_ptr = *buf;
1581-
buf_int64_ptr++;
1591+
/* verify if value_str not exceeds value_str_size limits */
1592+
if (value_str - value_str_start >= value_str_size - 1) {
1593+
elog(ERROR, "field \"%s\" is out of range in the line %s of the file %s",
1594+
name, str, DATABASE_FILE_LIST);
15821595
}
1596+
*value_str = *buf;
1597+
value_str++;
15831598
}
15841599
break;
15851600
case CONTROL_WAIT_NEXT_NAME:
@@ -1596,18 +1611,20 @@ get_control_value(const char *str, const char *name,
15961611

15971612
/* There is no close quotes */
15981613
if (state == CONTROL_INNAME || state == CONTROL_INVALUE)
1599-
goto bad_format;
1614+
control_string_bad_format(str);
16001615

16011616
/* Did not find target field */
16021617
if (is_mandatory)
16031618
elog(ERROR, "field \"%s\" is not found in the line %s of the file %s",
16041619
name, str, DATABASE_FILE_LIST);
16051620
return false;
1621+
}
16061622

1607-
bad_format:
1608-
elog(ERROR, "%s file has invalid format in line %s",
1609-
DATABASE_FILE_LIST, str);
1610-
return false; /* Make compiler happy */
1623+
static void
1624+
control_string_bad_format(const char* str)
1625+
{
1626+
elog(ERROR, "%s file has invalid format in line %s",
1627+
DATABASE_FILE_LIST, str);
16111628
}
16121629

16131630
/*
@@ -1841,8 +1858,8 @@ read_database_map(pgBackup *backup)
18411858

18421859
db_map_entry *db_entry = (db_map_entry *) pgut_malloc(sizeof(db_map_entry));
18431860

1844-
get_control_value(buf, "dbOid", NULL, &dbOid, true);
1845-
get_control_value(buf, "datname", datname, NULL, true);
1861+
get_control_value_int64(buf, "dbOid", &dbOid, true);
1862+
get_control_value_str(buf, "datname", datname, sizeof(datname), true);
18461863

18471864
db_entry->dbOid = dbOid;
18481865
db_entry->datname = pgut_strdup(datname);

‎src/pg_probackup.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1010,8 +1010,9 @@ extern CompressAlg parse_compress_alg(const char *arg);
10101010
extern const char* deparse_compress_alg(int alg);
10111011

10121012
/* in dir.c */
1013-
extern bool get_control_value(const char *str, const char *name,
1014-
char *value_str, int64 *value_int64, bool is_mandatory);
1013+
extern bool get_control_value_int64(const char *str, const char *name, int64 *value_int64, bool is_mandatory);
1014+
extern bool get_control_value_str(const char *str, const char *name,
1015+
char *value_str, size_t value_str_size, bool is_mandatory);
10151016
extern void dir_list_file(parray *files, const char *root, bool exclude,
10161017
bool follow_symlink, bool add_root, bool backup_logs,
10171018
bool skip_hidden, int external_dir_num, fio_location location);

0 commit comments

Comments
 (0)