git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
* [PATCH] lockfile: fix buffer overflow in path handling
@ 2013-07-06 19:48 Michael Haggerty
  2013-07-07  4:12 ` Jeff King
  0 siblings, 1 reply; 4+ messages in thread
From: Michael Haggerty @ 2013-07-06 19:48 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Michael Haggerty

The path of the file to be locked is held in lock_file::filename,
which is a fixed-length buffer of length PATH_MAX.  This buffer is
also (temporarily) used to hold the path of the lock file, which is
the path of the file being locked plus ".lock".  Because of this, the
path of the file being locked must be less than (PATH_MAX - 5)
characters long (5 chars are needed for ".lock" and one character for
the NUL terminator).

On entry into lock_file(), the path length was only verified to be
less than PATH_MAX characters, not less than (PATH_MAX - 5)
characters.

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.

Fix the problem by adding a check when entering lock_file() that the
original path is less than (PATH_MAX - 5) characters.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
This patch applies to maint.  I discovered it when working on
something else.  From a brief check of history, it looks like this
code has always been vulnerable to buffer overflows in one way or
another.

I haven't tried to evaluate whether this problem could be exploited
remotely.  Even if so, the ramifications are likely to be limited to
denial-of-service, because the data that are written past the end of
the buffer are not under the control of the user; they would be one of
the strings "lock\0", "ock\0", "ck\0", "k\0", or "\0", depending on
the length of the path to be locked.

I'm thinking of rewriting this code to use strbufs to prevent similar
problems in the future, but that is a more extensive change that
wouldn't be appropriate for maint.

 lockfile.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index c6fb77b..3ac49cb 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -124,13 +124,13 @@ static char *resolve_symlink(char *p, size_t s)
 
 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);
 	strcat(lk->filename, ".lock");
-- 
1.8.3.2

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] lockfile: fix buffer overflow in path handling
  2013-07-06 19:48 [PATCH] lockfile: fix buffer overflow in path handling Michael Haggerty
@ 2013-07-07  4:12 ` Jeff King
  2013-07-07 10:25   ` Michael Haggerty
  0 siblings, 1 reply; 4+ messages in thread
From: Jeff King @ 2013-07-07  4:12 UTC (permalink / raw)
  To: Michael Haggerty; +Cc: Junio C Hamano, git

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

> 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:

>  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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] lockfile: fix buffer overflow in path handling
  2013-07-07  4:12 ` Jeff King
@ 2013-07-07 10:25   ` Michael Haggerty
  2013-07-07 17:29     ` Junio C Hamano
  0 siblings, 1 reply; 4+ messages in thread
From: Michael Haggerty @ 2013-07-07 10:25 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git

On 07/07/2013 06:12 AM, Jeff King wrote:
> On Sat, Jul 06, 2013 at 09:48:52PM +0200, Michael Haggerty wrote:
> 
>> 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:
> 
>>  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.

Yes, the constant is an improvement and Peff's version is also fine with me.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] lockfile: fix buffer overflow in path handling
  2013-07-07 10:25   ` Michael Haggerty
@ 2013-07-07 17:29     ` Junio C Hamano
  0 siblings, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2013-07-07 17:29 UTC (permalink / raw)
  To: Michael Haggerty; +Cc: Jeff King, git

Michael Haggerty <mhagger@alum.mit.edu> writes:

>> But either way, the fix looks good to me.
>
> Yes, the constant is an improvement and Peff's version is also fine with me.

OK, will squash in.  Thanks both.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2013-07-07 17:29 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-06 19:48 [PATCH] lockfile: fix buffer overflow in path handling Michael Haggerty
2013-07-07  4:12 ` Jeff King
2013-07-07 10:25   ` Michael Haggerty
2013-07-07 17:29     ` Junio C Hamano

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git