Re: [Bug-wget] Issue in cookie path checking
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, yasuhisa.ishik...@kumaneko.org 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 dar...@gmail.com のメール：
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, yasuhisa.ishik...@kumaneko.org
yasuhisa.ishik...@kumaneko.org 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.orig2013-10-21 23:50:12.0 +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
Re: [Bug-wget] Issue in cookie path checking
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 dar...@gmail.com のメール：
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, yasuhisa.ishik...@kumaneko.org 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 dar...@gmail.com のメール：
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, yasuhisa.ishik...@kumaneko.org
yasuhisa.ishik...@kumaneko.org 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.orig2013-10-21 23:50:12.0 +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
Re: [Bug-wget] Issue in cookie path checking
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.
0001-2014-06-12-Yasuhisa-Ishikawa-yasuhisa.ishikawa-kuman.patch
Description: Binary data
Regards,
Yasuhisa Ishikawa
2014/06/04 3:09、Darshit Shah dar...@gmail.com のメール：
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, yasuhisa.ishik...@kumaneko.org
yasuhisa.ishik...@kumaneko.org 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.orig2013-10-21 23:50:12.0 +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
Re: [Bug-wget] Issue in cookie path checking
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, yasuhisa.ishik...@kumaneko.org
yasuhisa.ishik...@kumaneko.org 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.orig2013-10-21 23:50:12.0 +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
[Bug-wget] Issue in cookie path checking
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.orig2013-10-21 23:50:12.0 +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
