Closed Bug 238797 Opened 20 years ago Closed 20 years ago
create a flag with " in its name
fwiw, clicking delete <http://bugzilla.mozilla.org/editflagtypes.cgi?action=confirmdelete&id=89> gives me an ISE: Internal Server Error The server encountered an internal error or misconfiguration and was unable to complete your request. Please contact the server administrator, email@example.com and inform them of the time the error occurred, and anything you might have done that may have caused the error. More information about this error may be available in the server error log. Apache/1.3.27 Server at bugzilla.mozilla.org Port 80
sorry, it's > that's doing the damage, not "
Severity: major → critical
Flags: blocking2.18? → blocking2.18+
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → Bugzilla 2.18
Timeless: can you please NOT use b.m.o. as a playground for testing possible Bugzilla security bugs? Everyone else: Our filters are in a twist again. We've got several problems here. The problematic flag name is (to all intents and purposes) Foo'">Foo Currently, we "FILTER js" the name. This puts a \ before the ", but that doesn't stop it terminating the HTML attribute. Hence the dodgy rendering in the "Delete" link. It would seem we want "FILTER html". However, this doesn't do the trick either, because that doesn't escape the single quote character, so the JS dies. The right thing might be for the html filter to replace ' with ' (which does work cross-browser - " doesn't) but it's a TT built-in so it's a bit tricky to fix. The workaround seems to be to FILTER html FILTER js. (The other way around works too, but looks weirder.) Is this what we need to be doing for all JS which isn't inside script tags? The Developers Guide is silent on the HTML vs. JS filtering question. I'm not sure what the Internal Server error is about. Nothing appears in mecha's error log when I trigger it. Gerv
Right, this is similar to the FILTER url FILTER html stuff mentioned in the TT docs (which we avoid by using url_quote, but thats separate)
Whiteboard: [does not affect 2.16.x] [wanted for 2.18rc1]
fwiw, we already have FILTER html overridden in Bugzilla/Template.pm for purposes of escaping the @ in email addresses. Adding an escape for ' to our existing override would be trivial.
Note that this issue is already partially disclosed by bmo having a 'ignore me, I'm just here to cause ">trouble' flag type that shows up in requests.cgi dropdown. Not that it makes a lot of difference in practice, but yet another reason for not testing security issues on bmo.
This touches several templates, some are public-facing...
Comment on attachment 152662 [details] [diff] [review] Patch v1 I considered updating test 8 to check for this, but in order to do it right, it needs to know the filetype of the file being testing, and conditionally require both IFF the file is html... and that didn't look very obvious at first glance.
Attachment #152662 - Flags: review?(gerv)
Comment on attachment 152662 [details] [diff] [review] Patch v1 Dave: I believe that the "</script>" example is actually a special one - AIUI, this is the only bit of HTML which exhibits this problem. Browsers otherwise treat the contents of <script> tags as all JS data. (Obviously, </script> has to be special!) I don't know if this means that we actually need the HTML filtering inside <script> tags or not. Do you need to update the escaping section in the Developers Guide? The patch looks fine, assuming you've tested all the instances with various bits of potentially unsafe data and nothing breaks. Gerv
Attachment #152662 - Flags: review?(gerv) → review+
Comments from the security/standards folks please?
(In reply to comment #10) > (From update of attachment 152662 [details] [diff] [review]) > Dave: I believe that the "</script>" example is actually a special one - AIUI, > this is the only bit of HTML which exhibits this problem. Browsers otherwise > treat the contents of <script> tags as all JS data. (Obviously, </script> has > to be special!) > > I don't know if this means that we actually need the HTML filtering inside > <script> tags or not. Hmm, well, we probably at least need to filter for </script> then in those cases. Probably easier to just filter for HTML in general. It doesn't seem to break anything to do it. > Do you need to update the escaping section in the > Developers Guide? Yes, whatever we decide on here (I'd like input from Jesse and Ian on it) will
New patch, based on Jesse's recommendations. Adds '/' to the list of escaped characters in the js filter, adds the html filter to the two places that were inside HTML attributes that were missing it.
Attachment #152662 - Attachment is obsolete: true
Comment on attachment 152747 [details] [diff] [review] Patch v2 Looks good. r=gerv. Gerv
Attachment #152747 - Flags: review?(gerv) → review+
OK, the issue this is fixing on the trunk also happens to be present on the 2.16 branch as well (though it has nothing to do with flags there, obviously). Patch coming up.
Whiteboard: [does not affect 2.16.x] [wanted for 2.18rc1] → [wanted for 2.16.6] [ready for 2.18rc1]
Comment on attachment 152771 [details] [diff] [review] Patch for 2.16 r=vladd
Attachment #152771 - Flags: review?(gerv) → review+
Whiteboard: [wanted for 2.16.6] [ready for 2.18rc1] → [ready for 2.16.6] [ready for 2.18rc1]
Whiteboard: [ready for 2.16.6] [ready for 2.18rc1] → [fixed in 2.16.6] [fixed in 2.18rc1]
Target Milestone: Bugzilla 2.18 → Bugzilla 2.16
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.