persistent xss vector in (unescaped) filenames in revision views

Actually, I have deleted that branch.
All I had was a file with 'ok' in it and named "<input onfocus=alert(1)>

new example at http://bazaar.launchpad.net/~daveb/+junk/delete_me_v2/revision/2?start_revid=2

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 3/22/2011 1:14 PM, daveb wrote:
> new example at
> http://bazaar.launchpad.net/~daveb/+junk/delete_me_v2/revision/2?start_revid=2
>

I certainly get a popup box on loading that page.

 status: triaged
 importance: critical

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2IquMACgkQJdeBCYSNAAPvtACguNrSFZ4esKjfCt5nTuqFF1D/
+6IAn23CkQ3FJzLYUqC998e29Y96YEEY
=0UCC
-----END PGP SIGNATURE-----

because of the location of this bug I have deleted the repository - if you visited the https:// version and I had put alert(document.cookie) you would have seen your .launchpad.com lp cookie - which is apparently your session cookie.

Note that I tested this for the '/files' page, and the content is properly escaped. I'll make sure to keep the test case, though.

It is the '/revision' page specifically which is failing to escape the filenames.

The actual file viewer /view and /annotate, etc all also seem to do the right escaping. It is just the /revision page as far as I can work out.

I haven't tested the above, but this would not work if the cooke is a httponly cookie.

on a side note - if you test it in firefox4 or chrome 10 - the unescaped content makes it into the page but you won't see an alert 1 / inputbox where the shouldn't be (for the example(s) I put here).

I think I tracked down the specific case in question. However, we certainly need more comprehensive testing to cover all the different cases that could be triggered.

This seems to be the minimal fix:
=== modified file 'loggerhead/templatefunctions.py'
--- loggerhead/templatefunctions.py 2011-03-02 14:07:21 +0000
+++ loggerhead/templatefunctions.py 2011-03-22 14:51:59 +0000
@@ -53,12 +53,12 @@
                     cgi.escape(filename), cgi.escape(filename))
             else:
                 return revision_link(
- url, entry.revno, filename, '#' + filename)
+ url, entry.revno, filename, '#' + cgi.escape(filename, True))
     else:

I'm still poking around at some other places that might expose paths:
         def file_link(filename):
             return '<a href="%s%s" title="View changes to %s in revision %s">%s</a>' % (
- url(['/revision', entry.revno]), '#' + filename, cgi.escape(filename),
- cgi.escape(entry.revno), cgi.escape(filename))
+ url(['/revision', entry.revno]), '#' + cgi.escape(filename),
+ cgi.escape(filename), cgi.escape(entry.revno), cgi.escape(filename))
     return _pt('revisionfilechanges').expand(
         entry=entry, file_changes=file_changes, file_link=file_link, **templatefunctions)

@@ -128,7 +128,7 @@
 @templatefunc
 def revision_link(url, revno, path, frag=''):
     return '<a href="%s%s" title="View changes to %s in revision %s">%s</a>' % (
- url(['/revision', revno, path]), frag, cgi.escape(path),
+ url(['/revision', revno, cgi.escape(path)]), frag, cgi.escape(path),
         cgi.escape(revno), cgi.escape(path))

=== modified file 'loggerhead/templatefunctions.py'
--- loggerhead/templatefunctions.py 2011-03-02 14:07:21 +0000
+++ loggerhead/templatefunctions.py 2011-03-22 19:48:04 +0000
@@ -53,11 +53,11 @@
                     cgi.escape(filename), cgi.escape(filename))
             else:
                 return revision_link(
- url, entry.revno, filename, '#' + filename)
+ url, entry.revno, filename, '#' + cgi.escape(filename))
     else:
         def file_link(filename):
             return '<a href="%s%s" title="View changes to %s in revision %s">%s</a>' % (
- url(['/revision', entry.revno]), '#' + filename, cgi.escape(filename),
+ url(['/revision', entry.revno]), '#' + cgi.escape(filename), cgi.escape(filename),
                 cgi.escape(entry.revno), cgi.escape(filename))
     return _pt('revisionfilechanges').expand(
         entry=entry, file_changes=file_changes, file_link=file_link, **templatefunctions)

seems right to me. url is a dict, so url[...] is just a lookup and can use whatever data we put in it.

I've subscribed mkanat because I know he's running a fairly important loggerhead site as well. We'll want to tell savannah too. I don't think sourceforge run loggerhead though.... I might be wrong.

Thanks for the heads up. Is this affecting the 1.18 branch? If so, it should be patched on that branch as well and trigger a 1.18.1 release (which is a very easy thing to do).

On Tue, 22 Mar 2011 20:16:49 -0000, Max Kanat-Alexander <email address hidden> wrote:
> Thanks for the heads up. Is this affecting the 1.18 branch? If so, it
> should be patched on that branch as well and trigger a 1.18.1 release
> (which is a very easy thing to do).
>
> --
> You received this bug notification because you are a member of
> Loggerhead-team, which is a direct subscriber.
> https://bugs.launchpad.net/bugs/740142

Yes, it's been around for a pretty long while :(

Cheers,
mwh

I have mailed <email address hidden> the patch.

The candidate fix.

The attached fix is inadequate. A more complete one is in the linked branch.

Please use CVE-2011-0728 for this issue.

On Thu, Mar 24, 2011 at 1:18 PM, Kees Cook <email address hidden> wrote:
> Please use CVE-2011-0728 for this issue.

Can we share the ID with sourceforge etc at this point?

Yup, that's the intention. Feel free.

Pushed to lp:loggerhead and lp:loggerhead/1.18. I've released 1.18.1 with the fix.

William, after reviewing and testing your patches, I've gone ahead and published the loggerhead updates to the respective security pockets.

Thanks!

This bug was fixed in the package loggerhead - 1.17+bzr424-1ubuntu1.1

---------------
loggerhead (1.17+bzr424-1ubuntu1.1) maverick-security; urgency=low

  * SECURITY UPDATE: Cross-site scripting vulnerabilities by crafted branch
    contents. (LP: #740142)
    - debian/patches/bug-740142.diff: improve escaping of filenames.
    - CVE-2011-0728
 -- William Grant <email address hidden> Thu, 24 Mar 2011 13:20:04 +1100

This bug was fixed in the package loggerhead - 1.17+bzr400-1ubuntu0.1

---------------
loggerhead (1.17+bzr400-1ubuntu0.1) lucid-security; urgency=low

  * SECURITY UPDATE: Cross-site scripting vulnerabilities by crafted branch
    contents. (LP: #740142)
    - debian/patches/bug-740142.diff: improve escaping of filenames.
    - CVE-2011-0728
 -- William Grant <email address hidden> Thu, 24 Mar 2011 13:39:43 +1100

This bug was fixed in the package loggerhead - 1.17-0ubuntu1.1

---------------
loggerhead (1.17-0ubuntu1.1) karmic-security; urgency=low

  * SECURITY UPDATE: Cross-site scripting vulnerabilities by crafted branch
    contents. (LP: #740142)
    - debian/patches/bug-740142.diff: improve escaping of filenames.
    - CVE-2011-0728
 -- William Grant <email address hidden> Thu, 24 Mar 2011 14:01:44 +1100

I'm unsubscribing sponsors from bug as William Grant is going to get fixed package from Debian to natty.

This bug was fixed in the package loggerhead - 1.18.1-1

---------------
loggerhead (1.18.1-1) unstable; urgency=high

  * Bump python-simplejson from Recommends to Depends, as loggerhead
    breaks with python-json. LP: #586611
  * Switch to dh_python2. Closes: #616876
  * Switch to debhelper 7, drop cdbs.
  * Claim support for Bazaar 2.4.
  * New upstream release.
   + Fixes escaping of filenames in revision views. (CVE-2011-0728)
     LP: #740142

loggerhead (1.18-2) unstable; urgency=low

  * Run the test suite during package build.
 -- Jelmer Vernooij <email address hidden> Mon, 28 Mar 2011 19:19:09 +0000