Closed Bug 1843038 (CVE-2023-4050) Opened 10 months ago Closed 9 months ago

Stack buffer overflow in NSSCipherStrategy::DeserializeKey

Categories

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

defect

Tracking

()

RESOLVED FIXED
117 Branch
Tracking Status
firefox-esr102 116+ fixed
firefox-esr115 116+ fixed
firefox115 --- wontfix
firefox116 + fixed
firefox117 + fixed

People

(Reporter: markbrand, Assigned: nika)

References

(Regression)

Details

(4 keywords, Whiteboard: [adv-main116+] [adv-ESR115.1+] [adv-ESR102.14+])

Attachments

(5 files)

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.

Attached file server.py
Attached file key_overflow.asan.txt
Group: firefox-core-security → dom-core-security
Component: Security → Storage: Quota Manager
Product: Firefox → Core
Attachment #9343483 - Attachment mime type: text/x-python → text/plain
Assignee: nobody → nika
Status: NEW → ASSIGNED

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.
Attachment #9343735 - Flags: sec-approval?
Attachment #9343735 - Flags: approval-mozilla-release?
Attachment #9343735 - Flags: approval-mozilla-esr115?
Attachment #9343735 - Flags: approval-mozilla-beta?
Attachment #9343735 - Flags: approval-mozilla-release? → approval-mozilla-esr102?
Severity: -- → S3
Priority: -- → P2

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.

Flags: needinfo?(nika)

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.

Flags: needinfo?(jstutte)
Flags: needinfo?(nika)
Severity: S3 → S2
Flags: needinfo?(jstutte)

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

sec-approval+ = dveditz

Attachment #9343735 - Flags: sec-approval? → sec-approval+
Pushed by nlayzell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/08c2e3435979
Make CypherStrategy::DeserializeKey fallible, r=janv
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → 117 Branch

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

Approved for 116.0b8

Attachment #9343735 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Approved for 102.14esr
Approved for 115.1esr

Attachment #9343735 - Flags: approval-mozilla-esr115?
Attachment #9343735 - Flags: approval-mozilla-esr115+
Attachment #9343735 - Flags: approval-mozilla-esr102?
Attachment #9343735 - Flags: approval-mozilla-esr102+
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-
Whiteboard: [adv-main116+] [adv-ESR115.1+] [adv-ESR102.14+]
Group: core-security-release
Alias: CVE-2023-4050
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: