From fcc6541fdee2af8005766c3cd4b81e5edd8ae5ea Mon Sep 17 00:00:00 2001 From: Jeremy Date: Tue, 1 Dec 2015 12:27:29 -0500 Subject: [PATCH 1/6] src/common/compat.c:tor_vasprintf() - changed vsnprintf() to tor_vsnprintf() which ensures string is null terminated. --- src/common/compat.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/common/compat.c b/src/common/compat.c index 7d72b4b7fd..6f357530a6 100644 --- a/src/common/compat.c +++ b/src/common/compat.c @@ -580,7 +580,8 @@ tor_vasprintf(char **strp, const char *fmt, va_list args) return len; } strp_tmp = tor_malloc(len+1); - r = vsnprintf(strp_tmp, len+1, fmt, args); + /* use of tor_vsnprintf() will ensure string is null terminated */ + r = tor_vsnprintf(strp_tmp, len+1, fmt, args); if (r != len) { tor_free(strp_tmp); *strp = NULL; From 86a5305d46175c5d0c67564d3ee4e86a27f0c460 Mon Sep 17 00:00:00 2001 From: Jeremy Date: Tue, 1 Dec 2015 12:29:08 -0500 Subject: [PATCH 2/6] ext/eventdns.c multiple replacements of snprintf() with tor_snprintf() which always null terminates and returns -1 if result is truncated. --- src/ext/eventdns.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/ext/eventdns.c b/src/ext/eventdns.c index a0c7ff29fa..1894cbea53 100644 --- a/src/ext/eventdns.c +++ b/src/ext/eventdns.c @@ -388,7 +388,7 @@ debug_ntoa(u32 address) { static char buf[32]; u32 a = ntohl(address); - snprintf(buf, sizeof(buf), "%d.%d.%d.%d", + tor_snprintf(buf, sizeof(buf), "%d.%d.%d.%d", (int)(u8)((a>>24)&0xff), (int)(u8)((a>>16)&0xff), (int)(u8)((a>>8 )&0xff), @@ -436,12 +436,7 @@ evdns_log(int warn, const char *fmt, ...) if (!evdns_log_fn) return; va_start(args,fmt); -#ifdef _WIN32 - _vsnprintf(buf, sizeof(buf), fmt, args); -#else - vsnprintf(buf, sizeof(buf), fmt, args); -#endif - buf[sizeof(buf)-1] = '\0'; + tor_vsnprintf(buf, sizeof(buf), fmt, args); evdns_log_fn(warn, buf); va_end(args); } @@ -762,7 +757,7 @@ reply_handle(struct evdns_request *const req, u16 flags, u32 ttl, struct reply * /* we regard these errors as marking a bad nameserver */ if (req->reissue_count < global_max_reissues) { char msg[64]; - snprintf(msg, sizeof(msg), "Bad response %d (%s)", + tor_snprintf(msg, sizeof(msg), "Bad response %d (%s)", error, evdns_err_to_string(error)); nameserver_failed(req->ns, msg); if (!request_reissue(req)) return; @@ -1705,7 +1700,7 @@ evdns_server_request_add_ptr_reply(struct evdns_server_request *req, struct in_a assert(!(in && inaddr_name)); if (in) { a = ntohl(in->s_addr); - snprintf(buf, sizeof(buf), "%d.%d.%d.%d.in-addr.arpa", + tor_snprintf(buf, sizeof(buf), "%d.%d.%d.%d.in-addr.arpa", (int)(u8)((a )&0xff), (int)(u8)((a>>8 )&0xff), (int)(u8)((a>>16)&0xff), @@ -2638,7 +2633,7 @@ int evdns_resolve_reverse(const struct in_addr *in, int flags, evdns_callback_ty u32 a; assert(in); a = ntohl(in->s_addr); - snprintf(buf, sizeof(buf), "%d.%d.%d.%d.in-addr.arpa", + tor_snprintf(buf, sizeof(buf), "%d.%d.%d.%d.in-addr.arpa", (int)(u8)((a )&0xff), (int)(u8)((a>>8 )&0xff), (int)(u8)((a>>16)&0xff), From b3639c8291098eeeefb166914bad98a53e506b90 Mon Sep 17 00:00:00 2001 From: Jeremy Date: Tue, 1 Dec 2015 13:00:58 -0500 Subject: [PATCH 3/6] src/common/compat.c:tor_vasprintf() - vsnprintf() was properly checked but tor_vsnprintf() available so why not use it? --- src/common/compat.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/common/compat.c b/src/common/compat.c index 6f357530a6..e8b0897f59 100644 --- a/src/common/compat.c +++ b/src/common/compat.c @@ -573,7 +573,9 @@ tor_vasprintf(char **strp, const char *fmt, va_list args) int len, r; va_list tmp_args; va_copy(tmp_args, args); - len = vsnprintf(buf, sizeof(buf), fmt, tmp_args); + /* vsnprintf() was properly checked but tor_vsnprintf() available so + * why not use it? */ + len = tor_vsnprintf(buf, sizeof(buf), fmt, tmp_args); va_end(tmp_args); if (len < (int)sizeof(buf)) { *strp = tor_strdup(buf); From 4e19133dccfc4d252e9ed2695f6fe49bb4503ac8 Mon Sep 17 00:00:00 2001 From: Jeremy Date: Tue, 1 Dec 2015 15:41:03 -0500 Subject: [PATCH 4/6] src/common/util.c:expand_filename() - Perhaps use GetFullPathName() as a form of input validation on the filename argument. --- src/common/util.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/common/util.c b/src/common/util.c index b33c80fd45..e8044f9089 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -2873,6 +2873,9 @@ expand_filename(const char *filename) { tor_assert(filename); #ifdef _WIN32 + /* Might consider using GetFullPathName() as described here: + * http://etutorials.org/Programming/secure+programming/Chapter+3.+Input+Validation/3.7+Validating+Filenames+and+Paths/ + */ return tor_strdup(filename); #else if (*filename == '~') { From f48c607fd970aedaf0180a0a23b04eb5101abca0 Mon Sep 17 00:00:00 2001 From: Jeremy Date: Tue, 8 Dec 2015 13:25:15 -0500 Subject: [PATCH 5/6] Harden check_private_dir() to remove any potential race. Remove any potential race between stat() and chmod(). Replace stat() with fstat(). Replace chmod() with fchmod() --- src/common/util.c | 70 ++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 60 insertions(+), 10 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index e8044f9089..305b6ae495 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -2029,9 +2029,10 @@ int check_private_dir(const char *dirname, cpd_check_t check, const char *effective_user) { + int fd; int r; struct stat st; - char *f; + //char *f; #ifndef _WIN32 unsigned unwanted_bits = 0; const struct passwd *pw = NULL; @@ -2041,18 +2042,34 @@ check_private_dir(const char *dirname, cpd_check_t check, (void)effective_user; #endif + /* + * Goal is to harden the implementation by removing any + * potential for race between stat() and chmod(). + * chmod() accepts filename as argument. If an attacker can move + * the file between stat() and chmod(), a potential race exists. + * + * Several suggestions taken from: + * https://developer.apple.com/library/mac/documentation/Security/Conceptual/SecureCodingGuide/Articles/RaceConditions.html + */ tor_assert(dirname); - f = tor_strdup(dirname); - clean_name_for_stat(f); - log_debug(LD_FS, "stat()ing %s", f); - r = stat(sandbox_intern_string(f), &st); - tor_free(f); - if (r) { + + /* Open directory. + * O_NOFOLLOW to ensure that it does not follow symbolic links */ + fd = open(sandbox_intern_string(dirname), O_NOFOLLOW); + + /* Was there an error? Maybe the directory does not exist? */ + if (fd == -1) { + if (errno != ENOENT) { + /* Other directory error */ log_warn(LD_FS, "Directory %s cannot be read: %s", dirname, strerror(errno)); return -1; } + + /* Received ENOENT: Directory does not exist */ + + /* Should we create the directory? */ if (check & CPD_CREATE) { log_info(LD_GENERAL, "Creating directory %s", dirname); #if defined (_WIN32) @@ -2064,23 +2081,51 @@ check_private_dir(const char *dirname, cpd_check_t check, r = mkdir(dirname, 0700); } #endif + + /* check for mkdir() error */ if (r) { log_warn(LD_FS, "Error creating directory %s: %s", dirname, strerror(errno)); return -1; } - } else if (!(check & CPD_CHECK)) { + + /* we just created the directory. try to open it again. + * permissions on the directory will be checked again below.*/ + fd = open(sandbox_intern_string(dirname), O_NOFOLLOW); + + if ( fd == -1 ) return -1; + + } else if (!(check & CPD_CHECK)) { log_warn(LD_FS, "Directory %s does not exist.", dirname); return -1; } + /* XXXX In the case where check==CPD_CHECK, we should look at the * parent directory a little harder. */ return 0; } + + tor_assert(fd); + + //f = tor_strdup(dirname); + //clean_name_for_stat(f); + log_debug(LD_FS, "stat()ing %s", dirname); + //r = stat(sandbox_intern_string(f), &st); + r = fstat(fd, &st); + if (r == -1) { + log_warn(LD_FS, "fstat() on directory %s failed.", dirname); + close(fd); + return -1; + } + //tor_free(f); + + /* check that dirname is a directory */ if (!(st.st_mode & S_IFDIR)) { log_warn(LD_FS, "%s is not a directory", dirname); + close(fd); return -1; } + #ifndef _WIN32 if (effective_user) { /* Look up the user and group information. @@ -2097,7 +2142,6 @@ check_private_dir(const char *dirname, cpd_check_t check, running_uid = getuid(); running_gid = getgid(); } - if (st.st_uid != running_uid) { const struct passwd *pw = NULL; char *process_ownername = NULL; @@ -2113,6 +2157,7 @@ check_private_dir(const char *dirname, cpd_check_t check, pw ? pw->pw_name : "", (int)st.st_uid); tor_free(process_ownername); + close(fd); return -1; } if ( (check & (CPD_GROUP_OK|CPD_GROUP_READ)) @@ -2129,6 +2174,7 @@ check_private_dir(const char *dirname, cpd_check_t check, gr ? gr->gr_name : "", (int)st.st_gid); tor_free(process_groupname); + close(fd); return -1; } if (check & (CPD_GROUP_OK|CPD_GROUP_READ)) { @@ -2141,6 +2187,7 @@ check_private_dir(const char *dirname, cpd_check_t check, if (check & CPD_CHECK_MODE_ONLY) { log_warn(LD_FS, "Permissions on directory %s are too permissive.", dirname); + close(fd); return -1; } log_warn(LD_FS, "Fixing permissions on directory %s", dirname); @@ -2150,15 +2197,18 @@ check_private_dir(const char *dirname, cpd_check_t check, new_mode |= 0050; /* Group should have rx */ } new_mode &= ~unwanted_bits; /* Clear the bits that we didn't want set...*/ - if (chmod(dirname, new_mode)) { + if (fchmod(fd, new_mode)) { log_warn(LD_FS, "Could not chmod directory %s: %s", dirname, strerror(errno)); + close(fd); return -1; } else { + close(fd); return 0; } } #endif + close(fd); return 0; } From 75daeb5c6d9dd549b92f20edbd795c85e451cbcd Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 16 Feb 2016 11:33:21 -0500 Subject: [PATCH 6/6] changes file for bug17852 --- changes/bug17852 | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 changes/bug17852 diff --git a/changes/bug17852 b/changes/bug17852 new file mode 100644 index 0000000000..b36b55f9d6 --- /dev/null +++ b/changes/bug17852 @@ -0,0 +1,10 @@ + o Minor features (code hardening): + - Use tor_snprintf() and tor_vsnprintf() even in external and + low-level code, to harden against accidental failures to NUL- + terminate. Part of ticket 17852. Patch from 'jsturgix'. Found + with Flawfinder. + + o Minor bugfixes (private directory): + - Prevent a race condition when creating private directories. + Fixes part of bug 17852; bugfix on 0.2pre13. Part of ticket + 17852. Patch from 'jsturgix'. Found with Flawfinder.