Bug 1380701 - Remove the file broker protocol support for two-path operations. r=gcp
Now that all of the operations that took two paths are removed, we can
have less string manipulation running on untrusted inputs in a trusted
context.
Note that the path isn't null-terminated in transit, because we know
the message length and there's no longer any need to delimit anything.
(This is how the protocol worked before the two-path operations were
added.)
MozReview-Commit-ID: 5VHkMoPlWmU
--- a/security/sandbox/linux/SandboxBrokerClient.cpp
+++ b/security/sandbox/linux/SandboxBrokerClient.cpp
@@ -30,18 +30,17 @@ SandboxBrokerClient::SandboxBrokerClient
SandboxBrokerClient::~SandboxBrokerClient()
{
close(mFileDesc);
}
int
SandboxBrokerClient::DoCall(const Request* aReq, const char* aPath,
- const char* aPath2, void* aResponseBuff,
- bool expectFd)
+ void* aResponseBuff, bool expectFd)
{
// Remap /proc/self to the actual pid, so that the broker can open
// it. This happens here instead of in the broker to follow the
// principle of least privilege and keep the broker as simple as
// possible. (Note: when pid namespaces happen, this will also need
// to remap the inner pid to the outer pid.)
// We only remap the first path.
static const char kProcSelf[] = "/proc/self/";
@@ -59,48 +58,38 @@ SandboxBrokerClient::DoCall(const Reques
SANDBOX_LOG_ERROR("rewriting %s -> %s", aPath, rewrittenPath);
}
path = rewrittenPath;
} else {
SANDBOX_LOG_ERROR("not rewriting unexpectedly long path %s", aPath);
}
}
- struct iovec ios[3];
+ struct iovec ios[2];
int respFds[2];
// Set up iovecs for request + path.
ios[0].iov_base = const_cast<Request*>(aReq);
ios[0].iov_len = sizeof(*aReq);
ios[1].iov_base = const_cast<char*>(path);
- ios[1].iov_len = strlen(path) + 1;
- if (aPath2 != nullptr) {
- ios[2].iov_base = const_cast<char*>(aPath2);
- ios[2].iov_len = strlen(aPath2) + 1;
- } else {
- ios[2].iov_base = nullptr;
- ios[2].iov_len = 0;
- }
+ ios[1].iov_len = strlen(path);
if (ios[1].iov_len > kMaxPathLen) {
return -ENAMETOOLONG;
}
- if (ios[2].iov_len > kMaxPathLen) {
- return -ENAMETOOLONG;
- }
// Create response socket and send request.
if (socketpair(AF_UNIX, SOCK_SEQPACKET, 0, respFds) < 0) {
return -errno;
}
- const ssize_t sent = SendWithFd(mFileDesc, ios, 3, respFds[1]);
+ const ssize_t sent = SendWithFd(mFileDesc, ios, 2, respFds[1]);
const int sendErrno = errno;
MOZ_ASSERT(sent < 0 ||
static_cast<size_t>(sent) == ios[0].iov_len
- + ios[1].iov_len
- + ios[2].iov_len);
+ + ios[1].iov_len);
+
close(respFds[1]);
if (sent < 0) {
close(respFds[0]);
return -sendErrno;
}
// Set up iovecs for response.
Response resp;
@@ -151,76 +140,76 @@ SandboxBrokerClient::DoCall(const Reques
}
return resp.mError;
}
int
SandboxBrokerClient::Open(const char* aPath, int aFlags)
{
Request req = { SANDBOX_FILE_OPEN, aFlags, 0 };
- int maybeFd = DoCall(&req, aPath, nullptr, nullptr, true);
+ int maybeFd = DoCall(&req, aPath, nullptr, true);
if (maybeFd >= 0) {
// NSPR has opinions about file flags. Fix O_CLOEXEC.
if ((aFlags & O_CLOEXEC) == 0) {
fcntl(maybeFd, F_SETFD, 0);
}
}
return maybeFd;
}
int
SandboxBrokerClient::Access(const char* aPath, int aMode)
{
Request req = { SANDBOX_FILE_ACCESS, aMode, 0 };
- return DoCall(&req, aPath, nullptr, nullptr, false);
+ return DoCall(&req, aPath, nullptr, false);
}
int
SandboxBrokerClient::Stat(const char* aPath, statstruct* aStat)
{
Request req = { SANDBOX_FILE_STAT, 0, sizeof(statstruct) };
- return DoCall(&req, aPath, nullptr, (void*)aStat, false);
+ return DoCall(&req, aPath, (void*)aStat, false);
}
int
SandboxBrokerClient::LStat(const char* aPath, statstruct* aStat)
{
Request req = { SANDBOX_FILE_STAT, O_NOFOLLOW, sizeof(statstruct) };
- return DoCall(&req, aPath, nullptr, (void*)aStat, false);
+ return DoCall(&req, aPath, (void*)aStat, false);
}
int
SandboxBrokerClient::Chmod(const char* aPath, int aMode)
{
Request req = {SANDBOX_FILE_CHMOD, aMode, 0};
- return DoCall(&req, aPath, nullptr, nullptr, false);
+ return DoCall(&req, aPath, nullptr, false);
}
int
SandboxBrokerClient::Mkdir(const char* aPath, int aMode)
{
Request req = {SANDBOX_FILE_MKDIR, aMode, 0};
- return DoCall(&req, aPath, nullptr, nullptr, false);
+ return DoCall(&req, aPath, nullptr, false);
}
int
SandboxBrokerClient::Unlink(const char* aPath)
{
Request req = {SANDBOX_FILE_UNLINK, 0, 0};
- return DoCall(&req, aPath, nullptr, nullptr, false);
+ return DoCall(&req, aPath, nullptr, false);
}
int
SandboxBrokerClient::Rmdir(const char* aPath)
{
Request req = {SANDBOX_FILE_RMDIR, 0, 0};
- return DoCall(&req, aPath, nullptr, nullptr, false);
+ return DoCall(&req, aPath, nullptr, false);
}
int
SandboxBrokerClient::Readlink(const char* aPath, void* aBuff, size_t aSize)
{
Request req = {SANDBOX_FILE_READLINK, 0, aSize};
- return DoCall(&req, aPath, nullptr, aBuff, false);
+ return DoCall(&req, aPath, aBuff, false);
}
} // namespace mozilla
--- a/security/sandbox/linux/SandboxBrokerClient.h
+++ b/security/sandbox/linux/SandboxBrokerClient.h
@@ -40,16 +40,15 @@ class SandboxBrokerClient final : privat
int Rmdir(const char* aPath);
int Readlink(const char* aPath, void* aBuf, size_t aBufSize);
private:
int mFileDesc;
int DoCall(const Request* aReq,
const char* aPath,
- const char* aPath2,
void *aReponseBuff,
bool expectFd);
};
} // namespace mozilla
#endif // mozilla_SandboxBrokerClient_h
--- a/security/sandbox/linux/broker/SandboxBroker.cpp
+++ b/security/sandbox/linux/broker/SandboxBroker.cpp
@@ -496,37 +496,29 @@ SandboxBroker::ThreadMain(void)
// Permissive mode can only be enabled through an environment variable,
// therefore it is sufficient to fetch the value once
// before the main thread loop starts
bool permissive = SandboxInfo::Get().Test(SandboxInfo::kPermissive);
while (true) {
struct iovec ios[2];
- // We will receive the path strings in 1 buffer and split them back up.
- char recvBuf[2 * (kMaxPathLen + 1)];
- char pathBuf[kMaxPathLen + 1];
- char pathBuf2[kMaxPathLen + 1];
- size_t pathLen = 0;
- size_t pathLen2 = 0;
+ char recvBuf[kMaxPathLen + 1];
char respBuf[kMaxPathLen + 1]; // Also serves as struct stat
Request req;
Response resp;
int respfd;
// Make sure stat responses fit in the response buffer
MOZ_ASSERT((kMaxPathLen + 1) > sizeof(struct stat));
- // This makes our string handling below a bit less error prone.
- memset(recvBuf, 0, sizeof(recvBuf));
-
ios[0].iov_base = &req;
ios[0].iov_len = sizeof(req);
ios[1].iov_base = recvBuf;
- ios[1].iov_len = sizeof(recvBuf);
+ ios[1].iov_len = sizeof(recvBuf) - 1;
const ssize_t recvd = RecvWithFd(mFileDesc, ios, 2, &respfd);
if (recvd == 0) {
if (SandboxInfo::Get().Test(SandboxInfo::kVerbose)) {
SANDBOX_LOG_ERROR("EOF from pid %d", mChildPid);
}
break;
}
@@ -548,80 +540,45 @@ SandboxBroker::ThreadMain(void)
if (respfd == -1) {
SANDBOX_LOG_ERROR("no response fd from pid %d", mChildPid);
shutdown(mFileDesc, SHUT_RD);
break;
}
// Initialize the response with the default failure.
memset(&resp, 0, sizeof(resp));
- memset(&respBuf, 0, sizeof(respBuf));
resp.mError = -EACCES;
ios[0].iov_base = &resp;
ios[0].iov_len = sizeof(resp);
ios[1].iov_base = nullptr;
ios[1].iov_len = 0;
int openedFd = -1;
- // Clear permissions
- int perms;
-
- // Find end of first string, make sure the buffer is still
- // 0 terminated.
- size_t recvBufLen = static_cast<size_t>(recvd) - sizeof(req);
- if (recvBufLen > 0 && recvBuf[recvBufLen - 1] != 0) {
- SANDBOX_LOG_ERROR("corrupted path buffer from pid %d", mChildPid);
- shutdown(mFileDesc, SHUT_RD);
- break;
- }
-
- // First path should fit in maximum path length buffer.
- size_t first_len = strlen(recvBuf);
- if (first_len <= kMaxPathLen) {
- strcpy(pathBuf, recvBuf);
- // Skip right over the terminating 0, and try to copy in the
- // second path, if any. If there's no path, this will hit a
- // 0 immediately (we nulled the buffer before receiving).
- // We do not assume the second path is 0-terminated, this is
- // enforced below.
- strncpy(pathBuf2, recvBuf + first_len + 1, kMaxPathLen + 1);
-
- // First string is guaranteed to be 0-terminated.
- pathLen = first_len;
+ size_t origPathLen = static_cast<size_t>(recvd) - sizeof(req);
+ // Null-terminate to get a C-style string.
+ MOZ_RELEASE_ASSERT(origPathLen < sizeof(recvBuf));
+ recvBuf[origPathLen] = '\0';
- // Look up the first pathname but first translate relative paths.
- pathLen = ConvertToRealPath(pathBuf, sizeof(pathBuf), pathLen);
- perms = mPolicy->Lookup(nsDependentCString(pathBuf, pathLen));
+ // Look up the pathname but first translate relative paths.
+ // (Make a copy so we can get back the original path if needed.)
+ char pathBuf[kMaxPathLen + 1];
+ base::strlcpy(pathBuf, recvBuf, sizeof(pathBuf));
+ size_t pathLen = ConvertToRealPath(pathBuf, sizeof(pathBuf), origPathLen);
+ int perms = mPolicy->Lookup(nsDependentCString(pathBuf, pathLen));
- // We don't have read permissions on the requested dir.
- // Did we arrive from a symlink in a path that is not writable?
- // Then try to figure out the original path and see if that is readable.
- if (!(perms & MAY_READ)) {
- // Work on the original path,
- // this reverses ConvertToRealPath above.
- int symlinkPerms = SymlinkPermissions(recvBuf, first_len);
- if (symlinkPerms > 0) {
- perms = symlinkPerms;
- }
+ // We don't have read permissions on the requested dir.
+ // Did we arrive from a symlink in a path that is not writable?
+ // Then try to figure out the original path and see if that is readable.
+ if (!(perms & MAY_READ)) {
+ // Work on the original path,
+ // this reverses ConvertToRealPath above.
+ int symlinkPerms = SymlinkPermissions(recvBuf, origPathLen);
+ if (symlinkPerms > 0) {
+ perms = symlinkPerms;
}
-
- // Same for the second path.
- pathLen2 = strnlen(pathBuf2, kMaxPathLen);
- if (pathLen2 > 0) {
- // Force 0 termination.
- pathBuf2[pathLen2] = '\0';
- pathLen2 = ConvertToRealPath(pathBuf2, sizeof(pathBuf2), pathLen2);
- int perms2 = mPolicy->Lookup(nsDependentCString(pathBuf2, pathLen2));
-
- // Take the intersection of the permissions for both paths.
- perms &= perms2;
- }
- } else {
- // Failed to receive intelligible paths.
- perms = 0;
}
// And now perform the operation if allowed.
if (perms & CRASH_INSTEAD) {
// This is somewhat nonmodular, but it works.
resp.mError = -ENOSYS;
} else if (permissive || perms & MAY_ACCESS) {
// If the operation was only allowed because of permissive mode, log it.
@@ -721,19 +678,22 @@ SandboxBroker::ThreadMain(void)
}
} else {
AuditDenial(req.mOp, req.mFlags, perms, pathBuf);
}
break;
case SANDBOX_FILE_READLINK:
if (permissive || AllowOperation(R_OK, perms)) {
- ssize_t respSize = readlink(pathBuf, (char*)&respBuf, sizeof(respBuf));
+ ssize_t respSize = readlink(pathBuf, (char*)&respBuf, sizeof(respBuf) - 1);
if (respSize >= 0) {
- if (respSize > 0) {
+ if (respSize > 0) {
+ // Null-terminate for nsDependentCString.
+ MOZ_RELEASE_ASSERT(static_cast<size_t>(respSize) < sizeof(respBuf));
+ respBuf[respSize] = '\0';
// Record the mapping so we can invert the file to the original
// symlink.
nsDependentCString orig(pathBuf, pathLen);
nsDependentCString xlat(respBuf, respSize);
if (!orig.Equals(xlat) && xlat[0] == '/') {
if (SandboxInfo::Get().Test(SandboxInfo::kVerbose)) {
SANDBOX_LOG_ERROR("Recording mapping %s -> %s",
xlat.get(), orig.get());