Closed Bug 147486 Opened 22 years ago Closed 22 years ago

2.14.1/2.16 branches and trunk needs security audit against cross-site scripting bugs


(Bugzilla :: Bugzilla-General, defect, P1)




Bugzilla 2.16


(Reporter: preed, Assigned: preed)


(Whiteboard: [security] applied to 2.14.2)


(2 files, 2 obsolete files)

Overall problem described in bug 146447.

I found two more instances of 146447 in a quick grep of the 2_14_1-BRANCH (patch
for that coming shortly), which prompted bbaetz to suggest an audit of the
pending 2.14.2 code and the 2.16 BRANCH and trunk.

The 2.16 BRANCH and the trunk still need to be audited before we declare this
one dead, and I'll try to audit the 2.14.1 branch in the next couple of days.
Adding the usual suspects...
-> taking

As noted, I'll do the 2.14.1 audit, as time permits... I've already started
Assignee: justdave → preed
Attached patch One in editusers.cgi (obsolete) — Splinter Review
One example on the 2.14.1 BRANCH...

One fixed... ??? to go...
Comment on attachment 85219 [details] [diff] [review]
One in editusers.cgi

Theres a second instance of this. I know I said on IRC that it wasn't a bug,
but I think I was wrong.
Attachment #85219 - Flags: review-
Priority: -- → P1
Whiteboard: [security] wanted for 2.14.2
Target Milestone: --- → Bugzilla 2.16
Attached patch Ok, two in editusers.cgi (obsolete) — Splinter Review
bbaetz: are you sure? I thought you were right... either way, here's the other
one I was talking about...
Attachment #85219 - Attachment is obsolete: true
Comment on attachment 85227 [details] [diff] [review]
Ok, two in editusers.cgi

Nope, try again ;)

Theres explict html added at the top line of the chunk, so you have to
html_quote it earlier.

Actually, that applies to both chunks....
Attachment #85227 - Flags: review-
bbaetz: like this?
Attachment #85227 - Attachment is obsolete: true
Comment on attachment 85228 [details] [diff] [review]
2 in editusers.cgi, take 2

WFM. Where else? ;)
Attachment #85228 - Flags: review+
Comment on attachment 85228 [details] [diff] [review]
2 in editusers.cgi, take 2

>? .editusers.cgi.swp
>? patches
>Index: editusers.cgi
>RCS file: /cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v
>retrieving revision
>diff -u -r1.23.2.2 editusers.cgi
>--- editusers.cgi	11 May 2002 10:01:29 -0000
>+++ editusers.cgi	28 May 2002 09:13:16 -0000
>@@ -336,7 +336,7 @@
>             $s = "<STRIKE>";
>             $e = "</STRIKE>";
>         }
>-	$realname ||= "<FONT COLOR=\"red\">missing</FONT>";
>+	$realname = ($realname) ? html_quote($realname) : "<FONT COLOR=\"red\">missing</FONT>";

Nit: better as ...

+	$realname = ($realname ? html_quote($realname) : "<FONT

... since then the parentheses delineate the expression returning the value.

Attachment #85228 - Flags: review+
Checked in, taking Myk's suggestion (nit!! :-) for the ) placement.

Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision:; previous revision:

Now, as bbaetz said: "Where else?"
I am part way through a 2.16 audit - I have found bugs but not what I would
consider security problems.
JayPee - this went onto 2.14, but not 2.16 or trunk. Was there a reason for that?
Uhh... would "I'm an idiot" be reason enough for ya? :-)

I just mis-read matty's statements on IRC... I thought he said templates fixed
everything in 2.16/trunk, but that was just the user stuff, not the admin stuff,
I guess.

Checked in on the 2.16 branch:

Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision:; previous revision: 1.35

... and the trunk:

Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.36; previous revision: 1.35
Patch that went on the 2.16 branch/trunk; the original patch didn't apply
cleanly to these two. Also includes the semantic issues myk brought up in his
I talked with bbaetz on IRC about this bug last night for a few minutes; I'm
going to close it out, since I've spent about an hour and a half looking at the
2.14.1 branch code, and while I found a couple of things, bbaetz confirmed that
what I found was ok.

What I did is look at what things in bz that can entered freeform; bbaetz and I
came up with email addresses, "realnames" and comments. Comments are fine
(bbaetz said we'd notice real quick on bmo if they weren't), and this patch and
bug 146447 took care of the realnames stuff.

So, after having stared at 2.14.1 branch code for said 1.5 hours, I think I've
got better things to focus my attention towards; we knew this one was going to
be hit and miss.

It doesn't really matter anyway... as soon as I close this bug, someone will
find another instance of it... maybe that's what my "audit" should've consisted
of in the first place. :-)
Closed: 22 years ago
Resolution: --- → FIXED
Whiteboard: [security] wanted for 2.14.2 → [security] applied to 2.14.2
2.14.2 is out, removing security group.
Group: webtools-security?
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.