1843038 - (CVE-2023-4050) Stack buffer overflow in NSSCipherStrategy::DeserializeKey

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, defect, P2)

Description

Attachment: key_overflow_patch.diff

When deserializing a DecryptingInputStream, we call NSSCipherStrategy::DeserializeKey
on an untrusted input:

template <typename CipherStrategy>
bool DecryptingInputStream<CipherStrategy>::Deserialize(
    const mozilla::ipc::InputStreamParams& aParams) {
  MOZ_ASSERT(aParams.type() ==
             mozilla::ipc::InputStreamParams::TEncryptedFileInputStreamParams);
  const auto& params = aParams.get_EncryptedFileInputStreamParams();

  nsCOMPtr<nsIFileInputStream> stream;
  nsFileInputStream::Create(NS_GET_IID(nsIFileInputStream),
                            getter_AddRefs(stream));
  nsCOMPtr<nsIIPCSerializableInputStream> baseSerializable =
      do_QueryInterface(stream);

  if (NS_WARN_IF(
          !baseSerializable->Deserialize(params.fileInputStreamParams()))) {
    return false;
  }

  Init(WrapNotNull<nsCOMPtr<nsIInputStream>>(std::move(stream)),
       params.blockSize());
  mKey.init(mCipherStrategy.DeserializeKey(params.key())); // <-- here
  if (NS_WARN_IF(
          NS_FAILED(mCipherStrategy.Init(CipherMode::Decrypt, params.key())))) {
    return false;
  }

  return true;
}

This asserts that the size is correct, but does not check it at runtime in a release build:

NSSCipherStrategy::KeyType NSSCipherStrategy::DeserializeKey(
    const Span<const uint8_t>& aSerializedKey) {
  KeyType res;
  MOZ_ASSERT(res.size() == aSerializedKey.size());
  std::copy(aSerializedKey.cbegin(), aSerializedKey.cend(), res.begin());
  return res;
}

This means that the std::copy call can overflow the allocated stack buffer. I've provided a patch which will convert an outgoing request body of size 0x1234 to an EncryptedInputStream and trigger this issue. I don't seem to be able to attach multiple files to the report, so I'll add my server script as a comment.

I've only tested this on linux, where the incoming IPC message is processed on the main thread. In my build this means that this appears not to be exploitable (there is nothing on the stack which can be corrupted that is used prior to the stack-cookie check), so I'm reporting this without applying a deadline.

I've still marked this as a security bug, as it may still be exploitable on other platforms/compilers or if there are situations in which other threads handle incoming IPC messages; I'm not sufficiently familiar with Firefox' threading model.

Comment 1

[Mark Brand]

Attachment: server.py

Comment 2

[Mark Brand]

Attachment: key_overflow.asan.txt

Comment 3

[Daniel Veditz [:dveditz]]

Span is a bounds-checked class intended to avoid this problem -- completely defeated by using a raw std::copy()
[sorry, the Span is the source, not the dest]

Comment 4

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

Attachment: Bug 1843038 - Make CypherStrategy::DeserializeKey fallible, r=janv!

Comment 5

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

Comment on attachment 9343735 [details]
Bug 1843038 - Make CypherStrategy::DeserializeKey fallible, r=janv!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: While it is very easy to figure out what the issue is from the patch, as noted in comment 0 it is somewhat difficult, even with ACE in the child (which would be required to trigger this bug), to clobber the stack in a productive way and create a sandbox escape.
• 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?: Bug 1641178
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: I expect the patch should apply to earlier branches unchanged, as the impacted files haven't been modified much since they were introduced.
• How likely is this patch to cause regressions; how much testing does it need?: This is unlikely to cause regressions, upgrading an assertion to a deserialization failure.
• Is Android affected?: Yes

Beta/Release Uplift Approval Request

• User impact if declined: Potential IPC sandbox escape/security bug
• Is this code covered by automated tests?: No
• 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): This is unlikely to cause regressions, upgrading an assertion to a deserialization failure.
• 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 IPC sandbox escape/security bug
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is unlikely to cause regressions, upgrading an assertion to a deserialization failure.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:nika, could you consider increasing the severity of this security bug?

For more information, please visit BugBot documentation.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug is marked as tracked for firefox116 (beta) and tracked for firefox117 (nightly). However, the bug still has low severity.

:jstutte, could you please increase the severity for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 9343735 [details]
Bug 1843038 - Make CypherStrategy::DeserializeKey fallible, r=janv!

sec-approval+ = dveditz

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08c2e3435979
Make CypherStrategy::DeserializeKey fallible, r=janv

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/08c2e3435979

Comment 11

[Dianna Smith [:diannaS]]

Comment on attachment 9343735 [details]
Bug 1843038 - Make CypherStrategy::DeserializeKey fallible, r=janv!

Approved for 116.0b8

Comment 12

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/83e23781e1c2

Comment 13

[Dianna Smith [:diannaS]]

Comment on attachment 9343735 [details]
Bug 1843038 - Make CypherStrategy::DeserializeKey fallible, r=janv!

Approved for 102.14esr
Approved for 115.1esr

Comment 14

[Pulsebot]

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

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/3a68be3f458d

Comment 16

[Jesse Schwartzentruber (:truber)]

Attachment: advisory.txt

Comment 17

[Daniel Veditz [:dveditz]]

Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons