OOB Read in WebCore::SVGAnimationElement

• Component: SVG
• Status: RESOLVED FIXED

[Ken Buchanan]

Created attachment 103254 [details]
SVG animation crash repro

Upstreaming bug filed against Chromium: http://code.google.com/p/chromium/issues/detail?id=73030

Seeing crashes from B-Spline animation with certain properties. The provided URL and the attached file crash the renderer in slightly different ways.

This was analyzed for security implications and is not thought to have any, other than simple renderer crash.

I have a fix for this.

[Ken Buchanan]

Created attachment 103258 [details]
Patch

[Chris Evans]

You're missing:
- ChangeLog entries
- The addition of a new test for the bug you've just fixed

You won't get an r+ without these.

[Ken Buchanan]

Created attachment 103357 [details]
Patch

[Ken Buchanan]

(In reply to comment #2)
> You're missing:
> - ChangeLog entries
> - The addition of a new test for the bug you've just fixed
> 
> You won't get an r+ without these.

Better?

...If so, I'll get to finding another reviewer.

[Nikolas Zimmermann]

Comment on attachment 103357 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103357&action=review

Thanks that you're working on this Ken, first round of comments:

> LayoutTests/ChangeLog:5
> +        Merge branch 'master' into SVGbug

What's this?

> LayoutTests/ChangeLog:7
> +        Added some additional checking on on spline animation vector sizes, for potential mismatches with other vectors.

This doesn't say what's actually wrong in trunk. Can you rephrase that?

> LayoutTests/ChangeLog:11
> +

The bug title should come first. Like this:

OOB Read
https:://

Reviewed by NOBODY...

Added testcase covering keySpline array length problem.

> LayoutTests/svg/animations/animate-calcMode-spline-crash-bad-array-length.xhtml:23
> +                    document.body.innerHTML = "PASS";

PASS, if no assertion is triggered in a debug build.
Sounds better, eh?

> Source/WebCore/ChangeLog:5
> +        Merge branch 'master' into SVGbug

See the comments about LayoutTests/ChangeLog, please fix the WebCore ChangeLog as well.

> Source/WebCore/svg/SVGAnimationElement.cpp:459
> +    if (calcMode() == CalcModeSpline && index < m_keySplines.size()) {
>          ASSERT(m_keySplines.size() == m_keyPoints.size() - 1);

What's the condition for index >= m_keySplines.size(), when does that happen?

> Source/WebCore/svg/SVGAnimationElement.cpp:535
> -    if (calcMode == CalcModeSpline) {
> +    if (calcMode == CalcModeSpline && index < m_keySplines.size()) {

Ditto.

> Source/WebCore/svg/SVGAnimationElement.cpp:608
> +    else if (m_keyPoints.isEmpty() && mode == CalcModeSpline && m_keyTimes.size() > 1 && m_keySplines.size() == m_keyTimes.size() - 1)

Shouldn't we rather guarantee that if m_keyTimes.size() > 1, that m_keySplines.size() is always m_keyTimes.size() - 1 ?
If there's such a mismatch we could clear m_keySplines/keyTimes instead of keeping "invalid states" around and checking for them in various places (calculatePercentFromKeyPoints/currentValuesForValuesAnimation/updateAnimation). Seems too fragile for me.

What do you think?

[Ken Buchanan]

(In reply to comment #5)

Thanks for the review.


> What's the condition for index >= m_keySplines.size(), when does that happen?
> 

index is calculated based on the m_keyTimes array, which may or may not match the size of m_keySplines. These are obtained from the user data, so we need somewhere a check that they are in sync. It shouldn't happen but it can with invalid data.

> Shouldn't we rather guarantee that if m_keyTimes.size() > 1, that m_keySplines.size() is always m_keyTimes.size() - 1 ?
> If there's such a mismatch we could clear m_keySplines/keyTimes instead of keeping "invalid states" around and checking for them in various places (calculatePercentFromKeyPoints/currentValuesForValuesAnimation/updateAnimation). Seems too fragile for me.
> 
> What do you think?

This was my first thought, but it seemed awkward doing this in parseMappedAttribute() because you can't necessarily know the order that the keySplines and keyTimes attributes will be parsed. You're probably right that this is safer, though. I'll submit a new patch with this approach.

[Ken Buchanan]

Created attachment 103397 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 103397 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103397&action=review

Almost there, still some tweaks needed.

> Source/WebCore/svg/SVGAnimationElement.cpp:182
> +        if (calcMode() == CalcModeSpline && !m_keySplines.isEmpty() && m_keySplines.size() != m_keyTimes.size() - 1) {
> +            // There is an array size mismatch between keySplines and keyTimes
> +            m_keyTimes.clear();
> +        }

In general, this looks fine - it would be nice to avoid the duplication though, by adding a validateKeyTimes() method that could be called from here.
I still wonder whether parseMappedAttribute is the right place though.

Looking through the code I see stuff like

float SVGAnimationElement::calculatePercentFromKeyPoints(float percent) const
{
    ASSERT(!m_keyPoints.isEmpty());
    ASSERT(calcMode() != CalcModePaced);
    ASSERT(m_keyTimes.size() > 1);
...

and or
    } else if (!m_keyPoints.isEmpty() && mode != CalcModePaced)
        effectivePercent = calculatePercentFromKeyPoints(percent);
    else if (m_keyPoints.isEmpty() && mode == CalcModeSpline && m_keyTimes.size() > 1)
...

It should be easy to trigger an assertion now, when combinining keyTimes/keySplines (with length mismatch) and keyPoints, as this code assumes keyTimes is never empty.
Do you agree that eg. non-paced mode now needs additional logic like (if m_keyTimes.size() > 1) before calling this method.

[Ken Buchanan]

Created attachment 103936 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 103936 [details]
Patch

Looks good!, r=me.

[WebKit Review Bot]

Comment on attachment 103936 [details]
Patch

Clearing flags on attachment: 103936

Committed r93477: <http://trac.webkit.org/changeset/93477>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

What? Why isn't this a security bug if it's really a OOB read bug?  If not, then why does the bug title still says OOB read?

[Ken Buchanan]

(In reply to comment #13)
> What? Why isn't this a security bug if it's really a OOB read bug?  If not, then why does the bug title still says OOB read?

This was initially logged as a security bug in Chromium, but the Chrome security team downgraded it because there's no way to recover the extra read data. It's just a crash bug.