1198552 - Memory reporter opens /proc/self/{statm,smaps} in sandboxed content processes

Status: NEW

(Toolkit :: about:memory, defect)

Description

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

The memory reporter opens /proc/self/{statm,smaps} in sandboxed content processes, to get info on the process's memory usage.  This will need to be whitelisted in the sandbox broker policy for now, but there might be a better way to handle it.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

I have been told that it is flooding log kernel message now:
<Corsac> Oct  3 23:12:42 scapa kernel: [40278.600669] grsec: denied read of sensitive /proc/pid/statm entry via fd passed across exec by /usr/lib/firefox/firefox[Web Content:3451] uid/euid:1000/1000 gid/egid:1000/1000, parent /usr/lib/firefox/firefox[Gecko_IOThread:3343] uid/euid:1000/1000 gid/egid:1000/1000

gcp, do you care about that for the l3 sandboxing?

Comment 2

[Gian-Carlo Pascutto [:gcp]]

We do, although that appears to be a grsecurity kernel, not a normal one. This might end up with quite some work on the memory reporter code...

Comment 3

[Gian-Carlo Pascutto [:gcp]]

By the way, you might get this warning as far back as Firefox 54, I think? Even with write-only sandboxing, the file broker will still intercept the read and use fd passing to the child.

Comment 4

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

This has, in fact, been observed in Firefox 54: https://github.com/NixOS/nixpkgs/issues/25743#issuecomment-319113908

This bug probably belongs in the about:memory component.  I suspect that the procfs memory info isn't extremely important on desktop (or at least less important than it was on B2G); not unimportant, but maybe not important enough to make nontrivial changes just for the benefit of grsec kernels.  But I'll leave that to the people who are actively working on memory reporting these days.

One possible fix — and this does seem to work with procfs files — would be to open the files during startup, keep them open, and seek back to the beginning between reads.  (In this case everything is main-thread-only if I recall correctly, but for thread safety the same idea using pread() could work.)

Alternately, there's the option of having the parent process compute the system memory info on the content processes's behalf and adjoin it to their reports when they're sent up over IPC.  I was concerned that there might be some other grsec restriction about that, but what I've found suggests that same-user procfs access is allowed.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Moving component because I agree with jld. The sandboxing layer is doing what it has to do and grsecurity is "correctly" [1] warning that sensitive info is traveling to the parent. It's up to the memory reporting layer to decide if they really need this info or not. Any solution on the sandboxing side is always going to be a gross hack.

[1] I'm using quotation marks because it has no problem with this information AND everything else being available to the process that's exposed to the internet, yet warns when it's one of the only things available. So the warning is a bit backwards in this case.

Comment 6

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

(In reply to Gian-Carlo Pascutto [:gcp] from comment #5)
> [1] I'm using quotation marks because it has no problem with this
> information AND everything else being available to the process that's
> exposed to the internet, yet warns when it's one of the only things
> available. So the warning is a bit backwards in this case.

For what it's worth, the grsec maintainer mentioned that this is “to avoid fooling some suid apps into infoleak”, which is a little more understandable… and makes this a false positive, because the opener's and reader's credentials are equivalent.

Comment 7

[Nicholas Nethercote [inactive]]

Here's my understanding of things.

xpcom/base/nsMemoryReporterManager.cpp
- Reads /proc/self/statm to get the "vsize" and "resident" measurements, which are important.
- Reads /proc/self/smaps to get the "resident-unique" measurement, which is less important though still useful.
- These happen in the individual processes, both chrome and content.

xpcom/base/SystemMemoryReporter.cpp
- Reads /proc/self/smaps to implement the entire "system" memory reporter, which is only enabled if the "memory.system_memory_reporter" pref is set (and it's off by default).
- This only runs in the chrome process.

So only nsMemoryReporterManager.cpp needs changing. It shouldn't be too hard to get the chrome process to read /proc/<pid>/{statm,smaps} on behalf of all the content processes, and also report the measurements to the memory reporter manager on their behalf, and then the content processes wouldn't need to do anything.

jld, does all that sound right to you?

Comment 8

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

(In reply to Nicholas Nethercote [:njn] from comment #7)
> So only nsMemoryReporterManager.cpp needs changing. It shouldn't be too hard
> to get the chrome process to read /proc/<pid>/{statm,smaps} on behalf of all
> the content processes, and also report the measurements to the memory
> reporter manager on their behalf, and then the content processes wouldn't
> need to do anything.
> 
> jld, does all that sound right to you?

Yes, that sounds good.

Comment 9

[spender]

The current grsecurity code won't have any problems with this, so there's no need to add a workaround if it's solely for us.  I apologize for the delay on this, but I only heard about the issue relatively recently via searching on Twitter.  Please feel free to loop me in on problems like this in the future ([e-mail]) to avoid needless frustration; we definitely don't want to be putting Mozilla in a position of implementing workarounds for us, or wasting time on things we can resolve quickly together.

I had a very good experience working with Robert O'Callahan to eliminate the need for workarounds in 'rr' that didn't require weakening the security properties of restrictions it had trouble with.

Thank you!
-Brad

Comment 10

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Update_affecteds