git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org
Subject: Re: [PATCHv3 10/13] credentials: add "cache" helper
Date: Tue, 10 Jan 2012 12:53:12 -0500	[thread overview]
Message-ID: <20120110175312.GA7289@sigill.intra.peff.net> (raw)
In-Reply-To: <20120110174420.GA22184@burratino>

On Tue, Jan 10, 2012 at 11:44:21AM -0600, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> >   2. A hack in credential-cache won't help any unix-socket
> >      users who come along later.
> >
> >   3. The chdir trickery isn't that likely to fail (basically
> >      it's only a problem if your cwd is missing or goes away
> >      while you're running).  And because we only enable the
> >      hack when we get a too-long name, it can only fail in
> >      cases that would have failed under the previous code
> >      anyway.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> 
> A part of me worries that this assumption (3), and the additional
> assumption that nothing running concurrently cares about the cwd,
> might come back to bite us when the future (2) arrives.  But I don't
> see a better approach.

The problem is that when future (2) arrives, a credential-cache specific
hack won't be helping it at all. :)

To be honest, I don't really expect a lot of future unix-socket users.
It's not portable, and most of our IPC happens over pipes. The design of
the cache daemon is very specific in requiring a true
many-clients-to-one-server model, but also caring deeply about access
controls (making TCP sockets less good[1]).

[1] One could in theory do a loopback TCP socket and provide a random
    token read from an access-controlled file. But that ends up a lot
    more complicated and introduces new attack surfaces. Which is a bad
    thing for security-sensitive code like this.

> Follow-on ideas: on platforms that support it, it would be nice to use
> 
> 	ctx->orig_dirfd = open(".", O_RDONLY);
> 	if (ctx->orig_dirfd < 0)
> 		... error handling ...
> 	...
> 	if (ctx->orig_dirfd >= 0) {
> 		if (fchdir(ctx->orig_dirfd))
> 			die_errno("unable to restore original working directory");
> 		close(ctx->orig_dirfd);
> 	}

Yeah, I started by using fchdir, but noticed that we don't use it
anywhere else and didn't want to create a portability problem. It does
fix the "somebody deleted your cwd while you were gone from it" problem.
But if you have no cwd at all, the open call will still fail.

Still, it would be slightly more robust. I wonder how portable fchdir
is in practice (I guess we could always fall back to the getcwd code
path). Do you want to prepare a patch on top?

> [...]
> > +		dir = path;
> > +		dir_len = slash - path;
> [...]
> > +		if (chdir_len(dir, dir_len) < 0)
> > +			return -1;
> 
> Could avoid some complication by eliminating the dir_len var.
> 
> 		if (chdir_len(dir, slash - dir))
> 			return -1;

Ah, yeah. Originally I had written it so that "slash" didn't survive
untouched to there, but in the current code, that would work.

Junio, if you haven't merged it to next yet, it might be worth squashing
in the patch below. Otherwise I wouldn't worry about it.

> Neither seems worth holding up the patch, so
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks for the review (and for, as usual, a well-written bug report).

---
diff --git a/unix-socket.c b/unix-socket.c
index 91ed9dc..e8f19c6 100644
--- a/unix-socket.c
+++ b/unix-socket.c
@@ -43,7 +43,6 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
 	if (size > sizeof(sa->sun_path)) {
 		const char *slash = find_last_dir_sep(path);
 		const char *dir;
-		int dir_len;
 
 		if (!slash) {
 			errno = ENAMETOOLONG;
@@ -51,8 +50,6 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
 		}
 
 		dir = path;
-		dir_len = slash - path;
-
 		path = slash + 1;
 		size = strlen(path) + 1;
 		if (size > sizeof(sa->sun_path)) {
@@ -64,7 +61,7 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
 			errno = ENAMETOOLONG;
 			return -1;
 		}
-		if (chdir_len(dir, dir_len) < 0)
+		if (chdir_len(dir, slash - dir) < 0)
 			return -1;
 	}
 

  reply	other threads:[~2012-01-10 17:53 UTC|newest]

Thread overview: 54+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-12-10 10:28 [PATCHv3 0/13] credential helpers Jeff King
2011-12-10 10:30 ` [PATCHv3 01/13] test-lib: add test_config_global variant Jeff King
2011-12-10 10:30 ` [PATCHv3 02/13] t5550: fix typo Jeff King
2011-12-10 10:31 ` [PATCHv3 03/13] introduce credentials API Jeff King
2011-12-10 11:43   ` Jakub Narebski
2011-12-10 19:48     ` Jeff King
2011-12-10 10:31 ` [PATCHv3 04/13] credential: add function for parsing url components Jeff King
2011-12-10 10:31 ` [PATCHv3 05/13] http: use credential API to get passwords Jeff King
2011-12-10 10:31 ` [PATCHv3 06/13] credential: apply helper config Jeff King
2011-12-10 10:31 ` [PATCHv3 07/13] credential: add credential.*.username Jeff King
2011-12-10 10:31 ` [PATCHv3 08/13] credential: make relevance of http path configurable Jeff King
2011-12-10 11:50   ` Jakub Narebski
2011-12-10 19:50     ` Jeff King
2011-12-10 10:31 ` [PATCHv3 09/13] docs: end-user documentation for the credential subsystem Jeff King
2011-12-10 10:34 ` [PATCHv3 10/13] credentials: add "cache" helper Jeff King
2012-01-10  1:50   ` Jonathan Nieder
2012-01-10  4:44     ` Jeff King
2012-01-10  4:57       ` Jeff King
2012-01-10 16:59         ` Junio C Hamano
2012-01-17  6:02         ` Jeff King
2012-01-17  6:51           ` Junio C Hamano
2012-01-10 17:44       ` Jonathan Nieder
2012-01-10 17:53         ` Jeff King [this message]
2012-01-11 23:50           ` Jonathan Nieder
2012-01-12  3:07             ` Jeff King
2011-12-10 10:34 ` [PATCHv3 11/13] strbuf: add strbuf_add*_urlencode Jeff King
2011-12-10 11:57   ` Jakub Narebski
2011-12-10 20:09     ` Jeff King
2011-12-10 10:34 ` [PATCHv3 12/13] credentials: add "store" helper Jeff King
2011-12-10 10:35 ` [PATCHv3 13/13] t: add test harness for external credential helpers Jeff King
2011-12-10 10:39 ` [PATCHv2 0/9] echo characters in username prompt Jeff King
2011-12-10 10:40   ` [PATCHv2 1/9] imap-send: avoid buffer overflow Jeff King
2011-12-10 10:40   ` [PATCHv2 2/9] imap-send: don't check return value of git_getpass Jeff King
2011-12-10 10:40   ` [PATCHv2 3/9] move git_getpass to its own source file Jeff King
2011-12-10 10:40   ` [PATCHv2 4/9] refactor git_getpass into generic prompt function Jeff King
2011-12-10 10:41   ` [PATCHv2 5/9] add generic terminal " Jeff King
2011-12-15 12:48     ` Pete Wyckoff
2011-12-15 13:39       ` Jeff King
2011-12-15 21:59         ` Pete Wyckoff
2011-12-10 10:41   ` [PATCHv2 6/9] prompt: use git_terminal_prompt Jeff King
2011-12-10 10:41   ` [PATCHv2 7/9] credential: use git_prompt instead of git_getpass Jeff King
2011-12-10 10:41   ` [PATCHv2 8/9] Makefile: linux has /dev/tty Jeff King
2011-12-10 10:41   ` [PATCHv2 9/9] Makefile: OS X " Jeff King
2011-12-12 21:10     ` [PATCH 1/2] Makefile: Windows lacks /dev/tty Johannes Sixt
2011-12-12 21:12       ` [PATCH 2/2] Makefile: optionally exclude code that needs Unix sockets Johannes Sixt
2011-12-12 21:39         ` Jeff King
2011-12-12 23:31           ` Junio C Hamano
2011-12-13  0:58             ` Jeff King
2011-12-13  0:45           ` Junio C Hamano
2011-12-13 20:00             ` Johannes Sixt
2011-12-14  0:14               ` Junio C Hamano
2011-12-12 21:18       ` [PATCH 1/2] Makefile: Windows lacks /dev/tty Jeff King
2011-12-12 21:52         ` Johannes Sixt
2011-12-10 10:53 ` [PATCH 1/1] contrib: add credential helper for OS X Keychain Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20120110175312.GA7289@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).