Allow chrome_sandbox to act as a helper program and find the socket with a gi...

Allow chrome_sandbox to act as a helper program and find the socket with a given inode number.

BUG=none
TEST=none

[agl, 2009-10-23 17:51:08]

NACK. The sandbox cannot be C++. Nor can it link against base.

http://codereview.chromium.org/312003/diff/1/4
File sandbox/linux/suid/sandbox.cc (right):

http://codereview.chromium.org/312003/diff/1/4#newcode302
Line 302: if (argc == 3 && (0 == strcmp(argv[1], base::kFindInode))) {
Needs a big comment explaining what's going on. I guess you're using root privs
to search for the inode? The magic string should begin with '--' in that case to
make it clear that it's a flag.

Also, sandbox cannot link against base. Copy the code if need be.

[Lei Zhang, 2009-10-29 21:02:35]

Ok, added a block of comments and copied the relevant code out of
base/linux_utils.cc.

[agl, 2009-10-30 00:18:26]

http://codereview.chromium.org/312003/diff/8001/9003
File sandbox/linux/suid/linux_util.c (right):

http://codereview.chromium.org/312003/diff/8001/9003#newcode63
Line 63: snprintf(buf, sizeof(buf), "/proc/%ld/fd", pid_ul);
paranoia: open the directory with open and O_DIRECTORY. fstat the dir_fd and
skip it if st_uid != getuid(). Then use fdopendir().

http://codereview.chromium.org/312003/diff/8001/9003#newcode70
Line 70: dent->d_name) >= sizeof(buf)) {
On second reading of the manpage, change this to ">= sizeof(buf) - 1"

http://codereview.chromium.org/312003/diff/8001/9004
File sandbox/linux/suid/sandbox.c (right):

http://codereview.chromium.org/312003/diff/8001/9004#newcode313
Line 313: if (pid == ULONG_MAX || *endptr)
s/pid/inode/

http://codereview.chromium.org/312003/diff/8001/9004#newcode313
Line 313: if (pid == ULONG_MAX || *endptr)
s/ULONG_MAX/ULLONG_MAX/

[Lei Zhang, 2009-10-30 01:41:40]

http://codereview.chromium.org/312003/diff/8001/9003
File sandbox/linux/suid/linux_util.c (right):

http://codereview.chromium.org/312003/diff/8001/9003#newcode63
Line 63: snprintf(buf, sizeof(buf), "/proc/%ld/fd", pid_ul);
On 2009/10/30 00:18:26, agl wrote:
> paranoia: open the directory with open and O_DIRECTORY. fstat the dir_fd and
> skip it if st_uid != getuid(). Then use fdopendir().

It looks as though glibc's opendir() already tries to use O_DIRECTORY, and if
it's not supported, stat() the file to verify it's a directory.

http://codereview.chromium.org/312003/diff/8001/9003#newcode70
Line 70: dent->d_name) >= sizeof(buf)) {
On 2009/10/30 00:18:26, agl wrote:
> On second reading of the manpage, change this to ">= sizeof(buf) - 1"

Are you sure? I wrote a trivial test program and it seems it's ok for snprintf()
to return sizeof(buf) - 1. I.e. it's ok to have: char buf[5]; snprintf(buf, 5,
"1234");.

http://codereview.chromium.org/312003/diff/8001/9004
File sandbox/linux/suid/sandbox.c (right):

http://codereview.chromium.org/312003/diff/8001/9004#newcode313
Line 313: if (pid == ULONG_MAX || *endptr)
On 2009/10/30 00:18:26, agl wrote:
> s/ULONG_MAX/ULLONG_MAX/

Done.

http://codereview.chromium.org/312003/diff/8001/9004#newcode313
Line 313: if (pid == ULONG_MAX || *endptr)
On 2009/10/30 00:18:26, agl wrote:
> s/pid/inode/

Arg, thanks for catching that.

[agl, 2009-10-30 01:49:39]

http://codereview.chromium.org/312003/diff/8001/9003
File sandbox/linux/suid/linux_util.c (right):

http://codereview.chromium.org/312003/diff/8001/9003#newcode63
Line 63: snprintf(buf, sizeof(buf), "/proc/%ld/fd", pid_ul);
On 2009/10/30 01:41:40, Lei Zhang wrote:
> On 2009/10/30 00:18:26, agl wrote:
> > paranoia: open the directory with open and O_DIRECTORY. fstat the dir_fd and
> > skip it if st_uid != getuid(). Then use fdopendir().
> 
> It looks as though glibc's opendir() already tries to use O_DIRECTORY, and if
> it's not supported, stat() the file to verify it's a directory.

The important part was the check that st_uid == getuid() - i.e. we won't reveal
any information about processes that aren't owned by the invoking user.

http://codereview.chromium.org/312003/diff/8001/9003#newcode70
Line 70: dent->d_name) >= sizeof(buf)) {
On 2009/10/30 01:41:40, Lei Zhang wrote:
> On 2009/10/30 00:18:26, agl wrote:
> > On second reading of the manpage, change this to ">= sizeof(buf) - 1"
> 
> Are you sure? I wrote a trivial test program and it seems it's ok for
snprintf()
> to return sizeof(buf) - 1. I.e. it's ok to have: char buf[5]; snprintf(buf, 5,
> "1234");.

"If the  output was truncated due to this limit then the return value is the
number of characters (not including the trailing '\0') which would have  been 
written  to  the final  string  if  enough space had been available"

So, if it returns sizeof(buf), it may not have NUL terminated the buffer. Even
if it works on Linux, what's the bet that someone will use a different libc etc
and follow a stricter reading of the manpage?

[Lei Zhang, 2009-10-31 07:40:19]

http://codereview.chromium.org/312003/diff/8001/9003
File sandbox/linux/suid/linux_util.c (right):

http://codereview.chromium.org/312003/diff/8001/9003#newcode63
Line 63: snprintf(buf, sizeof(buf), "/proc/%ld/fd", pid_ul);
On 2009/10/30 01:49:39, agl wrote:
> On 2009/10/30 01:41:40, Lei Zhang wrote:
> > On 2009/10/30 00:18:26, agl wrote:
> > > paranoia: open the directory with open and O_DIRECTORY. fstat the dir_fd
and
> > > skip it if st_uid != getuid(). Then use fdopendir().
> > 
> > It looks as though glibc's opendir() already tries to use O_DIRECTORY, and
if
> > it's not supported, stat() the file to verify it's a directory.
> 
> The important part was the check that st_uid == getuid() - i.e. we won't
reveal
> any information about processes that aren't owned by the invoking user.

/proc/$chrome_renderer_pid/fd is owned by root. We want to check the ownership
of /proc/$chrome_renderer_pid.

http://codereview.chromium.org/312003/diff/8001/9003#newcode70
Line 70: dent->d_name) >= sizeof(buf)) {
On 2009/10/30 01:49:39, agl wrote:
> On 2009/10/30 01:41:40, Lei Zhang wrote:
> > On 2009/10/30 00:18:26, agl wrote:
> > > On second reading of the manpage, change this to ">= sizeof(buf) - 1"
> > 
> > Are you sure? I wrote a trivial test program and it seems it's ok for
> snprintf()
> > to return sizeof(buf) - 1. I.e. it's ok to have: char buf[5]; snprintf(buf,
5,
> > "1234");.
> 
> "If the  output was truncated due to this limit then the return value is the
> number of characters (not including the trailing '\0') which would have  been 
> written  to  the final  string  if  enough space had been available"
> 
> So, if it returns sizeof(buf), it may not have NUL terminated the buffer. Even
> if it works on Linux, what's the bet that someone will use a different libc
etc
> and follow a stricter reading of the manpage?

ok.

[agl, 2009-11-03 18:47:53]

LGTM

http://codereview.chromium.org/312003/diff/7005/11003
File sandbox/linux/suid/linux_util.c (right):

http://codereview.chromium.org/312003/diff/7005/11003#newcode72
Line 72: snprintf(buf, sizeof(buf), "/proc/%ld", pid_ul);
%lu, no?

[Lei Zhang, 2009-11-03 19:56:09]

On 2009/11/03 18:47:53, agl wrote:
> LGTM
> 
> http://codereview.chromium.org/312003/diff/7005/11003
> File sandbox/linux/suid/linux_util.c (right):
> 
> http://codereview.chromium.org/312003/diff/7005/11003#newcode72
> Line 72: snprintf(buf, sizeof(buf), "/proc/%ld", pid_ul);
> %lu, no?

yes, done.