1467889 - Fix off-by-one error in nsITransferable IPC string length logic

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

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

I'm not sure if this is actually a security bug, but I figure I should file it as one anyway.

---
The shared memory buffer used for nsCString objects contains an extra byte due to the copy into shmem occurring here:
https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/dom/base/nsContentUtils.cpp#7938

Unfortunately, we never actually check that the string which we copy in that code is not a substring (It's passed as a nsACString which may not be terminated), so that null terminator could actually be a random byte.

---
We then read that shared memory buffer into a nsCString on the other side here:
https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/dom/base/nsContentUtils.cpp#7786-7789

We create a nsDependentCString object pointing into the shmem. However, as the string length we use the full size of the buffer, including the "terminator". This means that every time a cstring is sent over IPC in a nsITransferable, it has an extra (probably) null, byte appended to it.

The comment in that section of code acknowledges this null, but doesn't actually handle it.

---
I think the best solution here is probably:

a) Stop passing nsCString objects in Shared Memory - we're going to copy it into a nsCString (within the nsISupportsCString object) on the other side anyway, so we aren't really being more efficient for long strings.
b) Don't try to store a null at the end of the shared memory buffer, and use precise string sizes.
c) Store a null explicitly at the end of the buffer, and subtract 1 from the buffer size before creating the nsDependentCString.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to :Nika Layzell from comment #0)
> a) Stop passing nsCString objects in Shared Memory - we're going to copy it
> into a nsCString (within the nsISupportsCString object) on the other side
> anyway, so we aren't really being more efficient for long strings.

Do we support copying/pasting a string that's longer than kMaximumMessageSize?  Shared memory is one of the less annoying ways to deal with that case.  Having a generic adaptive IPC string that uses nsCString or Shmem depending on length might be useful, and might help people who'd otherwise reinvent this wheel.

On that topic, bug 1458246 made the same mistake.

Maybe one of the layers in the IPC shared memory stack should poison the last page after the requested length, instead of leaving it zero-initialized, to catch one-byte overreads like these that expect '\0'.  (Also maybe the ns*CString temination check should be stronger than NS_ASSERTION.)  We might be able to get ASan to mark it inaccessible, too.

> b) Don't try to store a null at the end of the shared memory buffer, and use
> precise string sizes.

That probably isn't a problem in this case, since there's already a copy.  For a (hypothetical) general solution it would be nice not to force a copy if the consumer could use an nsDependentCString in-place, but:

> c) Store a null explicitly at the end of the buffer, and subtract 1 from the
> buffer size before creating the nsDependentCString.

Slight problem: a malicious child process could race this and overwrite the null.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I'm going to take this, because I have a plan:

1. Stop adding the extra NUL in the transferrable stuff.
2. Change both of the places where this is happening to nsDependentCSubstring; this will add a copy in the profiler case.
2a. File a bug to do something more efficient in the profiler — either send the string in chunks, or change its JSON wrangling to not need null termination.
3. Break off comment #1 into a separate bug.
4. Also file a followup bug for poisoning the padding, and maybe other ways to prevent this bug coming back.

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Sorry; I thought this bug was unassigned.  Do you still want it?

Comment 5

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

I have some WIP cleanup patches in this area which would accidentally fix this case, but I don't think they'll happen any time soon. Go ahead & take it :-).

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I tried a small patch to initialize the entire ipc::Shmem backing memory to 0xe4, the same as mozjemalloc.  That picks up the DependentCString bugs, but also finds a few places that expect zero-initialized shmem contents: everything WebRender breaks via RefCountedShmem not initializing its refcount (but there's a one-line fix), and reftests in general are a sea of red.

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I also found a few other bad cstrings that I hadn't caught the first time.  Some of them might not be exploitable, but they're all incorrect — if the page alignment is right(/wrong) they won't be null-terminated.  And all of them, except the profiler, already wind up copying the string eventually.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Patch: use nsDependentCSubstring with Shmem

This patch changes all the misuses of nsDependentCString with shared memory (at least, the ones that I've found) to use nsDependentCSubstring; see earlier comments for context.

The only case that would incur an extra copy because of this is the profiler, but it looks feasible to change that to stream the text in chunks.

Comment 9

[Markus Stange [:mstange]]

The profiler code is changing a bit in bug 1466783.

Comment 10

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

Comment on attachment 8989649 [details] [diff] [review]
Patch: use nsDependentCSubstring with Shmem

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

I'm a touch confused why we sending these nsCString objects over IPC as shared memory rather than just using the existing nsCString serialization logic. As far as I can tell, a copy is still going to happen into a nsStringBuffer in every one of these cases, so I don't see any particular reason to not use the standard codepath here.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to :Nika Layzell from comment #10)
> I'm a touch confused why we sending these nsCString objects over IPC as
> shared memory rather than just using the existing nsCString serialization
> logic. As far as I can tell, a copy is still going to happen into a
> nsStringBuffer in every one of these cases, so I don't see any particular
> reason to not use the standard codepath here.

The standard codepath (sending strings as parameters) serializes the strings inline in the messages, which means they are subject to the max message size restriction [1]. Using shmem avoids that problem (as well as, I think, avoiding a second copy in both processes).

[1] https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/ipc/glue/MessageLink.cpp#161

Comment 12

[Markus Stange [:mstange]]

Jed, can you double-check the state of things in the profiler now that bug 1466783 has landed? I'm pretty sure the profiler shmem contains the NUL byte at the end after that change.
And this may be a basic question, but how does changing nsDependentCString to nsDependentCSubstring introduce a copy?
And if the shmem contains the NUL byte, why are we passing aResult.Size<char>() as the length and not aResult.Size<char>() - 1?

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Markus Stange [:mstange] from comment #12)
> Jed, can you double-check the state of things in the profiler now that bug
> 1466783 has landed? I'm pretty sure the profiler shmem contains the NUL byte
> at the end after that change.

Does it?  This is untrusted input.  A compromised content process could send an unterminated string; or, if the parent tries to check for or enforce termination, the content process can just keep the memory mapped and overwrite the terminator after the check and before the actual use.

What I was trying to do here is have the shmem *not* contain the terminator, because it doesn't accomplish anything if we can't trust it.

> And this may be a basic question, but how does changing nsDependentCString
> to nsDependentCSubstring introduce a copy?

It's copied by PromiseFlatCString here: https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/tools/profiler/gecko/nsProfiler.cpp#530

See also xpcom/string/nsTPromiseFlatString.h.

> And if the shmem contains the NUL byte, why are we passing
> aResult.Size<char>() as the length and not aResult.Size<char>() - 1?

The shmem didn't contain the NUL byte when I wrote the patch.  After bug 1466783 it looks like it does include the NUL, but we're still violating nsDependentCString's precondition, and subtracting 1 from the length doesn't fix that if the content process is compromised.

Comment 14

[Markus Stange [:mstange]]

Thanks! So how should we proceed? Do you want to update the patch to make the shmem not include NUL anymore? Or should we just use "aResult.Size<char>() - 1" everywhere?

Comment 15

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Markus Stange [:mstange] from comment #14)
> Thanks! So how should we proceed? Do you want to update the patch to make
> the shmem not include NUL anymore? Or should we just use
> "aResult.Size<char>() - 1" everywhere?

It looks like that particular shmem needs to include the NUL now because of how ChunkedJSONWriteFunc::CopyDataIntoLazilyAllocatedBuffer works.  Subtracting 1 in nsProfiler::StartGathering would correct for it, but that still needs to be an nsDependentCSubstring for security.

Comment 16

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Patch: use nsDependentCSubstring with Shmem [v2]

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 8992818 [details] [diff] [review]
Patch: use nsDependentCSubstring with Shmem [v2]

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

Thanks!

Comment 18

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d10de6036b74

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/d10de6036b74

Comment 20

[Ryan VanderMeulen [:RyanVM]]

This grafts cleanly to Beta and ESR60 - should we nominate for backport?

Comment 21

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I need to look at this more closely; the profiler part in particular might be subtly wrong on other branches.

Comment 22

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

It looks like none of the other things being patched besides the profiler changed since ESR60.

The profiler part doesn't apply to (as in, isn't relevant for) and doesn't apply to (as in, the patch fails to auto-merge with) ESR60.  Beta is the problem there, because it has bug 1458246 but not bug 1466783.  I should be able to correct for that just by dropping the `- 1`.

Comment 23

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

It turns out that clipping off the last character of the child process profile doesn't break anything, because it's always '\n'.

But let's not do that.

Comment 24

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Patch for Beta 62

Comment 25

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Patch for ESR 60

Comment 26

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8992818 [details] [diff] [review]
Patch: use nsDependentCSubstring with Shmem [v2]

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Low risk sec-moderate.
Fix Landed on Version: 63

Approval Request Comment
[Feature/Bug causing the regression]: bug 1297539, bug 1275398, bug 1458246
[User impact if declined]: Possible sandbox escape vector; I think the impact is limited to reading data but I'm not 100% sure about that.
[Is this code covered by automated tests?]: Yes; in fact, I found some of these by pushing to Try with a debugging patch.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: These are all minor changes that shouldn't affect anything if the content process isn't malicious.
[String changes made/needed]: None.


WARNING: this patch doesn't crossport entirely cleanly; use the other attachments for landing.

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8997623 [details] [diff] [review]
Patch for Beta 62

Fix for a sec-moderate issue, has test coverage, let's uplift for beta 15.

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8992818 [details] [diff] [review]
Patch: use nsDependentCSubstring with Shmem [v2]

I approved the patch in https://bugzilla.mozilla.org/attachment.cgi?id=8997623 for beta rather than this one.

Comment 29

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

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8997624 [details] [diff] [review]
Patch for ESR 60

Fixes various IPC sec issues. Approved for ESR 60.2.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/f5636e2f42de