Christian Holler (:decoder) Reporter
	Description • 7 years ago

The following code performs unchecked arithmetic operations on integral data types with the result being used in memset:

https://dxr.mozilla.org/mozilla-central/rev/7ebcd45634eef3711dccf68e4e1390134d48b63b/dom/media/encoder/OpusTrackEncoder.cpp#348

Checking the limits on the variables, it seems that mChannels can have a value of 2, sizeof(AudioDataValue) a value of 2 or 4 depending on the datatype used, and frameToCopy can be INT32_MAX on 32-bit. The last variable seems to be capped by chunk duration, which is StreamTime:

https://dxr.mozilla.org/mozilla-central/rev/7ebcd45634eef3711dccf68e4e1390134d48b63b/dom/media/AudioSegment.h#164

That again seems to be limited to 

> const int64_t TRACK_RATE_MAX_BITS = 20;
> const int64_t TRACK_TICKS_MAX = INT64_MAX >> TRACK_RATE_MAX_BITS;
> const int64_t MEDIA_TIME_MAX = TRACK_TICKS_MAX;

So that should be enough to cause an overflow in the memset at least on 32 bit. That again leaves some of the data uninitialized, so I guess it is possible that some of that uninitialized data can be read through the encoder API?

I also wonder why the code uses int to get a duration that is potentially larger than INT32_MAX, but that is a different bug most likely.

We should probably use CheckedInt to ensure that there is no overflow happening.

If the analysis here isn't correct and this code is indeed safe, then we should add an annotation for the static analysis to ignore this particular code.

(This is part of a custom static analysis we have been working on in bug 1279569).

	Randell Jesup [:jesup] (needinfo me)
	Comment 1 • 7 years ago

Bryce, you may want to look at this.

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail Assignee
	Comment 2 • 7 years ago

Roger, I'll put this at the top of my queue.

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail Assignee
	Comment 3 • 7 years ago

I've had a look at the code, and think it's best to use a CheckedInt and be defensive. The code for durations is complicated enough for me that I can't figure out if this is an issue or not, but regardless, that code may change, and I'd rather see an explicit check to prevent issue.

I agree with there being some other typing oddities in that function, that I'd like to clean up while I'm there. However, as noted, that's probably best fixed in another bug which I can look at after this is sorted.

This is my first secure bug that I'm submitting code to. So I'd appreciate if someone could let me know/point me to the docs as to what the appropriate mechanism for submitting a patch, and for landing this against existing releases. I've got a patch locally that I want to eyeball some more, but think is good to go shortly for review.

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail Assignee
	Comment 4 • 7 years ago

For very long chunks, the opus encoder could run into issues with processing.
This changeset seeks to address that by using CheckedInt to prevent a potential
int overflow when encountering extremely long durations.

	Randell Jesup [:jesup] (needinfo me)
	Comment 5 • 7 years ago

btw, you might need to include CheckedInt.h

since it's not a sec-high or sec-crit, you can land normally. if it was high/crit, you need security-approval

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail Assignee
	Comment 6 • 7 years ago

For very long chunks, the opus encoder could run into issues with processing.
This changeset seeks to address that by using CheckedInt to prevent a potential
int overflow when encountering extremely long durations.

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail Assignee
	Comment 7 • 7 years ago

Patch updated with explicit import. Wasn't sure how bzexport would handle the reviewer. Looks like I've ended up doing another request -_-

Is this something that should just be left to ride the trains due to the security level?

	Timothy B. Terriberry (:derf)
	Comment 8 • 7 years ago

I don't have an issue with adding CheckedInt operations here, but I don't think it solves the real problem.

This code:
      if (frameCopied + frameToCopy > framesToFetch) {
        frameToCopy = framesToFetch - frameCopied;
      }

tries to bound frameToCopy by framesToFetch (frameCopied lies between 0 and framesToFetch).

framesToFetch is set here:

    const int framesToFetch = !mResampler ? GetPacketDuration()
                              : (GetPacketDuration() - framesLeft) * mSamplingRate / kOpusSamplingRate
                              + frameRoundUp;

mSamplingRate is bounded between 8000 and 192000. kOpusSamplingRate is 48000, and GetPacketDuration() bounded by 20 ms @ 48 kHz = 960 samples. frameRoundUp is bounded by the resampling ratio. So the worst case is when mSamplingRate is 192000 which gives framesToFetch < 4*960 + 4 < 3844.

So, the only way overflow can occur is
a) if frameCopied + frameToCopy overflows, resulting in an incorrect comparison. This is easily solved by comparing against framesToFetch - frameToCopy (the same as in the subsequent assignment).

b) if chunk.GetDuration() is greater than 2 billion samples to start with. This is easily solved by changing the type to StreamTime.

It may happen that if (a) occurs, the CheckedInt operation will _also_ overflow (I think that's true in all cases, but I haven't tried to prove it), but it seems like an indirect way to go about fixing the actual problem, and doesn't solve the overflow in (b) (which was also called out in comment #0).

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail Assignee
	Comment 9 • 7 years ago

derf: Agreed. My goal with the above patch is to fix the problematic memset call. My reading of the code with patch is that we may still overflow, but we will not overflow into the memset (if I'm wrong about the memset, let me know, as it means I'm reading both what has been said and the code wrong). I have a WIP changeset addressing the more general overflows including further on in the function, but it's taking me a little time to walk code paths to figure out the details such as those in comment #8.

Would you like to see this patch expanded to cover these? Do you think (a) and (b) are the only areas that need be resolved? I.e. once these checks are made safe there's no possible overflow later, such as during resampling?

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail Assignee
	Comment 10 • 7 years ago

This changeset updates the handling of the variables based on
chunk.GetDuration() within the OpusTrackEncoder. This changeset alters places
where overflow could have taken place previously. It also adds asserts with
the dual purpose of defense and documentation for future developers.

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail Assignee
	Comment 11 • 7 years ago

Further patch seeks to address the issues highlighted above. If I understand the code correctly, I don't think we should ever run into the memset issue with the changes made, but I've left the CheckedInt in with a comment and an MOZ_ASSERT_UNREACHABLE to be defensive. I've also added a couple of asserts with comments to highlight this:

> mSamplingRate is bounded between 8000 and 192000. kOpusSamplingRate is 48000, and GetPacketDuration() bounded by 20 ms @ 48 kHz = 960 samples. frameRoundUp is bounded by the resampling ratio. So the worst case is when mSamplingRate is 192000 which gives framesToFetch < 4*960 + 4 < 3844.

Aside from making sure the values are sane, these asserts serve to document the expected maximal values for people less familiar with the code (such as myself coming into this).

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail Assignee
	Comment 12 • 7 years ago

Comment on attachment 8853087 [details] [diff] [review]
Update handling of values derived from chunk duration in OpusTrackEncoder

># HG changeset patch
># User Bryce Van Dyk <bvandyk@mozilla.com>
># Date 1490900257 -46800
>#      Fri Mar 31 07:57:37 2017 +1300
># Node ID 4a1253a6fac1c1976e69f70436236c3feee267a1
># Parent  fdbc277dbcc49c3a6785bfefe4ba7d80e03817d2
>Bug 1349421 - Update handling of values derived from chunk duration in OpusTrackEncoder. r?derf
>
>This changeset updates the handling of the variables based on
>chunk.GetDuration() within the OpusTrackEncoder. This changeset alters places
>where overflow could have taken place previously. It also adds asserts with
>the dual purpose of defense and documentation for future developers.
>
>diff --git a/dom/media/encoder/OpusTrackEncoder.cpp b/dom/media/encoder/OpusTrackEncoder.cpp
>--- a/dom/media/encoder/OpusTrackEncoder.cpp
>+++ b/dom/media/encoder/OpusTrackEncoder.cpp
>@@ -330,41 +330,52 @@ OpusTrackEncoder::GetEncodedTrack(Encode
>     pcm.SetLength(GetPacketDuration() * mChannels);
>     AudioSegment::ChunkIterator iter(mSourceSegment);
>     int frameCopied = 0;
> 
>     while (!iter.IsEnded() && frameCopied < framesToFetch) {
>       AudioChunk chunk = *iter;
> 
>       // Chunk to the required frame size.
>-      int frameToCopy = chunk.GetDuration();
>-      if (frameCopied + frameToCopy > framesToFetch) {
>+      StreamTime frameToCopy = chunk.GetDuration();
>+      if (frameToCopy > framesToFetch - frameCopied) {
>         frameToCopy = framesToFetch - frameCopied;
>       }
>+      // Possible greatest value of framesToFetch = 3844: see
>+      // https://bugzilla.mozilla.org/show_bug.cgi?id=1349421#c8. frameToCopy
>+      // should not be able to exceed this value.
>+      MOZ_ASSERT(frameToCopy <= 3844, "frameToCopy exceeded expected range");
> 
>       if (!chunk.IsNull()) {
>         // Append the interleaved data to the end of pcm buffer.
>         AudioTrackEncoder::InterleaveTrackData(chunk, frameToCopy, mChannels,
>                                                pcm.Elements() + frameCopied * mChannels);
>       } else {
>         CheckedInt<int> memsetLength = CheckedInt<int>(frameToCopy) *
>                                        mChannels *
>                                        sizeof(AudioDataValue);
>         if (!memsetLength.isValid()) {
>-          LOG(LogLevel::Error, ("Error calculating length of pcm buffer!"));
>+          // This should never happen, but we use a defensive check because
>+          // we really don't want a bad memset
>+          MOZ_ASSERT_UNREACHABLE("memsetLength invalid!");
>           return NS_ERROR_FAILURE;
>         }
>         memset(pcm.Elements() + frameCopied * mChannels, 0,
>                memsetLength.value());
>       }
> 
>       frameCopied += frameToCopy;
>       iter.Next();
>     }
> 
>+    // Possible greatest value of framesToFetch = 3844: see
>+    // https://bugzilla.mozilla.org/show_bug.cgi?id=1349421#c8. frameCopied
>+    // should not be able to exceed this value.
>+    MOZ_ASSERT(frameCopied <= 3844, "frameCopied exceeded expected range");
>+
>     RefPtr<EncodedFrame> audiodata = new EncodedFrame();
>     audiodata->SetFrameType(EncodedFrame::OPUS_AUDIO_FRAME);
>     int framesInPCM = frameCopied;
>     if (mResampler) {
>       AutoTArray<AudioDataValue, 9600> resamplingDest;
>       // We want to consume all the input data, so we slightly oversize the
>       // resampled data buffer so we can fit the output data in. We cannot really
>       // predict the output frame count at each call.

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail Assignee
	Comment 13 • 7 years ago

Whoops, that's not what I had intended -_- There's a missing semicolon in the submitted second patch. Fixed version incoming.

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail Assignee
	Comment 14 • 7 years ago

This changeset updates the handling of the variables based on
chunk.GetDuration() within the OpusTrackEncoder. This changeset alters places
where overflow could have taken place previously. It also adds asserts with
the dual purpose of defense and documentation for future developers.

	Timothy B. Terriberry (:derf)
	Comment 15 • 7 years ago

Comment on attachment 8853126 [details] [diff] [review]
Update handling of values derived from chunk duration in OpusTrackEncoder

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

> Would you like to see this patch expanded to cover these?

Yes, thanks for doing so. My concern was that, although you were ensuring the value passed to memset() did not exceed 2 GB, that didn't directly guarantee that it would fit in the actual size of the pcm array (it might still have provided that guarantee by accident, though). As we all know, writing too many zero bytes can be bad: https://googleprojectzero.blogspot.com/2014/08/the-poisoned-nul-byte-2014-edition.html

> Do you think (a) and (b) are the only areas that need be resolved? I.e. once these checks are made safe there's no possible overflow later, such as during resampling?

Yes, I think that's correct. Opus generally operates on quite bounded units (packets here are 20 ms --- the format doesn't even support anything larger than 120 ms, the sampling rates are restricted to reasonable ranges, an encoded 20 ms packet will never be larger than 1276 bytes, etc.). That's why I was surprised to see this report and looked a little more closely. Thanks for the patch!

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail Assignee
	Comment 16 • 7 years ago

Thanks for the feedback!

I will land this shortly. Please let me know if there are any other concerns, or if there's any other steps I need take.

Apologies for the spam in the bug, getting used to the bzexport workflow.

	Timothy B. Terriberry (:derf)
	Comment 17 • 7 years ago

> (it might still have provided that guarantee by accident, though)

Thinking a little harder, I don't think it did. If the overflow in (b) occurred, it could set frameToCopy to a small, negative number, which would pass the check in (a). By normal C type promotion rules, this would get promoted to size_t when multiplied by sizeof(AudioDataValue), but with CheckedInt<int>, it gets coerced to the same type as the template (after checking that the value is in range for the template type, but that's not a problem here). So the result of the multiply is still a small, negative value, with no overflow. This then gets implicitly converted to a size_t when passed to memset(), which makes it a large, positive value, and we write well past the end of the array (almost certainly triggering a clean crash with a write this large, though).

I didn't look into the MSG code to determine if we can really add a single silence chunk that is roughly 6 hours long (at 192 kHz), which is what would be required to get the small negative value in (b), though.

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail Assignee
	Comment 18 • 7 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb5729592e7d80f3841c3d294bfa6035970d1162
Bug 1349421 - Adjust handling of chunk duration in opus encoder. r=jesup

https://hg.mozilla.org/integration/mozilla-inbound/rev/591140c0d411b596753c87c59087d03ee8356a5b
Bug 1349421 - Update handling of values derived from chunk duration in OpusTrackEncoder. r=derf

	Carsten Book [:Tomcat]
	Comment 19 • 7 years ago

https://hg.mozilla.org/mozilla-central/rev/fb5729592e7d
https://hg.mozilla.org/mozilla-central/rev/591140c0d411

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 7 years ago

Should we consider backporting this or can it ride the 55 train?

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail Assignee
	Comment 21 • 7 years ago

:jesup, what do you think? I think backporting is low risk as the change is small, but at the same time if we don't think this is likely to be an issue (sec-low) then we may as well let it ride.

	Randell Jesup [:jesup] (needinfo me)
	Comment 22 • 7 years ago

I'd be good with Aurora/54 for this.  Low risk.  not beta.

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 7 years ago

Doesn't sound worth considering for ESR backport either.

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail Assignee
	Comment 24 • 7 years ago

Comment on attachment 8852782 [details] [diff] [review]
Adjust handling of chunk duration in opus encoder

Approval Request Comment
[Feature/Bug causing the regression]: This is not a regression.
[User impact if declined]: Uninitialised memory may be leaked from the opus encoder via the MediaRecorder API.
[Is this code covered by automated tests?]: The bug being fixed here (possible data leak) is not covered by tests.
[Has the fix been verified in Nightly?]: The fix has been applied and does not break nightly. We do not have a test case to show leaking of data.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: One more patch on same bug to follow.
[Is the change risky?]: I do not believe so.
[Why is the change risky/not risky?]: Not risky based on being a small change. Multiple reviews of patches.
[String changes made/needed]: No.

	Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail Assignee
	Comment 25 • 7 years ago

Comment on attachment 8853126 [details] [diff] [review]
Update handling of values derived from chunk duration in OpusTrackEncoder

Approval Request Comment
As per previous patch request on same bug.

	Gerry Chang [:gchang]
	Comment 26 • 7 years ago

Comment on attachment 8852782 [details] [diff] [review]
Adjust handling of chunk duration in opus encoder

Fix a security issue. Aurora54+.

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 7 years ago
uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/d5a55164125f
https://hg.mozilla.org/releases/mozilla-aurora/rev/6fdfffd25064

	Christian Holler (:decoder) Reporter
	Comment 28 • 7 years ago

Why did we mark this sec-low in the first place? Exposing uninitialized memory is potentially sec-high.

	Randell Jesup [:jesup] (needinfo me)
	Comment 29 • 7 years ago

(In reply to Christian Holler (:decoder) from comment #28)
> Why did we mark this sec-low in the first place? Exposing uninitialized
> memory is potentially sec-high.

The data isn't directly exposed - Opus is a lossy encoder, and also random memory doesn't "look" like audio so much of the data (bits) will be thrown away by the encoder (it will look like noise).  Yes, you can get some bits out of it - but very few and very fuzzy bits.

Also it will be somewhere between hard and impossible to cause such an overflow that leads to this.