Move the SingletonSocket to a temporary directory

Move the SingletonSocket to a temporary directory

This is to workaround problems on certain network filesystems (notably AFS)
which do not support Unix domain sockets. We move the sockets into a temporary
folder and symlink. To avoid the possibility of a dangling link to a missing
(and later intercepted) remote directory, we create and check cookie files
and rely on the stickiness of /tmp/ to avoid a race condition in the check.

R=mattm
BUG=44606
TEST=ProcessSingletonLinuxTest.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=57623

[davidben, 2010-06-30 03:09:59]

Not sure if the backwards compatibility path is necessary. (In retrospect, it
probably isn't, given that it doesn't handle the reverse direction anyway.) Eh,
if you think it's not useful, I can remove it and save us some code.

[Evan Martin, 2010-06-30 18:30:43]

I believe this is incorrect.  Perhaps I misunderstand your change description; I
didn't read the code.

The point of the singleton socket is to protect your profile against concurrent
modifications, whether on the current machine or by multiple machines (in the
case of a networked home directory).  So we must keep it on the same filesystem
as your profile.

[davidben, 2010-06-30 18:51:55]

My understanding is that the symlink SingletonLock is what actually locks the
profile. The existing code already handled the case that we noticed the symlink
(from possibly another machine) but couldn't connect to the socket for whatever
reason[1]. I only moved the SingletonSocket; SingletonLock is still on the
correct filesystem.

[1] I'm not actually sure what NFS does with Unix domain sockets, but I infer
from the code that there are cases in which a socket created by a client on
another machine is not connect, and we appear to handle such cases correctly.
(Modulo hostnames being a poor identifier for machines. The dbus machine-id
might be better, but it can change across a reboot.) We don't appear to
distinguish any of the errors returned by connect(), so a missing socket vs. one
that is not connectable for some other reason shouldn't cause trouble.

[davidben, 2010-06-30 18:54:15]

On 2010/06/30 18:51:55, David Benjamin wrote:
> [1] I'm not actually sure what NFS does with Unix domain sockets

I guess this does change behavior if some network filesystems will allow a Unix
domain socket and even forward traffic over them, in that instead of your new
tab magically appearing on another machine, you'll get the little dialog
complaining that the profile is already locked by $HOSTNAME.

One could argue this is more reasonable behavior, but that can be restored by
first attempting to create SingletonSocket at the usual place, and falling back
to /tmp/ if that fails.

[davidben, 2010-07-08 21:50:14]

evanm: ping? Is my understanding of the two files correct, or does
SingletonSocket play a role in locking (as opposed to SingletonLock taking care
of it) as well as communication?

[mattm, 2010-07-08 22:15:53]

On 2010/07/08 21:50:14, David Benjamin wrote:
> evanm: ping? Is my understanding of the two files correct, or does
> SingletonSocket play a role in locking (as opposed to SingletonLock taking
care
> of it) as well as communication?

yes, SingletonLock does the locking now.   Abstractly this cl sounds ok, I'd
like to think about it a bit more though. 

re: the other question, I'm not aware of any case where a domain socket on a
networked filesystem would actually forward to a different system, so this
doesn't really change that case.

[Evan Martin, 2010-07-19 18:23:01]

On 2010/07/08 22:15:53, mattm wrote:
> On 2010/07/08 21:50:14, David Benjamin wrote:
> > evanm: ping? Is my understanding of the two files correct, or does
> > SingletonSocket play a role in locking (as opposed to SingletonLock taking
> care
> > of it) as well as communication?
> 
> yes, SingletonLock does the locking now.   Abstractly this cl sounds ok, I'd
> like to think about it a bit more though. 
> 
> re: the other question, I'm not aware of any case where a domain socket on a
> networked filesystem would actually forward to a different system, so this
> doesn't really change that case.

Ping, let's not drop this.  (I don't have a vested interest in either committing
it or forgetting it but we should pick one...)

Now that davidben has explained it to me I think it is doing the right thing.  I
would double check that we now look for the lock (in the user-data-dir) before
considering the socket; the comments at the top of process_singleton_linux.cc
say it happens in the other order which is not correct.  I guess that is review
feedback -- fix the comment.  :)

[mattm, 2010-07-19 22:39:40]

On 2010/07/19 18:23:01, Evan Martin wrote:
> On 2010/07/08 22:15:53, mattm wrote:
> > On 2010/07/08 21:50:14, David Benjamin wrote:
> > > evanm: ping? Is my understanding of the two files correct, or does
> > > SingletonSocket play a role in locking (as opposed to SingletonLock taking
> > care
> > > of it) as well as communication?
> > 
> > yes, SingletonLock does the locking now.   Abstractly this cl sounds ok, I'd
> > like to think about it a bit more though. 
> > 
> > re: the other question, I'm not aware of any case where a domain socket on a
> > networked filesystem would actually forward to a different system, so this
> > doesn't really change that case.
> 
> Ping, let's not drop this.  (I don't have a vested interest in either
committing
> it or forgetting it but we should pick one...)


Sorry for the delay, just looked it over again.

The main points I was thinking about are:

1. Should it default to using /tmp or try the profile dir first and then fall
back to tmp?
2. Have a separate SingletonSocketDirectory or just make SingletonSocket itself
a symlink when we're storing it in tmp?

By trying the user data dir before tmp we could save a few filesystem accesses
in the common (non-AFS) case, but I dunno if that's enough to worry about.

> Now that davidben has explained it to me I think it is doing the right thing. 
I
> would double check that we now look for the lock (in the user-data-dir) before
> considering the socket; the comments at the top of process_singleton_linux.cc
> say it happens in the other order which is not correct.  I guess that is
review
> feedback -- fix the comment.  :)

We do actually try to send on the socket before looking at the lock - the lock
could be wrong if the hostname changed, but if we can talk to someone on the
socket then its clear there's already an instance running anyway.  The lock is
then used before listening on the socket.

[Evan Martin, 2010-07-19 22:46:58]

On 2010/07/19 22:39:40, mattm wrote:
> 1. Should it default to using /tmp or try the profile dir first and then fall
> back to tmp?

I think the latter behavior is maybe better, since it's simpler for the common
case.

> 2. Have a separate SingletonSocketDirectory or just make SingletonSocket
itself
> a symlink when we're storing it in tmp?

I guess there are security concerns if you do it wrong.

> We do actually try to send on the socket before looking at the lock - the lock
> could be wrong if the hostname changed, but if we can talk to someone on the
> socket then its clear there's already an instance running anyway.  The lock is
> then used before listening on the socket.

I was worried that when the socket is in /tmp there's the potential that there's
someone listening but your homedir profile is still locked by a remote NFS user.
 But I guess if someone's listening we must know that they've locked that
profile?

[davidben, 2010-07-19 22:58:30]

On 2010/07/19 22:46:58, Evan Martin wrote:
> On 2010/07/19 22:39:40, mattm wrote:
> > 1. Should it default to using /tmp or try the profile dir first and then
fall
> > back to tmp?
> 
> I think the latter behavior is maybe better, since it's simpler for the common
> case.

I went with the former just so we don't have two different codepaths in this
already hairy code.


> > 2. Have a separate SingletonSocketDirectory or just make SingletonSocket
> itself
> > a symlink when we're storing it in tmp?
> 
> I guess there are security concerns if you do it wrong.

It does make it a bit more of a nuisance to play the chdir game. There isn't a
connectat() syscall, so I don't think you can safely check the socket directly.


> > We do actually try to send on the socket before looking at the lock - the
lock
> > could be wrong if the hostname changed, but if we can talk to someone on the
> > socket then its clear there's already an instance running anyway.  The lock
is
> > then used before listening on the socket.
> 
> I was worried that when the socket is in /tmp there's the potential that
there's
> someone listening but your homedir profile is still locked by a remote NFS
user.
>  But I guess if someone's listening we must know that they've locked that
> profile?

The CL should refuse to send to a sockets in directories that are readable or
writable by any other user. (Hence the funny chdir logic; it's there to avoid a
time-of-check vs time-of-access race in verifying that.)

[mattm, 2010-07-22 23:24:21]

lgtm with some fixes

http://codereview.chromium.org/2838034/diff/1/2
File base/scoped_temp_dir.cc (right):

http://codereview.chromium.org/2838034/diff/1/2#newcode55
base/scoped_temp_dir.cc:55: LOG(ERROR) << "ScopedTempDir unable to delete " <<
path_.value();
should clear path_ here so the destructor doesn't try to delete it again

http://codereview.chromium.org/2838034/diff/1/5
File chrome/browser/process_singleton_linux.cc (right):

http://codereview.chromium.org/2838034/diff/1/5#newcode393
chrome/browser/process_singleton_linux.cc:393: chdir_rv_ = old_fd_ = -1;
nit: use initializer syntax for these

http://codereview.chromium.org/2838034/diff/1/5#newcode765
chrome/browser/process_singleton_linux.cc:765: socket_path =
socket_path.Append(chrome::kSingletonSocketFilename);
couldn't these two lines just be
socket_path = FilePath(chrome::kSingletonSocketFilename);
?

http://codereview.chromium.org/2838034/diff/1/5#newcode953
chrome/browser/process_singleton_linux.cc:953: // but something doesn't like us.
Might as well fallback to home.
Comment doesn't match code (doesn't fallback to home). (I'd just update the
comment, don't think falling back for this has much use.)

http://codereview.chromium.org/2838034/diff/1/7
File chrome/common/chrome_constants.cc (right):

http://codereview.chromium.org/2838034/diff/1/7#newcode92
chrome/common/chrome_constants.cc:92: // chrome_process_util_linux would be
broken.
Please remove this comment too.  I just double checked, it's obsolete (and would
be worrying for this change if it weren't)

[mattm, 2010-07-22 23:24:53]

On 2010/07/19 22:58:30, David Benjamin wrote:
> On 2010/07/19 22:46:58, Evan Martin wrote:
> > On 2010/07/19 22:39:40, mattm wrote:
> > > 1. Should it default to using /tmp or try the profile dir first and then
> fall
> > > back to tmp?
> > 
> > I think the latter behavior is maybe better, since it's simpler for the
common
> > case.
> 
> I went with the former just so we don't have two different codepaths in this
> already hairy code.
> 
> 
> > > 2. Have a separate SingletonSocketDirectory or just make SingletonSocket
> > itself
> > > a symlink when we're storing it in tmp?
> > 
> > I guess there are security concerns if you do it wrong.
> 
> It does make it a bit more of a nuisance to play the chdir game. There isn't a
> connectat() syscall, so I don't think you can safely check the socket
directly.
> 
> 
> > > We do actually try to send on the socket before looking at the lock - the
> lock
> > > could be wrong if the hostname changed, but if we can talk to someone on
the
> > > socket then its clear there's already an instance running anyway.  The
lock
> is
> > > then used before listening on the socket.
> > 
> > I was worried that when the socket is in /tmp there's the potential that
> there's
> > someone listening but your homedir profile is still locked by a remote NFS
> user.
> >  But I guess if someone's listening we must know that they've locked that
> > profile?
> 
> The CL should refuse to send to a sockets in directories that are readable or
> writable by any other user. (Hence the funny chdir logic; it's there to avoid
a
> time-of-check vs time-of-access race in verifying that.)

I believe Evan was worried about multiple browsers run by the same user, if the
user has shared home, one browser running on another system that has the profile
lock, yet a browser running on the current system listening on the local socket
even though though lock is held by a different system.  But yeah, we don't start
listening until after aquiring the lock, so I don't think this is possible.

[mattm, 2010-07-22 23:36:46]

Oh yeah, I had one more concern.  I think there may be other threads started by
the time ProcessSingleton does its thing?  Might any of them access files by
relative path?

[davidben, 2010-07-22 23:44:31]

Do you think it's worth keeping the compatibility code to support listening from
the old socket? I'm not very convinced it actually does much, since the change
already doesn't allow an older Chromium to talk to a newer one.


> Oh yeah, I had one more concern.  I think there may be other threads started
by
> the time ProcessSingleton does its thing?  Might any of them access files by
> relative path?

Oh, eww. Yeah, I'm not sure how to deal with that. I guess we could fork or
something, but all this code is absurd enough as it is. What other threads are
created?

http://codereview.chromium.org/2838034/diff/1/2
File base/scoped_temp_dir.cc (right):

http://codereview.chromium.org/2838034/diff/1/2#newcode55
base/scoped_temp_dir.cc:55: LOG(ERROR) << "ScopedTempDir unable to delete " <<
path_.value();
On 2010/07/22 23:24:21, mattm wrote:
> should clear path_ here so the destructor doesn't try to delete it again

Done.

http://codereview.chromium.org/2838034/diff/1/5
File chrome/browser/process_singleton_linux.cc (right):

http://codereview.chromium.org/2838034/diff/1/5#newcode393
chrome/browser/process_singleton_linux.cc:393: chdir_rv_ = old_fd_ = -1;
On 2010/07/22 23:24:21, mattm wrote:
> nit: use initializer syntax for these

Done.

http://codereview.chromium.org/2838034/diff/1/5#newcode765
chrome/browser/process_singleton_linux.cc:765: socket_path =
socket_path.Append(chrome::kSingletonSocketFilename);
On 2010/07/22 23:24:21, mattm wrote:
> couldn't these two lines just be
> socket_path = FilePath(chrome::kSingletonSocketFilename);
> ?

Done.

http://codereview.chromium.org/2838034/diff/1/5#newcode953
chrome/browser/process_singleton_linux.cc:953: // but something doesn't like us.
Might as well fallback to home.
On 2010/07/22 23:24:21, mattm wrote:
> Comment doesn't match code (doesn't fallback to home). (I'd just update the
> comment, don't think falling back for this has much use.)

Done.

http://codereview.chromium.org/2838034/diff/1/7
File chrome/common/chrome_constants.cc (right):

http://codereview.chromium.org/2838034/diff/1/7#newcode92
chrome/common/chrome_constants.cc:92: // chrome_process_util_linux would be
broken.
On 2010/07/22 23:24:21, mattm wrote:
> Please remove this comment too.  I just double checked, it's obsolete (and
would
> be worrying for this change if it weren't)

Done.

[mattm, 2010-07-23 01:03:59]

On 2010/07/22 23:44:31, David Benjamin wrote:
> Do you think it's worth keeping the compatibility code to support listening
from
> the old socket? I'm not very convinced it actually does much, since the change
> already doesn't allow an older Chromium to talk to a newer one.

I don't see such compatability code, just code to support talking to the old
socket.

> 
> > Oh yeah, I had one more concern.  I think there may be other threads started
> by
> > the time ProcessSingleton does its thing?  Might any of them access files by
> > relative path?
> 
> Oh, eww. Yeah, I'm not sure how to deal with that. I guess we could fork or
> something, but all this code is absurd enough as it is. What other threads are
> created?

Just did a quick test (enable PlatformThread::SetName, break in
ProcessSingleton::NotifyOtherProcessWithTimeout and check thread names). 
Truncated, but still should be clear enough:  (except for why there claim to be
two CrBrowserMain threads)
CrBrowserMain
NetworkChangeNo
CrBrowserMain  (this one is the chrome/browser/browser_main_posix.cc
ShutdownDetector thread)
Chrome_DBThread
Chrome_FileThre
Chrome_ProcessL
Chrome_CacheThr
Chrome_Backgrou
Chrome_IOThread
WorkerPool/1175



> 
> http://codereview.chromium.org/2838034/diff/1/2
> File base/scoped_temp_dir.cc (right):
> 
> http://codereview.chromium.org/2838034/diff/1/2#newcode55
> base/scoped_temp_dir.cc:55: LOG(ERROR) << "ScopedTempDir unable to delete " <<
> path_.value();
> On 2010/07/22 23:24:21, mattm wrote:
> > should clear path_ here so the destructor doesn't try to delete it again
> 
> Done.
> 
> http://codereview.chromium.org/2838034/diff/1/5
> File chrome/browser/process_singleton_linux.cc (right):
> 
> http://codereview.chromium.org/2838034/diff/1/5#newcode393
> chrome/browser/process_singleton_linux.cc:393: chdir_rv_ = old_fd_ = -1;
> On 2010/07/22 23:24:21, mattm wrote:
> > nit: use initializer syntax for these
> 
> Done.
> 
> http://codereview.chromium.org/2838034/diff/1/5#newcode765
> chrome/browser/process_singleton_linux.cc:765: socket_path =
> socket_path.Append(chrome::kSingletonSocketFilename);
> On 2010/07/22 23:24:21, mattm wrote:
> > couldn't these two lines just be
> > socket_path = FilePath(chrome::kSingletonSocketFilename);
> > ?
> 
> Done.
> 
> http://codereview.chromium.org/2838034/diff/1/5#newcode953
> chrome/browser/process_singleton_linux.cc:953: // but something doesn't like
us.
> Might as well fallback to home.
> On 2010/07/22 23:24:21, mattm wrote:
> > Comment doesn't match code (doesn't fallback to home). (I'd just update the
> > comment, don't think falling back for this has much use.)
> 
> Done.
> 
> http://codereview.chromium.org/2838034/diff/1/7
> File chrome/common/chrome_constants.cc (right):
> 
> http://codereview.chromium.org/2838034/diff/1/7#newcode92
> chrome/common/chrome_constants.cc:92: // chrome_process_util_linux would be
> broken.
> On 2010/07/22 23:24:21, mattm wrote:
> > Please remove this comment too.  I just double checked, it's obsolete (and
> would
> > be worrying for this change if it weren't)
> 
> Done.

[davidben, 2010-07-23 01:31:02]

On 2010/07/23 01:03:59, mattm wrote:
> On 2010/07/22 23:44:31, David Benjamin wrote:
> > Do you think it's worth keeping the compatibility code to support listening
> from
> > the old socket? I'm not very convinced it actually does much, since the
change
> > already doesn't allow an older Chromium to talk to a newer one.
> 
> I don't see such compatability code, just code to support talking to the old
> socket.

Yeah, that's what I meant. I believe it should handle a new Chromium talking to
an older running one. I guess that's a legitimate use case, with the distro
updating a package and possibly confusing future invocations...

I think it won't work cleanly if an old Chromium is run while a new one is
running. I recall there being logic to kill the process and blow away the lock
if the hostnames match up but the socket is unreachable.


> > > Oh yeah, I had one more concern.  I think there may be other threads
started
> > by
> > > the time ProcessSingleton does its thing?  Might any of them access files
by
> > > relative path?
> > 
> > Oh, eww. Yeah, I'm not sure how to deal with that. I guess we could fork or
> > something, but all this code is absurd enough as it is. What other threads
are
> > created?
> 
> Just did a quick test (enable PlatformThread::SetName, break in
> ProcessSingleton::NotifyOtherProcessWithTimeout and check thread names). 
> Truncated, but still should be clear enough:  (except for why there claim to
be
> two CrBrowserMain threads)
> CrBrowserMain
> NetworkChangeNo
> CrBrowserMain  (this one is the chrome/browser/browser_main_posix.cc
> ShutdownDetector thread)
> Chrome_DBThread
> Chrome_FileThre
> Chrome_ProcessL
> Chrome_CacheThr
> Chrome_Backgrou
> Chrome_IOThread
> WorkerPool/1175
> 

Mmm. I presume then the threads start up early, but don't do anything meaningful
until we've decided it's safe to start up?

There seems to be similar directory-changing code in
chrome/net/url_fixer_upper.cc with calls to SetCurrentDirectory in
URLFixerUpper::FixUpRelativeFile that gets called somewhere in browser_init.cc,
so we perhaps do have some guarantees as to chdir changing. (Or perhaps a race.
:-D) But I'm unfamiliar with that code.

[mattm, 2010-07-23 02:21:17]

On 2010/07/23 01:31:02, David Benjamin wrote:
> On 2010/07/23 01:03:59, mattm wrote:
> > On 2010/07/22 23:44:31, David Benjamin wrote:
> > > Do you think it's worth keeping the compatibility code to support
listening
> > from
> > > the old socket? I'm not very convinced it actually does much, since the
> change
> > > already doesn't allow an older Chromium to talk to a newer one.
> > 
> > I don't see such compatability code, just code to support talking to the old
> > socket.
> 
> Yeah, that's what I meant. I believe it should handle a new Chromium talking
to
> an older running one. I guess that's a legitimate use case, with the distro
> updating a package and possibly confusing future invocations...
> 
> I think it won't work cleanly if an old Chromium is run while a new one is
> running. I recall there being logic to kill the process and blow away the lock
> if the hostnames match up but the socket is unreachable.

I think this is fine.  New chromium talking to an old one after an update is
indeed reasonable, but old talking to new isn't something that's likely to
happen except maybe to developers.

> 
> > > > Oh yeah, I had one more concern.  I think there may be other threads
> started
> > > by
> > > > the time ProcessSingleton does its thing?  Might any of them access
files
> by
> > > > relative path?
> > > 
> > > Oh, eww. Yeah, I'm not sure how to deal with that. I guess we could fork
or
> > > something, but all this code is absurd enough as it is. What other threads
> are
> > > created?
> > 
> > Just did a quick test (enable PlatformThread::SetName, break in
> > ProcessSingleton::NotifyOtherProcessWithTimeout and check thread names). 
> > Truncated, but still should be clear enough:  (except for why there claim to
> be
> > two CrBrowserMain threads)
> > CrBrowserMain
> > NetworkChangeNo
> > CrBrowserMain  (this one is the chrome/browser/browser_main_posix.cc
> > ShutdownDetector thread)
> > Chrome_DBThread
> > Chrome_FileThre
> > Chrome_ProcessL
> > Chrome_CacheThr
> > Chrome_Backgrou
> > Chrome_IOThread
> > WorkerPool/1175
> > 
> 
> Mmm. I presume then the threads start up early, but don't do anything
meaningful
> until we've decided it's safe to start up?

probably, yeah.

> There seems to be similar directory-changing code in
> chrome/net/url_fixer_upper.cc with calls to SetCurrentDirectory in
> URLFixerUpper::FixUpRelativeFile that gets called somewhere in
browser_init.cc,
> so we perhaps do have some guarantees as to chdir changing. (Or perhaps a
race.
> :-D) But I'm unfamiliar with that code.

Saw that too, I'm also unfamiliar with it, but it does provide some reassurance.

[davidben, 2010-08-17 18:15:12]

In the interest of not having this CL stagnate forever, what's the status? Evan:
have you had a chance to review it? This is somewhat hairy code, so I'd rather
have two people signing off to be sure. (On that note, Matt: was than an LGTM?
I'm always confused with LGTMs that are followed with a discussion. :D)

Some other thoughts:

We could, instead of symlinking the directory, symlink SingletonSocket directly
to the place where the new socket lives. While this does mean that we'd have to
play silly path tricks to get the socket directory for the fchdir thing, it
means we get both compatibility directions for free; I just did a test and
connecting via symlink works just fine. Though it does mean the compatibility
path would be vulnerable to the same sort of remote filesystem attack as before.
I don't know if the original problem serious enough to care about, to say
nothing of the compatibility path.

The chdir thing is also annoying, but I'm not sure how to get rid of it (unless
we decide we don't really care about the fairly mild security nuisances?). One
thought I had was to stick some random cookie in the profile directory, and
binding that cookie to the socket somehow. The remote browser could send that
cookie, but then we lose compatibility, and other mechanisms I can think of have
race problems. One possibility would be to embed the cookie in the name of the
socket at the end. Then you'd need to know the cookie to spoof the socket (short
of filling the temporary directory with 2^N different socket names --- probably
infeasible), and since the socket is originally placed in a read-only directory,
you shouldn't be able to get at it.

Yet another thought would be to use the DBus machine id,
/var/lib/dbus/machine-id (accessible value dbus_get_local_machine_id). DBus
generates a UUID for every machine; I believe it generates it on bootup if
missing. The documentation is at [1]. This could be placed in the lock's
contents or a separate file; before connecting to the socket, verify machine IDs
match. If so, it should be safe to assume the target of the symlink didn't dance
around. This could even be a replacement for the hostname logic, as the machine
id will be more stable and unique than the hostname. The hostname should, of
course, still be maintained for the dialog, but the actual checking can be the
id. (The "until the next reboot" bit is slightly worrying, but as it should not
get overwritten outside funny circumstances, I think it's fine. My concern is
that, should we reboot the machine and not cleanly shutdown Chrome, you'd get
the annoying dialog. In the current code, you need the hostname to change as
well to trigger it.)

Thoughts?

[1]
http://dbus.freedesktop.org/doc/api/html/group__DBusMisc.html#gc1c9b9fae55578...

[Evan Martin, 2010-08-18 17:35:24]

Is it waiting for my lgtm?  I was waiting for Matt, and figured his
opinion carried...

On Tue, Aug 17, 2010 at 11:15 AM,  <davidben@chromium.org> wrote:
> In the interest of not having this CL stagnate forever, what's the status?
> Evan:
> have you had a chance to review it? This is somewhat hairy code, so I'd
> rather
> have two people signing off to be sure. (On that note, Matt: was than an
> LGTM?
> I'm always confused with LGTMs that are followed with a discussion. :D)
>
> Some other thoughts:
>
> We could, instead of symlinking the directory, symlink SingletonSocket
> directly
> to the place where the new socket lives. While this does mean that we'd have
> to
> play silly path tricks to get the socket directory for the fchdir thing, it
> means we get both compatibility directions for free; I just did a test and
> connecting via symlink works just fine. Though it does mean the
> compatibility
> path would be vulnerable to the same sort of remote filesystem attack as
> before.
> I don't know if the original problem serious enough to care about, to say
> nothing of the compatibility path.
>
> The chdir thing is also annoying, but I'm not sure how to get rid of it
> (unless
> we decide we don't really care about the fairly mild security nuisances?).
> One
> thought I had was to stick some random cookie in the profile directory, and
> binding that cookie to the socket somehow. The remote browser could send
> that
> cookie, but then we lose compatibility, and other mechanisms I can think of
> have
> race problems. One possibility would be to embed the cookie in the name of
> the
> socket at the end. Then you'd need to know the cookie to spoof the socket
> (short
> of filling the temporary directory with 2^N different socket names ---
> probably
> infeasible), and since the socket is originally placed in a read-only
> directory,
> you shouldn't be able to get at it.
>
> Yet another thought would be to use the DBus machine id,
> /var/lib/dbus/machine-id (accessible value dbus_get_local_machine_id). DBus
> generates a UUID for every machine; I believe it generates it on bootup if
> missing. The documentation is at [1]. This could be placed in the lock's
> contents or a separate file; before connecting to the socket, verify machine
> IDs
> match. If so, it should be safe to assume the target of the symlink didn't
> dance
> around. This could even be a replacement for the hostname logic, as the
> machine
> id will be more stable and unique than the hostname. The hostname should, of
> course, still be maintained for the dialog, but the actual checking can be
> the
> id. (The "until the next reboot" bit is slightly worrying, but as it should
> not
> get overwritten outside funny circumstances, I think it's fine. My concern
> is
> that, should we reboot the machine and not cleanly shutdown Chrome, you'd
> get
> the annoying dialog. In the current code, you need the hostname to change as
> well to trigger it.)
>
> Thoughts?
>
> [1]
>
http://dbus.freedesktop.org/doc/api/html/group__DBusMisc.html#gc1c9b9fae55578...
>
> http://codereview.chromium.org/2838034/show
>

[davidben, 2010-08-18 17:41:04]

On 2010/08/18 17:35:24, Evan Martin wrote:
> Is it waiting for my lgtm?  I was waiting for Matt, and figured his
> opinion carried...

Well, it was mostly waiting for me to remember this existed. :-)

And I've never been all too sure on when things are "officially" signed off on.

[davidben, 2010-08-18 17:43:33]

On 2010/08/18 17:41:04, David Benjamin wrote:
> And I've never been all too sure on when things are "officially" signed off
on.

I guess then, to that effect, Matt (or Evan?), are you okay with this as-is, or
would you prefer I investigate some other options (see three comments above) in
light of the chdir nuisance?

[mattm, 2010-08-18 19:57:16]

On 2010/08/17 18:15:12, David Benjamin wrote:
> In the interest of not having this CL stagnate forever, what's the status?
Evan:
> have you had a chance to review it? This is somewhat hairy code, so I'd rather
> have two people signing off to be sure. (On that note, Matt: was than an LGTM?
> I'm always confused with LGTMs that are followed with a discussion. :D)

Yeah, I guess that was a lgtm, followed by a "hmm, maybe" followed by a "seems
okay after all" :)


> Some other thoughts:
> 
> We could, instead of symlinking the directory, symlink SingletonSocket
directly
> to the place where the new socket lives. While this does mean that we'd have
to
> play silly path tricks to get the socket directory for the fchdir thing, it
> means we get both compatibility directions for free; I just did a test and
> connecting via symlink works just fine. Though it does mean the compatibility
> path would be vulnerable to the same sort of remote filesystem attack as
before.
> I don't know if the original problem serious enough to care about, to say
> nothing of the compatibility path.

I do like this way, and I think with nice abstractions in FilePath it's not
really that ugly to do.   But, I'm also okay with just checking in as is if you
want to do that.

> The chdir thing is also annoying, but I'm not sure how to get rid of it
(unless
> we decide we don't really care about the fairly mild security nuisances?). One
> thought I had was to stick some random cookie in the profile directory, and
> binding that cookie to the socket somehow. The remote browser could send that
> cookie, but then we lose compatibility, and other mechanisms I can think of
have
> race problems. One possibility would be to embed the cookie in the name of the
> socket at the end. Then you'd need to know the cookie to spoof the socket
(short
> of filling the temporary directory with 2^N different socket names ---
probably
> infeasible), and since the socket is originally placed in a read-only
directory,
> you shouldn't be able to get at it.

Hm, I guess that would work, but the down sides of breaking compatibility and
increased complexity make it sound unattractive to me.

> Yet another thought would be to use the DBus machine id,
> /var/lib/dbus/machine-id (accessible value dbus_get_local_machine_id). DBus
> generates a UUID for every machine; I believe it generates it on bootup if
> missing. The documentation is at [1]. This could be placed in the lock's
> contents or a separate file; before connecting to the socket, verify machine
IDs
> match. If so, it should be safe to assume the target of the symlink didn't
dance
> around. This could even be a replacement for the hostname logic, as the
machine
> id will be more stable and unique than the hostname. The hostname should, of
> course, still be maintained for the dialog, but the actual checking can be the
> id. (The "until the next reboot" bit is slightly worrying, but as it should
not
> get overwritten outside funny circumstances, I think it's fine. My concern is
> that, should we reboot the machine and not cleanly shutdown Chrome, you'd get
> the annoying dialog. In the current code, you need the hostname to change as
> well to trigger it.)


Hmm, I'm not sure that entirely addresses the security issue.  If say, the
machine rebooted and the socket symlink was dangling, the attacker could still
create their own socket at the destination, since the machine-id is said to
normally stay the same across reboots (just not guaranteed to).

Using the machine-id instead of hostname for the current check could be good,
though we'd still need backwards compatibility, so it would  add some
complexity, but maybe not too much.  That would be something to do in a separate
CL anyway though, if we're not also using it for this purpose.

> Thoughts?
> 
> [1]
>
http://dbus.freedesktop.org/doc/api/html/group__DBusMisc.html#gc1c9b9fae55578...

[davidben, 2010-08-18 23:35:54]

Aargh. I had this nice long reply and then it disappeared because I got an
error.

On 2010/08/18 19:57:16, mattm wrote:
> > [symlinking the socket directly]
> 
> I do like this way, and I think with nice abstractions in FilePath it's not
> really that ugly to do.   But, I'm also okay with just checking in as is if
you
> want to do that.

Eh, I'm not too happy with this CL in general (if you hadn't noticed :D), so I'm
quite happy to make it nicer before checking it in.

Thinking about this again though, I'm not sure it's too much better. We don't
actually get the compatibility paths for free, because we still need the chdir
trick.

(This does, however, buy us an unsecured connection in the case of old-remote
with new-process.)

I think we can kill the chdir though with a variant of the cookie thing. See
below...

> > [...first cookie idea...]
> 
> Hm, I guess that would work, but the down sides of breaking compatibility and
> increased complexity make it sound unattractive to me.

So, if we assume that /tmp is sticky, we can assume that (modulo reboots), once
a legitimate socket directory has been created, it won't be moved or destroyed
until the owning process removes it. If we make sure every instance of that
directly is distinguishable by a cookie, it should work. So...

user-data-dir/SingletonSocket -> /tmp/XXXX/SingletonSocket
user-data-dir/SingletonCookie -> random_cookie
/tmp/XXXX/SingletonSocket -> the socket
/tmp/XXXX/SingletonCookie -> random_cookie

To connect:

1. Read user-data-dir/SingletonCookie
2. Read /tmp/XXXX from user-data-dir/SingletonSocket
3. Verify /tmp/XXXX/SingletonCookie
4. Connect to /tmp/XXXX/SingletonSocket
5. Verify /tmp/XXXX/SingletonCookie again, if fail, disconnect.

Sandwiching 4 between 3 and 5 should ensure the directory is the same because
SingletonCookie is only readable by the current user.

(We also may be thinking too hard about this. :-D)


> > [dbus stuff]
> 
> Hmm, I'm not sure that entirely addresses the security issue.  If say, the
> machine rebooted and the socket symlink was dangling, the attacker could still
> create their own socket at the destination, since the machine-id is said to
> normally stay the same across reboots (just not guaranteed to).
> 
> Using the machine-id instead of hostname for the current check could be good,
> though we'd still need backwards compatibility, so it would  add some
> complexity, but maybe not too much.  That would be something to do in a
separate
> CL anyway though, if we're not also using it for this purpose.

Oh, good point. I hadn't thought about /tmp clearing after a reboot.

[davidben, 2010-08-19 17:56:02]

Alright, well I just uploaded two new versions. The first was to rebase and
address merge conflicts. The second is another version that uses the cookie
idea.

The cookie thing seems to actually have been more code, but there's an extra
unit test, and I think it feels a little cleaner.

[mattm, 2010-08-19 21:30:27]

Hm, I just had a thought.  If the socket in tmp contains the cookie as part of
the filename (like /tmp/XXXX/SingletonSocket-YYYYY, wouldn't the act of opening
the socket implicitly verify the cookie?  Further, you wouldn't need to store
the cookie separately in the user-data-dir either, since it would be stored as
part of the symlink destination.

BTW, this cookie method all depends on the user-data-dir also being only
user-readable, right?  Is/should that be verified, or just depend on the user
not to change the permissions after it's created?

[davidben, 2010-08-19 21:43:16]

On 2010/08/19 21:30:27, mattm wrote:
> Hm, I just had a thought.  If the socket in tmp contains the cookie as part of
> the filename (like /tmp/XXXX/SingletonSocket-YYYYY, wouldn't the act of
opening
> the socket implicitly verify the cookie?  Further, you wouldn't need to store
> the cookie separately in the user-data-dir either, since it would be stored as
> part of the symlink destination.

Yeah, that was my original thought with the cookies. But /tmp/XXXX/ might be a
nasty FUSE filesystem that accepts every YYYY as a given socket. I don't know if
we care on that front, as this whole thing is contrived as it is. :-D

> BTW, this cookie method all depends on the user-data-dir also being only
> user-readable, right?  Is/should that be verified, or just depend on the user
> not to change the permissions after it's created?

Eh. I think it's probably fine... we create them only user-readable, right? In
that case, I think any manual munging of that directory should be considered
unsupported.


Sigh. This is really not supposed to be a hard problem. Aren't networked
filesystems wonderful?

[mattm, 2010-08-20 22:40:09]

I guess I'm happy enough with this approach.

On 2010/08/19 21:43:16, David Benjamin wrote:
> Yeah, that was my original thought with the cookies. But /tmp/XXXX/ might be a
> nasty FUSE filesystem that accepts every YYYY as a given socket. I don't know
if
> we care on that front, as this whole thing is contrived as it is. :-D

Haha, nice.  Hadn't thought of that.

> > BTW, this cookie method all depends on the user-data-dir also being only
> > user-readable, right?  Is/should that be verified, or just depend on the
user
> > not to change the permissions after it's created?
> 
> Eh. I think it's probably fine... we create them only user-readable, right? In
> that case, I think any manual munging of that directory should be considered
> unsupported.

Yeah, it's created user-readable only.

> Sigh. This is really not supposed to be a hard problem. Aren't networked
> filesystems wonderful?

http://codereview.chromium.org/2838034/diff/39001/40004
File chrome/browser/process_singleton_linux.cc (right):

http://codereview.chromium.org/2838034/diff/39001/40004#newcode29
chrome/browser/process_singleton_linux.cc:29: //
update the file comments to describe the new bits

http://codereview.chromium.org/2838034/diff/39001/40004#newcode229
chrome/browser/process_singleton_linux.cc:229: *output = buf;
Would be cleaner to use output->assign(buf, len);  (And then don't need the +1
on the string length and the setting the null char)

http://codereview.chromium.org/2838034/diff/39001/40004#newcode232
chrome/browser/process_singleton_linux.cc:232: *output = std::string();
output->clear()

http://codereview.chromium.org/2838034/diff/39001/40004#newcode961
chrome/browser/process_singleton_linux.cc:961: =
socket_dir_.path().Append(chrome::kSingletonSocketFilename);
unused?

http://codereview.chromium.org/2838034/diff/39001/40005
File chrome/browser/process_singleton_linux_uitest.cc (right):

http://codereview.chromium.org/2838034/diff/39001/40005#newcode122
chrome/browser/process_singleton_linux_uitest.cc:122: FilePath
socket_target_path = FilePath(std::string(buf));
std::string(buf, len)   (and the other occurrences)

[davidben, 2010-08-21 08:35:51]

Sent revised version off to try bots (as I no longer have easy access to a build
environment... will go set that up at some point). Also updated commit message
to reflect cookie approach.

http://codereview.chromium.org/2838034/diff/39001/40004
File chrome/browser/process_singleton_linux.cc (right):

http://codereview.chromium.org/2838034/diff/39001/40004#newcode29
chrome/browser/process_singleton_linux.cc:29: //
On 2010/08/20 22:40:09, mattm wrote:
> update the file comments to describe the new bits

Done.

http://codereview.chromium.org/2838034/diff/39001/40004#newcode229
chrome/browser/process_singleton_linux.cc:229: *output = buf;
On 2010/08/20 22:40:09, mattm wrote:
> Would be cleaner to use output->assign(buf, len);  (And then don't need the +1
> on the string length and the setting the null char)

Done.

http://codereview.chromium.org/2838034/diff/39001/40004#newcode232
chrome/browser/process_singleton_linux.cc:232: *output = std::string();
On 2010/08/20 22:40:09, mattm wrote:
> output->clear()

Done.

http://codereview.chromium.org/2838034/diff/39001/40004#newcode961
chrome/browser/process_singleton_linux.cc:961: =
socket_dir_.path().Append(chrome::kSingletonSocketFilename);
On 2010/08/20 22:40:09, mattm wrote:
> unused?

Ah, yeah. Removed.

http://codereview.chromium.org/2838034/diff/39001/40005
File chrome/browser/process_singleton_linux_uitest.cc (right):

http://codereview.chromium.org/2838034/diff/39001/40005#newcode122
chrome/browser/process_singleton_linux_uitest.cc:122: FilePath
socket_target_path = FilePath(std::string(buf));
On 2010/08/20 22:40:09, mattm wrote:
> std::string(buf, len)   (and the other occurrences)

Done.

[davidben, 2010-08-23 23:31:06]

And now that I'm back in the land of screwy AFS home directories and set up a
minimal build environment, I've been able to confirm that this patch does work
on AFS.

[mattm, 2010-08-24 02:54:12]

lgtm

http://codereview.chromium.org/2838034/diff/46001/47003
File chrome/browser/process_singleton.h (right):

http://codereview.chromium.org/2838034/diff/46001/47003#newcode23
chrome/browser/process_singleton.h:23: #if defined(OS_LINUX)
looks like this file was recently changed to use defined(USE_X11) for the
"linux" branch, so I guess this ifdef should be USE_X11 also.