Patchwork [10,of,14] chg: calculate sockdirfd

login
register
mail settings
Submitter Jun Wu
Date April 10, 2016, 11:57 p.m.
Message ID <1b8ee8aabe42b44cd865.1460332647@x1c>
Download mbox | patch
Permalink /patch/14508/
State Accepted
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - April 10, 2016, 11:57 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1460330280 -3600
#      Mon Apr 11 00:18:00 2016 +0100
# Node ID 1b8ee8aabe42b44cd865d41bf095c2cf7892e4dc
# Parent  68977496c0f5b4026c19cd8ec4ea22113401c030
chg: calculate sockdirfd

This is a part of the series to support long socket path. Before this patch,
sockdirfd is AT_FDCWD, making it effectively unused. This patch calculates
sockdirfd and makes sockname and lockfile basenames instead of full paths.
Yuya Nishihara - April 13, 2016, 3:07 p.m.
On Mon, 11 Apr 2016 00:57:27 +0100, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1460330280 -3600
> #      Mon Apr 11 00:18:00 2016 +0100
> # Node ID 1b8ee8aabe42b44cd865d41bf095c2cf7892e4dc
> # Parent  68977496c0f5b4026c19cd8ec4ea22113401c030
> chg: calculate sockdirfd
> 
> This is a part of the series to support long socket path. Before this patch,
> sockdirfd is AT_FDCWD, making it effectively unused. This patch calculates
> sockdirfd and makes sockname and lockfile basenames instead of full paths.
> 
> diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
> --- a/contrib/chg/chg.c
> +++ b/contrib/chg/chg.c
> @@ -29,6 +29,10 @@
>  #define UNIX_PATH_MAX (sizeof(((struct sockaddr_un *)NULL)->sun_path))
>  #endif
>  
> +#ifndef PATH_MAX
> +#define PATH_MAX 4096
> +#endif
> +
>  struct cmdserveropts {
>  	char sockname[UNIX_PATH_MAX];
>  	char redirectsockname[UNIX_PATH_MAX];
> @@ -140,8 +144,9 @@
>  static void setcmdserveropts(struct cmdserveropts *opts)
>  {
>  	int r;
> -	char sockdir[UNIX_PATH_MAX];
> +	char sockdir[PATH_MAX];
>  	const char *envsockname = getenv("CHGSOCKNAME");
> +	const char *basename = NULL;
>  	if (!envsockname) {
>  		/* by default, put socket file in secure directory
>  		 * (permission of socket file may be ignored on some Unices) */
> @@ -153,14 +158,38 @@
>  		if (r < 0 || (size_t)r >= sizeof(sockdir))
>  			abortmsg("too long TMPDIR (r = %d)", r);
>  		preparesockdir(sockdir);
> +		basename = "server";
> +	} else {
> +		/* otherwise, split the path the user provides */
> +		const char *lastsep = strrchr(envsockname, '/');
> +		if (lastsep) {
> +			size_t len = lastsep - envsockname + 1;
> +			if (len >= sizeof(sockdir))
> +				abortmsg("CHGSOCKNAME is too long");
> +			memcpy(sockdir, envsockname, len);
> +			sockdir[len] = '\0';
> +			basename = lastsep + 1;
> +		} else {
> +			r = snprintf(sockdir, sizeof(sockdir), ".");
> +			assert(r == 2);
> +			basename = envsockname;
> +		}
>  	}
>  
> -	const char *basename = (envsockname) ? envsockname : sockdir;
> -	const char *sockfmt = (envsockname) ? "%s" : "%s/server";
> -	const char *lockfmt = (envsockname) ? "%s.lock" : "%s/lock";
> -	r = snprintf(opts->sockname, sizeof(opts->sockname), sockfmt, basename);
> -	if (r < 0 || (size_t)r >= sizeof(opts->sockname))
> -		abortmsg("too long TMPDIR or CHGSOCKNAME (r = %d)", r);
> +	if (basename[0] == '\0')
> +		abortmsg("basename cannot be empty");

I still think we should avoid unnecessary path manipulation because we have
to be careful about pitfalls such as CHGSOCKNAME="/silly/basename/..". That's
why I prefer CHGSOCKDIR.
Jun Wu - April 13, 2016, 4:50 p.m.
On 04/13/2016 04:07 PM, Yuya Nishihara wrote:
> I still think we should avoid unnecessary path manipulation because we have
> to be careful about pitfalls such as CHGSOCKNAME="/silly/basename/..".
> That's why I prefer CHGSOCKDIR.

I still prefer the flexibility. People using ".." should know what they are
doing. I don't think it necessary to prevent people using developer-facing
features from doing wrong. Things like "rm -rf ~" are not protected.
Yuya Nishihara - April 14, 2016, 2:47 p.m.
On Wed, 13 Apr 2016 17:50:57 +0100, Jun Wu wrote:
> On 04/13/2016 04:07 PM, Yuya Nishihara wrote:
> > I still think we should avoid unnecessary path manipulation because we have
> > to be careful about pitfalls such as CHGSOCKNAME="/silly/basename/..".
> > That's why I prefer CHGSOCKDIR.  
> 
> I still prefer the flexibility. People using ".." should know what they are
> doing. I don't think it necessary to prevent people using developer-facing
> features from doing wrong. Things like "rm -rf ~" are not protected.

Then why do you check basename[0] == '\0' ?

I don't like being loose for processing paths because it tends to be a security
bug. I know that's okay right now, but can you be sure that basename = ".."
never ever trap someone who has to modify this function?

Also, I don't see how beneficial it is to allow putting all sockets into
a single directory.
Sean Farley - April 14, 2016, 4:51 p.m.
Yuya Nishihara <yuya@tcha.org> writes:

> On Wed, 13 Apr 2016 17:50:57 +0100, Jun Wu wrote:
>> On 04/13/2016 04:07 PM, Yuya Nishihara wrote:
>> > I still think we should avoid unnecessary path manipulation because we have
>> > to be careful about pitfalls such as CHGSOCKNAME="/silly/basename/..".
>> > That's why I prefer CHGSOCKDIR.  
>> 
>> I still prefer the flexibility. People using ".." should know what they are
>> doing. I don't think it necessary to prevent people using developer-facing
>> features from doing wrong. Things like "rm -rf ~" are not protected.
>
> Then why do you check basename[0] == '\0' ?
>
> I don't like being loose for processing paths because it tends to be a security
> bug. I know that's okay right now, but can you be sure that basename = ".."
> never ever trap someone who has to modify this function?
>
> Also, I don't see how beneficial it is to allow putting all sockets into
> a single directory.

I'm leaning on agreeing with Yuya here. For starters, we just had a
security bug last year or so with paths! Second, I also don't see the
benefit (but I could be convinced otherwise).
Jun Wu - April 15, 2016, 4:47 p.m.
On 04/14/2016 05:51 PM, Sean Farley wrote:
> I'm leaning on agreeing with Yuya here. For starters, we just had a security
> bug last year or so with paths! Second, I also don't see the benefit (but I
> could be convinced otherwise).

Okay. Let's do CHGSOCKDIR then. The code can be even a bit shorter. We don't
need to imitate emacsclient or any others.

Cheers!

Patch

diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
--- a/contrib/chg/chg.c
+++ b/contrib/chg/chg.c
@@ -29,6 +29,10 @@ 
 #define UNIX_PATH_MAX (sizeof(((struct sockaddr_un *)NULL)->sun_path))
 #endif
 
+#ifndef PATH_MAX
+#define PATH_MAX 4096
+#endif
+
 struct cmdserveropts {
 	char sockname[UNIX_PATH_MAX];
 	char redirectsockname[UNIX_PATH_MAX];
@@ -140,8 +144,9 @@ 
 static void setcmdserveropts(struct cmdserveropts *opts)
 {
 	int r;
-	char sockdir[UNIX_PATH_MAX];
+	char sockdir[PATH_MAX];
 	const char *envsockname = getenv("CHGSOCKNAME");
+	const char *basename = NULL;
 	if (!envsockname) {
 		/* by default, put socket file in secure directory
 		 * (permission of socket file may be ignored on some Unices) */
@@ -153,14 +158,38 @@ 
 		if (r < 0 || (size_t)r >= sizeof(sockdir))
 			abortmsg("too long TMPDIR (r = %d)", r);
 		preparesockdir(sockdir);
+		basename = "server";
+	} else {
+		/* otherwise, split the path the user provides */
+		const char *lastsep = strrchr(envsockname, '/');
+		if (lastsep) {
+			size_t len = lastsep - envsockname + 1;
+			if (len >= sizeof(sockdir))
+				abortmsg("CHGSOCKNAME is too long");
+			memcpy(sockdir, envsockname, len);
+			sockdir[len] = '\0';
+			basename = lastsep + 1;
+		} else {
+			r = snprintf(sockdir, sizeof(sockdir), ".");
+			assert(r == 2);
+			basename = envsockname;
+		}
 	}
 
-	const char *basename = (envsockname) ? envsockname : sockdir;
-	const char *sockfmt = (envsockname) ? "%s" : "%s/server";
-	const char *lockfmt = (envsockname) ? "%s.lock" : "%s/lock";
-	r = snprintf(opts->sockname, sizeof(opts->sockname), sockfmt, basename);
-	if (r < 0 || (size_t)r >= sizeof(opts->sockname))
-		abortmsg("too long TMPDIR or CHGSOCKNAME (r = %d)", r);
+	if (basename[0] == '\0')
+		abortmsg("basename cannot be empty");
+
+	opts->sockdirfd = open(sockdir, O_DIRECTORY);
+	if (opts->sockdirfd == -1)
+		abortmsgerrno("failed to open dir %s", sockdir);
+	fsetcloexec(opts->sockdirfd);
+
+	opts->sockname[sizeof(opts->sockname) - 1] = '\0';
+	strncpy(opts->sockname, basename, sizeof(opts->sockname));
+	if (opts->sockname[sizeof(opts->sockname) - 1] != '\0')
+		abortmsg("basename %s is too long", basename);
+
+	static const char lockfmt[] = "%s.lock";
 	r = snprintf(opts->lockfile, sizeof(opts->lockfile), lockfmt, basename);
 	if (r < 0 || (size_t)r >= sizeof(opts->lockfile))
 		abortmsg("too long TMPDIR or CHGSOCKNAME (r = %d)", r);