Re: [Bug-wget] Issue in cookie path checking

[Darshit Shah]

cookies.c:727:59: warning: pointer/integer type mismatch in
conditional expression ('char *' and 'int')
[-Wconditional-type-mismatch]
      if (!check_path_match (cookie->path, trailing_slash ?
strdupdelim (path, trailing_slash + 1) : '/'))
Hi Yasuhisa,

Thanks a lot or the patch file! However, I am unable to compile the
sources with your patch applied.

The issue occrus at cookies.c:653. I'm assuming you wanted to write:
return patch_matches (cookie_patch, path) != 0;

There's also an issue with the ternary operation you introduce at cookies.c:727
My compiler gave me the following warning message, which I do believe
is not a false positive and should be fixed:

cookies.c:727:59: warning: pointer/integer type mismatch in
conditional expression ('char *' and 'int')
[-Wconditional-type-mismatch]
     if (!check_path_match (cookie->path, trailing_slash ? strdupdelim
(path, trailing_slash + 1) : '/'))

It would be very nice of you if you could fix these issues. The code
seems to be correct otherwise to me.

On Thu, Jun 12, 2014 at 8:09 PM,  <address@hidden> wrote:
> Hi Darshit,
>
>  Sorry to be late. I have never used git so it takes long for me to learn
> how to generate path using git.
>
>  I’m not sure I have done it well. If there are something wrong with this
> attachment, please correct.
>
>
>
>
>
>
> Regards,
> Yasuhisa Ishikawa
>
> 2014/06/04 3:09、Darshit Shah <address@hidden> のメール：
>
>> Hi Yasuhisa,
>>
>> Thanks for the patch. The cookie domain patch checking code in Wget
>> was old and only based on a heuristic. However, we are currently in
>> the process of using libpsl a library that handles this for us. I have
>> submitted a patch which is currently in the pipeline that adds support
>> for using libpsl to perform cookie domain name checking.
>>
>> Your code may stil be useful in the fallback mechanism. Could you
>> please send a patch file as generated by `git format-patch` against
>> the current HEAD of the tree and also add the details to the relevant
>> ChangeLog file? It would make applying the patch so much easier for
>> us.
>>
>> On Thu, May 8, 2014 at 8:12 PM, address@hidden
>> <address@hidden> wrote:
>>> Hi all,
>>>
>>> I found two issues in path checking code in cookie.c.
>>>
>>> In cookie_handle_set_cookie(), path in Set-Cookie header should be
>>> checked so as not to be accepted when it is upper than that of
>>> requested document.
>>>
>>> However, current implementation works as:
>>>
>>> - check_path_match() validate the path of requested document
>>>  when its prefix is same with cookie_path.
>>>  path_matches(full_path, prefix) checks if full_path starts with prefix.
>>>  Current code allows /foo/bar/test.html to issue path=/ cookie.
>>>  Expected behavior is opposite. cookie_path must be child of current path.
>>>
>>> - cookie->path is compared with path(full document path including filename)
>>>  in stead of its parent path.
>>>
>>> I applied following fix, and it works as expected. Please consider to merge 
>>> this fix in next release.
>>>
>>> $ diff -c wget-1.15/src/cookies.c.orig wget-1.15/src/cookies.c
>>> *** wget-1.15/src/cookies.c.orig        2013-10-21 23:50:12.000000000 +0900
>>> --- wget-1.15/src/cookies.c     2014-05-08 22:47:57.317467164 +0900
>>> ***************
>>> *** 634,640 ****
>>>  static bool
>>>  check_path_match (const char *cookie_path, const char *path)
>>>  {
>>> !   return path_matches (path, cookie_path) != 0;
>>>  }
>>>
>>>  /* Prepend '/' to string S.  S is copied to fresh stack-allocated
>>> --- 634,640 ----
>>>  static bool
>>>  check_path_match (const char *cookie_path, const char *path)
>>>  {
>>> !   return path_matches (cookie_path, path) != 0;
>>>  }
>>>
>>>  /* Prepend '/' to string S.  S is copied to fresh stack-allocated
>>> ***************
>>> *** 707,713 ****
>>>    else
>>>      {
>>>        /* The cookie sets its own path; verify that it is legal. */
>>> !       if (!check_path_match (cookie->path, path))
>>>          {
>>>            DEBUGP (("Attempt to fake the path: %s, %s\n",
>>>                     cookie->path, path));
>>> --- 707,714 ----
>>>    else
>>>      {
>>>        /* The cookie sets its own path; verify that it is legal. */
>>> !       char *trailing_slash = strrchr (path, '/');
>>> !       if (!check_path_match (cookie->path, trailing_slash ? strdupdelim 
>>> (path, trailing_slash + 1) : '/'))
>>>          {
>>>            DEBUGP (("Attempt to fake the path: %s, %s\n",
>>>                     cookie->path, path));
>>> $
>>>
>>> Thanks,
>>> Yasuhisa Ishikawa
>>
>>
>>
>> --
>> Thanking You,
>> Darshit Shah
>
>



-- 
Thanking You,
Darshit Shah