On Fri, Mar 22, 2019 at 01:41:27AM +0100, Ingo Schwarze wrote:
> I committed the patches from both of you to
>
> http://mandoc.bsd.lv/cgi-bin/cvsweb/?cvsroot=cvsweb
> http://mandoc.bsd.lv/cvsweb/
Thanks! I hadn't actually realized there was an upstream past the
OpenBSD ports tree. Good to know.
> in preparation for rolling a new release, then briefly looked at
> the script myself. My impression is there are huge numbers of
> additional candidates for missing escaping, see the incomplete list
> below (which may of course also contain some false positives). I'm
> not yet sure what to do about it (don't waste your time right now
> trying to fix all the potential issues below).
Yes, I believe the rest are all either actually safe due to some sort of
additional validation, or come from the cvs repository and so are less
likely to be malicious (or at least not significantly more malicious
than anything else being served).
> Maybe just fix the most important ones, check and apply the additional
> idea (adding some HTTP header) from sthen@, release 2.1.1 even if
> it is not perfect, update the port, then move on to the 3.0 branch,
> cleaning up things for real over there, aiming for a later 3.1.1
> release?
This is definitely an old codebase with lots of room for improvements.
If there is already work underway to improve that I certainly wouldn't
want to put too much work into patching up an old one only to have to
redo that work. I think patching up the important things and getting a
bugfix out is great with more improvements on a single branch.
I think I only see two spots below where the input could come from
someplace and may not be completely validated, the one in cvswebMarkup
and in chooseCVSRoot.
> That way, we would quickly get a 2.1.1 release that everybody could
> update to without too much hassle, whereas updating to 3.1.1 might
> not be easy for some, and besides releasing 3.1.1 will certainly
> not be quick. At the same time, that plan would avoid wasting too
> much time on 2.1.1. Not sure yet...
>
> Yours,
> Ingo
>
>
>
> Additional candidates where escaping is likely missing:
> -------------------------------------------------------
> sub clickablePath($$) {
> $retval = "[$cvstree]";
> $retval .= ' ' . &link("[$cvstree]",
> sprintf('%s/%s#dirlist', $scriptname, $query));
$cvstree should be safe since it has to be something set in %CVSROOT via
@CVSrepositories in the config file. While I suppose it's possible for
that to contain stuff that needs escaping, it's unlikely to be
malicious.
if ($input{cvsroot} && $CVSROOT{$input{cvsroot}}) {
$cvstree = $input{cvsroot};
> $retval .= $_;
The current dir, again controlled by the content of the cvs repository
is probably fine.
> sub cvswebMarkup($$$) {
> print "Tag: <b>", $input{only_with_tag}, "</b><br>\n"
This one should be fixed, and I see it is. Sorry about missing that one.
> sub doDiff($$$$$$) {
> print $_;
I believe this can only be reached after a text/plain header, not sure
we need to escape those.
> sub getDirLogs($$@) {
> print "$state:$_" if ($verbose);
I believe that $verbose causes things to print all sorts of stuff useful
only when debugging, if we need to sanitize that output it seemed like a
lot of work for something that was only useful when actually coding on
cvsweb. If I misunderstood $verbose, which is totally possible, but I
don't think it's something that works in "production".
> print "$filename $rev Wanted: $revwanted ",
> "Revbranch: $revbranch Branch: $branch ",
> "Branchpoint: $branchpoint\n"
> "File revision $rev found for
> branch
> $branch\n"
> print
> "File info $rev found for
> $filename
>
> sub readLog($;$) {
> print if ($verbose);
> print "R:", $_ if ($verbose);
> print "D:", $_ if ($verbose);
> print "L:", $_ if ($verbose);
> print "E:", $_ if ($verbose);
>
> sub link($$) {
> sprintf '<a href="%s">%s</a>', hrefquote($url), $name;
I think this is as designed and we'd have to make sure $name is escaped
as necessary before calling `link`.
# Note that this doesn't htmlquote the first argument...
> sub printLog($;$) {
> print "<a name=\"rev$_\"></a>";
> print "<a name=\"$sym\"></a>";
> print "<a name=\"$sym\"></a>";
> sprintf(
> '%s?r1=%s%s', $scriptwhere,
> $_, htmlquote($barequery)
> )
> print "Revision <b>$_</b>";
> print "<i>", $author{$_}, "</i>\n";
> print "<br>Branch: <b>", $link ? link_tags($revsym{$br}) :
> $revsym{$br},
> print "<br>CVS Tags: <b>", $link ? link_tags($revsym{$_}) :
> $revsym{$_},
> print "<br>Branch point for: <b>",
> $link ? link_tags($branchpoint{$_}) : $branchpoint{$_}, "</b>\n"
> print
> "<br>Changes since <b>$prev: $difflines{$_}
> lines</b
All this seems to come from the cvs repo, so yes it should probably be
escaped, but I think we can consider it a non-malicious source for now.
> sub doLog($) {
> print "Current tag: $input{only_with_tag}<br>\n";
> print ">${_}</option>\n";
Not sure how I missed that one again. Sorry.
>
> sub human_readable_diff($) {
> print
> "<h3 style=\"text-align: center\">Diff for /$where_nd between
> versio
$where_nd comes from the $ENV{PATH_INFO}, and we do at least check
(indirectly via $fullname) that "$cvsroot/$where,v" exists before doing
that, so it is at least a something semi-safe.
> sub navigateHeader($$$$$) {
> <title>$path$filename - $title - $rev</title>$css
> print &link($backicon, "$swhere$query#rev$rev");
> print "<b>Return to ", &link($filename, "$swhere$query#rev$rev"),
I think most of these are at least validated against existing in the
filesystem or are static values, other than $rev which I think is
matched against [\w.] everywhere. Yes, *should* be escaped, but should
not be from a malicious source unvalidated.
> sub chooseCVSRoot() {
> print "<input type=\"hidden\" name=\"$k\"
> value=\"$input
> {$k}\">\n"
This one should also be fixed, again, can't believe I missed it.
> print "CVS Root: <b>[$cvstree]</b>";
$cvstree is still from the config file, so probably should be escaped,
but not malicious.
> sub download_link($$$;$) {
> print "><b>$textlink</b></a>";
I think this is either a $rev, which should be validated everywhere, or
a string in the code.
> sub html_header($) {
> <title>$title</title>
The scariest thing is this $title could be the $where talked about
before, so it should be escaped, but it shouldn't be malicious.
l8rZ,
--
andrew - http://afresh1.com
The programmer's national anthem is 'AAAAAAAARRRRGHHHHH!!'.
