persistent xss vector in (unescaped) filenames in revision views

Bug #740142 reported by David
266
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Launchpad itself
Fix Released
Critical
William Grant
loggerhead
Fix Released
Critical
William Grant
loggerhead (Ubuntu)
Fix Released
High
Unassigned
Karmic
Fix Released
Undecided
Unassigned
Lucid
Fix Released
Undecided
Unassigned
Maverick
Fix Released
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. )

Tags: patch

Related branches

CVE References

Revision history for this message
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)>

Revision history for this message
David (d--) wrote :
Revision history for this message
John A Meinel (jameinel) wrote : Re: [Bug 740142] Re: persistent xss vector in (unescaped) filenames in revision views

-----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)
Changed in loggerhead:
assignee: nobody → John A Meinel (jameinel)
Revision history for this message
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.

Revision history for this message
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.

Revision history for this message
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.

Revision history for this message
David (d--) wrote :

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

Revision history for this message
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).

Revision history for this message
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.

Revision history for this message
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
Revision history for this message
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.

Revision history for this message
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.

Revision history for this message
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).

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

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

Revision history for this message
Robert Collins (lifeless) wrote :

I have mailed <email address hidden> the patch.

Revision history for this message
Robert Collins (lifeless) wrote :

The candidate fix.

Revision history for this message
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
Revision history for this message
Kees Cook (kees) wrote :

Please use CVE-2011-0728 for this issue.

Revision history for this message
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?

Revision history for this message
William Grant (wgrant) wrote :
Revision history for this message
William Grant (wgrant) wrote :
Revision history for this message
William Grant (wgrant) wrote :
Revision history for this message
William Grant (wgrant) wrote :
Revision history for this message
William Grant (wgrant) wrote :
William Grant (wgrant)
Changed in loggerhead (Ubuntu):
importance: Undecided → High
assignee: nobody → William Grant (wgrant)
status: New → In Progress
Revision history for this message
Kees Cook (kees) wrote :

Yup, that's the intention. Feel free.

William Grant (wgrant)
Changed in loggerhead (Ubuntu):
status: In Progress → Confirmed
assignee: William Grant (wgrant) → nobody
William Grant (wgrant)
Changed in loggerhead:
status: In Progress → Fix Released
milestone: none → 1.18.1
visibility: private → public
Revision history for this message
William Grant (wgrant) wrote :

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

Revision history for this message
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!

Revision history for this message
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

Revision history for this message
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

Revision history for this message
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)
Changed in launchpad:
status: Triaged → Fix Released
assignee: nobody → William Grant (wgrant)
milestone: none → 11.04
Revision history for this message
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
Revision history for this message
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  
Everyone can see this security related information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.