Re: [PATCH 0/3] Fix segfault found by AFL

[Peter Colberg]

Hi Jason,

On Sun, Feb 19, 2017 at 08:04:49PM +0100, Jason A. Donenfeld wrote:
> Excellent. Running AFL on cgit is a great idea. I'll merge and review when
> I'm back at my desk on Tuesday.

Could you release a bugfix v1.1.1 afterwards? Debian is preparing for
the release of stretch and it would be perfect if I could upload a new
upstream version instead of cherry-picking the fixes.

Regards,
Peter
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: [PATCH 0/3] Fix segfault found by AFL

[Jason A. Donenfeld]

Excellent. Running AFL on cgit is a great idea. I'll merge and review when
I'm back at my desk on Tuesday.

On Feb 19, 2017 12:45, "John Keeping"  wrote:

I set AFL [0] loose on CGit's URL input yesterday and it managed to find
one issue that leads to a segfault via a null dereference.

Either of the first or third patches fixes the segfault, but I much
prefer the first as a solid fix, the third is a bit too subtle as a way
to ensure that the necessary invariant holds.

The second patch also fixes the route that AFL found, but it's possible
to get the same effect using broken out query parameters like
"?p=log=foo" but I'm including it because it seems to make sense to
use the value of the final "url" parameter we receive fully rather than
some combination of that and a previous URL.

[0] http://lcamtuf.coredump.cx/afl/

John Keeping (3):
  ui-shared: don't print path crumbs without a repo
  parsing: clear query path before starting
  cgit: don't set vpath unless repo is set

 cgit.c  | 12 ++--
 parsing.c   |  2 +-
 ui-shared.c |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

--
2.12.0.rc2.230.ga28edc07cd

___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: possible bug in config readme

[MonkZ]

Am 19.02.2017 um 16:17 schrieb John Keeping:
> On Sat, Feb 18, 2017 at 11:12:08PM +0100, MonkZ wrote:
>> As i have my cgit + uwsgi + nginx + gitolite setup in a LXD container, i
>> can publish it.
>> Would this be helpful?
> 
> Probably not necessary 
I was nearly done with it though :D - i've already stripped all private
data and custom configs.

> - I had another look at your config file and I
> realised that almost every "readme" line has a trailing space.  The CGit
> configfile parser strips whitespace around the "=" character but does
> not strip trailing whitespace.

WORKS Thank you very much!

> 
> Strangely the rest of the lines in the file do not have any trailing
> spaces and the line "readme=:install" doesn't but all the rest do (try
> "cat -E cgitrc" to show this).

The readme config block was copy/pasted from a howto... my fault.

> 
>> Am 18.02.2017 um 17:57 schrieb John Keeping:
>>> On Tue, Jan 24, 2017 at 11:19:09PM +0100, MonkZ wrote:
 i'm using cgit 1.1 and trying to configure a global list of possible
 readme files (see attachment), but all i can get to work are
 "cgit.readme" entries in git-config files.

 I would expect, that this list would be active until a repo config or
 git-config comes around to overwrite it.
>>>
>>> I can't see anything wrong with your config, and I have just tried
>>> something similar here and it seems to work.
>>>
>>> Having looked at the code, one small subtlety is that if there is a
>>> repository-specific readme configuration it overrides the default list
>>> rather than appending to it, but if you don't have any repository
>>> configuration then the list in cgitrc should be used.



signature.asc
Description: OpenPGP digital signature
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

Re: possible bug in config readme

[John Keeping]

On Sat, Feb 18, 2017 at 11:12:08PM +0100, MonkZ wrote:
> As i have my cgit + uwsgi + nginx + gitolite setup in a LXD container, i
> can publish it.
> Would this be helpful?

Probably not necessary - I had another look at your config file and I
realised that almost every "readme" line has a trailing space.  The CGit
configfile parser strips whitespace around the "=" character but does
not strip trailing whitespace.

Strangely the rest of the lines in the file do not have any trailing
spaces and the line "readme=:install" doesn't but all the rest do (try
"cat -E cgitrc" to show this).

> Am 18.02.2017 um 17:57 schrieb John Keeping:
> > On Tue, Jan 24, 2017 at 11:19:09PM +0100, MonkZ wrote:
> >> i'm using cgit 1.1 and trying to configure a global list of possible
> >> readme files (see attachment), but all i can get to work are
> >> "cgit.readme" entries in git-config files.
> >>
> >> I would expect, that this list would be active until a repo config or
> >> git-config comes around to overwrite it.
> > 
> > I can't see anything wrong with your config, and I have just tried
> > something similar here and it seems to work.
> > 
> > Having looked at the code, one small subtlety is that if there is a
> > repository-specific readme configuration it overrides the default list
> > rather than appending to it, but if you don't have any repository
> > configuration then the list in cgitrc should be used.
___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

[PATCH 0/3] Fix segfault found by AFL

[John Keeping]

I set AFL [0] loose on CGit's URL input yesterday and it managed to find
one issue that leads to a segfault via a null dereference.

Either of the first or third patches fixes the segfault, but I much
prefer the first as a solid fix, the third is a bit too subtle as a way
to ensure that the necessary invariant holds.

The second patch also fixes the route that AFL found, but it's possible
to get the same effect using broken out query parameters like
"?p=log=foo" but I'm including it because it seems to make sense to
use the value of the final "url" parameter we receive fully rather than
some combination of that and a previous URL.

[0] http://lcamtuf.coredump.cx/afl/

John Keeping (3):
  ui-shared: don't print path crumbs without a repo
  parsing: clear query path before starting
  cgit: don't set vpath unless repo is set

 cgit.c  | 12 ++--
 parsing.c   |  2 +-
 ui-shared.c |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

-- 
2.12.0.rc2.230.ga28edc07cd

___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit

[PATCH 1/3] ui-shared: don't print path crumbs without a repo

[John Keeping]

cgit_print_path_crumbs() can call repolink() which assumes that ctx.repo
is non-null.  Currently we don't have any commands that set want_vpath
without also setting want_repo so it shouldn't be possible to fail this
test, but the check in cgit.c is in the wrong order so it is possible to
specify a query string like "?p=log=foo/bar" to end up here without
a valid repository.

This was found by American fuzzy lop [0].

[0] http://lcamtuf.coredump.cx/afl/

Signed-off-by: John Keeping 
---
 ui-shared.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui-shared.c b/ui-shared.c
index 2e4fcd9..e5c9a02 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -1039,7 +1039,7 @@ void cgit_print_pageheader(void)
free(currenturl);
}
html("\n");
-   if (ctx.env.authenticated && ctx.qry.vpath) {
+   if (ctx.env.authenticated && ctx.repo && ctx.qry.vpath) {
html("");
html("path: ");
cgit_print_path_crumbs(ctx.qry.vpath);
-- 
2.12.0.rc2.230.ga28edc07cd

___
CGit mailing list
CGit@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/cgit