| 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
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.
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.
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.
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).
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);