Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24055003:
add a LinuxSandbox::HasOpenDirectories() sanity check (Closed)
|
Created:
7 years, 3 months ago by Mostyn Bramley-Moore Modified:
7 years, 1 month ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, jln+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionadd a LinuxSandbox::HasOpenDirectories() sanity check
BUG=269806
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233100
Patch Set 1 #
Total comments: 19
Patch Set 2 : try to be more careful #
Total comments: 30
Patch Set 3 : simply compare fd's #
Total comments: 12
Patch Set 4 : remove OpenProc etc #
Total comments: 6
Patch Set 5 : fix final nits #Patch Set 6 : test: guess false if we can't open /proc/self/fd regardless of errno #
Total comments: 2
Patch Set 7 : leave TODO #Messages
Total messages: 26 (0 generated)
This patch adds a sanity check suggested by jln in bug 269806. But as far as I can see, the check only works in debug builds- I'm not sure if this is intended.
*ping*
Thanks for the ping Mostyn, and sorry this took so long! Thanks for working on this. https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.cc File content/common/sandbox_linux.cc (right): https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.... content/common/sandbox_linux.cc:149: LOG(ERROR) << "InitializeSandbox() called after unexpected directries " Let's LOG(FATAL). https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.... content/common/sandbox_linux.cc:222: DIR* fd_dir = NULL; It's too dangerous and error-prone to make sure that we call closedir(). Either create a new OpenDirDeleter class and use a scoped_ptr, or use a ScopedClosureRunner. I would prefer the former. Don't forget to CHECK closedir(). https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.... content/common/sandbox_linux.cc:227: } style: else on the same line. https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.... content/common/sandbox_linux.cc:231: fd_dir = fdopendir(fd_fd); Declare fd_dir only here as needed. Add a comment that ownership of "fd_fd" is transfered to fd_dir as the API is not obvious. https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.... content/common/sandbox_linux.cc:232: if (!fd_dir) { Add a #if !DEFINED(NDEBUG) ... CHECK(fd_dir) above (see line 199 for why). https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.... content/common/sandbox_linux.cc:239: while ((e = readdir(fd_dir))) { Please, use readdir_r, I wouldn't want to rely on the fact that we're mono-threaded. You can just allocate the dirent on the stack. https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.... content/common/sandbox_linux.cc:250: if (fstatat(fd_fd, e->d_name, &s, 0) == 0) { This is quote subtle and error prone, so please CHECK this use of fstatat. https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.... content/common/sandbox_linux.cc:253: // We had to open /proc/self/fd/ so we really want to check if proc_fd_ and fd_fd should be treated the same, no ? It's a lot safer to check the exact same file descriptor than just counting. Unfortunately in theory fdopendir() could be at liberty to dup() etc, but your current test would already fail under such a scenario. You could still maintain a counter a DCHECK that it's less than 2. https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.h File content/common/sandbox_linux.h (right): https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.... content/common/sandbox_linux.h:61: // directories (besides /proc). Let's just say "that are not managed by the LinuxSandbox class". We could even add: "This would be a vulnerability as it would allow to bypass the setuid sandbox". Let's also make it private for now.
https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.cc File content/common/sandbox_linux.cc (right): https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.... content/common/sandbox_linux.cc:253: // We had to open /proc/self/fd/ so we really want to check if On 2013/10/22 01:10:50, jln wrote: > proc_fd_ and fd_fd should be treated the same, no ? Yes, except that we know fd_fd must be valid whereas proc_fd_ might not be. > It's a lot safer to check the exact same file descriptor than just counting. > > Unfortunately in theory fdopendir() could be at liberty to dup() etc, but your > current test would already fail under such a scenario. If we don't want to count duplicate file descriptors, then I suppose we should instead check for the device/inode pair of /proc and /proc/self/fd. > You could still maintain a counter a DCHECK that it's less than 2. Are these the situations we want to catch: 1) proc_fd_ is valid, /proc/self/fd is open, and a third distinct directory (ie neither /proc nor /proc/self/fd) is also open. 2) proc_fd_ is invalid and any directory besides /proc/self/fd is open.
https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.cc File content/common/sandbox_linux.cc (right): https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.... content/common/sandbox_linux.cc:149: LOG(ERROR) << "InitializeSandbox() called after unexpected directries " On 2013/10/22 01:10:50, jln wrote: > Let's LOG(FATAL). Done in patchset 2. https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.... content/common/sandbox_linux.cc:222: DIR* fd_dir = NULL; On 2013/10/22 01:10:50, jln wrote: > It's too dangerous and error-prone to make sure that we call closedir(). > > Either create a new OpenDirDeleter class and use a scoped_ptr, or use a > ScopedClosureRunner. > > I would prefer the former. Don't forget to CHECK closedir(). Done in patchset 2. https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.... content/common/sandbox_linux.cc:227: } On 2013/10/22 01:10:50, jln wrote: > style: else on the same line. Done. https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.... content/common/sandbox_linux.cc:231: fd_dir = fdopendir(fd_fd); On 2013/10/22 01:10:50, jln wrote: > Declare fd_dir only here as needed. Add a comment that ownership of "fd_fd" is > transfered to fd_dir as the API is not obvious. The OpenDirDeleter code should take care of this. https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.... content/common/sandbox_linux.cc:232: if (!fd_dir) { On 2013/10/22 01:10:50, jln wrote: > Add a #if !DEFINED(NDEBUG) ... CHECK(fd_dir) above (see line 199 for why). I ended up using this in a few places, so I created a REAL_DEBUG_CHECK macro to keep things concise. https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.... content/common/sandbox_linux.cc:239: while ((e = readdir(fd_dir))) { On 2013/10/22 01:10:50, jln wrote: > Please, use readdir_r, I wouldn't want to rely on the fact that we're > mono-threaded. > > You can just allocate the dirent on the stack. Done. https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.... content/common/sandbox_linux.cc:250: if (fstatat(fd_fd, e->d_name, &s, 0) == 0) { On 2013/10/22 01:10:50, jln wrote: > This is quote subtle and error prone, so please CHECK this use of fstatat. Done. https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.... content/common/sandbox_linux.cc:253: // We had to open /proc/self/fd/ so we really want to check if On 2013/10/22 19:10:04, Mostyn Bramley-Moore wrote: > On 2013/10/22 01:10:50, jln wrote: > > proc_fd_ and fd_fd should be treated the same, no ? > > Yes, except that we know fd_fd must be valid whereas proc_fd_ might not be. > > > It's a lot safer to check the exact same file descriptor than just counting. > > > > Unfortunately in theory fdopendir() could be at liberty to dup() etc, but your > > current test would already fail under such a scenario. > > If we don't want to count duplicate file descriptors, then I suppose we should > instead check for the device/inode pair of /proc and /proc/self/fd. > > > You could still maintain a counter a DCHECK that it's less than 2. > > Are these the situations we want to catch: > > 1) proc_fd_ is valid, /proc/self/fd is open, and a third distinct directory (ie > neither /proc nor /proc/self/fd) is also open. > > 2) proc_fd_ is invalid and any directory besides /proc/self/fd is open. In patchset 2 I replaced this counting logic with comparisons to the device/inode pairs for the managed directories. https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.h File content/common/sandbox_linux.h (right): https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.... content/common/sandbox_linux.h:61: // directories (besides /proc). On 2013/10/22 01:10:50, jln wrote: > Let's just say "that are not managed by the LinuxSandbox class". We could even > add: "This would be a vulnerability as it would allow to bypass the setuid > sandbox". > > Let's also make it private for now. Done in patchset 2.
Sorry for the delay. I didn't look at everything in detail yet.
Lots of remarks, but the most important is that the algorithm should be fairly
simple and look something like:
for (f in each_fd) {
if (isdirectory(f) && f != proc_fd) && f != proc_self_fd)
return true;
}
Remember that the numbers in /proc/self/fd/XX are the actual file descriptor
numbers.
https://chromiumcodereview.appspot.com/24055003/diff/54001/content/common/san...
File content/common/sandbox_linux.cc (right):
https://chromiumcodereview.appspot.com/24055003/diff/54001/content/common/san...
content/common/sandbox_linux.cc:64: #if !defined(NDEBUG)
Maybe do DEBUG_NOTREACHED() instead which does CHECK(false) in debug mode (see
below)
But I don't think you need this at all.
https://chromiumcodereview.appspot.com/24055003/diff/54001/content/common/san...
content/common/sandbox_linux.cc:66: // non official release mode.
Just explain what it does, the "for when using DCHECK would be incorrect" is
unclear out of context.
https://chromiumcodereview.appspot.com/24055003/diff/54001/content/common/san...
content/common/sandbox_linux.cc:72: class OpenDirDeleter {
Sorry, what I meant by creating an OpenDirDeleter() was to
create a scoped_ptr<DIR, DIRDeleter>, i.e. override the "deleter" part of a
scoped pointer. The goal was to keep things very simple without having to create
a giant class.
What you have created is essentially a "ScopedOpenDir", but with a slightly odd
interface.
https://chromiumcodereview.appspot.com/24055003/diff/54001/content/common/san...
content/common/sandbox_linux.cc:108:
FYI (since this class may disappear): always add DISALLOW_COPY_AND_ASSIGN()
https://chromiumcodereview.appspot.com/24055003/diff/54001/content/common/san...
content/common/sandbox_linux.cc:196: LOG(FATAL) << "InitializeSandbox() called
after unexpected directries "
nit: directories
https://chromiumcodereview.appspot.com/24055003/diff/54001/content/common/san...
content/common/sandbox_linux.cc:283: struct stat s;
Please, scope your variables as much as possible. For instance you can declare
these inside the "if", which will also allow you to avoid re-using the same name
twice for different purposes.
https://chromiumcodereview.appspot.com/24055003/diff/54001/content/common/san...
content/common/sandbox_linux.cc:286: if (proc_fd_ >= 0) {
You're handling the case where we don't have proc_fd_ available here, so
anything below can be a normal check.
But the structure is wrong.
Do something like:
int proc_fd = proc_fd_;
#if !defined(NDEBUG)
CHECK_GE(0, proc_fd_)
#endif
if (proc_fd_ == -1) {
proc_fd = open()
if (proc_fd < 0) return false
}
// Now the code can continue just knowing that you have a proc fd in proc_fd.
https://chromiumcodereview.appspot.com/24055003/diff/54001/content/common/san...
content/common/sandbox_linux.cc:288: REAL_DEBUG_CHECK(proc_self_fd != -1);
This should be a normal CHECK.
https://chromiumcodereview.appspot.com/24055003/diff/54001/content/common/san...
content/common/sandbox_linux.cc:295: REAL_DEBUG_CHECK(stat_res == 0);
This could be a normal check
https://chromiumcodereview.appspot.com/24055003/diff/54001/content/common/san...
content/common/sandbox_linux.cc:305: REAL_DEBUG_CHECK(proc_self_fd != -1);
This should be a normal CHECK
https://chromiumcodereview.appspot.com/24055003/diff/54001/content/common/san...
content/common/sandbox_linux.cc:313: REAL_DEBUG_CHECK(stat_res == 0);
This should be a normal CHECK
https://chromiumcodereview.appspot.com/24055003/diff/54001/content/common/san...
content/common/sandbox_linux.cc:322: // let's see if there are any other
directories open.
Are you hitting issue with implementation details of these libraries re-opening
these things?
I'm particularly worried about "something" keeping /proc open.
You should go through /proc/self/fd, and discard whatever number proc_fd is and
whatever number proc_self_fd is and that's all, no?
https://chromiumcodereview.appspot.com/24055003/diff/54001/content/common/san...
content/common/sandbox_linux.cc:324: scoped_ptr<OpenDirDeleter> odd(new
OpenDirDeleter());
As explained above, I was thinking of a custom deleter for the scoped_ptr. If
that's not possible for some reason (please explain why), maybe a
ScopedClosureRunner() ?
Let's use a ScopedOpendir class as a last resort.
On 2013/10/22 19:10:04, Mostyn Bramley-Moore wrote: > https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.cc > File content/common/sandbox_linux.cc (right): > > https://codereview.chromium.org/24055003/diff/1/content/common/sandbox_linux.... > content/common/sandbox_linux.cc:253: // We had to open /proc/self/fd/ so we > really want to check if > On 2013/10/22 01:10:50, jln wrote: > > proc_fd_ and fd_fd should be treated the same, no ? > > Yes, except that we know fd_fd must be valid whereas proc_fd_ might not be. > > > It's a lot safer to check the exact same file descriptor than just counting. > > > > Unfortunately in theory fdopendir() could be at liberty to dup() etc, but your > > current test would already fail under such a scenario. > > If we don't want to count duplicate file descriptors, then I suppose we should > instead check for the device/inode pair of /proc and /proc/self/fd. > > > You could still maintain a counter a DCHECK that it's less than 2. > > Are these the situations we want to catch: > > 1) proc_fd_ is valid, /proc/self/fd is open, and a third distinct directory (ie > neither /proc nor /proc/self/fd) is also open. > > 2) proc_fd_ is invalid and any directory besides /proc/self/fd is open. More simply, the situation we want to catch is "NO directory should be open in release mode". The way we do this, is by keeping a proc_fd_ open ourselves (in debug mode only so that if we make a mistake, security is not compromised). The issue is that counting directories in /proc/self/fd is slightly tedious because while counting we have 2 directory fd open. So when counting, we should skip the two fd which we know are open, but only these. To be extra careful, this should be checked: we should see *exactly* these two directory fds and nothing else.
https://codereview.chromium.org/24055003/diff/54001/content/common/sandbox_li... File content/common/sandbox_linux.cc (right): https://codereview.chromium.org/24055003/diff/54001/content/common/sandbox_li... content/common/sandbox_linux.cc:108: On 2013/10/25 19:49:52, jln wrote: > FYI (since this class may disappear): always add DISALLOW_COPY_AND_ASSIGN() Will do, thanks for the tip. https://codereview.chromium.org/24055003/diff/54001/content/common/sandbox_li... content/common/sandbox_linux.cc:286: if (proc_fd_ >= 0) { On 2013/10/25 19:49:52, jln wrote: > You're handling the case where we don't have proc_fd_ available here, so > anything below can be a normal check. I could leave a "the code after this point can assume we're in debug mode" comment, but isn't it better to use something more explicit, like a DCHECK? > But the structure is wrong. > > Do something like: > > int proc_fd = proc_fd_; > > #if !defined(NDEBUG) > CHECK_GE(0, proc_fd_) > #endif > > if (proc_fd_ == -1) { > proc_fd = open() > if (proc_fd < 0) return false > } > > // Now the code can continue just knowing that you have a proc fd in proc_fd. https://codereview.chromium.org/24055003/diff/54001/content/common/sandbox_li... content/common/sandbox_linux.cc:322: // let's see if there are any other directories open. On 2013/10/25 19:49:52, jln wrote: > Are you hitting issue with implementation details of these libraries re-opening > these things? No, I'm just trying to be more thorough and cover the possibility that you mentioned earlier, where one of these fd's has been dup'ed, without returning a false positive. I figure that it is safe to not return true in this case, since closing any one of a set of dup'ed fd's should close them all, and we are careful to close the original one. > I'm particularly worried about "something" keeping /proc open. > > You should go through /proc/self/fd, and discard whatever number proc_fd is and > whatever number proc_self_fd is and that's all, no? Would you prefer me to change to comparing fd's directly?
https://codereview.chromium.org/24055003/diff/54001/content/common/sandbox_li... File content/common/sandbox_linux.cc (right): https://codereview.chromium.org/24055003/diff/54001/content/common/sandbox_li... content/common/sandbox_linux.cc:286: if (proc_fd_ >= 0) { On 2013/10/25 20:41:15, Mostyn Bramley-Moore wrote: > On 2013/10/25 19:49:52, jln wrote: > > You're handling the case where we don't have proc_fd_ available here, so > > anything below can be a normal check. > > I could leave a "the code after this point can assume we're in debug mode" > comment, but isn't it better to use something more explicit, like a DCHECK? We should check that in debug mode we have proc_fd_ available. That's done with something like. #if !defined(NDEBUG) CHECK_GE(0, proc_fd_) #endif There is nothing else to do, after that, y ou can just handle the case where proc_fd == -1 as explained above. This will keep the code very simple, without convoluted logic. https://codereview.chromium.org/24055003/diff/54001/content/common/sandbox_li... content/common/sandbox_linux.cc:322: // let's see if there are any other directories open. On 2013/10/25 20:41:15, Mostyn Bramley-Moore wrote: > On 2013/10/25 19:49:52, jln wrote: > > Are you hitting issue with implementation details of these libraries > re-opening > > these things? > > No, I'm just trying to be more thorough and cover the possibility that you > mentioned earlier, where one of these fd's has been dup'ed, without returning a > false positive. No, we absolutely need to fail in this case, that's a dangerous situation. > I figure that it is safe to not return true in this case, since closing any one > of a set of dup'ed fd's should close them all, and we are careful to close the > original one. No, if a bunch of fd-s are duped, closing one won't close the others. > > I'm particularly worried about "something" keeping /proc open. > > > > You should go through /proc/self/fd, and discard whatever number proc_fd is > and > > whatever number proc_self_fd is and that's all, no? > > Would you prefer me to change to comparing fd's directly? Yes please.
Patchset 3 looks a lot simpler. https://codereview.chromium.org/24055003/diff/54001/content/common/sandbox_li... File content/common/sandbox_linux.cc (right): https://codereview.chromium.org/24055003/diff/54001/content/common/sandbox_li... content/common/sandbox_linux.cc:64: #if !defined(NDEBUG) On 2013/10/25 19:49:52, jln wrote: > Maybe do DEBUG_NOTREACHED() instead which does CHECK(false) in debug mode (see > below) > > But I don't think you need this at all. Removed this in favour of DIRDeleter. https://codereview.chromium.org/24055003/diff/54001/content/common/sandbox_li... content/common/sandbox_linux.cc:66: // non official release mode. On 2013/10/25 19:49:52, jln wrote: > Just explain what it does, the "for when using DCHECK would be incorrect" is > unclear out of context. Removed this in favour of DIRDeleter. https://codereview.chromium.org/24055003/diff/54001/content/common/sandbox_li... content/common/sandbox_linux.cc:72: class OpenDirDeleter { On 2013/10/25 19:49:52, jln wrote: > Sorry, what I meant by creating an OpenDirDeleter() was to > > create a scoped_ptr<DIR, DIRDeleter>, i.e. override the "deleter" part of a > scoped pointer. The goal was to keep things very simple without having to create > a giant class. > > What you have created is essentially a "ScopedOpenDir", but with a slightly odd > interface. I hadn't noticed this version of scoped_ptr- looks very useful. https://codereview.chromium.org/24055003/diff/54001/content/common/sandbox_li... content/common/sandbox_linux.cc:196: LOG(FATAL) << "InitializeSandbox() called after unexpected directries " On 2013/10/25 19:49:52, jln wrote: > nit: directories Done. https://codereview.chromium.org/24055003/diff/54001/content/common/sandbox_li... content/common/sandbox_linux.cc:283: struct stat s; On 2013/10/25 19:49:52, jln wrote: > Please, scope your variables as much as possible. For instance you can declare > these inside the "if", which will also allow you to avoid re-using the same name > twice for different purposes. Done. https://codereview.chromium.org/24055003/diff/54001/content/common/sandbox_li... content/common/sandbox_linux.cc:286: if (proc_fd_ >= 0) { > We should check that in debug mode we have proc_fd_ available. That's done with > something like. > > #if !defined(NDEBUG) > CHECK_GE(0, proc_fd_) > #endif > > There is nothing else to do, after that, y ou can just handle the case where > proc_fd == -1 as explained above. > > This will keep the code very simple, without convoluted logic. Rather than create a local proc_fd and having to be careful to close it, I reused proc_fd_ (splitting out the code the opens /proc into a separate function). This should then be handled safely by SealSandbox(). https://codereview.chromium.org/24055003/diff/54001/content/common/sandbox_li... content/common/sandbox_linux.cc:288: REAL_DEBUG_CHECK(proc_self_fd != -1); On 2013/10/25 19:49:52, jln wrote: > This should be a normal CHECK. Done. https://codereview.chromium.org/24055003/diff/54001/content/common/sandbox_li... content/common/sandbox_linux.cc:295: REAL_DEBUG_CHECK(stat_res == 0); On 2013/10/25 19:49:52, jln wrote: > This could be a normal check Removed this, since we don't need to stat /proc. https://codereview.chromium.org/24055003/diff/54001/content/common/sandbox_li... content/common/sandbox_linux.cc:305: REAL_DEBUG_CHECK(proc_self_fd != -1); On 2013/10/25 19:49:52, jln wrote: > This should be a normal CHECK Removed this case, since we return early if proc_fd_ is not available. https://codereview.chromium.org/24055003/diff/54001/content/common/sandbox_li... content/common/sandbox_linux.cc:313: REAL_DEBUG_CHECK(stat_res == 0); On 2013/10/25 19:49:52, jln wrote: > This should be a normal CHECK Removed this, since we don't need to stat /proc/self/fd. https://codereview.chromium.org/24055003/diff/54001/content/common/sandbox_li... content/common/sandbox_linux.cc:322: // let's see if there are any other directories open. On 2013/10/26 01:15:15, jln wrote: > On 2013/10/25 20:41:15, Mostyn Bramley-Moore wrote: > > On 2013/10/25 19:49:52, jln wrote: > > > Are you hitting issue with implementation details of these libraries > > re-opening > > > these things? > > > > No, I'm just trying to be more thorough and cover the possibility that you > > mentioned earlier, where one of these fd's has been dup'ed, without returning > a > > false positive. > > No, we absolutely need to fail in this case, that's a dangerous situation. > > > I figure that it is safe to not return true in this case, since closing any > one > > of a set of dup'ed fd's should close them all, and we are careful to close the > > original one. > > No, if a bunch of fd-s are duped, closing one won't close the others. Ah right, I misunderstood this part of the dup manpage :/ > > > I'm particularly worried about "something" keeping /proc open. > > > > > > You should go through /proc/self/fd, and discard whatever number proc_fd is > > and > > > whatever number proc_self_fd is and that's all, no? > > > > Would you prefer me to change to comparing fd's directly? > > Yes please. Done. https://codereview.chromium.org/24055003/diff/54001/content/common/sandbox_li... content/common/sandbox_linux.cc:324: scoped_ptr<OpenDirDeleter> odd(new OpenDirDeleter()); On 2013/10/25 19:49:52, jln wrote: > As explained above, I was thinking of a custom deleter for the scoped_ptr. If > that's not possible for some reason (please explain why), maybe a > ScopedClosureRunner() ? > > Let's use a ScopedOpendir class as a last resort. Done.
*ping* Is this CL still useful, or should I look into the new capabilities sandbox instead?
Sorry, long delay again :( This looks almost good! Yes, this is still needed and very useful. We'll still chroot() in the new sandbox and we need that to be safe. Thanks for spending time on this! https://chromiumcodereview.appspot.com/24055003/diff/134001/content/common/sa... File content/common/sandbox_linux.cc (right): https://chromiumcodereview.appspot.com/24055003/diff/134001/content/common/sa... content/common/sandbox_linux.cc:224: bool LinuxSandbox::OpenProc() { Please, remove this. https://chromiumcodereview.appspot.com/24055003/diff/134001/content/common/sa... content/common/sandbox_linux.cc:241: if (!OpenProc()) { You don't need to do the OpenProc() thing, it adds complexity. Just have something like: if (proc_fd_ >= 0) { proc_self_fd = openat(proc_fd_, "self/fd" ...); } else { proc_self_fd = openat(AT_FDCWD, "/proc/self/fd", ...); if ((proc_self_fd < 0) && errno = ENOENT) return false; } CHECK_LE(0, proc_self_fd); etc... https://chromiumcodereview.appspot.com/24055003/diff/134001/content/common/sa... content/common/sandbox_linux.cc:251: DIR *dir = fdopendir(proc_self_fd); Style: I don't like it but the style is T* ptr, not "T *ptr" You don't need this line anyways. https://chromiumcodereview.appspot.com/24055003/diff/134001/content/common/sa... content/common/sandbox_linux.cc:253: scoped_ptr<DIR, DIRDeleter> dir_fd(dir); Just have directly: scoped_ptr<DIR, DIRDeleter> dir(fdopendir()); CHECK(dir); Then use dir.get() when you need to pass the raw pointer to low level functions. https://chromiumcodereview.appspot.com/24055003/diff/134001/content/common/sa... content/common/sandbox_linux.cc:255: struct dirent e, *de; Style: on two different lines Unfortunately the style guide is TYPE* ptr; https://chromiumcodereview.appspot.com/24055003/diff/134001/content/common/sa... content/common/sandbox_linux.cc:267: // It's OK to use proc_self_fd here, fstatat won't modify it. Maybe it's more readable with dirfd(dir.get()) ? Your choice.
https://codereview.chromium.org/24055003/diff/134001/content/common/sandbox_l... File content/common/sandbox_linux.cc (right): https://codereview.chromium.org/24055003/diff/134001/content/common/sandbox_l... content/common/sandbox_linux.cc:224: bool LinuxSandbox::OpenProc() { On 2013/11/01 18:55:37, jln wrote: > Please, remove this. Done. https://codereview.chromium.org/24055003/diff/134001/content/common/sandbox_l... content/common/sandbox_linux.cc:241: if (!OpenProc()) { On 2013/11/01 18:55:37, jln wrote: > You don't need to do the OpenProc() thing, it adds complexity. > > Just have something like: > > if (proc_fd_ >= 0) { > proc_self_fd = openat(proc_fd_, "self/fd" ...); > } else { > proc_self_fd = openat(AT_FDCWD, "/proc/self/fd", ...); > if ((proc_self_fd < 0) && errno = ENOENT) return false; > } > > CHECK_LE(0, proc_self_fd); > > etc... Done- I assume you mean errno == ENOENT ? https://codereview.chromium.org/24055003/diff/134001/content/common/sandbox_l... content/common/sandbox_linux.cc:251: DIR *dir = fdopendir(proc_self_fd); On 2013/11/01 18:55:37, jln wrote: > Style: I don't like it but the style is T* ptr, not "T *ptr" > > You don't need this line anyways. Done. https://codereview.chromium.org/24055003/diff/134001/content/common/sandbox_l... content/common/sandbox_linux.cc:253: scoped_ptr<DIR, DIRDeleter> dir_fd(dir); On 2013/11/01 18:55:37, jln wrote: > Just have directly: > > scoped_ptr<DIR, DIRDeleter> dir(fdopendir()); > CHECK(dir); > > Then use dir.get() when you need to pass the raw pointer to low level functions. Done. https://codereview.chromium.org/24055003/diff/134001/content/common/sandbox_l... content/common/sandbox_linux.cc:255: struct dirent e, *de; On 2013/11/01 18:55:37, jln wrote: > Style: on two different lines > Unfortunately the style guide is TYPE* ptr; Done. https://codereview.chromium.org/24055003/diff/134001/content/common/sandbox_l... content/common/sandbox_linux.cc:267: // It's OK to use proc_self_fd here, fstatat won't modify it. On 2013/11/01 18:55:37, jln wrote: > Maybe it's more readable with dirfd(dir.get()) ? Your choice. IMO it's more obvious to people reading this code if we use proc_self_fd.
lgtm with a few nits. Thanks for working on this! https://chromiumcodereview.appspot.com/24055003/diff/194001/content/common/sa... File content/common/sandbox_linux.cc (right): https://chromiumcodereview.appspot.com/24055003/diff/194001/content/common/sa... content/common/sandbox_linux.cc:67: CHECK(closedir(d) == 0); You can even use PCHECK(). https://chromiumcodereview.appspot.com/24055003/diff/194001/content/common/sa... content/common/sandbox_linux.cc:159: "have been opened- the setuid sandbox may be at risk, if " I would drop the detail about the BPF sandbox and just say something simple like "This breaks the security of the setuid sandbox". https://chromiumcodereview.appspot.com/24055003/diff/194001/content/common/sa... File content/common/sandbox_linux.h (right): https://chromiumcodereview.appspot.com/24055003/diff/194001/content/common/sa... content/common/sandbox_linux.h:98: bool sealed_; This is not needed, I would rather remove it for now, as more states make this more complicated.
https://codereview.chromium.org/24055003/diff/194001/content/common/sandbox_l... File content/common/sandbox_linux.cc (right): https://codereview.chromium.org/24055003/diff/194001/content/common/sandbox_l... content/common/sandbox_linux.cc:67: CHECK(closedir(d) == 0); On 2013/11/02 00:10:56, jln wrote: > You can even use PCHECK(). Done. https://codereview.chromium.org/24055003/diff/194001/content/common/sandbox_l... content/common/sandbox_linux.cc:159: "have been opened- the setuid sandbox may be at risk, if " On 2013/11/02 00:10:56, jln wrote: > I would drop the detail about the BPF sandbox and just say something simple like > "This breaks the security of the setuid sandbox". Done. https://codereview.chromium.org/24055003/diff/194001/content/common/sandbox_l... File content/common/sandbox_linux.h (right): https://codereview.chromium.org/24055003/diff/194001/content/common/sandbox_l... content/common/sandbox_linux.h:98: bool sealed_; On 2013/11/02 00:10:56, jln wrote: > This is not needed, I would rather remove it for now, as more states make this > more complicated. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mostynb@opera.com/24055003/254001
Retried try job too often on linux_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
If I remove the "errno is ENOENT" part of the "guess false if we can't open /proc/self/fd directly and errno is ENOENT" then this seems to pass (though the tree is closed, and currently fails some unrelated ui tests). I think this is OK, but perhaps I misunderstand the point of the ENOENT check- why do you consider this error OK to guess false, while others are fatal? Should I instead add back the ENOENT check and try to figure out why /proc/self/fd can't be opened?
https://chromiumcodereview.appspot.com/24055003/diff/514001/content/common/sa... File content/common/sandbox_linux.cc (right): https://chromiumcodereview.appspot.com/24055003/diff/514001/content/common/sa... content/common/sandbox_linux.cc:231: if (proc_self_fd < 0) { I wonder what's happening for errno to not be ENOENT. The seccomp-bpf is not engaged (it would EPERM instead). Did you try to build release mode and try locally ? I'm a bit worried that we're missing something here. Feel free to: 1. Open a bug about it. 2. add a TODO() for yourself in a comment here and point to the bug. 3. Land this version while you investigate.
https://codereview.chromium.org/24055003/diff/514001/content/common/sandbox_l... File content/common/sandbox_linux.cc (right): https://codereview.chromium.org/24055003/diff/514001/content/common/sandbox_l... content/common/sandbox_linux.cc:231: if (proc_self_fd < 0) { On 2013/11/05 03:47:28, jln wrote: > I wonder what's happening for errno to not be ENOENT. The seccomp-bpf is not > engaged (it would EPERM instead). > > Did you try to build release mode and try locally ? Yes. I'll check this again once more before queueing though. > I'm a bit worried that we're missing something here. > > Feel free to: > 1. Open a bug about it. > 2. add a TODO() for yourself in a comment here and point to the bug. > 3. Land this version while you investigate. Filed crbug.com/314985 and left a TODO note.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mostynb@opera.com/24055003/634001
Retried try job too often on win7_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mostynb@opera.com/24055003/634001
Retried try job too often on win_rel for step(s) content_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mostynb@opera.com/24055003/634001
Message was sent while issue was closed.
Change committed as 233100 |
