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

[Michael Haggerty <mhagger@alum.mit.edu>]

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/