425838 - Using taint checking to prevent XSS attacks

Status: NEW

(Bugzilla :: Bugzilla-General, enhancement, P2)

Description

[Bradley Baetz (:bbaetz)]

Attachment: Patch

SS attacks suck. In the same way that taint checking on the DBI handle stopped most of the SQL injection and security issues, taint checking on output should stop XSS.

Proof-of-concept patch to come; I'm unlikely to be able to do much work on this in the next week due to other commitments.

things missing:

 - The trick taint should be in the filter
 - Trick taint should only be if the content_type matches the relevent filter
 - I removed is_tainted in favour of Scalar::Util's builtin (core module in 5.8, which we require), but theres a usage in Bugzilla::Install::Util, except I can't work out what that code does
 - For better output buffering, ->process should create a dummy IO object and just use that (which taint checks in ->write) for *all* types except for a stringref being passed in.
 - Need to check how emails work - we do want to taint check those, but not taint check misc strings that are constructed along the way. Although its possible that thats already taint checked by perl

The get_text change should be separate; I ran into it when I stuffed up ->process; it needs to use the inner template somehow

Theres actually not much thats caught by this - w/o the trick_taint changes all that is caught is the skin name in the css, which comes from reading a directory. In the non-error case its basically all from the DB. Of course, that means that verifying this doesn't break will be a huge pain...

This catches the latest bug.

Comment 1

[Max Kanat-Alexander]

Comment on attachment 312397 [details] [diff] [review]
Patch

This is a good idea, I've been thinking for a while about how to do this.

I'm a little concerned about throwing everything into an in-memory variable instead of pushing it to stdout immediately--for things like extremely long searches, won't this stop TT from doing output as it goes? Also, for email sending, I'd be a little concerned.

Perhaps we should be setting template->error instead of dying inside of process()?

Comment 2

[Bradley Baetz (:bbaetz)]

Everything is an internal var anyway, because TT uses strings for PROCESS and our top level template does that.

Myk had some patches to do a FLUSH type thing (eg http://lists.template-toolkit.org/pipermail/templates/2002-February/002814.html can't find the bz bug), but they were very hacky and weren't accepted upstream.

Using an IO::Handle object (see comments above) would remove this overhead in theory, although not in practice; the only real reason to do that would be to punt this patch upstream, and I can't see that happening.

Comment 3

[Bradley Baetz (:bbaetz)]

I've tried to think of a way to get all output (even stuff from the db) filtered, but can't really. We could taint everything in the vars hash, I guess, but there are the exceptions, and I'm not sure how to handle those; filterexceptions doesn't help because we need to know from the top level template.

Comment 4

[David Humphrey (:humph)]

Bradley, are you still working on this, or can we reassign to nobody so that it's clear a student could take this project over?

Comment 5

[Bradley Baetz (:bbaetz)]

Yeah, go for it - my free time is zero for a while.

I'm not sure that its a good student project, though:

 - its not really rewarding/'sexy' - the 'ideal' end result is that no-one notices any change. OTOH if whoever does this finds lots of security issues that that would be rewarding, but I'd hope that that isn't the case here
 - its hard to do incrementally - its not *entirely* all or nothing, but its close
 - theres a lot of detail and subtlety in working out what is/isn't OK. This is more a belt-and-suspenders type thing - without rearchitecting template toolkit there are limitations to what this can catch

Or I may just be pessimistic :)

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
>  - its not really rewarding/'sexy' - the 'ideal' end result is that no-one
> notices any change. OTOH if whoever does this finds lots of security issues
> that that would be rewarding, but I'd hope that that isn't the case here

  Some students like security.

>  - theres a lot of detail and subtlety in working out what is/isn't OK. This is
> more a belt-and-suspenders type thing - without rearchitecting template toolkit
> there are limitations to what this can catch

  Well, perhaps the additions to TT could also be part of the project, who knows.

Comment 7

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.