xsnap worker process inherits fds from kernel process #6055
Comments
warner
added
bug
CLOEXEC discussion on stackoverflow strongly suggests that we can.
Doesn't look like it.
It's not clear whether this is exposed to JS; I do see |
|
Any reason we're not spawning xsnap processes as Edit: I'm not sure if it'd help in this case. |
|
We no longer have LMDB at all so this particular fd is no longer an issue. The larger issue of fds being inherited is non-ideal. It is low risk because a contract still will not have access to a file descriptor duped into its vat. |
ivanlei
changed the title
|
We might still be leaking other FDs into the worker, like the SLOG_SENDER pipe. We should triple-check that we aren't leaking existing worker pipes into the environment of a new worker, but I just looked at a pismoA follower node and |
If |
toliaqat
added
good first issue
|
I just did a new which says that there are no DB files open, just the |
|
That sounds like we can close this issue then ? |
|
I've straced scenario2 to capture all of the files opened, until it settled into a steady state: Captured PIDs of what's running in the container before stopping the scenario2: I then inspected strace.txt manually and filtered out irrelevant log entries to see which, if any, files are being opened without Finally, I've captured and inspected As we can see:
Conclusion: |
|
To make sure that we don't regress, we could migrate that strace work from scenario2 to a3p-integration and add a z:acceptance test. |
Why a3p? That seems overkill. It doesn't need to be an integration test, just a "unit" test, or at least not a test that requires so much setup ceremony. |
Only because scenario2 was chosen for the manual testing. |
Just to be clear: all of that strace work had been done by hand with lots of eyeballing in between. Any sort of "migrate" or "add to automated testing" actually means an automated test has to be developed and tested with reproducible synthetic positive and negative results before it can catch any possible unintended leaks in the future. This sounds like a whole separate issue which has to be filed, prioritized, and scheduled. |
|
Yeah that's definitely not worth it. |
It's definitely a lot of work. But increasingly, that sort of work is included in the test plan for features / fixes such as this one. Whether it's cost-effective is always a judgement call, and in this case the call has been made, but in general, I think but we try not to postpone testing to separate issues. |
|
Just to explicitly confirm whether SQLite does use As can be seen above, SQLite does in fact use |
|
Another experiment, courtesy of ChatGPT: This is what we are looking for: |
|
Go: |
|
Node uses Golang does the same: So does SQLite: |
Describe the bug
Ari and I were looking at
lsofoutputs, and noticed that all of ourxsnapworker processes are holding open a file descriptor on the LMDBdata.mdbfile (which holds the kvStore).The worker would never open that file itself, which tells me that it was open in the parent process (kernel/Node.js) and inherited by the worker during the
spawn(). Which tells me that LMDB is not opening that file handle with theCLOEXEC"please close this fd when youexecinto a new process" flag.xsnapdoesn't know that this fd is open, but nevertheless we should not be allowing xsnap to see it. When we manage to implement #2386 andseccomp, this extra fd would represent inappropriate authority (an attacker who manages to compromise the worker process could use it to compromise the rest of the kernel, even thoughseccompprevents them from opening any new files).I don't remember if you can set
CLOEXECon a filehandle after it's been opened. We should look at the LMDB JS bindings and see if it has a way to influence this.Another option is to change
xsnapand have it conservatively close all fds higher than the command pipes, at startup.The text was updated successfully, but these errors were encountered: