Update BufferReader cursor even when it's unable to read

RESOLVED FIXED in Firefox -esr60

Status

()

defect
P1
normal
RESOLVED FIXED
7 months ago
11 days ago

People

(Reporter: chunmin, Assigned: chunmin)

Tracking

({csectype-bounds, sec-high})

unspecified
mozilla67
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr6066+ fixed, firefox64 wontfix, firefox65 wontfix, firefox66+ fixed, firefox67+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main66+][adv-esr60.6+])

Attachments

(3 attachments, 2 obsolete attachments)

Posted patch update-ptr.diff (obsolete) — Splinter Review

I found that the cursor of BufferReader may be set to a wrong position(maybe out of bound) if BufferReader::Seek() is called just after BufferReader::Read().

Example

Assume a buffer has 10 bytes from address 0 to 10, and we read 4 bytes (by ReadU32()) every time.

  1. At the first, the cursor to read the buffer is set to 0, and the remaining length is 10.
  2. After 2 rounds, the cursor will point to 8.
  3. At the 3 round, the call ReadU32() will fail since there are only 2 bytes left and the remaining length will be set to 0.
    • The Offset() now will be 10 since it's calculated by length - remaining, where length and remaining are 10 and 0 respectively.
    • However, the cursor isn't updated! It's still 8.
  4. When Seek(0) is called, the new cursor will be set to -2
  5. If Read() is called again, we will read something from -2
Group: core-security → media-core-security
Attachment #9037702 - Attachment is obsolete: true
Attachment #9038308 - Flags: review?(kinetik)
Comment on attachment 9038308 [details] [diff] [review]
Part1: Unit test for BufferReader

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

LGTM, some minor style nits

::: dom/media/gtest/TestBufferReader.cpp
@@ +10,5 @@
> +  // Allocate a buffer and create a BufferReader.
> +  const size_t LENGTH = 10;
> +  AutoTArray<uint8_t, LENGTH> buffer;
> +  buffer.SetLength(LENGTH);
> +  memset(buffer.Elements(), 0, LENGTH);

Maybe use PodZero here, or use mfbt/Array instead of AutoTArray and then you can use PodArrayZero.

@@ +23,5 @@
> +  // Keep reading to the end, and make sure the final read failed.
> +  const size_t COUNT = 4;
> +  ASSERT_NE(LENGTH % COUNT, 0);
> +  for (const uint8_t* ptr = reader.Peek(0); ptr != nullptr;
> +       ptr = reader.Read(COUNT));

Move the semicolon to its own line to make this slightly clearer.
Attachment #9038308 - Flags: review?(kinetik) → review+
Comment on attachment 9038309 [details] [diff] [review]
Part2: Update reading cursor when the BufferReader cannot read.

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

LGTM, looks like this is the only place mRemaining is updated without updating mPtr.
Attachment #9038309 - Flags: review?(kinetik) → review+

--assuming-- the offset is attacker-controlled, and the read data ends up in a media stream where it can be recovered by the attacker, this is an info leak.

(In reply to Matthew Gregan [:kinetik] from comment #3)

::: dom/media/gtest/TestBufferReader.cpp
@@ +10,5 @@

  • // Allocate a buffer and create a BufferReader.
  • const size_t LENGTH = 10;
  • AutoTArray<uint8_t, LENGTH> buffer;
  • buffer.SetLength(LENGTH);
  • memset(buffer.Elements(), 0, LENGTH);

Maybe use PodZero here, or use mfbt/Array instead of AutoTArray and then you
can use PodArrayZero.

I think I can just use uint8_t buffer[BUFFER_SIZE] = { 0 }; to do the same job here.

@@ +23,5 @@

  • // Keep reading to the end, and make sure the final read failed.
  • const size_t COUNT = 4;
  • ASSERT_NE(LENGTH % COUNT, 0);
  • for (const uint8_t* ptr = reader.Peek(0); ptr != nullptr;
  •   ptr = reader.Read(COUNT));
    

Move the semicolon to its own line to make this slightly clearer.

Done.

Attachment #9038308 - Attachment is obsolete: true
Attachment #9038928 - Flags: review+

(In reply to Chun-Min Chang[:chunmin] from comment #6)

I think I can just use uint8_t buffer[BUFFER_SIZE] = { 0 }; to do the same job here.

Even better!

Same problem in ByteReader. This patch should fix it in the same way as what part 2 does

Attachment #9038944 - Flags: review?(kinetik)
Comment on attachment 9038944 [details] [diff] [review]
Part3: Update reading cursor when the ByteReader cannot read.

># HG changeset patch
># User Chun-Min Chang <cchang@mozilla.com>
># Date 1548367866 28800
>#      Thu Jan 24 14:11:06 2019 -0800
># Node ID 5c21197815b08c7ec45507473eab730862fcab98
># Parent  c12e3387d1130d8c53780d1e9411e863039999e0
>Bug 1521214 - P3: Update reading cursor when the ByteReader failed reading. r=kinetik
>
>diff --git a/media/psshparser/PsshParser.cpp b/media/psshparser/PsshParser.cpp
>--- a/media/psshparser/PsshParser.cpp
>+++ b/media/psshparser/PsshParser.cpp
>@@ -56,16 +56,17 @@ class ByteReader {
>       MOZ_ASSERT(false);
>       return 0;
>     }
>     return mozilla::BigEndian::readUint32(ptr);
>   }
> 
>   const uint8_t* Read(size_t aCount) {
>     if (aCount > mRemaining) {
>+      mPtr += mRemaining;
>       mRemaining = 0;
>       return nullptr;
>     }
>     mRemaining -= aCount;
> 
>     const uint8_t* result = mPtr;
>     mPtr += aCount;
>
Attachment #9038944 - Attachment description: Part2: Udating reading cursot when ByteReader failed reading. → Part3: Udating reading cursot when ByteReader failed reading.
Attachment #9038944 - Flags: review?(kinetik) → review+
Comment on attachment 9038309 [details] [diff] [review]
Part2: Update reading cursor when the BufferReader cannot read.

># HG changeset patch
># User Chun-Min Chang <cchang@mozilla.com>
># Date 1548184883 28800
>#      Tue Jan 22 11:21:23 2019 -0800
># Node ID eeb27965fd829466a7e5dd073a9dc2887f1a8cb9
># Parent  1a27d213e86970241ef10177c9e0dade06411d83
>Bug 1521214 - P2: Update reading cursor when the BufferReader cannot read. r=kinetik
>
>diff --git a/dom/media/BufferReader.h b/dom/media/BufferReader.h
>--- a/dom/media/BufferReader.h
>+++ b/dom/media/BufferReader.h
>@@ -143,16 +143,17 @@ class MOZ_RAII BufferReader {
>               ("%s: failure", __func__));
>       return mozilla::Err(NS_ERROR_FAILURE);
>     }
>     return mozilla::BigEndian::readInt64(ptr);
>   }
> 
>   const uint8_t* Read(size_t aCount) {
>     if (aCount > mRemaining) {
>+      mPtr += mRemaining;
>       mRemaining = 0;
>       return nullptr;
>     }
>     mRemaining -= aCount;
> 
>     const uint8_t* result = mPtr;
>     mPtr += aCount;
>
Attachment #9038309 - Attachment description: Part2: Udating reading cursot when BufferReader failed reading. → Part2: Update reading cursor when the BufferReader cannot read.
Comment on attachment 9038944 [details] [diff] [review]
Part3: Update reading cursor when the ByteReader cannot read.

># HG changeset patch
># User Chun-Min Chang <cchang@mozilla.com>
># Date 1548367866 28800
>#      Thu Jan 24 14:11:06 2019 -0800
># Node ID 5c21197815b08c7ec45507473eab730862fcab98
># Parent  c12e3387d1130d8c53780d1e9411e863039999e0
>Bug 1521214 - P3: Update reading cursor when the ByteReader cannot read. r=kinetik
>
>diff --git a/media/psshparser/PsshParser.cpp b/media/psshparser/PsshParser.cpp
>--- a/media/psshparser/PsshParser.cpp
>+++ b/media/psshparser/PsshParser.cpp
>@@ -56,16 +56,17 @@ class ByteReader {
>       MOZ_ASSERT(false);
>       return 0;
>     }
>     return mozilla::BigEndian::readUint32(ptr);
>   }
> 
>   const uint8_t* Read(size_t aCount) {
>     if (aCount > mRemaining) {
>+      mPtr += mRemaining;
>       mRemaining = 0;
>       return nullptr;
>     }
>     mRemaining -= aCount;
> 
>     const uint8_t* result = mPtr;
>     mPtr += aCount;
>
Attachment #9038944 - Attachment description: Part3: Udating reading cursot when ByteReader failed reading. → Part3: Update reading cursor when the ByteReader cannot read.

Comment on attachment 9038309 [details] [diff] [review]
Part2: Update reading cursor when the BufferReader cannot read.

Security Approval Request

How easily could an exploit be constructed based on the patch?

It's easy to find what's wrong if they check the code in part1 (test case) as well. However, they need to create a media file with customized size and have a way to break our the media playback pipeline to get something from this bug

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yes

Which older supported branches are affected by this flaw?

All

If not all supported branches, which bug introduced the flaw?

None

Do you have backports for the affected branches?

No

If not, how different, hard to create, and risky will they be?

It should be very easy since there is no big change in the modified files for over one year.

How likely is this patch to cause regressions; how much testing does it need?

It's a small change for a special corner case. It should not cause regression.

Attachment #9038309 - Flags: sec-approval?

I don't see how this could be a security risk nor why it's a problem to not update the pointer as the read would have returned nullptr which is a fatal error in all use.

At the end of the day, what matters is that you can't read past the end of the buffer. You will read invalid content, but that will happen regardless if you attempt to read too much

What I'm more curious about however, is why the pssh code uses its own Byte Reader, that's duplicated code for nothing.

I don't think this patch should go in as is

(In reply to Jean-Yves Avenard [:jya] from comment #13)

I don't see how this could be a security risk nor why it's a problem to not update the pointer as the read would have returned nullptr which is a fatal error in all use.

At the end of the day, what matters is that you can't read past the end of the buffer. You will read invalid content, but that will happen regardless if you attempt to read too much

For bug 1500713, the MP3 files should be able to be parsed twice. The first time parsing is to search a Xing header. If nothing can be found, the parser should seek backward to the beginning of the file and then search for a VBRI header. If the reading cursor is wrong after seeking backward to the beginning of the file, the VBRI header cannot be recognized.

Marking P1 based on sec-high rating.

Priority: -- → P1

(In reply to Jean-Yves Avenard [:jya] from comment #14)

What I'm more curious about however, is why the pssh code uses its own Byte Reader, that's duplicated code for nothing.

I don't think this patch should go in as is

I prefer just making a small change on the original code and open other bug to replace ByteReader with BufferReader if needed, since no security risk on replacing.

Comment on attachment 9038309 [details] [diff] [review]
Part2: Update reading cursor when the BufferReader cannot read.

Sec-approval+ for the primary patch. We shouldn't check in the test until this ships fixed in a release though.
Attachment #9038309 - Flags: sec-approval? → sec-approval+

(In reply to Al Billings [:abillings] from comment #18)

Comment on attachment 9038309 [details] [diff] [review]
Part2: Update reading cursor when the BufferReader cannot read.

Sec-approval+ for the primary patch. We shouldn't check in the test until
this ships fixed in a release though.

checkin-needed: According comment 18, please only checking the part2. Thanks!

Keywords: checkin-needed
Comment on attachment 9038309 [details] [diff] [review]
Part2: Update reading cursor when the BufferReader cannot read.

># HG changeset patch
># User Chun-Min Chang <cchang@mozilla.com>
># Date 1548184883 28800
>#      Tue Jan 22 11:21:23 2019 -0800
># Node ID eeb27965fd829466a7e5dd073a9dc2887f1a8cb9
># Parent  1a27d213e86970241ef10177c9e0dade06411d83
>Bug 1521214: Update reading cursor when the BufferReader failed reading. r?kinetik
>
>diff --git a/dom/media/BufferReader.h b/dom/media/BufferReader.h
>--- a/dom/media/BufferReader.h
>+++ b/dom/media/BufferReader.h
>@@ -143,16 +143,17 @@ class MOZ_RAII BufferReader {
>               ("%s: failure", __func__));
>       return mozilla::Err(NS_ERROR_FAILURE);
>     }
>     return mozilla::BigEndian::readInt64(ptr);
>   }
> 
>   const uint8_t* Read(size_t aCount) {
>     if (aCount > mRemaining) {
>+      mPtr += mRemaining;
>       mRemaining = 0;
>       return nullptr;
>     }
>     mRemaining -= aCount;
> 
>     const uint8_t* result = mPtr;
>     mPtr += aCount;
>
Group: media-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Please request beta/esr60 approval when you get a chance.

Flags: needinfo?(cchang)

Comment on attachment 9038309 [details] [diff] [review]
Part2: Update reading cursor when the BufferReader cannot read.

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

None

User impact if declined

Potential users data leak

Is this code covered by automated tests?

No

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

This is just a small one-line change for a corner use case that I am working on (bug 1500713). The current code won't be affected.

String changes made/needed

ESR Uplift Approval Request

If this is not a sec:{high,crit} bug, please state case for ESR consideration

User impact if declined

Potential users data leak

Fix Landed on Version

nightly

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

This is just a small one-line change for a corner use case that I am working on (bug 1500713). The current code won't be affected.

String or UUID changes made by this patch

Flags: needinfo?(cchang)
Attachment #9038309 - Flags: approval-mozilla-esr60?
Attachment #9038309 - Flags: approval-mozilla-beta?
Comment on attachment 9038309 [details] [diff] [review]
Part2: Update reading cursor when the BufferReader cannot read.

Fix for sec-high issue, ok to uplift for beta 6.
Attachment #9038309 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]

Whatever happened with Part 3 here? Is that patch still needed somewhere?

Flags: needinfo?(cchang)
Comment on attachment 9038309 [details] [diff] [review]
Part2: Update reading cursor when the BufferReader cannot read.

Approving this patch for 60.6esr anyway to fix the sec issue. At this point, if Part 3 is still relevant, we should probably move it into a new bug to avoid further confusion in this one.
Attachment #9038309 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+

(In reply to Ryan VanderMeulen [:RyanVM] from comment #27)

Whatever happened with Part 3 here? Is that patch still needed somewhere?

Part 3 has no risk issue so I think we can leave it here.

Flags: needinfo?(cchang)
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main66+][adv-esr60.6+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.