persistent xss vector in (unescaped) filenames in revision views

Reported by David on 2011-03-22
266
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Launchpad itself
Critical
William Grant
loggerhead
Critical
William Grant
loggerhead (Ubuntu)
High
Unassigned
Karmic
Undecided
Unassigned
Lucid
Undecided
Unassigned
Maverick
Undecided
Unassigned

Bug Description

Loggerhead does not escape filenames when showing filenames in a revision view.
I have made an example of this available at

https://bazaar.launchpad.net/~daveb/+junk/delete_me_v2/revision/2
(note: If you focus over the input box you should an alert dialogue with the number 1 in it. )

David (d--) wrote :

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

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

Changed in loggerhead:
importance: Undecided → Critical
status: New → Triaged
John A Meinel (jameinel) on 2011-03-22
Changed in loggerhead:
assignee: nobody → John A Meinel (jameinel)
David (d--) wrote :

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.

John A Meinel (jameinel) wrote :

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.

John A Meinel (jameinel) wrote :

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.

David (d--) wrote :

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

David (d--) wrote :

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

John A Meinel (jameinel) wrote :

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.

John A Meinel (jameinel) wrote :

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

Changed in launchpad:
status: New → Triaged
importance: Undecided → Critical
Robert Collins (lifeless) wrote :

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

Robert Collins (lifeless) wrote :

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.

Max Kanat-Alexander (mkanat) 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).

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

Robert Collins (lifeless) wrote :

I have mailed <email address hidden> the patch.

Robert Collins (lifeless) wrote :

The candidate fix.

William Grant (wgrant) wrote :

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

Changed in loggerhead:
assignee: John A Meinel (jameinel) → William Grant (wgrant)
status: Triaged → In Progress
Kees Cook (kees) wrote :

Please use CVE-2011-0728 for this issue.

Robert Collins (lifeless) wrote :

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?

William Grant (wgrant) wrote :
William Grant (wgrant) wrote :
William Grant (wgrant) wrote :
William Grant (wgrant) wrote :
William Grant (wgrant) wrote :
William Grant (wgrant) on 2011-03-24
Changed in loggerhead (Ubuntu):
importance: Undecided → High
assignee: nobody → William Grant (wgrant)
status: New → In Progress
Kees Cook (kees) wrote :

Yup, that's the intention. Feel free.

William Grant (wgrant) on 2011-03-24
Changed in loggerhead (Ubuntu):
status: In Progress → Confirmed
assignee: William Grant (wgrant) → nobody
William Grant (wgrant) on 2011-03-24
Changed in loggerhead:
status: In Progress → Fix Released
milestone: none → 1.18.1
visibility: private → public
William Grant (wgrant) wrote :

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

Steve Beattie (sbeattie) wrote :

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

Thanks!

Launchpad Janitor (janitor) wrote :

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

Launchpad Janitor (janitor) wrote :

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

Launchpad Janitor (janitor) wrote :

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

Changed in loggerhead (Ubuntu Karmic):
status: New → Fix Released
Changed in loggerhead (Ubuntu Lucid):
status: New → Fix Released
Changed in loggerhead (Ubuntu Maverick):
status: New → Fix Released
William Grant (wgrant) on 2011-03-25
Changed in launchpad:
status: Triaged → Fix Released
assignee: nobody → William Grant (wgrant)
milestone: none → 11.04
Artur Rona (ari-tczew) wrote :

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

tags: added: patch
Launchpad Janitor (janitor) wrote :

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

Changed in loggerhead (Ubuntu):
status: Confirmed → Fix Released
To post a comment you must log in.
This report contains Public Security information  Edit
Everyone can see this security related information.

Other bug subscribers