From 01824385aead50e557ca1af28640460fa9877d51 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 17 Feb 2014 11:20:21 -0500 Subject: [PATCH] Prevent potential overruns of fixed-size buffers. Coverity identified a number of places in which it couldn't prove that a string being copied into a fixed-size buffer would fit. We believe that most, perhaps all of these are in fact safe, or are copying data that is coming from a trusted source so that any overrun is not really a security issue. Nonetheless it seems prudent to forestall any risk by using strlcpy() and similar functions. Fixes by Peter Eisentraut and Jozef Mlich based on Coverity reports. In addition, fix a potential null-pointer-dereference crash in contrib/chkpass. The crypt(3) function is defined to return NULL on failure, but chkpass.c didn't check for that before using the result. The main practical case in which this could be an issue is if libc is configured to refuse to execute unapproved hashing algorithms (e.g., "FIPS mode"). This ideally should've been a separate commit, but since it touches code adjacent to one of the buffer overrun changes, I included it in this commit to avoid last-minute merge issues. This issue was reported by Honza Horak. Security: CVE-2014-0065 for buffer overruns, CVE-2014-0066 for crypt() --- contrib/chkpass/chkpass.c | 24 ++++++++++++++++++++---- contrib/pg_standby/pg_standby.c | 2 +- src/backend/access/transam/xlog.c | 6 +++--- src/backend/tsearch/spell.c | 2 +- src/backend/utils/adt/datetime.c | 11 ++++++----- src/bin/initdb/findtimezone.c | 4 ++-- src/bin/pg_basebackup/pg_basebackup.c | 8 ++++---- src/common/exec.c | 4 ++-- src/interfaces/ecpg/preproc/pgc.l | 2 +- src/interfaces/libpq/fe-protocol2.c | 2 +- src/interfaces/libpq/fe-protocol3.c | 2 +- src/test/regress/pg_regress.c | 6 +++--- src/timezone/pgtz.c | 2 +- 13 files changed, 46 insertions(+), 29 deletions(-) diff --git a/contrib/chkpass/chkpass.c b/contrib/chkpass/chkpass.c index dc66075f98..1795b8cde4 100644 --- a/contrib/chkpass/chkpass.c +++ b/contrib/chkpass/chkpass.c @@ -94,11 +94,13 @@ chkpass_in(PG_FUNCTION_ARGS) mysalt[2] = 0; /* technically the terminator is not necessary * but I like to play safe */ - if ((crypt_output = crypt(str, mysalt)) == NULL) + crypt_output = crypt(str, mysalt); + if (crypt_output == NULL) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("crypt() failed"))); - strcpy(result->password, crypt_output); + + strlcpy(result->password, crypt_output, sizeof(result->password)); PG_RETURN_POINTER(result); } @@ -148,9 +150,16 @@ chkpass_eq(PG_FUNCTION_ARGS) chkpass *a1 = (chkpass *) PG_GETARG_POINTER(0); text *a2 = PG_GETARG_TEXT_PP(1); char str[9]; + char *crypt_output; text_to_cstring_buffer(a2, str, sizeof(str)); - PG_RETURN_BOOL(strcmp(a1->password, crypt(str, a1->password)) == 0); + crypt_output = crypt(str, a1->password); + if (crypt_output == NULL) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("crypt() failed"))); + + PG_RETURN_BOOL(strcmp(a1->password, crypt_output) == 0); } PG_FUNCTION_INFO_V1(chkpass_ne); @@ -160,7 +169,14 @@ chkpass_ne(PG_FUNCTION_ARGS) chkpass *a1 = (chkpass *) PG_GETARG_POINTER(0); text *a2 = PG_GETARG_TEXT_PP(1); char str[9]; + char *crypt_output; text_to_cstring_buffer(a2, str, sizeof(str)); - PG_RETURN_BOOL(strcmp(a1->password, crypt(str, a1->password)) != 0); + crypt_output = crypt(str, a1->password); + if (crypt_output == NULL) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("crypt() failed"))); + + PG_RETURN_BOOL(strcmp(a1->password, crypt_output) != 0); } diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c index 144edd8a07..8ddd486c89 100644 --- a/contrib/pg_standby/pg_standby.c +++ b/contrib/pg_standby/pg_standby.c @@ -327,7 +327,7 @@ SetWALFileNameForCleanup(void) if (strcmp(restartWALFileName, nextWALFileName) > 0) return false; - strcpy(exclusiveCleanupFileName, restartWALFileName); + strlcpy(exclusiveCleanupFileName, restartWALFileName, sizeof(exclusiveCleanupFileName)); return true; } diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 85a0ce9018..76c244c915 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -5844,7 +5844,7 @@ recoveryStopsAfter(XLogRecord *record) recoveryStopAfter = true; recoveryStopXid = InvalidTransactionId; (void) getRecordTimestamp(record, &recoveryStopTime); - strncpy(recoveryStopName, recordRestorePointData->rp_name, MAXFNAMELEN); + strlcpy(recoveryStopName, recordRestorePointData->rp_name, MAXFNAMELEN); ereport(LOG, (errmsg("recovery stopping at restore point \"%s\", time %s", @@ -6311,7 +6311,7 @@ StartupXLOG(void) * Save archive_cleanup_command in shared memory so that other processes * can see it. */ - strncpy(XLogCtl->archiveCleanupCommand, + strlcpy(XLogCtl->archiveCleanupCommand, archiveCleanupCommand ? archiveCleanupCommand : "", sizeof(XLogCtl->archiveCleanupCommand)); @@ -9107,7 +9107,7 @@ XLogRestorePoint(const char *rpName) xl_restore_point xlrec; xlrec.rp_time = GetCurrentTimestamp(); - strncpy(xlrec.rp_name, rpName, MAXFNAMELEN); + strlcpy(xlrec.rp_name, rpName, MAXFNAMELEN); rdata.buffer = InvalidBuffer; rdata.data = (char *) &xlrec; diff --git a/src/backend/tsearch/spell.c b/src/backend/tsearch/spell.c index 1ca6442329..1bc226d334 100644 --- a/src/backend/tsearch/spell.c +++ b/src/backend/tsearch/spell.c @@ -255,7 +255,7 @@ NIAddSpell(IspellDict *Conf, const char *word, const char *flag) } Conf->Spell[Conf->nspell] = (SPELL *) tmpalloc(SPELLHDRSZ + strlen(word) + 1); strcpy(Conf->Spell[Conf->nspell]->word, word); - strncpy(Conf->Spell[Conf->nspell]->p.flag, flag, MAXFLAGLEN); + strlcpy(Conf->Spell[Conf->nspell]->p.flag, flag, MAXFLAGLEN); Conf->nspell++; } diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c index 946adfad99..0d32428e40 100644 --- a/src/backend/utils/adt/datetime.c +++ b/src/backend/utils/adt/datetime.c @@ -89,10 +89,10 @@ const char *const days[] = {"Sunday", "Monday", "Tuesday", "Wednesday", * Note that this table must be strictly alphabetically ordered to allow an * O(ln(N)) search algorithm to be used. * - * The text field is NOT guaranteed to be NULL-terminated. + * The token field is NOT guaranteed to be NULL-terminated. * - * To keep this table reasonably small, we divide the lexval for TZ and DTZ - * entries by 15 (so they are on 15 minute boundaries) and truncate the text + * To keep this table reasonably small, we divide the value for TZ and DTZ + * entries by 15 (so they are on 15 minute boundaries) and truncate the token * field at TOKMAXLEN characters. * Formerly, we divided by 10 rather than 15 but there are a few time zones * which are 30 or 45 minutes away from an even hour, most are on an hour @@ -107,7 +107,7 @@ static datetkn *timezonetktbl = NULL; static int sztimezonetktbl = 0; static const datetkn datetktbl[] = { -/* text, token, lexval */ + /* token, type, value */ {EARLY, RESERV, DTK_EARLY}, /* "-infinity" reserved for "early time" */ {DA_D, ADBC, AD}, /* "ad" for years > 0 */ {"allballs", RESERV, DTK_ZULU}, /* 00:00:00 */ @@ -187,7 +187,7 @@ static const datetkn datetktbl[] = { static int szdatetktbl = sizeof datetktbl / sizeof datetktbl[0]; static const datetkn deltatktbl[] = { - /* text, token, lexval */ + /* token, type, value */ {"@", IGNORE_DTF, 0}, /* postgres relative prefix */ {DAGO, AGO, 0}, /* "ago" indicates negative time offset */ {"c", UNITS, DTK_CENTURY}, /* "century" relative */ @@ -4215,6 +4215,7 @@ ConvertTimeZoneAbbrevs(TimeZoneAbbrevTable *tbl, tbl->numabbrevs = n; for (i = 0; i < n; i++) { + /* do NOT use strlcpy here; token field need not be null-terminated */ strncpy(newtbl[i].token, abbrevs[i].abbrev, TOKMAXLEN); newtbl[i].type = abbrevs[i].is_dst ? DTZ : TZ; TOVAL(&newtbl[i], abbrevs[i].offset / MINS_PER_HOUR); diff --git a/src/bin/initdb/findtimezone.c b/src/bin/initdb/findtimezone.c index 0d28185f3c..bc1f2c2c97 100644 --- a/src/bin/initdb/findtimezone.c +++ b/src/bin/initdb/findtimezone.c @@ -68,7 +68,7 @@ pg_open_tzfile(const char *name, char *canonname) if (canonname) strlcpy(canonname, name, TZ_STRLEN_MAX + 1); - strcpy(fullname, pg_TZDIR()); + strlcpy(fullname, pg_TZDIR(), sizeof(fullname)); if (strlen(fullname) + 1 + strlen(name) >= MAXPGPATH) return -1; /* not gonna fit */ strcat(fullname, "/"); @@ -375,7 +375,7 @@ identify_system_timezone(void) } /* Search for the best-matching timezone file */ - strcpy(tmptzdir, pg_TZDIR()); + strlcpy(tmptzdir, pg_TZDIR(), sizeof(tmptzdir)); bestscore = -1; resultbuf[0] = '\0'; scan_available_timezones(tmptzdir, tmptzdir + strlen(tmptzdir) + 1, diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c index b5682d6f51..3d155e8907 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -921,9 +921,9 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res, int rownum) FILE *file = NULL; if (basetablespace) - strcpy(current_path, basedir); + strlcpy(current_path, basedir, sizeof(current_path)); else - strcpy(current_path, PQgetvalue(res, rownum, 1)); + strlcpy(current_path, PQgetvalue(res, rownum, 1), sizeof(current_path)); /* * Get the COPY data @@ -1454,7 +1454,7 @@ BaseBackup(void) disconnect_and_exit(1); } - strcpy(xlogstart, PQgetvalue(res, 0, 0)); + strlcpy(xlogstart, PQgetvalue(res, 0, 0), sizeof(xlogstart)); /* * 9.3 and later sends the TLI of the starting point. With older servers, @@ -1565,7 +1565,7 @@ BaseBackup(void) progname); disconnect_and_exit(1); } - strcpy(xlogend, PQgetvalue(res, 0, 0)); + strlcpy(xlogend, PQgetvalue(res, 0, 0), sizeof(xlogend)); if (verbose && includewal) fprintf(stderr, "transaction log end point: %s\n", xlogend); PQclear(res); diff --git a/src/common/exec.c b/src/common/exec.c index 4aadfd7fd0..037bef2210 100644 --- a/src/common/exec.c +++ b/src/common/exec.c @@ -68,7 +68,7 @@ validate_exec(const char *path) if (strlen(path) >= strlen(".exe") && pg_strcasecmp(path + strlen(path) - strlen(".exe"), ".exe") != 0) { - strcpy(path_exe, path); + strlcpy(path_exe, path, sizeof(path_exe) - 4); strcat(path_exe, ".exe"); path = path_exe; } @@ -277,7 +277,7 @@ resolve_symlinks(char *path) } /* must copy final component out of 'path' temporarily */ - strcpy(link_buf, fname); + strlcpy(link_buf, fname, sizeof(link_buf)); if (!getcwd(path, MAXPGPATH)) { diff --git a/src/interfaces/ecpg/preproc/pgc.l b/src/interfaces/ecpg/preproc/pgc.l index 3bd63b353d..744a8d3a02 100644 --- a/src/interfaces/ecpg/preproc/pgc.l +++ b/src/interfaces/ecpg/preproc/pgc.l @@ -1334,7 +1334,7 @@ parse_include(void) yytext[i] = '\0'; memmove(yytext, yytext+1, strlen(yytext)); - strncpy(inc_file, yytext, sizeof(inc_file)); + strlcpy(inc_file, yytext, sizeof(inc_file)); yyin = fopen(inc_file, "r"); if (!yyin) { diff --git a/src/interfaces/libpq/fe-protocol2.c b/src/interfaces/libpq/fe-protocol2.c index fe7f8b8d2b..f3fddaa036 100644 --- a/src/interfaces/libpq/fe-protocol2.c +++ b/src/interfaces/libpq/fe-protocol2.c @@ -500,7 +500,7 @@ pqParseInput2(PGconn *conn) if (!conn->result) return; } - strncpy(conn->result->cmdStatus, conn->workBuffer.data, + strlcpy(conn->result->cmdStatus, conn->workBuffer.data, CMDSTATUS_LEN); checkXactStatus(conn, conn->workBuffer.data); conn->asyncStatus = PGASYNC_READY; diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c index e2ba8d651f..47cd7f487f 100644 --- a/src/interfaces/libpq/fe-protocol3.c +++ b/src/interfaces/libpq/fe-protocol3.c @@ -206,7 +206,7 @@ pqParseInput3(PGconn *conn) if (!conn->result) return; } - strncpy(conn->result->cmdStatus, conn->workBuffer.data, + strlcpy(conn->result->cmdStatus, conn->workBuffer.data, CMDSTATUS_LEN); conn->asyncStatus = PGASYNC_READY; break; diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c index 9bc19eaddc..3a492449fb 100644 --- a/src/test/regress/pg_regress.c +++ b/src/test/regress/pg_regress.c @@ -1221,7 +1221,7 @@ results_differ(const char *testname, const char *resultsfile, const char *defaul */ platform_expectfile = get_expectfile(testname, resultsfile); - strcpy(expectfile, default_expectfile); + strlcpy(expectfile, default_expectfile, sizeof(expectfile)); if (platform_expectfile) { /* @@ -1276,7 +1276,7 @@ results_differ(const char *testname, const char *resultsfile, const char *defaul { /* This diff was a better match than the last one */ best_line_count = l; - strcpy(best_expect_file, alt_expectfile); + strlcpy(best_expect_file, alt_expectfile, sizeof(best_expect_file)); } free(alt_expectfile); } @@ -1304,7 +1304,7 @@ results_differ(const char *testname, const char *resultsfile, const char *defaul { /* This diff was a better match than the last one */ best_line_count = l; - strcpy(best_expect_file, default_expectfile); + strlcpy(best_expect_file, default_expectfile, sizeof(best_expect_file)); } } diff --git a/src/timezone/pgtz.c b/src/timezone/pgtz.c index 8a3c53f27a..3c161df15a 100644 --- a/src/timezone/pgtz.c +++ b/src/timezone/pgtz.c @@ -83,7 +83,7 @@ pg_open_tzfile(const char *name, char *canonname) * Loop to split the given name into directory levels; for each level, * search using scan_directory_ci(). */ - strcpy(fullname, pg_TZDIR()); + strlcpy(fullname, pg_TZDIR(), sizeof(fullname)); orignamelen = fullnamelen = strlen(fullname); fname = name; for (;;)