280464 - XSS vulnerability in registry/file.cgi

Status: RESOLVED FIXED

(Webtools Graveyard :: Bonsai, defect)

Description

[Michael Krax]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

http://webtools.mozilla.org/registry/who.cgi?email=%3E%3Cscript%3Ealert(document.cookie)%3C/script%3E

http://webtools.mozilla.org/registry/file.cgi?file=%3Cscript%3Ealert(document.cookie)%3C/script%3E



Reproducible: Always

Comment 1

[timeless]

this is at least partly a duplicate of bug 276985 which you reported. registry
is shared between tinderbox and bonsai.

Comment 2

[cls]

Attachment: Do not use make_cgi_args

So the problem stems from the use of make_cgi_args.  This function just takes
the values from the url and re-encodes them so that they can be used in a link.
 It does no sanity checking so we're better off just not using it.

Comment 3

[cls]

Comment on attachment 178967 [details] [diff] [review]
Do not use make_cgi_args

Argh! Wrong bug!

Comment 4

[Mike Taylor [:bear]]

changing severity to major so this bubbles to the top of my bonsai todo list

Comment 5

[Reed Loden [:reed]]

Attachment: patch - v1

I think this should fix it.

Comment 6

[Reed Loden [:reed]]

Attachment: patch - v1

I think this should fix it.

Comment 7

[Dave Miller [:justdave]]

Comment on attachment 249972 [details] [diff] [review]
patch - v1

yikes.

This only partially fixes it.  We've got a ton of unescaped stuff in the generated URLs still.

The individual parameters to the URLs need to be encoded so that & and = are %codes if they occur within a parameter.  The & characters in the URLs all need to be & as well.

If I'm not mistaken, the whole point of registry is purely output formatting.  If that's the case, this is a perfect candidate to be tossed into a template, which would probably make all the encoding/escaping issues easier to deal with, too.

Comment 8

[Reed Loden [:reed]]

Attachment: patch - v2

Oops, I just had my use of url_encode() and html_quote() backwards. :)

This should fix all the issues now.

Comment 9

[Dave Miller [:justdave]]

Comment on attachment 249981 [details] [diff] [review]
patch - v2

a) Undefined subroutine &main::html_quote called at /opt/webtools/registry/file.cgi line 31.

b) this only fixes file.cgi, who.cgi is reported with the same problem in comment 0

Comment 10

[Dave Miller [:justdave]]

OK, both of these issues are addressed in bug 276985, whose patch needs to be applied first before this one.  Setting dependency appropriately, will re-review in a moment.

Comment 11

[Dave Miller [:justdave]]

Comment on attachment 249981 [details] [diff] [review]
patch - v2

ok, the review- still stands.  With this patch applied, I still get the js alert in the testcase.

Comment 12

[Dave Miller [:justdave]]

ok, after the changes given on IRC, the testcases here no longer work, but this one does:

http://webtools.mozilla.org/registry/file.cgi?file=%22%22%3E%3Cscript%3Ealert(document.cookie)%3C/script%3E

Comment 13

[Dave Miller [:justdave]]

Attachment: Patch v3

This is a wholesale replacement for file.cgi, there is NONE of the original code left.  The entire thing is re-presentation of items passed in via the query string, so I moved the majority of the display logic into a template.

As a consequence of being a wholesale rewrite, the license has also been changed from NPL to MPL.

Comment 14

[Dave Miller [:justdave]]

patch v3 is available for testing at http://webtools.mozilla.org/registry2/file.cgi

That URL will go away once this is put into production.

Comment 15

[Dave Miller [:justdave]]

removing the dependency... this patch also has no dependencies on the other bug (or on lloydcgi.pl, which can go away once the other one is checked in as well)

Comment 16

[Dave Miller [:justdave]]

Oh, this also adds a dependency on Template Toolkit. Anyone also running Bugzilla on the same machine will already have this present -- we didn't on dm-webtools01, I just had to install it for this.

Comment 17

[Dave Miller [:justdave]]

Attachment: Patch v4

Revised patch based on some IRC discussion.  Gets rid of the <dt> usage, and fixes a couple code style nits in file.cgi.

Comment 18

[Dave Miller [:justdave]]

Attachment: Patch v5

Adds a DOCTYPE declaration, more general cleanup.  Moves the processing of the "@extra" stuff into the template.

Comment 19

[Dave Miller [:justdave]]

Attachment: Patch v5.1

minor cleanup of the previous patch based on stuff learned doing who.cgi on bug 276985.  The main reason for a new patch is the revised license header that says (C) 2007 instead of (C) 2006 ;)

Comment 20

[Dave Miller [:justdave]]

Checking in file.cgi;
/cvsroot/mozilla/webtools/registry/file.cgi,v  <--  file.cgi
new revision: 1.4; previous revision: 1.3
done
RCS file: /cvsroot/mozilla/webtools/registry/file.html.tmpl,v
done
Checking in file.html.tmpl;
/cvsroot/mozilla/webtools/registry/file.html.tmpl,v  <--  file.html.tmpl
initial revision: 1.1
done

Comment 21

[Yvan Boily [:ygjb][:yvan]]

Adding keywords to bugs for metrics, no action required.  Sorry about bugmail spam.