Closed Bug 974017 Opened 6 years ago Closed 6 years ago

[webvtt] Limit the line cue setting to the range of -1000~1000

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: reyre, Assigned: reyre)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-dos)

Attachments

(1 file, 1 obsolete file)

I've reopened the spec bug (https://www.w3.org/Bugs/Public/show_bug.cgi?id=22708) for this since having a very large line setting impacts the step algorithm making it very, very slow. I'm taking steps to do this in vtt.js right now and not waiting for the spec to catch up, since WebVTT is set to have its pref flipped in Firefox.

Maybe it should be even lower.
Other than making the implementation slow is there anything beyond a denial of service attack here? Doesn't seem like it needs to be a hidden bug if that's all it is.
Keywords: csectype-dos
Yeah, it will just make it slow, possibly very slow, depending on if the line setting is large enough.
Very slow at the request of the page author barring XSS attacks. I'm fine with opening this bug.
Group: core-security
The specification community group has asked me to see if I can optimize this algorithm instead of limiting the value. So I'll try to do that first. I think there shouldn't be a problem with it.
Attached patch Update vtt.js r=rillian (obsolete) — Splinter Review
This implements an optimization for large line settings instead of clamping the line value at some arbitrary integer.
Attachment #8404892 - Flags: review?(giles)
Comment on attachment 8404892 [details] [diff] [review]
Update vtt.js r=rillian

Review of attachment 8404892 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like an improvement to me! Do we have any tests for this?

::: content/media/webvtt/vtt.jsm
@@ +129,5 @@
>      // Accept a setting if its a valid (signed) integer.
>      integer: function(k, v) {
>        if (/^-?\d+$/.test(v)) { // integer
>          // Only take values in the range of -1000 ~ 1000
> +        this.set(k, parseInt(v, 10));

Comment needs updating too, no? The limit no longer happens here.

@@ +856,5 @@
> +      var step = this.lineHeight,
> +          maxSteps = Math.floor((options.container[axes.ref] / step) + 1);
> +      if (maxSteps < (toMove / step)) {
> +        toMove = maxSteps * step;
> +      }

toMove and container are both in px rather than steps, so it might be cleaner to use maxMove instead of maxSteps here instead of converting to steps and then back.
Attachment #8404892 - Flags: review?(giles) → review+
Thanks for the quick review.

(In reply to Ralph Giles (:rillian) from comment #6)

> Looks like an improvement to me! Do we have any tests for this?

We have a few tests in vtt.js that test that the result is still the same as the regular algorithm, but we don't have any tests to make sure the optimization is actually happening. I've verified that manually for now. I'm not sure how we would be able to accomplish that since this code isn't really exposed to our test suite. We might be able to run some kind of time based test and make sure it's not over a certain amount... possibly. I'll open a bug for that on the GitHub repo.

> ::: content/media/webvtt/vtt.jsm
> @@ +129,5 @@
> >      // Accept a setting if its a valid (signed) integer.
> >      integer: function(k, v) {
> >        if (/^-?\d+$/.test(v)) { // integer
> >          // Only take values in the range of -1000 ~ 1000
> > +        this.set(k, parseInt(v, 10));
> 
> Comment needs updating too, no? The limit no longer happens here.

Good catch, I'll update.

> @@ +856,5 @@
> > +      var step = this.lineHeight,
> > +          maxSteps = Math.floor((options.container[axes.ref] / step) + 1);
> > +      if (maxSteps < (toMove / step)) {
> > +        toMove = maxSteps * step;
> > +      }
> 
> toMove and container are both in px rather than steps, so it might be
> cleaner to use maxMove instead of maxSteps here instead of converting to
> steps and then back.

Makes sense. I'll update.
Thanks for review Ralph.

Carrying forward r=rillian.
Attachment #8404892 - Attachment is obsolete: true
I've filed the bug to test line setting optimization as https://github.com/mozilla/vtt.js/issues/308.
https://hg.mozilla.org/mozilla-central/rev/3abaf19dcbff
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.