Patchwork [7,of,7] chg: raise the length limit of socket path

login
register
mail settings
Submitter Jun Wu
Date April 4, 2016, 2:30 a.m.
Message ID <f280444a28189c586417.1459737054@x1c>
Download mbox | patch
Permalink /patch/14324/
State Changes Requested
Headers show

Comments

Jun Wu - April 4, 2016, 2:30 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1459729375 -3600
#      Mon Apr 04 01:22:55 2016 +0100
# Node ID f280444a28189c58641751a01447af1b80978115
# Parent  f7c6cfae4dba1c48a63bdea1b9aa816e7f634d02
chg: raise the length limit of socket path

Previous patches made both the chg server and the client use relative paths
for connect and bind. Now it's time to raise the length limit. This patch
replace UNIX_PATH_MAX (usually 107) with PATH_MAX (usually 4096) so chg will
work even with very long $CHGSOCKNAME as long as the basename is short.
Yuya Nishihara - April 4, 2016, 12:59 p.m.
On Mon, 4 Apr 2016 03:30:54 +0100, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1459729375 -3600
> #      Mon Apr 04 01:22:55 2016 +0100
> # Node ID f280444a28189c58641751a01447af1b80978115
> # Parent  f7c6cfae4dba1c48a63bdea1b9aa816e7f634d02
> chg: raise the length limit of socket path
> 
> Previous patches made both the chg server and the client use relative paths
> for connect and bind. Now it's time to raise the length limit. This patch
> replace UNIX_PATH_MAX (usually 107) with PATH_MAX (usually 4096) so chg will
> work even with very long $CHGSOCKNAME as long as the basename is short.
> 
> diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
> --- a/contrib/chg/chg.c
> +++ b/contrib/chg/chg.c
> @@ -25,14 +25,14 @@
>  #include "hgclient.h"
>  #include "util.h"
>  
> -#ifndef UNIX_PATH_MAX
> -#define UNIX_PATH_MAX (sizeof(((struct sockaddr_un *)NULL)->sun_path))
> +#ifndef PATH_MAX
> +#define PATH_MAX 4096
>  #endif
>  
>  struct cmdserveropts {
> -	char sockname[UNIX_PATH_MAX];
> -	char redirectsockname[UNIX_PATH_MAX];
> -	char lockfile[UNIX_PATH_MAX];
> +	char sockname[PATH_MAX];
> +	char redirectsockname[PATH_MAX];
> +	char lockfile[PATH_MAX];

uh, 12k on stack? please.
Yuya Nishihara - April 4, 2016, 1:35 p.m.
On Mon, 4 Apr 2016 03:30:54 +0100, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1459729375 -3600
> #      Mon Apr 04 01:22:55 2016 +0100
> # Node ID f280444a28189c58641751a01447af1b80978115
> # Parent  f7c6cfae4dba1c48a63bdea1b9aa816e7f634d02
> chg: raise the length limit of socket path
> 
> Previous patches made both the chg server and the client use relative paths
> for connect and bind. Now it's time to raise the length limit. This patch
> replace UNIX_PATH_MAX (usually 107) with PATH_MAX (usually 4096) so chg will
> work even with very long $CHGSOCKNAME as long as the basename is short.
> 
> diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
> --- a/contrib/chg/chg.c
> +++ b/contrib/chg/chg.c
> @@ -25,14 +25,14 @@
>  #include "hgclient.h"
>  #include "util.h"
>  
> -#ifndef UNIX_PATH_MAX
> -#define UNIX_PATH_MAX (sizeof(((struct sockaddr_un *)NULL)->sun_path))
> +#ifndef PATH_MAX
> +#define PATH_MAX 4096
>  #endif
>  
>  struct cmdserveropts {
> -	char sockname[UNIX_PATH_MAX];
> -	char redirectsockname[UNIX_PATH_MAX];
> -	char lockfile[UNIX_PATH_MAX];
> +	char sockname[PATH_MAX];
> +	char redirectsockname[PATH_MAX];
> +	char lockfile[PATH_MAX];

Somewhat related: perhaps, we should keep sockdir and file names separately,
and make the "validate" command return only file names. That will reduce a
possible bug of accessing unsafe socket.

I'm okay to drop CHGSOCKNAME and add CHGSOCKDIR instead, though it would
mean chg can no longer be a client for plain unix command server.
Jun Wu - April 4, 2016, 3 p.m.
On 04/04/2016 01:59 PM, Yuya Nishihara wrote:
> uh, 12k on stack? please.

I thought about that and felt it's okay since we only have one cmdserveropts
instance.
Jun Wu - April 4, 2016, 3:14 p.m.
On 04/04/2016 02:35 PM, Yuya Nishihara wrote:
> Somewhat related: perhaps, we should keep sockdir and file names
> separately,

The issue is if chdir errors with EPERM as Simon said, we may want to fallback
to full path. It's even better if we try the parent dir recursively until we
hit the root dir.

> and make the "validate" command return only file names. That will reduce a
> possible bug of accessing unsafe socket.

Same issue but if we do this and enforce that the user must give a directory 
that has +x, things will be much easier to handle and safer. We can just keep a 
dirfd and throw away the dir string.

I prefer this way (parse CHGSOCKNAME and save a dirfd during startup and we
then forget about directory strings). cc Simon for opinions.

> I'm okay to drop CHGSOCKNAME and add CHGSOCKDIR instead, though it would
> mean chg can no longer be a client for plain unix command server.

This is not necessary since we can always parse CHGSOCKNAME to get CHGSOCKDIR
during startup.
Yuya Nishihara - April 4, 2016, 3:20 p.m.
On Mon, 4 Apr 2016 16:00:04 +0100, Jun Wu wrote:
> On 04/04/2016 01:59 PM, Yuya Nishihara wrote:
> > uh, 12k on stack? please.  
> 
> I thought about that and felt it's okay since we only have one cmdserveropts
> instance.

% ulimit -s
8192

doh, it is kB. Sorry, I tend to live in small world where only 256bytes
stack available.

That said, I would allocate >1kB memory on heap.
Yuya Nishihara - April 4, 2016, 3:36 p.m.
On Mon, 4 Apr 2016 16:14:05 +0100, Jun Wu wrote:
> On 04/04/2016 02:35 PM, Yuya Nishihara wrote:
> > Somewhat related: perhaps, we should keep sockdir and file names
> > separately,  
> 
> The issue is if chdir errors with EPERM as Simon said, we may want to fallback
> to full path. It's even better if we try the parent dir recursively until we
> hit the root dir.
> 
> > and make the "validate" command return only file names. That will reduce a
> > possible bug of accessing unsafe socket.  
> 
> Same issue but if we do this and enforce that the user must give a directory
> that has +x, things will be much easier to handle and safer. We can just keep a 
> dirfd and throw away the dir string.

Simon's comment is interesting, but I don't see benefit to allow u-x on the
socket dir. The socket dir shouldn't be world-accessible, and not be world-
movable (to prevent local attack), but there should be no reason to hide it
from the owner.

> I prefer this way (parse CHGSOCKNAME and save a dirfd during startup and we
> then forget about directory strings). cc Simon for opinions.
> 
> > I'm okay to drop CHGSOCKNAME and add CHGSOCKDIR instead, though it would
> > mean chg can no longer be a client for plain unix command server.  
> 
> This is not necessary since we can always parse CHGSOCKNAME to get CHGSOCKDIR
> during startup.

I meant CHGSOCKDIR would make more sense because we have more than one sockets
and a lock file in it.
Jun Wu - April 4, 2016, 3:53 p.m.
On 04/04/2016 04:36 PM, Yuya Nishihara wrote:
> Simon's comment is interesting, but I don't see benefit to allow u-x on the
> socket dir. The socket dir shouldn't be world-accessible, and not be world-
> movable (to prevent local attack), but there should be no reason to hide it
> from the owner.

Okay. I will add a dirfd and only handle basenames then. Everything looks simpler!

> I meant CHGSOCKDIR would make more sense because we have more than one
> sockets and a lock file in it.

With chgserver.skiphash=True, the user probably wants a plain file path.
Advanced users or developers may want to specify file path as well.

I feel what chgserver uses is a set of files with a common prefix (basename),
instead of a directory. So two chg servers can use a same directory as long
as they use different basenames. The default lockfilepath should probably be 
changed to "dir/basename.lock" from "dir/lock".
Simon Farnsworth - April 4, 2016, 3:55 p.m.
On 04/04/2016 16:14, Jun Wu wrote:
> On 04/04/2016 02:35 PM, Yuya Nishihara wrote:
>> Somewhat related: perhaps, we should keep sockdir and file names
>> separately,
>
> The issue is if chdir errors with EPERM as Simon said, we may want to
> fallback
> to full path. It's even better if we try the parent dir recursively
> until we
> hit the root dir.
>
>> and make the "validate" command return only file names. That will
>> reduce a
>> possible bug of accessing unsafe socket.
>
> Same issue but if we do this and enforce that the user must give a
> directory that has +x, things will be much easier to handle and safer.
> We can just keep a dirfd and throw away the dir string.
>
> I prefer this way (parse CHGSOCKNAME and save a dirfd during startup and we
> then forget about directory strings). cc Simon for opinions.
>

I want very clear error messages (with perror-like translation of errno) 
so that if it does go wrong, it's easy for the user to work out what 
they did and to fix it without coming to us for help.

Something like (pseudocode):

abortmsg("chg: cannot chdir: %s: %s (errno = %d)",
     dir,
     stringerror(errno),
     errno
);

in analogy to bash's:
-bash: cd: simonfar/: Permission denied

That way, the user can see that it's an external fault, and diagnose 
using shell commands.

In a perfect world, it'd also be possible to fall back to a straight 
connect() without the chdir(), but that's less of a concern for me.

>> I'm okay to drop CHGSOCKNAME and add CHGSOCKDIR instead, though it would
>> mean chg can no longer be a client for plain unix command server.
>
> This is not necessary since we can always parse CHGSOCKNAME to get
> CHGSOCKDIR
> during startup.
Jun Wu - April 4, 2016, 5:03 p.m.
On 04/04/2016 04:55 PM, Simon Farnsworth wrote:
> I want very clear error messages (with perror-like translation of errno) so
> that if it does go wrong, it's easy for the user to work out what they did and
> to fix it without coming to us for help.

Sure. I will make sure it will print these details.
Yuya Nishihara - April 5, 2016, 1:43 p.m.
On Mon, 4 Apr 2016 16:53:29 +0100, Jun Wu wrote:
> On 04/04/2016 04:36 PM, Yuya Nishihara wrote:
> > I meant CHGSOCKDIR would make more sense because we have more than one
> > sockets and a lock file in it.  
> 
> With chgserver.skiphash=True, the user probably wants a plain file path.
> Advanced users or developers may want to specify file path as well.

Even with skiphash=True, chg should create (or test) a private directory. A
bare socket file can be insecure as chg tries to connect to it and spawns new
server if failed to connect.

I've initially added CHGSOCKNAME only for testing purpose, so it doesn't ensure
that the socket path is good. That is my fault.

> I feel what chgserver uses is a set of files with a common prefix (basename),
> instead of a directory. So two chg servers can use a same directory as long
> as they use different basenames. The default lockfilepath should probably be 
> changed to "dir/basename.lock" from "dir/lock".

What's the benefit of it? I don't think users would care much how sockets were
managed.
Jun Wu - April 5, 2016, 3:07 p.m.
On 04/05/2016 02:43 PM, Yuya Nishihara wrote:
> On Mon, 4 Apr 2016 16:53:29 +0100, Jun Wu wrote:
>> On 04/04/2016 04:36 PM, Yuya Nishihara wrote:
>>> I meant CHGSOCKDIR would make more sense because we have more than one
>>> sockets and a lock file in it.
>>
>> With chgserver.skiphash=True, the user probably wants a plain file path.
>> Advanced users or developers may want to specify file path as well.
>
> Even with skiphash=True, chg should create (or test) a private directory. A
> bare socket file can be insecure as chg tries to connect to it and spawns
> new server if failed to connect.

Yes. the /tmp/chg$UID directory created by chg by default is guaranteed secure
and I like this behavior.

> I've initially added CHGSOCKNAME only for testing purpose, so it doesn't
> ensure that the socket path is good. That is my fault.

We can apply the directory check for CHGSOCKNAME as well. I will do that
in the next dirfd patch. I personally want a way to skip the check though.

>> I feel what chgserver uses is a set of files with a common prefix
>> (basename), instead of a directory. So two chg servers can use a same
>> directory as long as they use different basenames. The default
>> lockfilepath should probably be changed to "dir/basename.lock" from
>> "dir/lock".
>
> What's the benefit of it? I don't think users would care much how sockets
> were managed.

Flexibility. For advanced users and developers. For example, with
skiphash=False, I may want to manually set
CHGSOCKNAME=/tmp/chg1000/server-a576a2e8e8aa to test if redirect works or not.

I also believe it's the user's responsibility to take care of the permissions,
if they do not use our default setting and specify socket path themselves.
And maybe they don't think other users are enemies and just want to let other
users access their private socket sometimes?

That said, I will still add the check to notice them and make the check
skip-able so we have good security in all common cases and maximum flexibility.

Other software with similar pattern do not enforce private directories either,
for example, ssh ControlMaster and emacsclient.
Yuya Nishihara - April 5, 2016, 4:29 p.m.
On Tue, 5 Apr 2016 16:07:21 +0100, Jun Wu wrote:
> On 04/05/2016 02:43 PM, Yuya Nishihara wrote:
> > I've initially added CHGSOCKNAME only for testing purpose, so it doesn't
> > ensure that the socket path is good. That is my fault.  
> 
> We can apply the directory check for CHGSOCKNAME as well. I will do that
> in the next dirfd patch. I personally want a way to skip the check though.
> 
> >> I feel what chgserver uses is a set of files with a common prefix
> >> (basename), instead of a directory. So two chg servers can use a same
> >> directory as long as they use different basenames. The default
> >> lockfilepath should probably be changed to "dir/basename.lock" from
> >> "dir/lock".  
> >
> > What's the benefit of it? I don't think users would care much how sockets
> > were managed.  
> 
> Flexibility. For advanced users and developers. For example, with
> skiphash=False, I may want to manually set
> CHGSOCKNAME=/tmp/chg1000/server-a576a2e8e8aa to test if redirect works or not.
>
> I also believe it's the user's responsibility to take care of the permissions,
> if they do not use our default setting and specify socket path themselves.
> And maybe they don't think other users are enemies and just want to let other
> users access their private socket sometimes?

As long as CHGSOCKNAME is a debug option, it should be fine. (and we need a
debug option, right?) So feel free to disregard my CHGSOCKDIR rant. Let's
revisit it later.

> That said, I will still add the check to notice them and make the check
> skip-able so we have good security in all common cases and maximum flexibility.

Separate user/debug options? such as CHGFORCESOCKNAME and CHGSOCKDIR.

> Other software with similar pattern do not enforce private directories either,
> for example, ssh ControlMaster and emacsclient.

Oh, then how could I write that code? I don't remember where I've copied that
concept from. ;)
Jun Wu - April 5, 2016, 4:34 p.m.
On 04/05/2016 05:29 PM, Yuya Nishihara wrote:
> Oh, then how could I write that code? I don't remember where I've copied
> that concept from.;)

I guess you copied the idea from emacsclient. It will create a private
directory at /tmp/emacs$UID by default as well. What I meaned is its
--socket-name parameter accepts a file name and does not check the mode of
the directory.

Patch

diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
--- a/contrib/chg/chg.c
+++ b/contrib/chg/chg.c
@@ -25,14 +25,14 @@ 
 #include "hgclient.h"
 #include "util.h"
 
-#ifndef UNIX_PATH_MAX
-#define UNIX_PATH_MAX (sizeof(((struct sockaddr_un *)NULL)->sun_path))
+#ifndef PATH_MAX
+#define PATH_MAX 4096
 #endif
 
 struct cmdserveropts {
-	char sockname[UNIX_PATH_MAX];
-	char redirectsockname[UNIX_PATH_MAX];
-	char lockfile[UNIX_PATH_MAX];
+	char sockname[PATH_MAX];
+	char redirectsockname[PATH_MAX];
+	char lockfile[PATH_MAX];
 	size_t argsize;
 	const char **args;
 	int lockfd;
@@ -130,7 +130,7 @@ 
 static void setcmdserveropts(struct cmdserveropts *opts)
 {
 	int r;
-	char sockdir[UNIX_PATH_MAX];
+	char sockdir[PATH_MAX];
 	const char *envsockname = getenv("CHGSOCKNAME");
 	if (!envsockname) {
 		/* by default, put socket file in secure directory