Page MenuHomeFreeBSD

Prevent buffer overrun in tzsetup (for -C with overly large parameter)
AcceptedPublic

Authored by se on Jan 22 2019, 1:56 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 9, 12:46 PM
Unknown Object (File)
Mon, Mar 4, 8:08 AM
Unknown Object (File)
Feb 22 2024, 8:01 AM
Unknown Object (File)
Jan 25 2024, 7:46 AM
Unknown Object (File)
Jan 14 2024, 6:16 AM
Unknown Object (File)
Dec 20 2023, 4:02 AM
Unknown Object (File)
Dec 4 2023, 5:25 PM
Unknown Object (File)
Nov 15 2023, 4:37 PM
Subscribers

Details

Reviewers
hselasky
Group Reviewers
Src Committers
Summary

The tzsetup command has been included in a CI report due to its use of strcpy(). While this particular use is save and the report is a false positive given the constant strings copied in to a PATH_MAX sized buffer, there is an actual potential buffer overrun nearby where the unbounded size value of chrootenv is assigned to fixed size buffers.

While the strcpy() uses are safe unless the _PATH_* defines are changed, I propose to silence the CI report by use of strlcpy.
The sprintf() calls are replaced by snprintf() to prevent overflow. (Not reported by CI.)

Test Plan

Build the tzsetup binary with the patch.
Assert that there are no compiler warnings/errors.
Verify that the command still operates correctly (finds the required files with and without "-C chrootenv").

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 22072

Event Timeline

This revision is now accepted and ready to land.Jul 31 2019, 9:25 AM
usr.sbin/tzsetup/tzsetup.c
946

You could also compile time assert that the buffer is big enough.

This is a truncation that in my opinion should result in program error.
Also note that for a non-chrooted run the paths could already be in the arrays.
So I would do it more or less like below (untested, compiled):

index 6446800ce51..0220ab83a6f 100644
--- a/usr.sbin/tzsetup/tzsetup.c
+++ b/usr.sbin/tzsetup/tzsetup.c
@@ -74,9 +74,13 @@ __FBSDID("$FreeBSD$");
 #define DITEM_LEAVE_MENU        (1 << 16)
 #define DITEM_RECREATE          (1 << 18)

-static char    path_zonetab[MAXPATHLEN], path_iso3166[MAXPATHLEN],
-               path_zoneinfo[MAXPATHLEN], path_localtime[MAXPATHLEN],
-               path_db[MAXPATHLEN], path_wall_cmos_clock[MAXPATHLEN];
+static char
+               path_zonetab[MAXPATHLEN] = _PATH_ZONETAB,
+               path_iso3166[MAXPATHLEN] = _PATH_ISO3166,
+               path_zoneinfo[MAXPATHLEN] = _PATH_ZONEINFO,
+               path_localtime[MAXPATHLEN] = _PATH_LOCALTIME,
+               path_db[MAXPATHLEN] = _PATH_DB,
+               path_wall_cmos_clock[MAXPATHLEN] = _PATH_WALL_CMOS_CLOCK;

 static int reallydoit = 1;
 static int reinstall = 0;
@@ -943,23 +947,21 @@ main(int argc, char **argv)
        if (argc - optind > 1)
                usage();

-       if (chrootenv == NULL) {
-               strcpy(path_zonetab, _PATH_ZONETAB);
-               strcpy(path_iso3166, _PATH_ISO3166);
-               strcpy(path_zoneinfo, _PATH_ZONEINFO);
-               strcpy(path_localtime, _PATH_LOCALTIME);
-               strcpy(path_db, _PATH_DB);
-               strcpy(path_wall_cmos_clock, _PATH_WALL_CMOS_CLOCK);
-       } else {
-               sprintf(path_zonetab, "%s/%s", chrootenv, _PATH_ZONETAB);
-               sprintf(path_iso3166, "%s/%s", chrootenv, _PATH_ISO3166);
-               sprintf(path_zoneinfo, "%s/%s", chrootenv, _PATH_ZONEINFO);
-               sprintf(path_localtime, "%s/%s", chrootenv, _PATH_LOCALTIME);
-               sprintf(path_db, "%s/%s", chrootenv, _PATH_DB);
-               sprintf(path_wall_cmos_clock, "%s/%s", chrootenv,
-                   _PATH_WALL_CMOS_CLOCK);
+#define SPRINTF(a, p) do { \
+       int r = snprintf(a, sizeof(a), "%s/%s", chrootenv, p); \
+       if (r < 0 || r >= (int)sizeof(a)) \
+               errx(1, "Chroot path %s too long.\n", chrootenv); \
+       } while (0);
+
+       if (chrootenv != NULL) {
+               SPRINTF(path_zonetab, _PATH_ZONETAB);
+               SPRINTF(path_iso3166, _PATH_ISO3166);
+               SPRINTF(path_zoneinfo, _PATH_ZONEINFO);
+               SPRINTF(path_localtime, _PATH_LOCALTIME);
+               SPRINTF(path_db, _PATH_DB);
+               SPRINTF(path_wall_cmos_clock, _PATH_WALL_CMOS_CLOCK);
        }
-
+#undef SPRINTF
        /* Override the user-supplied umask. */
        (void)umask(S_IWGRP | S_IWOTH);