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

Status: RESOLVED FIXED

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

Description

[J. Paul Reed [:preed]]

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.

Comment 1

[J. Paul Reed [:preed]]

Adding the usual suspects...

Comment 2

[J. Paul Reed [:preed]]

-> taking

As noted, I'll do the 2.14.1 audit, as time permits... I've already started
anyway... 

Comment 3

[J. Paul Reed [:preed]]

Attachment: One in editusers.cgi

One example on the 2.14.1 BRANCH...

One fixed... ??? to go...

Comment 4

[Bradley Baetz (:bbaetz)]

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.

Comment 5

[J. Paul Reed [:preed]]

Attachment: 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...

Comment 6

[Bradley Baetz (:bbaetz)]

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

Comment 7

[J. Paul Reed [:preed]]

Attachment: 2 in editusers.cgi, take 2

bbaetz: like this?

Comment 8

[Bradley Baetz (:bbaetz)]

Comment on attachment 85228 [details] [diff] [review]
2 in editusers.cgi, take 2

WFM. Where else? ;)

Comment 9

[Myk Melez [:myk] [@mykmelez]]

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

Comment 10

[J. Paul Reed [:preed]]

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

Comment 11

[Matthew Tuck [:CodeMachine]]

I am part way through a 2.16 audit - I have found bugs but not what I would
consider security problems.

Comment 12

[Bradley Baetz (:bbaetz)]

JayPee - this went onto 2.14, but not 2.16 or trunk. Was there a reason for that?

Comment 13

[J. Paul Reed [:preed]]

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

Comment 14

[J. Paul Reed [:preed]]

Attachment: 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.

Comment 15

[J. Paul Reed [:preed]]

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

Comment 16

[Dave Miller [:justdave]]

2.14.2 is out, removing security group.