Repository: trafficserver Updated Branches: refs/heads/master fd08c6efc -> 2c1bdddc5
TS-4134: Fix privilege elevation issues. This closes #425. Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/2c1bdddc Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/2c1bdddc Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/2c1bdddc Branch: refs/heads/master Commit: 2c1bdddc5e0946ef2fca0d821603e03f3d19cef2 Parents: fd08c6e Author: Alan M. Carroll <[email protected]> Authored: Thu Jan 14 19:28:44 2016 -0600 Committer: James Peach <[email protected]> Committed: Sat Jan 16 10:56:36 2016 -0800 ---------------------------------------------------------------------- iocore/net/SSLUtils.cc | 4 +- lib/ts/BaseLogFile.cc | 7 +-- lib/ts/Diags.cc | 7 +-- lib/ts/ink_cap.cc | 97 +++++++++++++++++++++++++----------- lib/ts/ink_cap.h | 23 +++++++-- mgmt/LocalManager.cc | 3 +- mgmt/Rollback.cc | 4 +- proxy/Main.cc | 18 ------- proxy/Plugin.cc | 4 +- proxy/http/remap/RemapConfig.cc | 4 +- 10 files changed, 100 insertions(+), 71 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/trafficserver/blob/2c1bdddc/iocore/net/SSLUtils.cc ---------------------------------------------------------------------- diff --git a/iocore/net/SSLUtils.cc b/iocore/net/SSLUtils.cc index 03d1b4e..91d5486 100644 --- a/iocore/net/SSLUtils.cc +++ b/iocore/net/SSLUtils.cc @@ -1874,12 +1874,10 @@ SSLParseCertificateConfiguration(const SSLConfigParams *params, SSLCertLookup *l return false; } -#if TS_USE_POSIX_CAP // elevate/allow file access to root read only files/certs uint32_t elevate_setting = 0; REC_ReadConfigInteger(elevate_setting, "proxy.config.ssl.cert.load_elevated"); - ElevateAccess elevate_access(elevate_setting != 0); // destructor will demote for us -#endif /* TS_USE_POSIX_CAP */ + ElevateAccess elevate_access(elevate_setting ? ElevateAccess::FILE_PRIVILEGE : 0); // destructor will demote for us line = tokLine(file_buf, &tok_state); while (line != NULL) { http://git-wip-us.apache.org/repos/asf/trafficserver/blob/2c1bdddc/lib/ts/BaseLogFile.cc ---------------------------------------------------------------------- diff --git a/lib/ts/BaseLogFile.cc b/lib/ts/BaseLogFile.cc index f134269..349da82 100644 --- a/lib/ts/BaseLogFile.cc +++ b/lib/ts/BaseLogFile.cc @@ -323,7 +323,8 @@ BaseLogFile::open_file(int perm) // open actual log file (not metainfo) log_log_trace("BaseLogFile: attempting to open %s\n", m_name.get()); - m_fp = fopen(m_name.get(), "a+"); + + m_fp = elevating_fopen(m_name.get(), "a+"); // error check if (m_fp) { @@ -478,7 +479,7 @@ void BaseMetaInfo::_read_from_file() { _flags |= DATA_FROM_METAFILE; // mark attempt - int fd = open(_filename, O_RDONLY); + int fd = elevating_open(_filename, O_RDONLY); if (fd < 0) { log_log_error("Could not open metafile %s for reading: %s\n", _filename, strerror(errno)); } else { @@ -522,7 +523,7 @@ BaseMetaInfo::_read_from_file() void BaseMetaInfo::_write_to_file() { - int fd = open(_filename, O_WRONLY | O_CREAT | O_TRUNC, LOGFILE_DEFAULT_PERMS); + int fd = elevating_open(_filename, O_WRONLY | O_CREAT | O_TRUNC, LOGFILE_DEFAULT_PERMS); if (fd < 0) { log_log_error("Could not open metafile %s for writing: %s\n", _filename, strerror(errno)); return; http://git-wip-us.apache.org/repos/asf/trafficserver/blob/2c1bdddc/lib/ts/Diags.cc ---------------------------------------------------------------------- diff --git a/lib/ts/Diags.cc b/lib/ts/Diags.cc index 7e5747b..cda43b0 100644 --- a/lib/ts/Diags.cc +++ b/lib/ts/Diags.cc @@ -821,9 +821,6 @@ Diags::set_stdout_output(const char *_bind_stdout) stdout_log = NULL; } - // get root - ElevateAccess elevate; - // create backing BaseLogFile for stdout stdout_log = new BaseLogFile(_bind_stdout); @@ -860,9 +857,7 @@ Diags::set_stderr_output(const char *_bind_stderr) delete stderr_log; stderr_log = NULL; } - // get root - ElevateAccess elevate; - + // create backing BaseLogFile for stdout stderr_log = new BaseLogFile(_bind_stderr); http://git-wip-us.apache.org/repos/asf/trafficserver/blob/2c1bdddc/lib/ts/ink_cap.cc ---------------------------------------------------------------------- diff --git a/lib/ts/ink_cap.cc b/lib/ts/ink_cap.cc index 01e5d5e..625903d 100644 --- a/lib/ts/ink_cap.cc +++ b/lib/ts/ink_cap.cc @@ -307,37 +307,74 @@ EnableDeathSignal(int signum) #endif } +int +elevating_open(char const *path, unsigned int flags, unsigned int fperms) +{ + int fd = open(path, flags, fperms); + if (fd < 0 && (EPERM == errno || EACCES == errno)) { + ElevateAccess access(ElevateAccess::FILE_PRIVILEGE); + fd = open(path, flags, fperms); + } + return fd; +} + +int +elevating_open(char const *path, unsigned int flags) +{ + int fd = open(path, flags); + if (fd < 0 && (EPERM == errno || EACCES == errno)) { + ElevateAccess access(ElevateAccess::FILE_PRIVILEGE); + fd = open(path, flags); + } + return fd; +} + +FILE* +elevating_fopen(char const *path, const char* mode) +{ + FILE* f = fopen(path, mode); + if (NULL == f && (EPERM == errno || EACCES == errno)) { + ElevateAccess access(ElevateAccess::FILE_PRIVILEGE); + f = fopen(path, mode); + } + return f; +} + #if TS_USE_POSIX_CAP /** Acquire file access privileges to bypass DAC. @a level is a mask of the specific file access capabilities to acquire. */ void -ElevateAccess::acquireFileAccessCap(unsigned level) +ElevateAccess::acquirePrivilege(unsigned priv_mask) { unsigned cap_count = 0; cap_value_t cap_list[2]; cap_t new_cap_state; - Debug("privileges", "[acquireFileAccessCap] level= %x\n", level); + Debug("privileges", "[acquirePrivilege] level= %x\n", level); ink_assert(NULL == cap_state); - if (level) { - this->cap_state = cap_get_proc(); // save current capabilities - new_cap_state = cap_get_proc(); // and another instance to modify. + // Some privs aren't checked or used here because they are kept permanently in the + // the capability list. See @a eff_list in @c RestrictCapabilities + // It simplifies things elsewhere to be able to specify them so that the cases for + // POSIX capabilities and user impersonation have the same interface. - if (level & ElevateAccess::FILE_PRIVILEGE) { - cap_list[cap_count] = CAP_DAC_OVERRIDE; - ++cap_count; - } + if (priv_mask & ElevateAccess::FILE_PRIVILEGE) { + cap_list[cap_count] = CAP_DAC_OVERRIDE; + ++cap_count; + } - if (level & ElevateAccess::TRACE_PRIVILEGE) { - cap_list[cap_count] = CAP_SYS_PTRACE; - ++cap_count; - } + if (priv_mask & ElevateAccess::TRACE_PRIVILEGE) { + cap_list[cap_count] = CAP_SYS_PTRACE; + ++cap_count; + } - ink_release_assert(cap_count <= sizeof(cap_list)); + ink_release_assert(cap_count <= sizeof(cap_list)); + if (cap_count > 0) { + this->cap_state = cap_get_proc(); // save current capabilities + new_cap_state = cap_get_proc(); // and another instance to modify. cap_set_flag(new_cap_state, CAP_EFFECTIVE, cap_count, cap_list, CAP_SET); if (cap_set_proc(new_cap_state) != 0) { @@ -345,12 +382,13 @@ ElevateAccess::acquireFileAccessCap(unsigned level) } cap_free(new_cap_state); + elevated = true; } } /** Restore previous capabilities. */ void -ElevateAccess::releaseFileAccessCap() +ElevateAccess::releasePrivilege() { Debug("privileges", "[releaseFileAccessCap]"); @@ -379,7 +417,7 @@ ElevateAccess::ElevateAccess(unsigned lvl) ElevateAccess::~ElevateAccess() { - if (elevated == true) { + if (elevated) { demote(); #if !TS_USE_POSIX_CAP DEBUG_CREDENTIALS("privileges"); @@ -389,28 +427,31 @@ ElevateAccess::~ElevateAccess() } void -ElevateAccess::elevate(unsigned level) +ElevateAccess::elevate(unsigned priv_mask) { #if TS_USE_POSIX_CAP - acquireFileAccessCap(level); + acquirePrivilege(priv_mask); #else - (void)level; - // Since we are setting a process-wide credential, we have to block any other thread - // attempting to elevate until this one demotes. - ink_mutex_acquire(&lock); - ImpersonateUserID(0, IMPERSONATE_EFFECTIVE); + if (priv_mask) { + // Since we are setting a process-wide credential, we have to block any other thread + // attempting to elevate until this one demotes. + ink_mutex_acquire(&lock); + ImpersonateUserID(0, IMPERSONATE_EFFECTIVE); + elevated = true; + } #endif - elevated = true; } void ElevateAccess::demote() { + if (elevated) { #if TS_USE_POSIX_CAP - releaseFileAccessCap(); + releasePrivilege(); #else - ImpersonateUserID(saved_uid, IMPERSONATE_EFFECTIVE); - ink_mutex_release(&lock); + ImpersonateUserID(saved_uid, IMPERSONATE_EFFECTIVE); + ink_mutex_release(&lock); #endif - elevated = false; + elevated = false; + } } http://git-wip-us.apache.org/repos/asf/trafficserver/blob/2c1bdddc/lib/ts/ink_cap.h ---------------------------------------------------------------------- diff --git a/lib/ts/ink_cap.h b/lib/ts/ink_cap.h index 19e94a4..0978965 100644 --- a/lib/ts/ink_cap.h +++ b/lib/ts/ink_cap.h @@ -38,6 +38,18 @@ extern bool PreserveCapabilities(); /// Initialize and restrict the capabilities of a thread. /// @return true on success extern bool RestrictCapabilities(); +/** Open a file, elevating privilege only if needed. + + @internal This is necessary because the CI machines run the regression tests + as a normal user, not as root, so attempts to get privilege fail even though + the @c open would succeed without elevation. So, try that first and ask for + elevation only on an explicit permission failure. +*/ +extern int elevating_open(char const *path, unsigned int flags, unsigned int fperms); +/// Open a file, elevating privilege only if needed. +extern int elevating_open(char const *path, unsigned int flags); +/// Open a file, elevating privilege only if needed. +extern FILE* elevating_fopen(char const *path, const char* mode); /** Control generate of core file on crash. @a flag sets whether core files are enabled on crash. @@ -60,8 +72,9 @@ class ElevateAccess { public: typedef enum { - FILE_PRIVILEGE = 0x1u, // Access filesystem objects with privilege - TRACE_PRIVILEGE = 0x2u // Trace other processes with privilege + FILE_PRIVILEGE = 0x1u, ///< Access filesystem objects with privilege + TRACE_PRIVILEGE = 0x2u, ///< Trace other processes with privilege + LOW_PORT_PRIVILEGE = 0x4u ///< Bind to privilege ports. } privilege_level; ElevateAccess(unsigned level = FILE_PRIVILEGE); @@ -75,8 +88,10 @@ private: uid_t saved_uid; unsigned level; - void acquireFileAccessCap(unsigned level); - void releaseFileAccessCap(); + /// Acquire the privileges marked in @a mask for this process. + void acquirePrivilege(unsigned priv_mask); + /// Restore the privilege set to the state before acquiring them. + void releasePrivilege(); #if !TS_USE_POSIX_CAP static ink_mutex lock; // only one thread at a time can elevate #else http://git-wip-us.apache.org/repos/asf/trafficserver/blob/2c1bdddc/mgmt/LocalManager.cc ---------------------------------------------------------------------- diff --git a/mgmt/LocalManager.cc b/mgmt/LocalManager.cc index 44cc63b..6036a7b 100644 --- a/mgmt/LocalManager.cc +++ b/mgmt/LocalManager.cc @@ -1021,8 +1021,9 @@ void LocalManager::bindProxyPort(HttpProxyPort &port) { int one = 1; + int priv = (port.m_port < 1024 && 0 != geteuid()) ? ElevateAccess::LOW_PORT_PRIVILEGE : 0; - ElevateAccess access(port.m_port < 1024 && geteuid() != 0); + ElevateAccess access(priv); /* Setup reliable connection, for large config changes */ if ((port.m_fd = socket(port.m_family, SOCK_STREAM, 0)) < 0) { http://git-wip-us.apache.org/repos/asf/trafficserver/blob/2c1bdddc/mgmt/Rollback.cc ---------------------------------------------------------------------- diff --git a/mgmt/Rollback.cc b/mgmt/Rollback.cc index 217902c..f1361f4 100644 --- a/mgmt/Rollback.cc +++ b/mgmt/Rollback.cc @@ -260,7 +260,7 @@ Rollback::statFile(version_t version, struct stat *buf) } ats_scoped_str filePath(createPathStr(version)); - ElevateAccess access(root_access_needed); + ElevateAccess access(root_access_needed ? ElevateAccess::FILE_PRIVILEGE : 0); statResult = stat(filePath, buf); @@ -278,7 +278,7 @@ Rollback::openFile(version_t version, int oflags, int *errnoPtr) int fd; ats_scoped_str filePath(createPathStr(version)); - ElevateAccess access(root_access_needed); + ElevateAccess access(root_access_needed ? ElevateAccess::FILE_PRIVILEGE : 0); // TODO: Use the original permissions // Anyhow the _1 files should not be created inside Syconfdir. http://git-wip-us.apache.org/repos/asf/trafficserver/blob/2c1bdddc/proxy/Main.cc ---------------------------------------------------------------------- diff --git a/proxy/Main.cc b/proxy/Main.cc index 5fc8f86..45881e3 100644 --- a/proxy/Main.cc +++ b/proxy/Main.cc @@ -1422,24 +1422,6 @@ change_uid_gid(const char *user) #endif } -/** Open a file, elevating privilege only if needed. - - @internal This is necessary because the CI machines run the regression tests - as a normal user, not as root, so attempts to get privilege fail even though - the @c open would succeed without elevation. So, try that first and ask for - elevation only on an explicit permission failure. -*/ -static int -elevating_open(char const *path, unsigned int flags, unsigned int fperms) -{ - int fd = open(path, flags, fperms); - if (fd < 0 && (EPERM == errno || EACCES == errno)) { - ElevateAccess access; - fd = open(path, flags, fperms); - } - return fd; -} - /* * Binds stdout and stderr to files specified by the parameters * http://git-wip-us.apache.org/repos/asf/trafficserver/blob/2c1bdddc/proxy/Plugin.cc ---------------------------------------------------------------------- diff --git a/proxy/Plugin.cc b/proxy/Plugin.cc index a621b92..b7b450c 100644 --- a/proxy/Plugin.cc +++ b/proxy/Plugin.cc @@ -94,11 +94,9 @@ plugin_load(int argc, char *argv[], bool validateOnly) // elevate the access to read files as root if compiled with capabilities, if not // change the effective user to root { -#if TS_USE_POSIX_CAP uint32_t elevate_access = 0; REC_ReadConfigInteger(elevate_access, "proxy.config.plugin.load_elevated"); - ElevateAccess access(elevate_access != 0); -#endif /* TS_USE_POSIX_CAP */ + ElevateAccess access(elevate_access ? ElevateAccess::FILE_PRIVILEGE : 0); void *handle = dlopen(path, RTLD_NOW); if (!handle) { http://git-wip-us.apache.org/repos/asf/trafficserver/blob/2c1bdddc/proxy/http/remap/RemapConfig.cc ---------------------------------------------------------------------- diff --git a/proxy/http/remap/RemapConfig.cc b/proxy/http/remap/RemapConfig.cc index 4a0e850..8b5f58e 100644 --- a/proxy/http/remap/RemapConfig.cc +++ b/proxy/http/remap/RemapConfig.cc @@ -760,11 +760,9 @@ remap_load_plugin(const char **argv, int argc, url_mapping *mp, char *errbuf, in Debug("remap_plugin", "New remap plugin info created for \"%s\"", c); { -#if TS_USE_POSIX_CAP uint32_t elevate_access = 0; REC_ReadConfigInteger(elevate_access, "proxy.config.plugin.load_elevated"); - ElevateAccess access(elevate_access != 0); -#endif /* TS_USE_POSIX_CAP */ + ElevateAccess access(elevate_access ? ElevateAccess::FILE_PRIVILEGE : 0); if ((pi->dlh = dlopen(c, RTLD_NOW)) == NULL) { #if defined(freebsd) || defined(openbsd)
