On Thursday 20 August 2009 23:19:11 Ashish Kurmi wrote:
> Hi toad,
> I have written comments for each function. Please have a look and tell me if
> you need any other information.
Okay, this begins to make sense.
"a" means || operator between different auxiliary expressions. || operator is
defined by the CSS spec:
# A double bar (||) separates two or more options: one or more of them must
occur, in any order.
So would it be fair to say that recursiveDoubleBarVerifier really doesn't have
to be recursive, that we could just apply each auxiliary expression to each
value part? (And no, I'm not asking you to do that, just to check my logic).
Also, is there any fundamental reason you are passing space separated strings
of values around, rather than String arrays? As I mentioned below, I believe
this will cause bugs, if values can be double quoted ... keywords can't be, but
strings and URLs can contain spaces. The possibility of spaces inside quotes
inside url() means that recursiveParserExpressionVerifier likely fails in such
cases:
background: url("../my image.png");
The double quote removal loop at the top of recursiveParserExpressionVerifier
will strip the quotes, so we have:
first value token: url()
second value token: ../my image.png
Then we look up background and get the expression 6a7a8a9a10
So we handle the a's. This means passing to recursiveDoubleBarVerifier:
url() ../my image.png
Which will be split up again on the basis of spaces, and will fail. There is no
way to introduce a // so I am fairly sure it's not exploitable, but it is
broken.
So IMHO we need to:
- Tokenise values into String[] based on spaces but ignoring spaces inside
String's.
- Exclusively pass around String[] after that.
Thoughts?
I appreciate that you might not have time to finish it now, I am willing to do
the necessary fixes myself, especially if you are planning to finish the SVG
filter (are you?), but I'd appreciate comments on the above. Thanks!
>
> Thanks and best regards,
> Ashish Deviprasad Kurmi
>
> On Thu, Aug 20, 2009 at 1:02 AM, Matthew Toseland <toad at
> amphibian.dyndns.org
> > wrote:
>
> > I have merged the BMP filter, and the (minor) XHTML-related changes to the
> > HTML filter. The CSS filter looks interesting, certainly much work has gone
> > into it and when it is fully debugged, documented and merged it should be a
> > major improvement. However, it is none of these things as far as I can see.
> > I have been unable to complete a code review, but there are several obvious
> > issues:
> > - The code should not be based on the old generated filter as it is
> > unreadable, and is generated. I have tried to merge it by just copying the
> > files over to avoid this.
> > - The recursiveParserExpressionVerifier and functions it calls are
> > particularly difficult to understand. Could you please document the format
> > of the parse expressions in a comment?
> > - I suspect that there is a problem with spaces here, if some of the values
> > are quoted and contain spaces, then sticking them back together with spaces
> > to pass into recursiveDoubleBarVerifier and then splitting them up again
> > will cause problems, no? But apart from that, most CSS value parts aren't
> > supposed to be quoted in the first place?
> > - The lazy init for elementVerifiers etc is overkill imho.
> > - I am uncertain whether it is a good thing or a bad thing to only allow
> > URIs which don't get changed by the callback.
> > - Charset detection is broken, and this may be exploitable; certainly it
> > will break some pages. Section 4.4 of the spec syntax page gives rules for
> > determining the charset.
> > - We implemented one vendor specific extension in the old code.
> > - Some very wierd things are valid with strings, but I think your tokeniser
> > deals with this...
> > - Misuse of static: you put the callback onto the static
> > CSSPropertyVerifier in the constructor, this will break any time multiple
> > CSS filters with different callbacks are run in parallel.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 835 bytes
Desc: This is a digitally signed message part.
URL:
<https://emu.freenetproject.org/pipermail/devl/attachments/20090821/9cd4b049/attachment.pgp>
