From owner-svn-src-stable-10@freebsd.org Tue Jun 7 16:56:16 2016 Return-Path: Delivered-To: svn-src-stable-10@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 95F3AB6EA9D; Tue, 7 Jun 2016 16:56:16 +0000 (UTC) (envelope-from truckman@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 7246F1752; Tue, 7 Jun 2016 16:56:16 +0000 (UTC) (envelope-from truckman@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id u57GuFpT057463; Tue, 7 Jun 2016 16:56:15 GMT (envelope-from truckman@FreeBSD.org) Received: (from truckman@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id u57GuFr9057462; Tue, 7 Jun 2016 16:56:15 GMT (envelope-from truckman@FreeBSD.org) Message-Id: <201606071656.u57GuFr9057462@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: truckman set sender to truckman@FreeBSD.org using -f From: Don Lewis Date: Tue, 7 Jun 2016 16:56:15 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-10@freebsd.org Subject: svn commit: r301557 - stable/10/usr.sbin/pw X-SVN-Group: stable-10 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-stable-10@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: SVN commit messages for only the 10-stable src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 Jun 2016 16:56:16 -0000 Author: truckman Date: Tue Jun 7 16:56:15 2016 New Revision: 301557 URL: https://svnweb.freebsd.org/changeset/base/301557 Log: MFC r300564 Fix CID 1006692 in /usr/sbin/pw pw_log() function and other fixes The length of the name returned from the $LOGNAME and $USER can be very long and it was being concatenated to a fixed length buffer with no bounds checking. Fix this problem by limiting the length of the name copied. Additionally, this name is actually used to create a format string to be used in adding log file entries so embedded % characters in the name could confuse *printf(), and embedded whitespace could confuse a log file parser. Handle the former by escaping each % with an additional %, and handle the latter by simply stripping it out. Clean up the code by moving the variable declarations to the top of the function, formatting them to conform with style, and moving intialization elsewhere. Reduce code indentation by returning early in a couple of places. Reported by: Coverity CID: 1006692 Reviewed by: markj (previous version) Differential Revision: https://reviews.freebsd.org/D6490 Modified: stable/10/usr.sbin/pw/pw_log.c Directory Properties: stable/10/ (props changed) Modified: stable/10/usr.sbin/pw/pw_log.c ============================================================================== --- stable/10/usr.sbin/pw/pw_log.c Tue Jun 7 16:53:05 2016 (r301556) +++ stable/10/usr.sbin/pw/pw_log.c Tue Jun 7 16:56:15 2016 (r301557) @@ -29,40 +29,90 @@ static const char rcsid[] = "$FreeBSD$"; #endif /* not lint */ +#include +#include #include #include #include #include "pw.h" -static FILE *logfile = NULL; +static FILE *logfile = NULL; void pw_log(struct userconf * cnf, int mode, int which, char const * fmt,...) { - if (cnf->logfile && *cnf->logfile) { - if (logfile == NULL) { /* With umask==0 we need to control file access modes on create */ - int fd = open(cnf->logfile, O_WRONLY | O_CREAT | O_APPEND, 0600); + va_list argp; + time_t now; + const char *cp, *name; + struct tm *t; + int fd, i, rlen; + char nfmt[256], sname[32]; - if (fd != -1) - logfile = fdopen(fd, "a"); + if (cnf->logfile == NULL || cnf->logfile[0] == '\0') { + return; + } + + if (logfile == NULL) { + /* With umask==0 we need to control file access modes on create */ + fd = open(cnf->logfile, O_WRONLY | O_CREAT | O_APPEND, 0600); + if (fd == -1) { + return; } - if (logfile != NULL) { - va_list argp; - time_t now = time(NULL); - struct tm *t = localtime(&now); - char nfmt[256]; - const char *name; - - if ((name = getenv("LOGNAME")) == NULL && (name = getenv("USER")) == NULL) - name = "unknown"; - /* ISO 8601 International Standard Date format */ - strftime(nfmt, sizeof nfmt, "%Y-%m-%d %T ", t); - sprintf(nfmt + strlen(nfmt), "[%s:%s%s] %s\n", name, Which[which], Modes[mode], fmt); - va_start(argp, fmt); - vfprintf(logfile, nfmt, argp); - va_end(argp); - fflush(logfile); + logfile = fdopen(fd, "a"); + if (logfile == NULL) { + return; } } + + if ((name = getenv("LOGNAME")) == NULL && + (name = getenv("USER")) == NULL) { + strcpy(sname, "unknown"); + } else { + /* + * Since "name" will be embedded in a printf-like format, + * we must sanitize it: + * + * Limit its length so other information in the message + * is not truncated + * + * Squeeze out embedded whitespace for the benefit of + * log file parsers + * + * Escape embedded % characters with another % + */ + for (i = 0, cp = name; + *cp != '\0' && i < (int)sizeof(sname) - 1; cp++) { + if (*cp == '%') { + if (i < (int)sizeof(sname) - 2) { + sname[i++] = '%'; + sname[i++] = '%'; + } else { + break; + } + } else if (!isspace(*cp)) { + sname[i++] = *cp; + } /* else do nothing */ + } + if (i == 0) { + strcpy(sname, "unknown"); + } else { + sname[i] = '\0'; + } + } + now = time(NULL); + t = localtime(&now); + /* ISO 8601 International Standard Date format */ + strftime(nfmt, sizeof nfmt, "%Y-%m-%d %T ", t); + rlen = sizeof(nfmt) - strlen(nfmt); + if (rlen <= 0 || snprintf(nfmt + strlen(nfmt), rlen, + "[%s:%s%s] %s\n", sname, Which[which], Modes[mode], + fmt) >= rlen) { + warnx("log format overflow, user name=%s", sname); + } else { + va_start(argp, fmt); + vfprintf(logfile, nfmt, argp); + va_end(argp); + fflush(logfile); + } }