Skip to content

Commit e3208fe

Browse files
tglsfdctglsfdc
committed
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()
1 parent 7a362a1 commit e3208fe

File tree

13 files changed

+53
-29
lines changed

13 files changed

+53
-29
lines changed

‎contrib/chkpass/chkpass.c

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ chkpass_in(PG_FUNCTION_ARGS)
7070
char *str = PG_GETARG_CSTRING(0);
7171
chkpass *result;
7272
char mysalt[4];
73+
char *crypt_output;
7374
static char salt_chars[] =
7475
"./0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
7576

@@ -92,7 +93,15 @@ chkpass_in(PG_FUNCTION_ARGS)
9293
mysalt[1] = salt_chars[random() & 0x3f];
9394
mysalt[2] = 0; /* technically the terminator is not necessary
9495
* but I like to play safe */
95-
strcpy(result->password, crypt(str, mysalt));
96+
97+
crypt_output = crypt(str, mysalt);
98+
if (crypt_output == NULL)
99+
ereport(ERROR,
100+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
101+
errmsg("crypt() failed")));
102+
103+
strlcpy(result->password, crypt_output, sizeof(result->password));
104+
96105
PG_RETURN_POINTER(result);
97106
}
98107

@@ -141,9 +150,16 @@ chkpass_eq(PG_FUNCTION_ARGS)
141150
chkpass *a1 = (chkpass *) PG_GETARG_POINTER(0);
142151
text *a2 = PG_GETARG_TEXT_PP(1);
143152
char str[9];
153+
char *crypt_output;
144154

145155
text_to_cstring_buffer(a2, str, sizeof(str));
146-
PG_RETURN_BOOL(strcmp(a1->password, crypt(str, a1->password)) == 0);
156+
crypt_output = crypt(str, a1->password);
157+
if (crypt_output == NULL)
158+
ereport(ERROR,
159+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
160+
errmsg("crypt() failed")));
161+
162+
PG_RETURN_BOOL(strcmp(a1->password, crypt_output) == 0);
147163
}
148164

149165
PG_FUNCTION_INFO_V1(chkpass_ne);
@@ -153,7 +169,14 @@ chkpass_ne(PG_FUNCTION_ARGS)
153169
chkpass *a1 = (chkpass *) PG_GETARG_POINTER(0);
154170
text *a2 = PG_GETARG_TEXT_PP(1);
155171
char str[9];
172+
char *crypt_output;
156173

157174
text_to_cstring_buffer(a2, str, sizeof(str));
158-
PG_RETURN_BOOL(strcmp(a1->password, crypt(str, a1->password)) != 0);
175+
crypt_output = crypt(str, a1->password);
176+
if (crypt_output == NULL)
177+
ereport(ERROR,
178+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
179+
errmsg("crypt() failed")));
180+
181+
PG_RETURN_BOOL(strcmp(a1->password, crypt_output) != 0);
159182
}

‎contrib/pg_standby/pg_standby.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -338,7 +338,7 @@ SetWALFileNameForCleanup(void)
338338
if (strcmp(restartWALFileName, nextWALFileName) > 0)
339339
return false;
340340

341-
strcpy(exclusiveCleanupFileName, restartWALFileName);
341+
strlcpy(exclusiveCleanupFileName, restartWALFileName, sizeof(exclusiveCleanupFileName));
342342
return true;
343343
}
344344

‎src/backend/access/transam/xlog.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4529,7 +4529,7 @@ recoveryStopsHere(XLogRecord *record, bool *includeThis)
45294529

45304530
recordRestorePointData = (xl_restore_point *) XLogRecGetData(record);
45314531
recordXtime = recordRestorePointData->rp_time;
4532-
strncpy(recordRPName, recordRestorePointData->rp_name, MAXFNAMELEN);
4532+
strlcpy(recordRPName, recordRestorePointData->rp_name, MAXFNAMELEN);
45334533
}
45344534
else
45354535
return false;
@@ -4624,7 +4624,7 @@ recoveryStopsHere(XLogRecord *record, bool *includeThis)
46244624
}
46254625
else
46264626
{
4627-
strncpy(recoveryStopName, recordRPName, MAXFNAMELEN);
4627+
strlcpy(recoveryStopName, recordRPName, MAXFNAMELEN);
46284628

46294629
ereport(LOG,
46304630
(errmsg("recovery stopping at restore point \"%s\", time %s",
@@ -4957,7 +4957,7 @@ StartupXLOG(void)
49574957
* Save archive_cleanup_command in shared memory so that other processes
49584958
* can see it.
49594959
*/
4960-
strncpy(XLogCtl->archiveCleanupCommand,
4960+
strlcpy(XLogCtl->archiveCleanupCommand,
49614961
archiveCleanupCommand ? archiveCleanupCommand : "",
49624962
sizeof(XLogCtl->archiveCleanupCommand));
49634963

@@ -7685,7 +7685,7 @@ XLogRestorePoint(const char *rpName)
76857685
xl_restore_point xlrec;
76867686

76877687
xlrec.rp_time = GetCurrentTimestamp();
7688-
strncpy(xlrec.rp_name, rpName, MAXFNAMELEN);
7688+
strlcpy(xlrec.rp_name, rpName, MAXFNAMELEN);
76897689

76907690
rdata.buffer = InvalidBuffer;
76917691
rdata.data = (char *) &xlrec;

‎src/backend/tsearch/spell.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -255,7 +255,7 @@ NIAddSpell(IspellDict *Conf, const char *word, const char *flag)
255255
}
256256
Conf->Spell[Conf->nspell] = (SPELL *) tmpalloc(SPELLHDRSZ + strlen(word) + 1);
257257
strcpy(Conf->Spell[Conf->nspell]->word, word);
258-
strncpy(Conf->Spell[Conf->nspell]->p.flag, flag, MAXFLAGLEN);
258+
strlcpy(Conf->Spell[Conf->nspell]->p.flag, flag, MAXFLAGLEN);
259259
Conf->nspell++;
260260
}
261261

‎src/backend/utils/adt/datetime.c

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -91,10 +91,10 @@ char *days[] = {"Sunday", "Monday", "Tuesday", "Wednesday",
9191
* Note that this table must be strictly alphabetically ordered to allow an
9292
* O(ln(N)) search algorithm to be used.
9393
*
94-
* The text field is NOT guaranteed to be NULL-terminated.
94+
* The token field is NOT guaranteed to be NULL-terminated.
9595
*
96-
* To keep this table reasonably small, we divide the lexval for TZ and DTZ
97-
* entries by 15 (so they are on 15 minute boundaries) and truncate the text
96+
* To keep this table reasonably small, we divide the value for TZ and DTZ
97+
* entries by 15 (so they are on 15 minute boundaries) and truncate the token
9898
* field at TOKMAXLEN characters.
9999
* Formerly, we divided by 10 rather than 15 but there are a few time zones
100100
* which are 30 or 45 minutes away from an even hour, most are on an hour
@@ -109,7 +109,7 @@ static datetkn *timezonetktbl = NULL;
109109
static int sztimezonetktbl = 0;
110110

111111
static const datetkn datetktbl[] = {
112-
/* text, token, lexval */
112+
/* token, type, value */
113113
{EARLY, RESERV, DTK_EARLY}, /* "-infinity" reserved for "early time" */
114114
{DA_D, ADBC, AD}, /* "ad" for years > 0 */
115115
{"allballs", RESERV, DTK_ZULU}, /* 00:00:00 */
@@ -189,7 +189,7 @@ static const datetkn datetktbl[] = {
189189
static int szdatetktbl = sizeof datetktbl / sizeof datetktbl[0];
190190

191191
static datetkn deltatktbl[] = {
192-
/* text, token, lexval */
192+
/* token, type, value */
193193
{"@", IGNORE_DTF, 0}, /* postgres relative prefix */
194194
{DAGO, AGO, 0}, /* "ago" indicates negative time offset */
195195
{"c", UNITS, DTK_CENTURY}, /* "century" relative */
@@ -4213,6 +4213,7 @@ ConvertTimeZoneAbbrevs(TimeZoneAbbrevTable *tbl,
42134213
tbl->numabbrevs = n;
42144214
for (i = 0; i < n; i++)
42154215
{
4216+
/* do NOT use strlcpy here; token field need not be null-terminated */
42164217
strncpy(newtbl[i].token, abbrevs[i].abbrev, TOKMAXLEN);
42174218
newtbl[i].type = abbrevs[i].is_dst ? DTZ : TZ;
42184219
TOVAL(&newtbl[i], abbrevs[i].offset / MINS_PER_HOUR);

‎src/bin/initdb/findtimezone.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ pg_open_tzfile(const char *name, char *canonname)
6868
if (canonname)
6969
strlcpy(canonname, name, TZ_STRLEN_MAX + 1);
7070

71-
strcpy(fullname, pg_TZDIR());
71+
strlcpy(fullname, pg_TZDIR(), sizeof(fullname));
7272
if (strlen(fullname) + 1 + strlen(name) >= MAXPGPATH)
7373
return -1; /* not gonna fit */
7474
strcat(fullname, "/");
@@ -375,7 +375,7 @@ identify_system_timezone(void)
375375
}
376376

377377
/* Search for the best-matching timezone file */
378-
strcpy(tmptzdir, pg_TZDIR());
378+
strlcpy(tmptzdir, pg_TZDIR(), sizeof(tmptzdir));
379379
bestscore = -1;
380380
resultbuf[0] = '\0';
381381
scan_available_timezones(tmptzdir, tmptzdir + strlen(tmptzdir) + 1,

‎src/bin/pg_basebackup/pg_basebackup.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -903,9 +903,9 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res, int rownum)
903903
FILE *file = NULL;
904904

905905
if (basetablespace)
906-
strcpy(current_path, basedir);
906+
strlcpy(current_path, basedir, sizeof(current_path));
907907
else
908-
strcpy(current_path, PQgetvalue(res, rownum, 1));
908+
strlcpy(current_path, PQgetvalue(res, rownum, 1), sizeof(current_path));
909909

910910
/*
911911
* Get the COPY data
@@ -1432,7 +1432,7 @@ BaseBackup(void)
14321432
disconnect_and_exit(1);
14331433
}
14341434

1435-
strcpy(xlogstart, PQgetvalue(res, 0, 0));
1435+
strlcpy(xlogstart, PQgetvalue(res, 0, 0), sizeof(xlogstart));
14361436

14371437
/*
14381438
* 9.3 and later sends the TLI of the starting point. With older servers,
@@ -1544,7 +1544,7 @@ BaseBackup(void)
15441544
progname);
15451545
disconnect_and_exit(1);
15461546
}
1547-
strcpy(xlogend, PQgetvalue(res, 0, 0));
1547+
strlcpy(xlogend, PQgetvalue(res, 0, 0), sizeof(xlogend));
15481548
if (verbose && includewal)
15491549
fprintf(stderr, "transaction log end point: %s\n", xlogend);
15501550
PQclear(res);

‎src/interfaces/ecpg/preproc/pgc.l

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1315,7 +1315,7 @@ parse_include(void)
13151315
yytext[i] = '\0';
13161316
memmove(yytext, yytext+1, strlen(yytext));
13171317

1318-
strncpy(inc_file, yytext, sizeof(inc_file));
1318+
strlcpy(inc_file, yytext, sizeof(inc_file));
13191319
yyin = fopen(inc_file, "r");
13201320
if (!yyin)
13211321
{

‎src/interfaces/libpq/fe-protocol2.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -500,7 +500,7 @@ pqParseInput2(PGconn *conn)
500500
if (!conn->result)
501501
return;
502502
}
503-
strncpy(conn->result->cmdStatus, conn->workBuffer.data,
503+
strlcpy(conn->result->cmdStatus, conn->workBuffer.data,
504504
CMDSTATUS_LEN);
505505
checkXactStatus(conn, conn->workBuffer.data);
506506
conn->asyncStatus = PGASYNC_READY;

‎src/interfaces/libpq/fe-protocol3.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ pqParseInput3(PGconn *conn)
206206
if (!conn->result)
207207
return;
208208
}
209-
strncpy(conn->result->cmdStatus, conn->workBuffer.data,
209+
strlcpy(conn->result->cmdStatus, conn->workBuffer.data,
210210
CMDSTATUS_LEN);
211211
conn->asyncStatus = PGASYNC_READY;
212212
break;

‎src/port/exec.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ validate_exec(const char *path)
6868
if (strlen(path) >= strlen(".exe") &&
6969
pg_strcasecmp(path + strlen(path) - strlen(".exe"), ".exe") != 0)
7070
{
71-
strcpy(path_exe, path);
71+
strlcpy(path_exe, path, sizeof(path_exe) - 4);
7272
strcat(path_exe, ".exe");
7373
path = path_exe;
7474
}
@@ -277,7 +277,7 @@ resolve_symlinks(char *path)
277277
}
278278

279279
/* must copy final component out of 'path' temporarily */
280-
strcpy(link_buf, fname);
280+
strlcpy(link_buf, fname, sizeof(link_buf));
281281

282282
if (!getcwd(path, MAXPGPATH))
283283
{

‎src/test/regress/pg_regress.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1233,7 +1233,7 @@ results_differ(const char *testname, const char *resultsfile, const char *defaul
12331233
*/
12341234
platform_expectfile = get_expectfile(testname, resultsfile);
12351235

1236-
strcpy(expectfile, default_expectfile);
1236+
strlcpy(expectfile, default_expectfile, sizeof(expectfile));
12371237
if (platform_expectfile)
12381238
{
12391239
/*
@@ -1288,7 +1288,7 @@ results_differ(const char *testname, const char *resultsfile, const char *defaul
12881288
{
12891289
/* This diff was a better match than the last one */
12901290
best_line_count = l;
1291-
strcpy(best_expect_file, alt_expectfile);
1291+
strlcpy(best_expect_file, alt_expectfile, sizeof(best_expect_file));
12921292
}
12931293
free(alt_expectfile);
12941294
}
@@ -1316,7 +1316,7 @@ results_differ(const char *testname, const char *resultsfile, const char *defaul
13161316
{
13171317
/* This diff was a better match than the last one */
13181318
best_line_count = l;
1319-
strcpy(best_expect_file, default_expectfile);
1319+
strlcpy(best_expect_file, default_expectfile, sizeof(best_expect_file));
13201320
}
13211321
}
13221322

‎src/timezone/pgtz.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ pg_open_tzfile(const char *name, char *canonname)
8383
* Loop to split the given name into directory levels; for each level,
8484
* search using scan_directory_ci().
8585
*/
86-
strcpy(fullname, pg_TZDIR());
86+
strlcpy(fullname, pg_TZDIR(), sizeof(fullname));
8787
orignamelen = fullnamelen = strlen(fullname);
8888
fname = name;
8989
for (;;)

0 commit comments

Comments
 (0)