Copilot commented on code in PR #13046:
URL: https://github.com/apache/trafficserver/pull/13046#discussion_r3017551639
##########
src/traffic_server/traffic_server.cc:
##########
@@ -1757,6 +1758,70 @@ change_uid_gid(const char *user)
#endif
}
+#if !TS_USE_POSIX_CAP
+/**
+ * Recursively chown a directory and all its contents to the given uid/gid.
+ */
+void
+chown_dir_recursive(const char *dir, uid_t uid, gid_t gid)
+{
+ if (chown(dir, uid, gid) != 0) {
+ Warning("chown_dir_recursive: failed to chown '%s': %s", dir,
strerror(errno));
+ }
+
+ std::error_code ec;
+
+ for (const auto &entry : std::filesystem::recursive_directory_iterator(dir,
ec)) {
+ if (chown(entry.path().c_str(), uid, gid) != 0) {
+ Warning("chown_dir_recursive: failed to chown '%s': %s",
entry.path().c_str(), strerror(errno));
+ }
Review Comment:
chown_dir_recursive() uses chown() on entry.path(), which follows symlinks.
If rundir/logdir (or a child) is attacker-writable, a symlink placed there
could cause ownership changes on arbitrary targets before privileges are
dropped. Consider using lchown() (or skipping symlinks via
symlink_status/is_symlink checks) and ensure recursive traversal does not
follow directory symlinks.
##########
src/traffic_server/traffic_server.cc:
##########
@@ -1757,6 +1758,70 @@ change_uid_gid(const char *user)
#endif
}
+#if !TS_USE_POSIX_CAP
+/**
+ * Recursively chown a directory and all its contents to the given uid/gid.
+ */
+void
+chown_dir_recursive(const char *dir, uid_t uid, gid_t gid)
+{
+ if (chown(dir, uid, gid) != 0) {
+ Warning("chown_dir_recursive: failed to chown '%s': %s", dir,
strerror(errno));
+ }
+
+ std::error_code ec;
+
+ for (const auto &entry : std::filesystem::recursive_directory_iterator(dir,
ec)) {
+ if (chown(entry.path().c_str(), uid, gid) != 0) {
+ Warning("chown_dir_recursive: failed to chown '%s': %s",
entry.path().c_str(), strerror(errno));
+ }
+ }
+
+ if (ec) {
+ Warning("chown_dir_recursive: error iterating '%s': %s", dir,
ec.message().c_str());
+ }
+}
+
+/**
+ * On systems without POSIX capabilities, privilege is dropped late — after
+ * initialization has already created files and directories as root. This
+ * causes runtime operations (plugin reloads, log rotation) to fail because
+ * the now-unprivileged process can't write to root-owned paths. Fix this by
+ * chowning affected directories to the target user before dropping privileges.
+ */
+void
+chown_owned_dirs(const char *user)
+{
+ struct passwd *pwd;
+ struct passwd pbuf;
+ char buf[4096];
+
+ if (*user == '#') {
+ uid_t uid = static_cast<uid_t>(atoi(&user[1]));
+ if (getpwuid_r(uid, &pbuf, buf, sizeof(buf), &pwd) != 0 || pwd == nullptr)
{
+ Warning("chown_owned_dirs: cannot resolve uid %d", uid);
Review Comment:
Warning("chown_owned_dirs: cannot resolve uid %d", uid) uses %d for a uid_t
argument. On platforms where uid_t is unsigned / wider than int, this is
undefined behavior / can log incorrect values. Use an appropriate format (e.g.,
cast to long and print with %ld, or use an unsigned format matching uid_t).
```suggestion
Warning("chown_owned_dirs: cannot resolve uid %llu",
static_cast<unsigned long long>(uid));
```
##########
src/traffic_server/traffic_server.cc:
##########
@@ -1757,6 +1758,70 @@ change_uid_gid(const char *user)
#endif
}
+#if !TS_USE_POSIX_CAP
+/**
+ * Recursively chown a directory and all its contents to the given uid/gid.
+ */
+void
+chown_dir_recursive(const char *dir, uid_t uid, gid_t gid)
+{
+ if (chown(dir, uid, gid) != 0) {
+ Warning("chown_dir_recursive: failed to chown '%s': %s", dir,
strerror(errno));
+ }
+
+ std::error_code ec;
+
+ for (const auto &entry : std::filesystem::recursive_directory_iterator(dir,
ec)) {
+ if (chown(entry.path().c_str(), uid, gid) != 0) {
+ Warning("chown_dir_recursive: failed to chown '%s': %s",
entry.path().c_str(), strerror(errno));
+ }
+ }
+
+ if (ec) {
+ Warning("chown_dir_recursive: error iterating '%s': %s", dir,
ec.message().c_str());
+ }
+}
+
+/**
+ * On systems without POSIX capabilities, privilege is dropped late — after
+ * initialization has already created files and directories as root. This
+ * causes runtime operations (plugin reloads, log rotation) to fail because
+ * the now-unprivileged process can't write to root-owned paths. Fix this by
+ * chowning affected directories to the target user before dropping privileges.
+ */
+void
+chown_owned_dirs(const char *user)
+{
+ struct passwd *pwd;
+ struct passwd pbuf;
+ char buf[4096];
+
+ if (*user == '#') {
+ uid_t uid = static_cast<uid_t>(atoi(&user[1]));
+ if (getpwuid_r(uid, &pbuf, buf, sizeof(buf), &pwd) != 0 || pwd == nullptr)
{
+ Warning("chown_owned_dirs: cannot resolve uid %d", uid);
+ return;
+ }
Review Comment:
chown_owned_dirs() performs a recursive chown but does not have the same
“already unprivileged” guard as change_uid_gid(). When traffic_server is
started as a non-root user (e.g., regression runs), this will emit warnings for
every failed chown attempt. Add an early check (getuid/geteuid) and skip
chowning when not running with sufficient privilege.
##########
src/traffic_server/traffic_server.cc:
##########
@@ -1757,6 +1758,70 @@ change_uid_gid(const char *user)
#endif
}
+#if !TS_USE_POSIX_CAP
+/**
+ * Recursively chown a directory and all its contents to the given uid/gid.
+ */
+void
+chown_dir_recursive(const char *dir, uid_t uid, gid_t gid)
+{
+ if (chown(dir, uid, gid) != 0) {
+ Warning("chown_dir_recursive: failed to chown '%s': %s", dir,
strerror(errno));
+ }
+
+ std::error_code ec;
+
+ for (const auto &entry : std::filesystem::recursive_directory_iterator(dir,
ec)) {
+ if (chown(entry.path().c_str(), uid, gid) != 0) {
+ Warning("chown_dir_recursive: failed to chown '%s': %s",
entry.path().c_str(), strerror(errno));
+ }
+ }
+
+ if (ec) {
+ Warning("chown_dir_recursive: error iterating '%s': %s", dir,
ec.message().c_str());
+ }
+}
+
+/**
+ * On systems without POSIX capabilities, privilege is dropped late — after
+ * initialization has already created files and directories as root. This
+ * causes runtime operations (plugin reloads, log rotation) to fail because
+ * the now-unprivileged process can't write to root-owned paths. Fix this by
+ * chowning affected directories to the target user before dropping privileges.
+ */
+void
+chown_owned_dirs(const char *user)
+{
Review Comment:
These helper functions are file-local implementation details but currently
have external linkage. Marking chown_dir_recursive() and chown_owned_dirs() as
static (or placing them in an anonymous namespace) would avoid exporting new
symbols and reduce the chance of name collisions at link time.
##########
src/traffic_server/traffic_server.cc:
##########
@@ -1757,6 +1758,70 @@ change_uid_gid(const char *user)
#endif
}
+#if !TS_USE_POSIX_CAP
+/**
+ * Recursively chown a directory and all its contents to the given uid/gid.
+ */
+void
+chown_dir_recursive(const char *dir, uid_t uid, gid_t gid)
+{
+ if (chown(dir, uid, gid) != 0) {
+ Warning("chown_dir_recursive: failed to chown '%s': %s", dir,
strerror(errno));
+ }
+
+ std::error_code ec;
+
+ for (const auto &entry : std::filesystem::recursive_directory_iterator(dir,
ec)) {
+ if (chown(entry.path().c_str(), uid, gid) != 0) {
+ Warning("chown_dir_recursive: failed to chown '%s': %s",
entry.path().c_str(), strerror(errno));
+ }
+ }
+
+ if (ec) {
+ Warning("chown_dir_recursive: error iterating '%s': %s", dir,
ec.message().c_str());
+ }
+}
+
+/**
+ * On systems without POSIX capabilities, privilege is dropped late — after
+ * initialization has already created files and directories as root. This
+ * causes runtime operations (plugin reloads, log rotation) to fail because
+ * the now-unprivileged process can't write to root-owned paths. Fix this by
+ * chowning affected directories to the target user before dropping privileges.
+ */
+void
+chown_owned_dirs(const char *user)
+{
+ struct passwd *pwd;
+ struct passwd pbuf;
+ char buf[4096];
+
+ if (*user == '#') {
+ uid_t uid = static_cast<uid_t>(atoi(&user[1]));
+ if (getpwuid_r(uid, &pbuf, buf, sizeof(buf), &pwd) != 0 || pwd == nullptr)
{
+ Warning("chown_owned_dirs: cannot resolve uid %d", uid);
+ return;
+ }
+ } else {
+ if (getpwnam_r(user, &pbuf, buf, sizeof(buf), &pwd) != 0 || pwd ==
nullptr) {
+ Warning("chown_owned_dirs: cannot resolve user '%s'", user);
+ return;
Review Comment:
getpwuid_r/getpwnam_r are called with a fixed 4096-byte buffer. If the
platform requires a larger buffer, these calls can fail with ERANGE and the
code will silently skip chowning, leaving root-owned files behind. Consider
sizing the buffer via sysconf(_SC_GETPW_R_SIZE_MAX) (as done in
src/tscore/ink_cap.cc) or retrying with a dynamically grown buffer on ERANGE.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
