"Yet another html filter" (allowHTML)

Re: "Yet another html filter" (allowHTML)

sjdev86 — Fri, 23 Apr 2010 12:02:21 -0500

@sirdarckcat - thanks, I spotted some of those vectors coming through recently. I didn't realise that php's DOMDocument automatically placed cdata tags in between script / style tags (which caused malicious html coming afterwards to be allowed through).

I need to go over that in more detail (since the fix I've implemented is crude). I'm certainly not going to claim it's "safe" in its current form. The only aim is to keep improving it, based on the feedback / attacks from you guys!

Re: "Yet another html filter" (allowHTML)

sirdarckcat — Thu, 22 Apr 2010 08:21:39 -0500

Lots of bypasses by a couple of friends and users of another forum!
https://foro.elhacker.net/nivel_web/cyh_bypass_de_filtros_de_xss-t289955.0.html

They are fixed now, but I dont think it's very safe atm..

Greetings!!

Re: "Yet another html filter" (allowHTML)

sjdev86 — Mon, 19 Apr 2010 14:21:08 -0500

@sirdarckcat - better you didn't come across it too early, fewer holes to pick!

I think we can safely say that better character checking of style property names is needed (whitelist now in place) to guard against #1. Blacklisting style comments is all I can imagine for #2.

Re: "Yet another html filter" (allowHTML)

Gareth Heyes — Thu, 08 Apr 2010 06:40:52 -0500

@sirdarckcat

Nice ones :)

Re: "Yet another html filter" (allowHTML)

sirdarckcat — Thu, 08 Apr 2010 01:45:35 -0500

btw, thanks guys.. theres a new filter and noone told me :(

background:url(/*this-is-a-comment-on-IE);background-image:url(still-a-comment*/);

CSS is not easy dude :P

Re: "Yet another html filter" (allowHTML)

sirdarckcat — Thu, 08 Apr 2010 01:42:49 -0500

nice try, good luck next time

hola

greetings!!

Re: "Yet another html filter" (allowHTML)

sjdev86 — Tue, 02 Mar 2010 15:23:51 -0600

I'd literally just sat down to work on the url filtering (I wasn't happy with using a single regex for them, so decided to bypass the anti-samy rule), when I saw some new entries coming through in the demo logs - I thought it might be you. ;)

I've made some small updates as a result (good timing on your part) and will test the new entries against the default regex as well for comparison.

I have found type-hinting to be increasinly useful as time has gone by.

Re: "Yet another html filter" (allowHTML)

Gareth Heyes — Tue, 02 Mar 2010 14:34:30 -0600

Nice design, I've checked out the code too and it's much better. Tried a few things and it seemed to catch them. The check seems a bit weak though I almost able to sneak a vector through.

Good work! I'll keep my eye on this one. BTW I like the fact you use type hinting! Wish more devs did this

Re: "Yet another html filter" (allowHTML)

sjdev86 — Sat, 20 Feb 2010 18:40:09 -0600

The shorthand lists in antisamy are now allowed for (eg. background can take on background-image values as well). A couple of very minor issues encountered so far (the default rules not allowing everything I would have expected), but it is a very small minority:

AllOWED

test

NOT ALLOWED

test

Re: "Yet another html filter" (allowHTML)

Gareth Heyes — Tue, 16 Feb 2010 13:45:52 -0600

@sjdev86

Wow very nice I will test soon

Re: "Yet another html filter" (allowHTML)

sjdev86 — Tue, 16 Feb 2010 10:05:14 -0600

I've now switched everything over to use the anti-samy policy file.

Still a couple more things to implement (eg. cross-referencing shorthand lists), but it's working very nicely (IMO!) so far.

Re: "Yet another html filter" (allowHTML)

Gareth Heyes — Mon, 15 Feb 2010 03:17:51 -0600

@rvdh

C'mon give it a shot I know you want to :)

Re: "Yet another html filter" (allowHTML)

rvdh — Sun, 14 Feb 2010 17:47:18 -0600

LMAO I can see where this thread is heading.

Re: "Yet another html filter" (allowHTML)

Kyo — Sun, 14 Feb 2010 06:30:18 -0600

not an exploit (still tinkering), but i find it odd that it doesn't allow title for img

Re: "Yet another html filter" (allowHTML)

sjdev86 — Sat, 13 Feb 2010 14:09:40 -0600

Is anyone able to verify whether the mbstring function "mb_detect_encoding" is vulnerable to the buffer overflow vulnerability? I don't currently have access to anything below php 5.29.

http://www.securiteam.com/unixfocus/6X00P0ANFM.html seems to suggest that it is one of the functions that should be "safe in their nature".

Re: "Yet another html filter" (allowHTML)

sjdev86 — Thu, 11 Feb 2010 10:27:43 -0600

I suppose that one alternative would be to hook into the anti-samy policy file, using xpath to find the approriate rules for the attribute or property. The input value could then be matched against the resulting regex / literal rules.

EDIT: current demo now using anti-samy

Re: "Yet another html filter" (allowHTML)

Gareth Heyes — Thu, 11 Feb 2010 04:03:44 -0600

@sjdev86

Not exploitable currently true but IMO you should do strict whitelists and only allow values that conform to it. A global whitelist should come before it which you do which is pretty cool.

Re: "Yet another html filter" (allowHTML)

sjdev86 — Wed, 10 Feb 2010 06:33:58 -0600

Anti-samy does look good. I've refined the attribute filtering somewhat, although I haven't gone as far as producing a rule for every different attribute / style value.

At this point, something like

test

will still get through the value whitelist (allow letters, numbers, spaces and # % ' , - . _ characters), but I wouldn't have thought that would be exploitable (which is my primary concern for now)?

Re: "Yet another html filter" (allowHTML)

Gareth Heyes — Wed, 10 Feb 2010 04:17:50 -0600

I'd take a look at how the css rules work in anti-samy, it's pretty good:-
http://i8jesus.com:9080/AntiSamyDemoWebApp/test.jsp

Re: "Yet another html filter" (allowHTML)

sjdev86 — Tue, 09 Feb 2010 17:12:59 -0600

Thanks for the hole picking. I'll get on to re-factoring / tightening up the character whitelist (and the decoding has been given the boot from plain text output).

Re: "Yet another html filter" (allowHTML)

Gareth Heyes — Tue, 09 Feb 2010 15:40:32 -0600

You really need to stop autodecoding everything

http://pastebin.ca/1791900

Also your css value whitelist sucks

test

Re: "Yet another html filter" (allowHTML)

sjdev86 — Tue, 09 Feb 2010 12:08:27 -0600

Alright, I'm happy with it now.

The only thing I haven't addressed is mbstring, as I'm still thinking about the best alternative (any thoughts on dealing with the charset welcome).

Re: "Yet another html filter" (allowHTML)

sjdev86 — Mon, 08 Feb 2010 17:53:31 -0600

Getting a bit too close there.

Looks like php's DOMDocument automatically decoded the hex entities, which rendered the encoding check useless. Better get that css whitelist up.

EDIT: initial attempt at css whitelist now in place

Re: "Yet another html filter" (allowHTML)

Anonymous User — Mon, 08 Feb 2010 16:42:29 -0600

Damn - almost gotcha... :)

Re: "Yet another html filter" (allowHTML)

Gareth Heyes — Mon, 08 Feb 2010 13:22:19 -0600

@sjdev86

That's a really good change, double urlencoded data would most probably be bad input

Re: "Yet another html filter" (allowHTML)

sjdev86 — Mon, 08 Feb 2010 10:52:20 -0600

Gives me something to aim for then, doesn't it. ;)

I was just playing around with the decoding options. So far I'm taking the approach that if passing the attribute through the decoding function changes the value in any way, then it's presumed to be bad input and the attribute is removed (otherwise further checks are carried out).

The only situation I can think of where a legitimate user entering html might fall foul of this is if they've copy & pasted a urlencoded link into the attribute value. Other than that, there's no reason for any encoded data to ever end up in an attribute (that I can think of).

test BECOMES test

Re: "Yet another html filter" (allowHTML)

Gareth Heyes — Mon, 08 Feb 2010 08:51:35 -0600

@sjdev86

If you are comparing the input continually then I guess that would be ok to run through htmlspecialchars but try use whitelist wherever possible even in attributes.

I'd also be tempted to avoid certain characters completely like for example just because the HTML spec says you can use x and unlimited characters does it mean you should? Specifications are fine for making things easy to understand and implement but should be ignored whenever their definition makes it easier to exploit your system.

I like your code, if it is improved and you take this approach I'll definitely recommend it after mario, sirdarckcat and thornmaker have broken it first though :)

Re: "Yet another html filter" (allowHTML)

sjdev86 — Mon, 08 Feb 2010 08:13:25 -0600

The decoding function should catch double encoding (since it continues to cycle until nothing changes), however I agree that your approach seems the more sensible one.

I'll re-configure it for a pass / fail approach - if it passes all checks, leave input as is (still run it through htmlspecialchars once passed?), otherwise remove that data entirely.

Re: "Yet another html filter" (allowHTML)

Gareth Heyes — Mon, 08 Feb 2010 08:00:12 -0600

@sjdev86

Yeah whitelist each property and only allow the ones you know about rather than blacklist.

The reason that decode is a mistake is because your filter is potentially creating new vectors by converting. I'd recommend you inspect but do not convert that way you'll avoid potential issues in future like this for example:-
test

Your filter is performing a auto decode of urlencoded data, after that it is then encoding it with html entities. Some vectors include html entities and also can function with double urlencoding I'd recommend you only leave input as it is supplied or remove it if it is dangerous. IMHO Designing a filter like this involves thinking what could potentially break not what breaks currently.

Re: "Yet another html filter" (allowHTML)

sjdev86 — Mon, 08 Feb 2010 07:31:53 -0600

Thanks for the feedback, Gareth.

I'll build in a whitelist for the style attribute (always figured I'd have to if I was going to allow it). I've got mbstring dotted around the place in various components I'm building, so that's going to need re-thinking.

I'm interested in the decoding issue - I was under the (false?) impression that it was a good idea to try and decode the input as much as possible, then check for and neutralise any resulting evil characters afterwards. Bad idea in general, or just in regards to "urlencoded" characters like %22?