Implement sandbox to run WAL redo routines in

[hlinnaka]

No description provided.

[knizhnik]

I wonder what kind of attacks we want to address here?
What kind of vulnerabilities and injections we are expecting here?

Do we assume that Postgres core (without user extension) always produce valid WAL records?

If we are talking about extension, then do we plan to support only well known (verified) extension or any user defined extension? How can extension produce incorrect WAL records? Does extension use custom WAL records (or logical WAL records) and we expect that Postgres is not checking their correctness? Or do we expect that "Byzantine" extension will get direct access to WAL files and intentionally corrupt them? If so, how we are going to protect ourselves from DooS kind of attacks, when WAL records are formally valid, but cause some very expensive operations consuming a lot of disk and CPU resources? Sandbox will not help much in this case, doesn't it?

Final question: if we rewrite redo handlers in some safe language, like Rust, then sandbox will not needed, will it? I looked at redo handlers and it seems to be non-trivial, because them are using a lot AM specific code. But it should be more efficient that running Postgres in VM.

As far as I understand the primary idea of multi-tenant storage nodes was to reduce per-client overhead. But if we have to proceed WAL records of each client in separate VM, will it be even more expensive than avoid multi-tenancy support at storage level as well as at computation nodes, and running separate Pager process for each client?

[kelvich]

Do we assume that Postgres core (without user extension) always produce valid WAL records?

User can buy exploit for yet-not-disclosed CVE and take over the processing node -- after that we can expect malicious WAL to come into the storage node. It may just crash it or take over the node if there is some bug in redo. Knowing the code that we are running at processing node (postgres redo) this user may look for wrong boundary checks around wal reading and just execute his own code. So IMO the whole attack is not a theoretical one -- attacker will just need to wait few month until next CVE to write malicious WAL and finding exploitable problem in the redo code should also not be a big problem.

if we rewrite redo handlers in some safe language, like Rust, then sandbox will not needed, will it?

Rust safety means that type system when it is properly used prevents data races and dangling pointers. But array boundary checks are still on programmer -- and that can lead to the problems.

it should be more efficient that running Postgres in VM

I believe we may have same performance between host page server <-> postgres in vm as it is between two processes talking to each other via shared memory queue. We actually may avoid running OS in vm at all (not sure that we should, but we can) -- just some parts of postgres reading one memory queue and writing to another one, no files, no processes, etc.

Some links:

https://firecracker-microvm.github.io/
https://mirage.io/
https://qemu.readthedocs.io/en/latest/system/ivshmem.html

[knizhnik]

But array boundary checks are still on programmer -- and that can lead to the problems.

I am not Rust expert, but it seems to be not true.
Rust performs out-of-bounds checking at runtime.
May be there is some compiler switch which allows to disable them or may be compiler can avoid them when it can prove that there is no out-of-bounds access, but by default them are performed:
https://users.rust-lang.org/t/bound-checking-really-present/21809
https://users.rust-lang.org/t/how-to-avoid-bounds-checking/4433/4

[kelvich]

Yes, it is usually switched on in a debug build and switched off for release build. We may keep that for release, but working with raw memory means unsafe { } code blocks with all guarantees off. To be able to write something to rust array/vector from WAL you would need to first read len from raw buffer, then memcpy() some data of that len. After that rust runtime can do boundary checks. But will you believe this len in a first place? -- that is what I meant by manual boundary checks. So I don't understand how rewriting redo can generally help. It may be harder or impossible to penetrate such a system, but causing crash/oom can be done instead.

[kelvich]

Thinking about that a bit more it seems that we may skip calling VM redo code in getPage requests and use redo only to do periodical WAL chains compaction. So on getPage request we may return page and few records on top of it and use VM only to full page image from time to time (to avoid sending that from compute node) when WAL chain becomes big.

[adsharma]

XLogInsert(RmgrId rmid, uint8 info) - can we get a frequency histogram of <rmid, info> for a typical WAL?

That'll help us determine how much needs to be implemented in rust and where we can defer to redo code running elsewhere?

[kelvich]

One wild idea that came once during a talk with @ericseppanen is that if we will be able to compile and run postgres for a wasm target we also may use it as a sandbox to apply WAL and while wasm should be generally slower absence of inter-vm communication may speed things up. I don't expect that this approach would be really working for variety of reasons but it worth of investigating.

[hlinnaka]

Yeah, webassembly is worth investigating. Another interesting project I bumped into recently is https://gvisor.dev/. Also, there's https://github.com/rust-vmm for a full VM-level sandbox

[ericseppanen]

I threw out the wasm idea as a random crazy thought, but having thought about it more I don't think sandboxing is a great idea.

I would think that WAL processing is performance-critical. Having to cross a sandbox boundary seems like a lot of overhead.

If we could reimplement WAL handling in Rust, we would get a memory safety guarantee. That code would have to be free of Rust panics (which could be used as DoS attacks), but that's not too difficult.

There are some earlier comments above talking about bounds checks that I think are incorrect: Rust bounds checks are always enabled: there is no way (short of a buggy library using unsafe code) to disable bounds checks in release mode or in any other mode. The only things that I can think of that are off by default in release mode are integer overflow checks, and they can be turned back on in Cargo.toml.

[hlinnaka]

If we could reimplement WAL handling in Rust, we would get a memory safety guarantee. That code would have to be free of Rust panics (which could be used as DoS attacks), but that's not too difficult.

Yes. But then we would have to reimplement the WAL handling in Rust. And keep maintaining that for every PostgreSQL major release. Sometimes there have been bug fixes in minor releases too. It's not a huge amount of code, but it's still a fair amount and it's pretty deep down and scattered in the Postgres sources. I would really rather not go there if we can find another way.

The first TODO here is to set up some performance testing, though, to see how much overhead we're really talking about. It might be a non-issue, given all the other overhead, or it could be super-critical. Or maybe it can be mitigated e.g. by writing a full-page image WAL record every 1000 modifications to a page in the compute node, so that there's less WAL to replay in the common cases.

On a more philosophical level, I wish we can find a good, fast, way to run safely Postgres code in page server and elsewhere. Aside from replaying WAL, that will open a lot of possibilities like doing logical decoding, and running VACUUM and other background jobs in the background.

[ericseppanen]

And keep maintaining that for every PostgreSQL major release. Sometimes there have been bug fixes in minor releases too.

We could have fuzz tests that run against every release. The fuzzer could generate inputs, run them through postgres, run the same inputs through our code, and verify the result is the same.

It's not a huge amount of code, but it's still a fair amount and it's pretty deep down and scattered in the Postgres sources.

Would we need to extract the "deep down" parts into a standalone module, to run in the sandbox? Or is the sandboxed code a full copy of postgres?

[ericseppanen]

I do agree that we should understand the performance first. I was guessing that this is performance-critical-- if that's not true, then I don't think heroic "rewrite it in rust" efforts are called for :)

[ericseppanen]

Another idea to add to the list here: Linux SECCOMP_SET_MODE_STRICT will allow you to run a process that can only read & write from an already-opened file descriptor. I think the intent is that the sandboxed program inherits some pipe fds for communication, and isn't allowed to do anything else.

[ericseppanen]

I chatted with @kelvich about this. I will try to summarize:

• wasm will take an unknown amount of effort to work. I'm scared of this becoming an infinite time-sink.
• It's probably too early to talk about pushing this work to a cloud-level VM.
• Firecracker VMs look OK. Should investigate.
• containers don't provide great security guarantees (large syscall surface area). dockerd is overkill, runc is a little under-developed and hard to get right.
• Linux seccomp looks OK. Should investigate.

We didn't talk much about the "native Rust" option. My current thinking:

"Rewrite it in rust" may take a lot of time, and will need constant maintenance to stay in sync with new Postgres releases. So even if we might like this as a long-term option, it would be nice to have something easy in the short term. Plus, even if we do a Rust wal_redo implementation, it would be good to keep the C wal_redo_main so we can verify that the two implementations return the same result.

[hlinnaka]

neondatabase/postgres#52 will take care of this, with seccomp

[mklgallegos]

hi @funbringer it looks like this issue is waiting on a code review from you. Not trying to bother, just wanted to check in since this issue is schedule to be completed in milestone v0.1 (concludes at end of the month).

Let me know if this looks like something that will slip past the end of the month.

[funbringer]

@mklgallegos Eric's patch has been superseded by neondatabase/postgres#56, and I've already pushed a bunch of bug-fixes in neondatabase/postgres#58. There's also an on-going discussion here, which will hopefully be resolved after I've finished yet another patch.

[mklgallegos]

Hi @funbringer - this item was put into the done category during the milestone v0.2 planning session on 2021-07-30. I'm closing out. Please re-open if I am mistaken.