1804564 - (CVE-2023-25732) Write beyond bounds in EncodeInputStream()

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[mozillabugs]

EncodeInputStream() (xpcom/io/base64.cpp) repeatedly reads from the specified stream and encodes into aDest until the stream is exhausted. However, the function uses the requested input data length aCount or the (then-current) stream length count64 returned by aInputStream->Available() to calculate how much memory to allocate for the output buffer aDest (line 209, below). If aInputStream can return additional data beyond the specified input data length -- which is permissible for nsIInputStream objects -- EncodeInputStream() will write beyond bounds:

  186: template <typename T>
  187: nsresult EncodeInputStream(nsIInputStream* aInputStream, T& aDest,
  188:                            uint32_t aCount, uint32_t aOffset) {
  189:   nsresult rv;
  190:   uint64_t count64 = aCount;
  191: 
  192:   if (!aCount) {
  193:     rv = aInputStream->Available(&count64);
  194:     if (NS_WARN_IF(NS_FAILED(rv))) {
  195:       return rv;
  196:     }
  197:     // if count64 is over 4GB, it will be failed at the below condition,
  198:     // then will return NS_ERROR_OUT_OF_MEMORY
  199:     aCount = (uint32_t)count64;
  200:   }
  201: 
  202:   const auto base64LenOrErr = CalculateBase64EncodedLength(count64, aOffset);
  203:   if (base64LenOrErr.isErr()) {
  204:     // XXX For some reason, it was NS_ERROR_OUT_OF_MEMORY here instead of
  205:     // NS_ERROR_FAILURE, so we keep that.
  206:     return NS_ERROR_OUT_OF_MEMORY;
  207:   }
  208: 
  209:   if (!aDest.SetLength(base64LenOrErr.inspect(), mozilla::fallible)) {
  210:     return NS_ERROR_OUT_OF_MEMORY;
  211:   }
  212: 
  213:   EncodeInputStream_State<T> state;
  214:   state.charsOnStack = 0;
  215:   state.c[2] = '\0';
  216:   state.buffer = aOffset + aDest.BeginWriting();
  217: 
  218:   while (true) {
  219:     uint32_t read = 0;
  220: 
  221:     rv = aInputStream->ReadSegments(&EncodeInputStream_Encoder<T>,
  222:                                     (void*)&state, aCount, &read);
  223:     if (NS_FAILED(rv)) {
  224:       if (rv == NS_BASE_STREAM_WOULD_BLOCK) {
  225:         MOZ_CRASH("Not implemented for async streams!");
  226:       }
  227:       if (rv == NS_ERROR_NOT_IMPLEMENTED) {
  228:         MOZ_CRASH("Requires a stream that implements ReadSegments!");
  229:       }
  230:       return rv;
  231:     }
  232: 
  233:     if (!read) {
  234:       break;
  235:     }
  236:   }
  ...
  249:   return NS_OK;
  250: 
  251: }

(code from trunk)

The bug is that the while loop beginning line 218 doesn't adjust aCount to account for the input it has processed. This means that the 2nd and succeeding iterations of the loop use the original value, thus emptying the stream of its entire contents, even if that exceeds aCount bytes.

It is unclear whether this bug can be manifested. EncodeInputStream() is called only by Base64EncodeInputStream() (same module), which is called only via three paths. The first two probably do not allow the bug to be manifested, but the third might:

1. ReadAsDataURL() (FileReaderSync.cpp), which first converts the stream into an nsStringInputStream by calling ConvertAsyncToSyncStream(), so the stream is a fixed size, so the bug cannot be invoked on this path.

2. HTMLCanvasElement::ToDataURLImpl(), which appears to create a fixed-size stream representing canvas element contents. The stream isa imgIEncoder, which isa nsIAsyncInputStream. However, all code paths initialize the stream using its InitFromData() method, which synchronously fills the stream. So, the bug cannot be invoked on this path.

3. nsScriptableBase64Encoder::EncodeTo[C]String() (xpcom/io/nsScriptableBase64Encoder.cpp), which is called only via the interface returned when using ModuleID::Anonymous404 (CreateInstanceImpl() in xpcom/components/StaticComponents.cpp). This interface appears to be available to privileged code by using the "@mozilla.org/scriptablebase64encoder;1" interface name (StaticComponents.cpp)

Comment 1

[Frederik Braun [:freddy]]

Given the reporter's report that only the 3rd path seems relevant, I looked into usage of @mozilla.org/scriptablebase64encoder;1, of which I could only find one in toolkit/components/places/BookmarkHTMLUtils.sys.mjs's function base64EncodeString(aString). That function creates a stream based on a fixed-input parameter aString.

It seems this code is never called in a hazardous way, which seems to make this a theoretical future bug and not a current security issue.

How do we usually rate them, sec-low or sec-moderate?

Comment 2

[mozillabugs]

The write beyond bounds also can occur if aCount is less than the actual length of the stream, even if the stream itself cannot return more than its initial length. To illustrate this issue, follow these steps:

1. Put ffbug_2027.htm and ffbug_2027_worker.js (attached) on a webserver (I used a local one; you'll need to edit "127.0.0.1" if you use a different DNS name/IP).
2. Start FF and attach a debugger to it.
3. Set a BP on EncodeInputStream() line 190.
4. Load ffbug_2027.htm.
5. When the BP fires, observe that aCount is the total stream length (0x10).
6. Modify aCount to be 8 to simulate the case in which the stream is longer than aCount.
7. Run to line 209. Notice that base64LenOrErr == 0xc.
8. Run to line 218 and examine the memory pointed to by state.buffer. The validly-writable length is (0xc+1)*2 == 0x1a bytes (see nsTSubstring::StartBulkWriteImpl() in xpcom/string/nsTSubstring.cpp).
9. Run through line 222. Notice that it wrote 0x10 bytes of data into the buffer.
10. Run again through line 222. Notice that it wrote an additional 0x18 bytes, thus overrunning the buffer by 0x0e bytes.
11. Run again through line 222. Notice that it does nothing.
12. Step through line 240, and notice that it writes another 8 bytes to the already-overrun buffer.

Comment 3

[mozillabugs]

Attachment: ffbug_2027.htm

Comment 4

[mozillabugs]

Attachment: ffbug_2027_worker.js

Comment 5

[Daniel Veditz [:dveditz]]

Randell: not sure if this use of FileReaderSync() will interest you given your work on the File system feature

Comment 6

[Jens Stutte [:jstutte]]

(In reply to mozillabugs from comment #2)

6. Modify aCount to be 8 to simulate the case in which the stream is longer than aCount.

I assume this modification is meant to be done in the debugger.

(In reply to Frederik Braun [:freddy] from comment #1)

Given the reporter's report that only the 3rd path seems relevant, I looked into usage of @mozilla.org/scriptablebase64encoder;1, of which I could only find one in toolkit/components/places/BookmarkHTMLUtils.sys.mjs's function base64EncodeString(aString). That function creates a stream based on a fixed-input parameter aString.

It seems this code is never called in a hazardous way, which seems to make this a theoretical future bug and not a current security issue.

IIUC according to Freddy's assessment we do not have such a dynamic situation in our current code base and the POC relies on the debugger variable modification. This is still a bug as it could get used differently in the future, of course.

Comment 7

[mozillabugs]

(In reply to Jens Stutte [:jstutte] from comment #6)

(In reply to mozillabugs from comment #2)

6. Modify aCount to be 8 to simulate the case in which the stream is longer than aCount.

I assume this modification is meant to be done in the debugger.

Yes. I meant these steps to simulate what would happen if the stream is longer than aCount, as by having a stream that is initially longer, or a dynamic stream that becomes longer while the buggy while loop is executing.

Comment 8

[Andrew McCreight [:mccr8]]

Nika thinks this could be an issue if this is being used for a file, and the file changes. Or something. Maybe not sec-high?

Comment 9

[Andrew McCreight [:mccr8]]

Nika might have a chance to fix this. Thanks!

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1804564 - Respect aCount better in EncodeInputStream, r=mccr8!

Previously we'd always read the full stream, but if an aCount lower than
the actual size of the stream is provided, we should only encode that
many bytes.

This patch also improves handling of streams shorter than aCount bytes,
ensuring that the generated string is of the correct length.

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9311868 [details]
Bug 1804564 - Respect aCount better in EncodeInputStream, r=mccr8!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: While the patch draws attention to the clearly broken code, to create an exploit, you'd need to investigate the callers and find one which is exploitable, which is nontrivial.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• 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?: This function doesn't appear to have been changed since 2020, so this patch is likely to apply to all affected branches.
• How likely is this patch to cause regressions; how much testing does it need?: The patch is fairly unlikely to cause regressions , as it is only fixing broken behaviour which would've cause problems in edge cases.
• Is Android affected?: Yes

Comment 12

[Tom Ritter [:tjr]]

Comment on attachment 9311868 [details]
Bug 1804564 - Respect aCount better in EncodeInputStream, r=mccr8!

Approved to land and - if we think there's a situation this could happen - request uplift.

Comment 13

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1804564 - Respect aCount better in EncodeInputStream, r=mccr8!

Previously we'd always read the full stream, but if an aCount lower than
the actual size of the stream is provided, we should only encode that
many bytes.

This patch also improves handling of streams shorter than aCount bytes,
ensuring that the generated string is of the correct length.

Original Revision: https://phabricator.services.mozilla.com/D166616

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9311868 [details]
Bug 1804564 - Respect aCount better in EncodeInputStream, r=mccr8!

Beta/Release Uplift Approval Request

• User impact if declined: Potential out-of-bounds write
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• 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): Fix to bug in existing function which could cause errors in edge cases, without changing the semantics in cases which were already working.
• String changes made/needed: None
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Potential out-of-bounds write
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Fix to bug in existing function which could cause errors in edge cases, without changing the semantics in cases which were already working.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Respect aCount better in EncodeInputStream, r=mccr8,necko-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/8a0fa777a6a9525721bceed324e9e1ae332d2715
https://hg.mozilla.org/mozilla-central/rev/8a0fa777a6a9

Comment 16

[Pascal Chevrel:pascalc]

Comment on attachment 9311868 [details]
Bug 1804564 - Respect aCount better in EncodeInputStream, r=mccr8!

Approved for 110 beta 5, thanks.

Comment 17

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/018c2ad970fd

Comment 18

[Frederik Braun [:freddy]]

For posterity, we're rating this lower at moderate severity due to the issue being latent and not actually present (pretty much in accordance to earlier conversations here). Thanks for reporting this security bug to us!

Comment 19

[mozillabugs]

Thank you for fixing this bug, and for paying a bounty for a latent bug!

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9311868 [details]
Bug 1804564 - Respect aCount better in EncodeInputStream, r=mccr8!

Approved for 102.8esr.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/bcbb5f3892ca

Comment 22

[Tom Ritter [:tjr]]

Attachment: advisory.txt