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()

diff --git a/contrib/chkpass/chkpass.c b/contrib/chkpass/chkpass.c
index 0c9fec0e676fc1140a2b4338c8df0a904b551d3f..1795b8cde4256f301440335ab24906d4fd72d634 100644 (file)
--- a/contrib/chkpass/chkpass.c
+++ b/contrib/chkpass/chkpass.c
@@ -70,6 +70,7 @@ chkpass_in(PG_FUNCTION_ARGS)
        char       *str = PG_GETARG_CSTRING(0);
        chkpass    *result;
        char            mysalt[4];
+       char       *crypt_output;
        static char salt_chars[] =
        "./0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
 
@@ -92,7 +93,15 @@ chkpass_in(PG_FUNCTION_ARGS)
        mysalt[1] = salt_chars[random() & 0x3f];
        mysalt[2] = 0;                          /* technically the terminator is not necessary
                                                                 * but I like to play safe */
-       strcpy(result->password, crypt(str, mysalt));
+
+       crypt_output = crypt(str, mysalt);
+       if (crypt_output == NULL)
+               ereport(ERROR,
+                               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                                errmsg("crypt() failed")));
+
+       strlcpy(result->password, crypt_output, sizeof(result->password));
+
        PG_RETURN_POINTER(result);
 }
 
@@ -141,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);
@@ -153,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 84941ede137a7cb30f979ef584038edb00a48c46..0f1e0c1dfc86d2392bc3f16b7b01ae006267d45a 100644 (file)
--- a/contrib/pg_standby/pg_standby.c
+++ b/contrib/pg_standby/pg_standby.c
@@ -338,7 +338,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 d639c4ac43a8f7e94fabcbe618be4ee2f4cc9550..49bb4538c934ade0932834eeeaac65708b54bd4b 100644 (file)
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3017,7 +3017,7 @@ KeepFileRestoredFromArchive(char *path, char *xlogfname)
                                                        xlogfpath, oldpath)));
                }
 #else
-               strncpy(oldpath, xlogfpath, MAXPGPATH);
+               strlcpy(oldpath, xlogfpath, MAXPGPATH);
 #endif
                if (unlink(oldpath) != 0)
                        ereport(FATAL,
@@ -5913,7 +5913,7 @@ recoveryStopsHere(XLogRecord *record, bool *includeThis)
 
                recordRestorePointData = (xl_restore_point *) XLogRecGetData(record);
                recordXtime = recordRestorePointData->rp_time;
-               strncpy(recordRPName, recordRestorePointData->rp_name, MAXFNAMELEN);
+               strlcpy(recordRPName, recordRestorePointData->rp_name, MAXFNAMELEN);
        }
        else
                return false;
@@ -6008,7 +6008,7 @@ recoveryStopsHere(XLogRecord *record, bool *includeThis)
                }
                else
                {
-                       strncpy(recoveryStopName, recordRPName, MAXFNAMELEN);
+                       strlcpy(recoveryStopName, recordRPName, MAXFNAMELEN);
 
                        ereport(LOG,
                                (errmsg("recovery stopping at restore point \"%s\", time %s",
@@ -6348,7 +6348,7 @@ StartupXLOG(void)
         * see them
         */
        XLogCtl->RecoveryTargetTLI = recoveryTargetTLI;
-       strncpy(XLogCtl->archiveCleanupCommand,
+       strlcpy(XLogCtl->archiveCleanupCommand,
                        archiveCleanupCommand ? archiveCleanupCommand : "",
                        sizeof(XLogCtl->archiveCleanupCommand));
 
@@ -8760,7 +8760,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 449aa6a0a552c3fc40e4d585e99def7950c426e6..4acc33eac32d7e2ff67a536b21abd832144321c5 100644 (file)
--- 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 4763a6fe588fa7396d20f4d34c19e1f12501b168..4105f175f9f6dc6e35b0b8c5f99e6b379206899f 100644 (file)
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -90,10 +90,10 @@ char           *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
@@ -108,7 +108,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 */
@@ -188,7 +188,7 @@ static const datetkn datetktbl[] = {
 static int     szdatetktbl = sizeof datetktbl / sizeof datetktbl[0];
 
 static 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 */
@@ -4201,6 +4201,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 6d6f96add06dfd5f6aaa3ca5b570d2d41978ac8d..6d381515e1f04dca5c310a2187b4d922efdd33da 100644 (file)
--- 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 9d840a15fa899c75d7e900ce00dac66e81b5942b..26cc75897a873dcf02058aa16b077e64cb4070f9 100644 (file)
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -735,9 +735,9 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res, int rownum)
        FILE       *file = NULL;
 
        if (PQgetisnull(res, rownum, 0))
-               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
@@ -1053,7 +1053,7 @@ BaseBackup(void)
                                progname);
                disconnect_and_exit(1);
        }
-       strcpy(xlogstart, PQgetvalue(res, 0, 0));
+       strlcpy(xlogstart, PQgetvalue(res, 0, 0), sizeof(xlogstart));
        if (verbose && includewal)
                fprintf(stderr, "transaction log start point: %s\n", xlogstart);
        PQclear(res);
@@ -1153,7 +1153,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/interfaces/ecpg/preproc/pgc.l b/src/interfaces/ecpg/preproc/pgc.l
index f2e7eddf86eb7dbb8318a66709f4f1c6a301120f..7ae8556127ed993d582cc1c96f920aaba8682b09 100644 (file)
--- a/src/interfaces/ecpg/preproc/pgc.l
+++ b/src/interfaces/ecpg/preproc/pgc.l
@@ -1315,7 +1315,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 1ba5885cd3b419a97f4fc5ea897eac209f364897..af4c41293a51a598eef83443750526201cafdbf2 100644 (file)
--- 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 d289f82285fea00d5de20542e43ea103493f9e58..6f8a4703706fa1455414d483d02703baa694f098 100644 (file)
--- 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/port/exec.c b/src/port/exec.c
index c79e8ba35e0bbd056ce4a23fa80b6d6a1b4e2ab6..0726dbe8506ff4b6fd43d171225446db5efb5ae8 100644 (file)
--- a/src/port/exec.c
+++ b/src/port/exec.c
@@ -66,7 +66,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;
        }
@@ -275,7 +275,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/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index d991a5ccc7065abb0eb29fc199cb5cb1d3ef7ede..a6466ebddff660f229a7f7d3ea25af41e656a520 100644 (file)
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -1233,7 +1233,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)
        {
                /*
@@ -1288,7 +1288,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);
        }
@@ -1316,7 +1316,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 d5bc83ecf81ecc89fc618b715b7e64711912e209..80c563501828d5247ac555f8f7a9044047a1705f 100644 (file)
--- 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 (;;)