Re: Key management with tests

Greetings,

* Bruce Momjian (bruce(at)momjian(dot)us) wrote:
> On Wed, Feb 3, 2021 at 01:16:32PM -0500, Bruce Momjian wrote:
> > On Wed, Feb 3, 2021 at 10:33:57AM -0500, Stephen Frost wrote:
> > > I doubt anyone would actually stipulate that they *guarantee* detection
> > > of malicious writes, and I don't think we should either, but certainly
> > > the other systems which provide TDE do so in a manner that provides both
> > > confidentiality and integrity. The big O, at least, documents that they
> > > use SHA-1 for their integrity checking, though they also provide an
> > > option which disables it. If we used an additional fork to provide the
> > > integrity then we could also give users the option of either having
> > > integrity included or not.
> >
> > I thought more about this at an abstract level. If you are worried
> > about malicious users _reading_ data, you can encrypt the sensitive
> > parts, e.g., heap/index/WAL/temp, and leave some unencrypted, like
> > pg_xact. Reading pg_xact is pretty useless if you can't read the heap
> > pages. Reading postgresql.conf.auto, the external key retrieval
> > scripts, etc. are useless too.
> >
> > However, when you are trying to protect against write access, you have
> > to really encrypt _everything_, because the system is very
> > interdependent, and changing one part where _reading_ is safe can affect
> > other parts that must remain secure. You can modify
> > postgresql.conf.auto to capture the cluster key, or maybe even change
> > something to dump out the data keys from memory. You can modify pg_xact
> > to affect how heap pages are interpreted.
> >
> > My point is that being able to detect malicious heap/index writes really
> > doesn't gain us any security since there are much more serious writes
> > that can be made, and protecting against those more serious writes would
> > cause unacceptable Postgres source code changes which will probably
> > never be implemented.
>
> I looked further. First, I don't think we are going to be able to
> protect at all against users who have _write_ access on the OS running
> Postgres. It would be too easy to just read process memory, or modify
> ~/.profile.

I don't think anyone is really expecting that we'll be able to come up
with a way to protect against attackers who have fully compromised the
OS to the point where they can read/write OS memory, or even the PG unix
account. I'm certainly not suggesting that there is a way to do that or
that it's an attack vector we are trying to address here.

> I think the only possible option would be to try to give some protection
> against users with write access to PGDATA, where PGDATA is on another
> server, e.g., via NFS. We can't protect against all db modifications,
> for reasons outlined above, but we might be able to protect against
> write users being able to _read_ the keys and therefore decrypt data.

That certainly seems like a worthy goal. I also really want to stress
that I don't think anyone is expecting us to be able to "protect"
against users who have write access to the system- write access to files
is really an OS level issue and there's not much we can do once someone
has found a way to circumvent that (we can try to help the OS by doing
things like using SELinux, of course, but that's a different
discussion). At the point that an attacker has gotten write access, the
best we can do is complain loudly if we detect unexpected modifications.
Ideally, we would be able to do that for everything, but certainly doing
it for the principal data would go a long way and is far better than
nothing.

Now, that said, I don't know that we absolutely must have that in the
first release of TDE support for PG. In thinking about this, I would
say we have two basic options:

- Keep the same page layout, requiring that integrity data must be
stored elsewhere, eg: another fork
- Use a different page layout when TDE is enabled, making room for
integrity information to be included on each page

There's a set of pros and cons for these:

Same page layout pros:

- Simpler and less impactful on the overall system
- With integrity data stored elsewhere, could possibly be something
that's optional to enable/disable on a per-table basis
- Potential to do things like have an unencrypted primary and an
encrypted replica, providing an easier migration path

Same page layout cons:

- Integrity information must be stored elsewhere
- Increases the reads/memory that is needed, since we have to look up
the integrity information on every read.
- Increases the writes that have to be done since we'd be dirtying
multiple pages instead of just the main fork (though this isn't
exactly unusual- there's the vis map, and indexes, etc, but it'd be
yet another thing we're updating)

Different page layout pros:

- Avoids extra reads/writes for the integrity information
- Once done, this might provide us with a way to add other page level
information in the future while still being able to work with older
page formats

Different page layout cons:

- Wouldn't be able to have an encrypted replica follow an unencrypted
primary, migration would require logical replication or similar
- More core code changes, and extensions, to handle a different page
layout when cluster is initialized with TDE+integrity

While I've been thinking about this, I have to admit that either
approach could be done later and it's probably best to accept that and
push it off until we have the initial TDE work done. I had been
thinking that changing the page layout would be better to do in the same
release as TDE, but having been playing around with that approach for a
while it just seems like it's too much to try and include at the same
time. We should be sure to be clear and document that though.

> Looking at PGDATA, we have, at least:
>
> postgresql.conf
> pg_hba.conf
> postmaster.opts
> postgresql.conf.auto
>
> which could be exploited to cause reading of the cluster key or process
> memory. The first two can be located outside of PGDATA but the last two
> currently cannot.

There are certainly already users out there who intentionally make
postgresql.auto.conf owned by root/root, zero-sized, and monitor it to
make sure that it isn't updated. postgresql.conf actually is also often
monitored for changes by a change management system of some kind and may
also be owned by root/root already. I suspect that postmaster.opts is
not monitored as closely, but that's probably due more to the fact that
we don't really document it as a configuration system file and it can't
be put outside of PGDATA. Having a way to move it outside of PGDATA or
just not have it be used at all (do we really need it..?) would be
another way to address that risk though.

> The problem is that this is a limited use-case, and there are probably
> other problems I am not considering. It seems too error-prone to even
> try protect against this, but it does limit the value of this feature.

I don't think we need to consider it a failing of the capability every
time we think of something else that really should be addressed when
considering this attack vector. We aren't going to be releasing this
and saying "we guarantee that this protects against an attacker who has
write access to PGDATA". Instead, we would be documenting "XYZ, when
enabled, is used to validate the integrity of ABC data. Individuals
concerned with unexpected modifications to their system should consider
independently monitoring files D, E, F. Note that there is currently no
explicit protection against or detection of unexpected or malicious
modification of other parts of the system such as the transaction
record.", or something along those lines. Hardening guidelines would
also recommend things like having postgresql.conf moved out of PGDATA
and owned by root/root, etc. Users would then have the ability to
evaluate if what we're providing is sufficient for their requirements
or not, and to then provide us with feedback about what they feel is
still missing before they would be able to use PG for their use-case.

To that end, I would hope that we'd eventually develop a way to detect
unexpected modifications in other parts of the system, both as a way to
discover filesystem corruption earlier but also in the case of a
malicious attacker. The latter would involve more work, of course, but
it doesn't seem insurmountable. I don't think it's necessary to get
into that today though.

I am concerned when statements are made that we are just never going to
do something-or-other because we think it'd be a lot of source code
changes or won't be completely perfect against every attack we can think
of. There was a good bit of that with RLS which also made it a
particularly difficult feature to push forward, but, thanks to clearly
documenting what was and wasn't addressed, clearly admitting that there
are covert channel attacks that might be possible due to how it works,
it's been pretty well accepted and there hasn't been some huge number of
issues or CVEs that have been associated with it or mismatched
expectations that users of it have had regarding what it does and
doesn't protect against.

Thanks,

Stephen