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 anyway...
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.
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...
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....
Created attachment 85228 [details] [diff] [review] 2 in editusers.cgi, take 2 bbaetz: like this?
Comment on attachment 85228 [details] [diff] [review] 2 in editusers.cgi, take 2 WFM. Where else? ;)
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 184.108.40.206 >diff -u -r220.127.116.11 editusers.cgi >--- editusers.cgi 11 May 2002 10:01:29 -0000 18.104.22.168 >+++ 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
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: 22.214.171.124; previous revision: 126.96.36.199 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?
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: 188.8.131.52; 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
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.
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. :-)
2.14.2 is out, removing security group.