306088 - SVG security review: Parsing

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

T Rowley mentioned that this is part of the SVG attack surface and could do with
a security review:

  * Parsing, pretty much all of
    content/svg/content/src/*.cpp::SetValueString,
    but in particular these:

    * tranform list - nsSVGTransformList::SetValueString
http://lxr.mozilla.org/mozilla/source/content/svg/content/src/nsSVGTransformList.cpp#129

    * path "d" attribute - nsSVGPathDataParser.cpp
http://lxr.mozilla.org/mozilla/source/content/svg/content/src/nsSVGPathDataParser.cpp

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: simplified nsSVGTransformList::SetValueString

The majority of SetValueString's look fine; there's a lot of places where I
wish we had better string APIs to avoid having to do pointer monkey business,
but it's probably faster in the long run.  However, I ended up rewriting
nsSVGTransformList::SetValueString in terms of a helper function for parsing
parameters, to make it clearer -- the original implementation had way too much
repetition of the same code, and also didn't error out in case more than the
necessary number of parameters was given to a transform.

The path segment parsing code also looked fine to me. (Could be simplified a
bit, there was a lot of repetition of the same whitespace-eating loop, but not
relevant from a security point of view.)

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

In on trunk, asking for a+ for branch.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 194512 [details] [diff] [review]
simplified nsSVGTransformList::SetValueString

(Either 1.8b4/1.8b5)

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 194512 [details] [diff] [review]
simplified nsSVGTransformList::SetValueString

>   const char* delimiters1 = "\x20\x9\xD\xA,(";
>   const char* delimiters2 = "()";
>-  const char* delimiters3 = "\x20\x9\xD\xA,";

These (delimiters3 in its new name and location) should all be const char [],
not const char *.

>-      char* arg2 = nsCRT::strtok(args, delimiters3, &args);
>-      char* end;
>-      float tx = (float) PR_strtod(arg1, &end);
>-      float ty = arg2 ? (float) PR_strtod(arg2, &end) : 0.0f;

Do we care if there's junk at the end (if *end is not '\0' on return from
PR_strtod)?

>+  while ((arg = nsCRT::strtok(argrest, arg_delimiters, &argrest))) {
>+    if (num_args_found < nvars) {
>+      f = (float) PR_strtod(arg, &argend);
>+      if (arg == argend)
>+        return -1;
>+
>+      vars[num_args_found] = f;
>+    }
>+
>+    arg = argrest;
>+    num_args_found++;
>+  }

Seems like arbitrary non-delimiter, non-double-literal chars could start at
argend, and continue up to the next delimiter.	What does the SVG spec require
for such malformed inputs?

/be

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #4)
> (From update of attachment 194512 [details] [diff] [review] [edit])
> >   const char* delimiters1 = "\x20\x9\xD\xA,(";
> >   const char* delimiters2 = "()";
> >-  const char* delimiters3 = "\x20\x9\xD\xA,";
> 
> These (delimiters3 in its new name and location) should all be const char [],
> not const char *.

This same thing occurs in a number of other places in the code; we can probably
do a followup patch to change it en-masse.

> >-      char* arg2 = nsCRT::strtok(args, delimiters3, &args);
> >-      char* end;
> >-      float tx = (float) PR_strtod(arg1, &end);
> >-      float ty = arg2 ? (float) PR_strtod(arg2, &end) : 0.0f;
> 
> Do we care if there's junk at the end (if *end is not '\0' on return from
> PR_strtod)?
> 
> >+  while ((arg = nsCRT::strtok(argrest, arg_delimiters, &argrest))) {
> >+    if (num_args_found < nvars) {
> >+      f = (float) PR_strtod(arg, &argend);
> >+      if (arg == argend)
> >+        return -1;
> >+
> >+      vars[num_args_found] = f;
> >+    }
> >+
> >+    arg = argrest;
> >+    num_args_found++;
> >+  }
> 
> Seems like arbitrary non-delimiter, non-double-literal chars could start at
> argend, and continue up to the next delimiter.  What does the SVG spec require
> for such malformed inputs?

Not sure; anyone who can read the spec better than I can know?  From
http://www.w3.org/TR/SVG/coords.html#TransformAttribute it looks like such junk
should be invalid.  I can add "|| *argend == '\0'" to the -1 return clause to
catch that case.

Comment 6

[tor]

I read the spec as requiring us to be strict - values with garbage data should
be in error.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ok, checked in with added || argend != '\0' check on branch and trunk.