1864123 - (CVE-2023-6865) EncryptingOutputStream can write random contents of memory to its base stream

Status: RESOLVED FIXED

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

Description

[Jan Varga [:janv]]

EncryptingOutputStream can write random contents of memory to its base stream. This can happen when original data is smaller than 4K. The stream encrypts only actual payload, but data is written to the base stream in whole 4K blocks. EncryptingOutputStream is used by IndexedDB and CacheAPI to store data on disk for private browsing, so the base stream in that case is quota::FileOutputStream, so random contents of memory can end up being stored on disk.

We have a fix for this which fills unused payload with random values.

We suggest to treat this as sec-other since Cache API in private browsing is still gated by a pref and it' still disabled by default. The issue is more serious in the Cache API case, because files stored on disk are fundamental part of the implementation. IndexedDB stores data in separate files only to support storing of DOM Blobs/Files which is not used so often.

IndexedDB in private browsing shipped in FF 115, so the fix might need to be uplifted even to ESR 115.

Comment 1

[Jens Stutte [:jstutte]]

Please note also, that until bug 1850008 landed the written memory was poisoned, so in ESR we do not see the potential to really expose uninitialized memory on disk, the written files just show less entropy than expected for full encryption.

Comment 2

[Jan Varga [:janv]]

Attachment: Bug 1864123 - Optimize flushing to base stream; r=#dom-storage

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

I'm classifying this as sec-moderate on the basis of "Disclosure of sensitive information that represents a violation of privacy but by itself does not expose sensitive user data or uniquily identify the user." from the severity ratings. This may still be higher than necessary as there is no way to see this data without read access to the user's profile directory which generally is not something our threat models protect against in general but is a factor of concern in private browsing threat models. The key thing is to correctly characterize that this is not sec-high and should be landable now. To recap the situation here:

• For the encrypted data we write to disk for currently enabled-by-default IDB blob storage and for soon-to-be-enabled Cache API storage, we write the data in 4k chunks, post-snappy-compression. If we have less payload data than 4k in our final chunk of a Response we are writing, we fully encrypt that payload data, but we ended up writing uninitialized memory for the remainder of that 4k chunk. With poisoning active, this means we would be writing the poison values. Bug 1850008 changed poisoning behavior in Fx120 so that we still poison all freed memory in EARLY_BETA_OR_EARLIER but that we only poison 64 bytes (kCacheLineSize) in late beta and release.
  • This does mean that Fx120 which is currently in RC would be experiencing this problem and so we will want to request uplift to 120, but we do not believe it justifies releases of its own.
• The write process happens in the parent process which means it's parent process memory that would be written to disk.
• The data buffer in question is expected to be 4k and so would not fall into the same jemalloc allocation size class as any keying material (std::array<uint8_t, 32>). The size class is likely to line up with bulk data.
• The fix is a minimal change to ensure we are writing initialized values for all data above the 64 byte poison line. We write a cryptographically random stream for entropy purposes versus truncation because it is 1) less risky/complex of a change, 2) we intend to change our behavior in this case to effectively pad the plaintext so that the ciphertext is quantized / rounded upwards as an anti-fingerprinting measure.
• The uninitialized data is never available to content absent a security exploit that allows arbitrary execution of code in the content process. Consumption of Cache API Response objects inherently involves providing the file descriptor of the file to the content process and in that case a compromised content process could read the extra data into memory. I have not done a full analysis of buffering at play, it's possible that a SPECTRE attack could read the contents of the data.
• Otherwise, an attacker would need read access to the user's profile directory, which usually would correlate with complete control of the computer, but if the attacker only had read access to the profile and no access to any other data/aspect of the computer, would at least correlate with a more serious loss of privacy given access to the places history database, the cookies database, the password store, etc.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

At DOM sec-bug triage we discussed this and the potential of exposure of parent process memory to the content process indirectly does make this sec-high, although such an exploit would be quite difficult.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 9362989 [details]
Bug 1864123 - Optimize flushing to base stream; r=#dom-storage

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Extremely difficult. An attacker would also need to convince the user to use private browsing mode if they are not already using it. An attacker would probably need to do a lot of work to get the uninitialized memory written to disk to have a high probability of having data they want.

In terms of vulnerable versions, there are 2 things going on here:

1. Fx having enabled PrivateBrowsing for IndexedDB in v115 in bug 1831058 surfaced the functionality that writes uninitialized memory to disk. This was mitigated by freed memory poisoning in release.
2. Fx having limited free poisoning to 64 bytes in v120 in bug 1850008. This makes the data written to disk actually uninitialized memory in late beta and release rather than poison bytes.

So Firefox 120 is the first version there is a problem on.

We would want this fix on ESR even though it's not vulnerable.

• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: 120
• If not all supported branches, which bug introduced the flaw?: Bug 1850008
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Very little chance of regressions.
• Is Android affected?: Yes

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.
:janv, could you consider increasing the severity of this security bug?

For more information, please visit BugBot documentation.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Are we going to file a follow-up for ensuring we're 0-initing or otherwise consistently initing the first cache line's worth of data? (The poisoning means it's 0xe5 now.) Alternately should the existing patch be revised to do that?

Comment 8

[Tom Ritter [:tjr]]

Comment on attachment 9362989 [details]
Bug 1864123 - Optimize flushing to base stream; r=#dom-storage

Approved to land and uplift

Comment 9

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #7)

Are we going to file a follow-up for ensuring we're 0-initing or otherwise consistently initing the first cache line's worth of data? (The poisoning means it's 0xe5 now.) Alternately should the existing patch be revised to do that?

Yeah, that's probably a good idea, but I think you have more context for filing a bug for that.
Can you please do that ?
Thank you.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a6e753295d9
Optimize flushing to base stream; r=dom-storage-reviewers,asuth,hsingh

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/9a6e753295d9

Comment 12

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

The patch landed in nightly and beta is affected.
:janv, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox121 to wontfix.

For more information, please visit BugBot documentation.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jan Varga [:janv] from comment #9)

Yeah, that's probably a good idea, but I think you have more context for filing a bug for that.
Can you please do that ?

Did this happen?

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

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

Did this happen?

I have filed Bug 1867394 to this end.

I am going to fill out the uplift permission request now.

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Bug 1864123 - Optimize flushing to base stream; r=#dom-storage

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

Comment 16

[Phabricator Automation]

Uplift Approval Request

• Code covered by automated testing: yes
• Is Android affected?: yes
• Steps to reproduce for manual QE testing: n/a
• Explanation of risk level: Patch was intentionally strucured to minimize
• Fix verified in Nightly: yes
• User impact if declined: Please see the sec-approval details at https://bugzilla.mozilla.org/show_bug.cgi?id=1864123#c5 but to recap: an attacker who compromises a content process could escalate their privileges without this fix. Note that the bug currently has this marked as wontfix on 120 but it would be pretty good to have this on 120 if it can ride along.
• String changes made/needed: no
• Needs manual QE test: no
• Risk associated with taking this patch: Very low.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9366183 [details]
Bug 1864123 - Optimize flushing to base stream; r=#dom-storage

Approved for 121.0b6.

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Bug 1864123 - Optimize flushing to base stream; r=#dom-storage

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

Comment 19

[Phabricator Automation]

Uplift Approval Request

• Fix verified in Nightly: yes
• User impact if declined: Please see the sec-approval details at https://bugzilla.mozilla.org/show_bug.cgi?id=1864123#c5 but to recap: an attacker who compromises a content process could escalate their privileges without this fix.
• Needs manual QE test: no
• String changes made/needed: no
• Risk associated with taking this patch: Very low
• Code covered by automated testing: yes
• Is Android affected?: yes
• Explanation of risk level: Patch was intentionally strucured to minimize
• Steps to reproduce for manual QE testing: n/a

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9366186 [details]
Bug 1864123 - Optimize flushing to base stream; r=#dom-storage

Approved for 115.6esr.

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/ca8ce1830705

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/8cc3a3bb4ea0

Comment 23

[Iulian Moraru]

Backed out changeset 8cc3a3bb4ea0 (bug 1864123) for causing mass build bustages on EncryptingOutputStream_impl.h. Backed out from esr115.

• Push with failures

• Failure log

• Backout link

[task 2023-11-30T00:10:54.295Z] 00:10:54     INFO -  gmake[4]: Entering directory '/builds/worker/workspace/obj-build/dom/indexedDB'
[task 2023-11-30T00:10:54.298Z] 00:10:54     INFO -  /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ --sysroot /builds/worker/fetches/sysroot-i686-linux-gnu -m32 -o ActorsParent.o -c  -I/builds/worker/workspace/obj-build/dist/stl_wrappers -I/builds/worker/workspace/obj-build/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fstack-clash-protection -DDEBUG=1 -DOS_POSIX=1 -DOS_LINUX=1 -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/dom/indexedDB -I/builds/worker/workspace/obj-build/dom/indexedDB -I/builds/worker/workspace/obj-build/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/checkouts/gecko/dom/base -I/builds/worker/checkouts/gecko/dom/storage -I/builds/worker/checkouts/gecko/ipc/glue -I/builds/worker/checkouts/gecko/third_party/sqlite3/src -I/builds/worker/checkouts/gecko/xpcom/build -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/mozilla-config.h -fno-sized-deallocation -fno-aligned-new -fcrash-diagnostics-dir=/builds/worker/artifacts -march=pentium-m -msse -msse2 -mfpmath=sse -fno-exceptions -fPIC -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -gdwarf-4 -Xclang -load -Xclang /builds/worker/workspace/obj-build/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -gline-tables-only -fno-omit-frame-pointer -funwind-tables -Werror -Wall -Wbitfield-enum-conversion -Wdeprecated-this-capture -Wempty-body -Wformat-type-confusion -Wignored-qualifiers -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtautological-constant-in-range-compare -Wtype-limits -Wno-error=tautological-type-limit-compare -Wunreachable-code -Wunreachable-code-return -Wunused-but-set-parameter -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wno-range-loop-analysis -Wc++2a-compat -Wenum-compare-conditional -Wenum-float-conversion -Wno-error=deprecated -Wno-error=deprecated-anon-enum-enum-conversion -Wno-error=deprecated-enum-enum-conversion -Wno-error=deprecated-pragma -Wno-error=deprecated-this-capture -Wcomma -Wimplicit-fallthrough -Wstring-conversion -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wno-error=atomic-alignment -Wno-error=deprecated-builtins -Wformat -Wformat-security -Wno-psabi -Wthread-safety -Wno-error=builtin-macro-redefined -Wno-unknown-warning-option -fno-strict-aliasing -ffp-contract=off  -MD -MP -MF .deps/ActorsParent.o.pp   /builds/worker/checkouts/gecko/dom/indexedDB/ActorsParent.cpp
[task 2023-11-30T00:10:54.299Z] 00:10:54     INFO -  In file included from /builds/worker/checkouts/gecko/dom/indexedDB/ActorsParent.cpp:24:
[task 2023-11-30T00:10:54.299Z] 00:10:54     INFO -  In file included from /builds/worker/checkouts/gecko/dom/indexedDB/DatabaseFileInfo.h:12:
[task 2023-11-30T00:10:54.299Z] 00:10:54     INFO -  In file included from /builds/worker/checkouts/gecko/dom/indexedDB/DatabaseFileManager.h:11:
[task 2023-11-30T00:10:54.299Z] 00:10:54     INFO -  In file included from /builds/worker/checkouts/gecko/dom/indexedDB/IndexedDBCipherKeyManager.h:11:
[task 2023-11-30T00:10:54.300Z] 00:10:54     INFO -  In file included from /builds/worker/workspace/obj-build/dist/include/mozilla/dom/quota/IPCStreamCipherStrategy.h:10:
[task 2023-11-30T00:10:54.300Z] 00:10:54     INFO -  In file included from /builds/worker/workspace/obj-build/dist/include/mozilla/dom/quota/NSSCipherStrategy.h:20:
[task 2023-11-30T00:10:54.300Z] 00:10:54  WARNING -  /builds/worker/workspace/obj-build/dist/include/ScopedNSSTypes.h:283:22: warning: result of comparison 'index_type' (aka 'unsigned int') > 4294967295 is always false [-Wtautological-type-limit-compare]
[task 2023-11-30T00:10:54.300Z] 00:10:54     INFO -      if (key.Length() > std::numeric_limits<unsigned int>::max()) {
[task 2023-11-30T00:10:54.301Z] 00:10:54     INFO -          ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2023-11-30T00:10:54.301Z] 00:10:54     INFO -  In file included from /builds/worker/checkouts/gecko/dom/indexedDB/ActorsParent.cpp:129:
[task 2023-11-30T00:10:54.301Z] 00:10:54    ERROR -  /builds/worker/workspace/obj-build/dist/include/mozilla/dom/quota/EncryptingOutputStream_impl.h:225:37: error: no member named 'GenerateRandomBytesInto' in 'nsIRandomGenerator'; did you mean 'GenerateRandomBytes'?
[task 2023-11-30T00:10:54.302Z] 00:10:54     INFO -      nsresult rv = mRandomGenerator->GenerateRandomBytesInto(
[task 2023-11-30T00:10:54.302Z] 00:10:54     INFO -                                      ^~~~~~~~~~~~~~~~~~~~~~~
[task 2023-11-30T00:10:54.303Z] 00:10:54     INFO -                                      GenerateRandomBytes
[task 2023-11-30T00:10:54.303Z] 00:10:54     INFO -  /builds/worker/workspace/obj-build/dist/include/nsIRandomGenerator.h:34:36: note: 'GenerateRandomBytes' declared here
[task 2023-11-30T00:10:54.303Z] 00:10:54     INFO -    JS_HAZ_CAN_RUN_SCRIPT NS_IMETHOD GenerateRandomBytes(uint32_t aLength, uint8_t **aBuffer) = 0;
[task 2023-11-30T00:10:54.304Z] 00:10:54     INFO -                                     ^
[task 2023-11-30T00:10:54.304Z] 00:10:54     INFO -  In file included from /builds/worker/checkouts/gecko/dom/indexedDB/ActorsParent.cpp:129:
[task 2023-11-30T00:10:54.304Z] 00:10:54    ERROR -  /builds/worker/workspace/obj-build/dist/include/mozilla/dom/quota/EncryptingOutputStream_impl.h:226:9: error: cannot initialize a parameter of type 'uint32_t' (aka 'unsigned int') with an rvalue of type 'pointer' (aka 'unsigned char *')
[task 2023-11-30T00:10:54.305Z] 00:10:54     INFO -          unusedPayload.Elements(), unusedPayload.Length());
[task 2023-11-30T00:10:54.305Z] 00:10:54     INFO -          ^~~~~~~~~~~~~~~~~~~~~~~~
[task 2023-11-30T00:10:54.306Z] 00:10:54     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/dom/quota/EncryptingOutputStream_impl.h:71:17: note: in instantiation of member function 'mozilla::dom::quota::EncryptingOutputStream<mozilla::dom::quota::NSSCipherStrategy>::FlushToBaseStream' requested here
[task 2023-11-30T00:10:54.306Z] 00:10:54     INFO -    nsresult rv = FlushToBaseStream();
[task 2023-11-30T00:10:54.306Z] 00:10:54     INFO -                  ^
[task 2023-11-30T00:10:54.306Z] 00:10:54     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/dom/quota/EncryptingOutputStream.h:68:12: note: in instantiation of member function 'mozilla::dom::quota::EncryptingOutputStream<mozilla::dom::quota::NSSCipherStrategy>::Close' requested here
[task 2023-11-30T00:10:54.307Z] 00:10:54     INFO -    explicit EncryptingOutputStream(nsCOMPtr<nsIOutputStream> aBaseStream,
[task 2023-11-30T00:10:54.307Z] 00:10:54     INFO -             ^
[task 2023-11-30T00:10:54.307Z] 00:10:54     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:642:19: note: in instantiation of member function 'mozilla::dom::quota::EncryptingOutputStream<mozilla::dom::quota::NSSCipherStrategy>::EncryptingOutputStream' requested here
[task 2023-11-30T00:10:54.308Z] 00:10:54     INFO -    RefPtr<T> p(new T(std::forward<Args>(aArgs)...));
[task 2023-11-30T00:10:54.308Z] 00:10:54     INFO -                    ^
[task 2023-11-30T00:10:54.308Z] 00:10:54     INFO -  /builds/worker/checkouts/gecko/dom/indexedDB/ActorsParent.cpp:20698:11: note: in instantiation of function template specialization 'mozilla::MakeRefPtr<mozilla::dom::quota::EncryptingOutputStream<mozilla::dom::quota::NSSCipherStrategy>, nsCOMPtr<nsIOutputStream>, const unsigned int &, const std::array<unsigned char, 32> &>' requested here
[task 2023-11-30T00:10:54.309Z] 00:10:54     INFO -            MakeRefPtr<EncryptingOutputStream<IndexedDBCipherStrategy>>(
[task 2023-11-30T00:10:54.309Z] 00:10:54     INFO -            ^
[task 2023-11-30T00:10:54.309Z] 00:10:54     INFO -  /builds/worker/workspace/obj-build/dist/include/nsIRandomGenerator.h:34:65: note: passing argument to parameter 'aLength' here
[task 2023-11-30T00:10:54.309Z] 00:10:54     INFO -    JS_HAZ_CAN_RUN_SCRIPT NS_IMETHOD GenerateRandomBytes(uint32_t aLength, uint8_t **aBuffer) = 0;
[task 2023-11-30T00:10:54.309Z] 00:10:54     INFO -                                                                  ^
[task 2023-11-30T00:10:54.310Z] 00:10:54     INFO -  1 warning and 2 errors generated.
[task 2023-11-30T00:10:54.310Z] 00:10:54    ERROR -  gmake[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:665: ActorsParent.o] Error 1
[task 2023-11-30T00:10:54.310Z] 00:10:54     INFO -  gmake[4]: Leaving directory '/builds/worker/workspace/obj-build/dom/indexedDB'

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Per Slack discussion with asuth, this patch as-written depends on the patches from bug 1690111. While we are intending to get those uplifted to ESR eventually still, they've been blocked on some lingering stability issues. I'm not sure if we want to try waiting on those patches to get uplifted or rework the ESR uplift around them.

Comment 25

[Jan Varga [:janv]]

In theory, we don't need bug 1690111 for this. It's just that random bytes would have to be generated into a temporary buffer and then copied, instead of generating them directly to the intended place.

Comment 26

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/b66eb4c63b10

Comment 27

[Jason Kratzer [:jkratzer]]

Attachment: advisory.txt

Comment 28

[Jason Kratzer [:jkratzer]]

Attachment: advisory.txt

Comment 29

[Daniel Veditz [:dveditz]]

Bulk-unhiding security bugs fixed in Firefox 119-121 (Fall 2023). Use "moo-doctrine-subsidy" to filter