[Re: XSS vuln in cvsweb]

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[Re: XSS vuln in cvsweb]

Peter J. Philipp-3
Hi,

I have have created a patch for cvsweb port that needs review and help in
getting it into the port itself.  I'd like to apologize to Marc Espie for
contacting him regarding this port based on his last check-in on this port,  
and thanks to Stuart Henderson for directing me here.

The patch changed since my last submission to misc@ and since I am a complete
newbie in perl this would need a pro to look at it whether it's correct.

I have produced the patch with 'diff -u cvsweb.orig cvsweb' directly in the
/var/www/cgi-bin directory.  Credit goes to Ezio Paglia for finding this XSS
vuln.  Also the cvsweb at openbsd.org is affected and can be checked with:

https://cvsweb.openbsd.org/src/sbin/clri/clri.c?f=%22%3E%3Cscript%3Ealert(%27XSS%27)%3C/script%3E

in chrome the XSS check activates immediately, I don't know what firefox does.

Patch after end of signature and forwarded message:

-peter

----- Forwarded message from Stuart Henderson <[hidden email]> -----

Date: Fri, 15 Mar 2019 12:16:06 -0000 (UTC)
From: Stuart Henderson <[hidden email]>
To: [hidden email]
Subject: Re: XSS vuln in cvsweb
User-Agent: slrn/1.0.2 (OpenBSD)

On 2019-03-15, Peter J. Philipp <[hidden email]> wrote:
> Hi all,
>
> I have been notified by a wonderful security researcher that my site was
> vulnerable to XSS attacks.  The first one was on software I wrote, and the
> second one was on software I got from OpenBSD ports.  Not sure if I should
> be writing this to the ports mailing list though.
>
> I have written Marc Espie with a patch that I produced for cvsweb, but
> haven't heard from him in 11 hours so I want to get this out to everyone.

Yes, it should go to the ports mailing list. Check the "maintainer" line
in "pkg_info cvsweb". I don't know why you would send it to espie@.

----- End forwarded message -----

--- cvsweb.orig Thu Mar 14 18:30:06 2019
+++ cvsweb Fri Mar 15 10:23:05 2019
@@ -998,8 +998,9 @@
  if (scalar %tags || $input{only_with_tag}) {
  print "<form method=\"get\" action=\"./\">\n";
  foreach my $var (@stickyvars) {
+ my $tmpvar = htmlquote($input{$var});
  print
-    "<input type=\"hidden\" name=\"$var\" value=\"$input{$var}\">\n"
+    "<input type=\"hidden\" name=\"$var\" value=\"$tmpvar\">\n"
     if (defined($input{$var})
     && (!defined($DEFAULTVALUE{$var})
     || $input{$var} ne $DEFAULTVALUE{$var})
@@ -2612,7 +2613,7 @@
  sprintf(
  '%s/%s?annotate=%s%s', $scriptname,
  urlencode($where),     $_,
- $barequery
+ htmlquote($barequery)
  )
  );
  }
@@ -2625,7 +2626,7 @@
  '[select for diffs]',
  sprintf(
  '%s?r1=%s%s', $scriptwhere,
- $_,           $barequery
+ $_,           htmlquote($barequery)
  )
  );
  } else {
@@ -2828,7 +2829,7 @@
 
  foreach (@stickyvars) {
  printf('<input type="hidden" name="%s" value="%s">', $_,
-    $input{$_})
+    htmlquote($input{$_}))
     if (defined($input{$_})
     && ((!defined($DEFAULTVALUE{$_})
     || $input{$_} ne $DEFAULTVALUE{$_}) && $input{$_} ne ""));
@@ -3267,7 +3268,7 @@
  join ('', $scriptname,
  urlencode($wherepath),
  (!$last || $lastslash ? '/' : ''),
- $query,
+ htmlquote($query),
  (!$last || $lastslash ? "#dirlist" : "")
  ));
  } else {    # do not make a link to the current dir
@@ -3508,6 +3509,7 @@
  # Special Characters; RFC 1866
  s/&/&amp;/g;
  s/\"/&quot;/g;
+ s/%22/&quot;/g;
  s/</&lt;/g;
  s/>/&gt;/g;
 

Reply | Threaded
Open this post in threaded view
|

Re: [Re: XSS vuln in cvsweb]

Ingo Schwarze
Hi,

the trouble with cvsweb is that it is important OpenBSD project
infrastructure (consider cvsweb.openbsd.org) that has been abandoned
upstream 13 years ago, our version is 16 years old, and the port
has no maintainer.  Does anybody consider it funny to run a software
in production that is closely related to version control, but
(according to my knowledge) is not currently under version control
itself?

Given that i'm still using it on bsd.lv, too, i'm willing to host
a CVS repo for it and the release tarballs (historic and future)
on bsd.lv, pick up upstream maintenance, and also maintain the port.

Does that sound reasonable to people round here?
If so, does anyone know whether a copy of the original CVS repository
that it resided in still exists somewhere?  It seems to have vanished
from https://svnweb.freebsd.org/base/projects/ ...

I don't consider the XSS urgent, but it would of course get fixed
in the process.  If people wanted, they could test and commit patches
to the port beforehand, but i'm not sure it's needed.

Yours,
  Ingo

Reply | Threaded
Open this post in threaded view
|

Re: [Re: XSS vuln in cvsweb]

Stuart Henderson
On 2019/03/15 16:05, Ingo Schwarze wrote:

> Hi,
>
> the trouble with cvsweb is that it is important OpenBSD project
> infrastructure (consider cvsweb.openbsd.org) that has been abandoned
> upstream 13 years ago, our version is 16 years old, and the port
> has no maintainer.  Does anybody consider it funny to run a software
> in production that is closely related to version control, but
> (according to my knowledge) is not currently under version control
> itself?
>
> Given that i'm still using it on bsd.lv, too, i'm willing to host
> a CVS repo for it and the release tarballs (historic and future)
> on bsd.lv, pick up upstream maintenance, and also maintain the port.
>
> Does that sound reasonable to people round here?
> If so, does anyone know whether a copy of the original CVS repository
> that it resided in still exists somewhere?  It seems to have vanished
> from https://svnweb.freebsd.org/base/projects/ ...

Definitely.

> I don't consider the XSS urgent, but it would of course get fixed
> in the process.  If people wanted, they could test and commit patches
> to the port beforehand, but i'm not sure it's needed.

I wondered about doing something like this. Untested (I don't have an
installation handy) but it should disable inline scripts and some other
types of content, while still permitting inline css that cvsweb needs.

Index: cvsweb.cgi
--- cvsweb.cgi.orig
+++ cvsweb.cgi
@@ -3572,8 +3572,11 @@ sub http_header(;$) {
 
  if ($is_mod_perl) {
  Apache->request->content_type($content_type);
+ Apache->request->header_out(
+ "Content-Security-Policy" => "default-src 'self'; style-src 'self' 'unsafe-inline';");
  } else {
  print "Content-Type: $content_type\r\n";
+ print "Content-Security-Policy: default-src 'self'; style-src 'self' 'unsafe-inline';\r\n";
  }
 
  if ($allow_compress && $maycompress) {

Reply | Threaded
Open this post in threaded view
|

Re: [Re: XSS vuln in cvsweb]

Peter J. Philipp-3
In reply to this post by Ingo Schwarze
would this help any?

https://people.freebsd.org/~scop/cvsweb/

There is subsequent versions.

Regards,

-peter

On 3/15/19 4:05 PM, Ingo Schwarze wrote:

> Hi,
>
> the trouble with cvsweb is that it is important OpenBSD project
> infrastructure (consider cvsweb.openbsd.org) that has been abandoned
> upstream 13 years ago, our version is 16 years old, and the port
> has no maintainer.  Does anybody consider it funny to run a software
> in production that is closely related to version control, but
> (according to my knowledge) is not currently under version control
> itself?
>
> Given that i'm still using it on bsd.lv, too, i'm willing to host
> a CVS repo for it and the release tarballs (historic and future)
> on bsd.lv, pick up upstream maintenance, and also maintain the port.
>
> Does that sound reasonable to people round here?
> If so, does anyone know whether a copy of the original CVS repository
> that it resided in still exists somewhere?  It seems to have vanished
> from https://svnweb.freebsd.org/base/projects/ ...
>
> I don't consider the XSS urgent, but it would of course get fixed
> in the process.  If people wanted, they could test and commit patches
> to the port beforehand, but i'm not sure it's needed.
>
> Yours,
>    Ingo

Reply | Threaded
Open this post in threaded view
|

Re: [Re: XSS vuln in cvsweb]

Stuart Henderson-6
On 2019/03/15 16:28, Peter J. Philipp wrote:
> would this help any?
>
> https://people.freebsd.org/~scop/cvsweb/
>
> There is subsequent versions.

Those are the 13 year old ones that Ingo mentioned.

> Regards,
>
> -peter
>
> On 3/15/19 4:05 PM, Ingo Schwarze wrote:
> > Hi,
> >
> > the trouble with cvsweb is that it is important OpenBSD project
> > infrastructure (consider cvsweb.openbsd.org) that has been abandoned
> > upstream 13 years ago, our version is 16 years old, and the port
> > has no maintainer.  Does anybody consider it funny to run a software
> > in production that is closely related to version control, but
> > (according to my knowledge) is not currently under version control
> > itself?
> >
> > Given that i'm still using it on bsd.lv, too, i'm willing to host
> > a CVS repo for it and the release tarballs (historic and future)
> > on bsd.lv, pick up upstream maintenance, and also maintain the port.
> >
> > Does that sound reasonable to people round here?
> > If so, does anyone know whether a copy of the original CVS repository
> > that it resided in still exists somewhere?  It seems to have vanished
> > from https://svnweb.freebsd.org/base/projects/ ...
> >
> > I don't consider the XSS urgent, but it would of course get fixed
> > in the process.  If people wanted, they could test and commit patches
> > to the port beforehand, but i'm not sure it's needed.
> >
> > Yours,
> >    Ingo

Reply | Threaded
Open this post in threaded view
|

Re: [Re: XSS vuln in cvsweb]

Andrew Hewus Fresh
In reply to this post by Peter J. Philipp-3
On Fri, Mar 15, 2019 at 02:25:35PM +0100, Peter J. Philipp wrote:

> I have have created a patch for cvsweb port that needs review and help in
> getting it into the port itself.  I'd like to apologize to Marc Espie for
> contacting him regarding this port based on his last check-in on this port,  
> and thanks to Stuart Henderson for directing me here.
>
> The patch changed since my last submission to misc@ and since I am a complete
> newbie in perl this would need a pro to look at it whether it's correct.
>
> I have produced the patch with 'diff -u cvsweb.orig cvsweb' directly in the
> /var/www/cgi-bin directory.  Credit goes to Ezio Paglia for finding this XSS
> vuln.  Also the cvsweb at openbsd.org is affected and can be checked with:
I looked this over and updated the patch to be against the port.  It
seems to be good and I only found a couple other places that needed to
be escaped, the "stickyvars" section and the tr1/tr2 inputs in doLog,
although r1, r2, tr1 and tr2 are part of "unsafevars" so their content
is pretty limited already.

It was a pretty quick look so I don't doubt that there are more, and I
didn't actually get a chance to test it out, so hopefully someone else
can.

> https://cvsweb.openbsd.org/src/sbin/clri/clri.c?f=%22%3E%3Cscript%3Ealert(%27XSS%27)%3C/script%3E
>
> in chrome the XSS check activates immediately, I don't know what firefox does.

With NoScript it throws up a big error saying there was an attempted
XSS, without NoScript it throws up an alert box saying "XSS".

Index: Makefile
===================================================================
RCS file: /cvs/ports/devel/cvsweb/Makefile,v
retrieving revision 1.60
diff -u -p -r1.60 Makefile
--- Makefile 4 Sep 2018 12:46:10 -0000 1.60
+++ Makefile 16 Mar 2019 00:17:58 -0000
@@ -3,7 +3,7 @@
 COMMENT= CGI script to browse CVS repository trees
 
 DISTNAME= cvsweb-2.0.6
-REVISION= 26
+REVISION= 27
 CATEGORIES= devel www
 HOMEPAGE= http://www.freebsd.org/projects/cvsweb.html
 
Index: patches/patch-cvsweb_cgi
===================================================================
RCS file: /cvs/ports/devel/cvsweb/patches/patch-cvsweb_cgi,v
retrieving revision 1.13
diff -u -p -r1.13 patch-cvsweb_cgi
--- patches/patch-cvsweb_cgi 7 Apr 2013 20:07:24 -0000 1.13
+++ patches/patch-cvsweb_cgi 16 Mar 2019 00:17:58 -0000
@@ -1,6 +1,7 @@
 $OpenBSD: patch-cvsweb_cgi,v 1.13 2013/04/07 20:07:24 naddy Exp $
---- cvsweb.cgi.orig Thu Sep 26 22:56:05 2002
-+++ cvsweb.cgi Sun Apr  7 14:15:55 2013
+Index: cvsweb.cgi
+--- cvsweb.cgi.orig
++++ cvsweb.cgi
 @@ -1,4 +1,4 @@
 -#!/usr/bin/perl -wT
 +#!/usr/bin/perl -w
@@ -37,7 +38,18 @@ $OpenBSD: patch-cvsweb_cgi,v 1.13 2013/0
  );
 
  @LOGSORTKEYS = qw(cvs date rev);
-@@ -2014,20 +2009,6 @@ sub doDiff($$$$$$) {
+@@ -1003,8 +998,9 @@ if (-d $fullname) {
+ if (scalar %tags || $input{only_with_tag}) {
+ print "<form method=\"get\" action=\"./\">\n";
+ foreach my $var (@stickyvars) {
++ my $tmpvar = htmlquote($input{$var});
+ print
+-    "<input type=\"hidden\" name=\"$var\" value=\"$input{$var}\">\n"
++    "<input type=\"hidden\" name=\"$var\" value=\"$tmpvar\">\n"
+    if (defined($input{$var})
+    && (!defined($DEFAULTVALUE{$var})
+    || $input{$var} ne $DEFAULTVALUE{$var})
+@@ -2014,20 +2010,6 @@ sub doDiff($$$$$$) {
  my @difftype       = @{$difftype->{'opts'}};
  my $human_readable = $difftype->{'colored'};
 
@@ -58,7 +70,25 @@ $OpenBSD: patch-cvsweb_cgi,v 1.13 2013/0
  if ($human_readable) {
  if ($hr_ignwhite) {
  push @difftype, '-w';
-@@ -2658,7 +2639,7 @@ sub printLog($;$) {
+@@ -2631,7 +2613,7 @@ sub printLog($;$) {
+ sprintf(
+ '%s/%s?annotate=%s%s', $scriptname,
+ urlencode($where),     $_,
+- $barequery
++ htmlquote($barequery)
+ )
+ );
+ }
+@@ -2644,7 +2626,7 @@ sub printLog($;$) {
+ '[select for diffs]',
+ sprintf(
+ '%s?r1=%s%s', $scriptwhere,
+- $_,           $barequery
++ $_,           htmlquote($barequery)
+ )
+ );
+ } else {
+@@ -2658,7 +2640,7 @@ sub printLog($;$) {
  if (/^1\.1\.1\.\d+$/) {
  print " <i>(vendor branch)</i>";
  }
@@ -67,3 +97,57 @@ $OpenBSD: patch-cvsweb_cgi,v 1.13 2013/0
  my ($est) = $mytz[(localtime($date{$_}))[8]];
  print ", <i>", scalar localtime($date{$_}), " $est</i> (";
  } else {
+@@ -2847,7 +2829,7 @@ sub doLog($) {
+
+ foreach (@stickyvars) {
+ printf('<input type="hidden" name="%s" value="%s">', $_,
+-    $input{$_})
++    htmlquote($input{$_}))
+    if (defined($input{$_})
+    && ((!defined($DEFAULTVALUE{$_})
+    || $input{$_} ne $DEFAULTVALUE{$_}) && $input{$_} ne ""));
+@@ -2860,7 +2842,7 @@ sub doLog($) {
+ print $sel;
+ print "</select>\n";
+ $diffrev = $revdisplayorder[$#revdisplayorder];
+- $diffrev = $input{"r1"} if (defined($input{"r1"}));
++ $diffrev = htmlquote($input{"r1"}) if (defined($input{"r1"}));
+ print
+    "<input type=\"text\" size=\"$inputTextSize\" name=\"tr1\" value=\"$diffrev\" onchange=\"this.form.r1.selectedIndex=0\"></td>\n";
+ print "<td><br></td>\n</tr>\n";
+@@ -2871,7 +2853,7 @@ sub doLog($) {
+ print $sel;
+ print "</select>\n";
+ $diffrev = $revdisplayorder[0];
+- $diffrev = $input{"r2"} if (defined($input{"r2"}));
++ $diffrev = htmlquote($input{"r2"}) if (defined($input{"r2"}));
+ print
+    "<input type=\"text\" size=\"$inputTextSize\" name=\"tr2\" value=\"$diffrev\" onchange=\"this.form.r2.selectedIndex=0\"></td>\n";
+ print "<td><input type=\"submit\" value=\"  Get Diffs  \" accesskey=\"G\"></td>\n";
+@@ -2916,7 +2898,8 @@ sub doLog($) {
+ next if ($_ eq "f");
+ next if ($_ eq "only_with_tag");
+ next if ($_ eq "logsort");
+- print "<input type=\"hidden\" name=\"$_\" value=\"$input{$_}\">\n"
++ printf "<input type=\"hidden\" name=\"%s\" value=\"%s\">\n",
++ $_, htmlquote($input{$_})
+    if (defined($input{$_})
+    && (!defined($DEFAULTVALUE{$_})
+    || $input{$_} ne $DEFAULTVALUE{$_}) && $input{$_} ne "");
+@@ -3286,7 +3269,7 @@ sub clickablePath($$) {
+ join ('', $scriptname,
+ urlencode($wherepath),
+ (!$last || $lastslash ? '/' : ''),
+- $query,
++ htmlquote($query),
+ (!$last || $lastslash ? "#dirlist" : "")
+ ));
+ } else {    # do not make a link to the current dir
+@@ -3527,6 +3510,7 @@ sub htmlquote($) {
+ # Special Characters; RFC 1866
+ s/&/&amp;/g;
+ s/\"/&quot;/g;
++ s/%22/&quot;/g;
+ s/</&lt;/g;
+ s/>/&gt;/g;
+
Reply | Threaded
Open this post in threaded view
|

Re: [Re: XSS vuln in cvsweb]

Peter J. Philipp-3
On Fri, Mar 15, 2019 at 05:22:47PM -0700, Andrew Hewus Fresh wrote:

> > I have produced the patch with 'diff -u cvsweb.orig cvsweb' directly in the
> > /var/www/cgi-bin directory.  Credit goes to Ezio Paglia for finding this XSS
> > vuln.  Also the cvsweb at openbsd.org is affected and can be checked with:
>
> I looked this over and updated the patch to be against the port.  It
> seems to be good and I only found a couple other places that needed to
> be escaped, the "stickyvars" section and the tr1/tr2 inputs in doLog,
> although r1, r2, tr1 and tr2 are part of "unsafevars" so their content
> is pretty limited already.
>
> It was a pretty quick look so I don't doubt that there are more, and I
> didn't actually get a chance to test it out, so hopefully someone else
> can.
>
> > https://cvsweb.openbsd.org/src/sbin/clri/clri.c?f=%22%3E%3Cscript%3Ealert(%27XSS%27)%3C/script%3E
> >
> > in chrome the XSS check activates immediately, I don't know what firefox does.
>
> With NoScript it throws up a big error saying there was an attempted
> XSS, without NoScript it throws up an alert box saying "XSS".

> Index: Makefile
...

Hi,

Thanks for the help!  I have applied your patch and it went cleanly (commands
were cd /usr/ports/devel/cvsweb; patch -p0 < cvsweb.patch), I then rebuilt
the port with "make reinstall" no problem there either.  The CGI is the newly
built CGI as discovered with ls.  A quick test shows that it applied the
XSS escapes.

Best Regards,
-peter

Reply | Threaded
Open this post in threaded view
|

Re: [Re: XSS vuln in cvsweb]

Ingo Schwarze
Hi,

Peter J. Philipp wrote on Sat, Mar 16, 2019 at 07:52:23AM +0100:
> On Fri, Mar 15, 2019 at 05:22:47PM -0700, Andrew Hewus Fresh wrote:

>> I looked this over and updated the patch to be against the port.  It
>> seems to be good and I only found a couple other places that needed to
>> be escaped, the "stickyvars" section and the tr1/tr2 inputs in doLog,
>> although r1, r2, tr1 and tr2 are part of "unsafevars" so their content
>> is pretty limited already.
>>
>> It was a pretty quick look so I don't doubt that there are more, and I
>> didn't actually get a chance to test it out, so hopefully someone else
>> can.

> Thanks for the help!  I have applied your patch and it went cleanly (commands
> were cd /usr/ports/devel/cvsweb; patch -p0 < cvsweb.patch), I then rebuilt
> the port with "make reinstall" no problem there either.  The CGI is the newly
> built CGI as discovered with ls.  A quick test shows that it applied the
> XSS escapes.

I committed the patches from both of you to

  http://mandoc.bsd.lv/cgi-bin/cvsweb/?cvsroot=cvsweb
  http://mandoc.bsd.lv/cvsweb/

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).

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?

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));
                                $retval .= $_;
sub cvswebMarkup($$$) {
                print "Tag: <b>", $input{only_with_tag}, "</b><br>\n"

sub doDiff($$$$$$) {
                print $_;

sub getDirLogs($$@) {
                print "$state:$_" if ($verbose);
                                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;

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

sub doLog($) {
                print "Current tag: $input{only_with_tag}<br>\n";
                        print ">${_}</option>\n";

sub human_readable_diff($) {
        print
            "<h3 style=\"text-align: center\">Diff for /$where_nd between versio

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"),

sub chooseCVSRoot() {
                        print "<input type=\"hidden\" name=\"$k\" value=\"$input
{$k}\">\n"
                print "CVS Root: <b>[$cvstree]</b>";

sub download_link($$$;$) {
        print "><b>$textlink</b></a>";

sub html_header($) {
<title>$title</title>

Reply | Threaded
Open this post in threaded view
|

Re: [Re: XSS vuln in cvsweb]

Andrew Hewus Fresh
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!!'.