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

[address@hidden]

Darshit,

 Sorry, I’m not familiar with git and "git clone”ed branch so I could not run 
make before.
 I hope this work….

0001-2014-06-16-Yasuhisa-Ishikawa-yasuhisa.ishikawa-kuman.patch
Description: Binary data

Yasuhisa Ishikawa


2014/06/16 18:06、Darshit Shah <address@hidden> のメール：

> 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