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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to