Re: [PATCH] lockfile: fix buffer overflow in path handling

From: Jeff King <peff@peff.net>
Date: Jul 7, 2013, 04:12 UTC
Message-ID: <20130707041236.GB30898@sigill.intra.peff.net>
In-Reply-To: <1373140132-12351-1-git-send-email-mhagger@alum.mit.edu>

On Sat, Jul 06, 2013 at 09:48:52PM +0200, Michael Haggerty wrote:

Show 13 quoted lines

> When and if resolve_symlink() is called, then that function is
> correctly told to treat the buffer as (PATH_MAX - 5) characters long.
> This part is correct.  However:
> 
> * If LOCK_NODEREF was specified, then resolve_symlink() is never
>   called.
> 
> * If resolve_symlink() is called but the path is not a symlink, then
>   the length check is never applied.
> 
> So it is possible for a path with length (PATH_MAX - 5 <= len <
> PATH_MAX) to make it through the checks.  When ".lock" is strcat()ted
> to such a path, the lock_file::filename buffer is overflowed.

Thanks for posting this. I independently discovered this about a month ago while working on an unrelated series, and then let it languish unseen and forgotten at the base of that almost-done series.

So definitely a problem, and my patch looked almost identical to yours. The only difference is:

Show 14 quoted lines

>  static int lock_file(struct lock_file *lk, const char *path, int flags)
>  {
> -	if (strlen(path) >= sizeof(lk->filename))
> -		return -1;
> -	strcpy(lk->filename, path);
>  	/*
>  	 * subtract 5 from size to make sure there's room for adding
>  	 * ".lock" for the lock file name
>  	 */
> +	if (strlen(path) >= sizeof(lk->filename)-5)
> +		return -1;
> +	strcpy(lk->filename, path);
>  	if (!(flags & LOCK_NODEREF))
>  		resolve_symlink(lk->filename, sizeof(lk->filename)-5);

It might be worth consolidating the magic "-5" into a constant near the comment, like this:

diff --git a/lockfile.c b/lockfile.c
index c6fb77b..2aeb2bb 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -124,15 +124,16 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 
 static int lock_file(struct lock_file *lk, const char *path, int flags)
 {
-	if (strlen(path) >= sizeof(lk->filename))
-		return -1;
-	strcpy(lk->filename, path);
 	/*
 	 * subtract 5 from size to make sure there's room for adding
 	 * ".lock" for the lock file name
 	 */
+	static const size_t max_path_len = sizeof(lk->filename) - 5;
+	if (strlen(path) >= max_path_len)
+		return -1;
+	strcpy(lk->filename, path);
 	if (!(flags & LOCK_NODEREF))
-		resolve_symlink(lk->filename, sizeof(lk->filename)-5);
+		resolve_symlink(lk->filename, max_path_len);
 	strcat(lk->filename, ".lock");
 	lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (0 <= lk->fd) {

But either way, the fix looks good to me.

-Peff