Date: Thu, 6 Dec 2012 11:36:24 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Pawel Jakub Dawidek <pjd@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Eitan Adler <eadler@FreeBSD.org> Subject: Re: svn commit: r243895 - head/usr.sbin/pw Message-ID: <20121206104345.S1043@besplex.bde.org> In-Reply-To: <20121205204030.GB1423@garage.freebsd.pl> References: <201212051356.qB5Duks1068301@svn.freebsd.org> <20121205204030.GB1423@garage.freebsd.pl>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 5 Dec 2012, Pawel Jakub Dawidek wrote:
> On Wed, Dec 05, 2012 at 01:56:46PM +0000, Eitan Adler wrote:
>>
>> Log:
>> Avoid overflow of file buffer
>
> Buffer won't overflow, but the path will be truncated, which is neither
> detected nor handled.
Good enough for software not related to security :-).
>> Submitted by: db
>> Approved by: cperciva
>> MFC after: 2 weeks
>>
>> Modified:
>> head/usr.sbin/pw/pw_user.c
>>
>> Modified: head/usr.sbin/pw/pw_user.c
>> ==============================================================================
>> --- head/usr.sbin/pw/pw_user.c Wed Dec 5 13:56:43 2012 (r243894)
>> +++ head/usr.sbin/pw/pw_user.c Wed Dec 5 13:56:46 2012 (r243895)
>> @@ -394,7 +394,7 @@ pw_user(struct userconf * cnf, int mode,
>> /*
>> * Remove crontabs
>> */
>> - sprintf(file, "/var/cron/tabs/%s", pwd->pw_name);
The old version could have used strcpy() followed by strcat() to be equally
insecure. sprintf() was used for convenience.
>> + snprintf(file, sizeof(file), "/var/cron/tabs/%s", pwd->pw_name);
The new version could use strcpy() followed strlcat() without checking the
return value to be equally insecure. Now using snprintf() is not very
convenient if you actually check its return value.
Style bug: a line 1 longer than 80 columns was expanded to be much longer.
>> if (access(file, F_OK) == 0) {
This check ameliorates the previous bugs -- most corrupted strings will point
to a file that doesn't exist, so we reach here.
>> sprintf(file, "crontab -u %s -r", pwd->pw_name);
Why bother fixing the above without touching this? This may overrun the
buffer too. The fixed part of the string happens to be 1 shorter, so that
if the above didn't overrun, then this won't either.
Style bugs:
- a line longer than 80 characters
- the variable 'file' is abused by reusing it to hold a non-file. `file'
has size MAXPATHLEN, so it is good enough for holding a pathname
(syscalls would fail if it were larger and the extra space was used),
but it is a little too small for a command containing most of the
failing pathname.
>> system(file);
pw_user.c still uses many other similar sprintf()'s. Some of them are:
% char file[MAXPATHLEN];
% char home[MAXPATHLEN];
% strlcpy(home, pwd->pw_dir, sizeof(home));
Truncation using strlcpy() is also popular. pw_user.c already uses
strlcpy() a lot. It never checks for truncation of course. I only
quoted strlcpy() here because I misread it at first as being part of
a silly combination with sprintf(), with sprintf() to provide the
buffer overflow and strlcpy() to provide the truncation, and 2 calls
used to build up a string that could be built with 1 snprintf().
pw_user.c isn't actually that silly. It just has a weird mixture
of insecure strlcpy()'s with more-insecure sprintf()'s, after blind
replacement of strncpy()'s with unchecked strlcpy()'s.
% sprintf(line, "%s/%s", _PATH_MAILDIR, pwd->pw_name);
% sprintf(file, "%s/%s", _PATH_MAILDIR, pwd->pw_name);
% sprintf(home, "%s/%s", cnf->home, user);
% static char shellpath[256];
% sprintf(shellpath, "%s/%s", p, sh);
% sprintf(shellpath, "%s/%s", p, shells[i]);
shellpath is much shorter than MAXPATHLEN, so even useful strings might
overrun it.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121206104345.S1043>