If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Bugzilla 2.16

Status

()

Bugzilla
Bugzilla-General
P1
major
RESOLVED FIXED
16 years ago
5 years ago

People

(Reporter: preed, Assigned: preed)

Tracking

2.14.1
Bugzilla 2.16
x86
Linux

Details

(Whiteboard: [security] applied to 2.14.2)

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

16 years ago
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.
(Assignee)

Comment 1

16 years ago
Adding the usual suspects...
(Assignee)

Comment 2

16 years ago
-> taking

As noted, I'll do the 2.14.1 audit, as time permits... I've already started
anyway... 
Assignee: justdave → preed
(Assignee)

Comment 3

16 years ago
Created attachment 85219 [details] [diff] [review]
One in editusers.cgi

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-

Updated

16 years ago
Priority: -- → P1
Whiteboard: [security] wanted for 2.14.2
Target Milestone: --- → Bugzilla 2.16
(Assignee)

Comment 5

16 years ago
Created attachment 85227 [details] [diff] [review]
Ok, two in editusers.cgi

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

Comment 7

16 years ago
Created attachment 85228 [details] [diff] [review]
2 in editusers.cgi, take 2

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 1.23.2.2
>diff -u -r1.23.2.2 editusers.cgi
>--- editusers.cgi	11 May 2002 10:01:29 -0000	1.23.2.2
>+++ 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
COLOR=\"red\">missing</FONT>");

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

r=myk
Attachment #85228 - Flags: review+
(Assignee)

Comment 10

16 years ago
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: 1.23.2.3; previous revision: 1.23.2.2
done

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?
(Assignee)

Comment 13

16 years ago
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: 1.35.2.1; previous revision: 1.35
done

... and the trunk:

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

Comment 14

16 years ago
Created attachment 85895 [details] [diff] [review]
patch for 2.16 branch/trunk

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
r2.
(Assignee)

Comment 15

16 years ago
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. :-)
Status: NEW → RESOLVED
Last Resolved: 16 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.