Re: Key management with tests

From: Bruce Momjian <bruce(at)momjian(dot)us>
To: Stephen Frost <sfrost(at)snowman(dot)net>
Cc: Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Tom Kincaid <tomjohnkincaid(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Andres Freund <andres(at)anarazel(dot)de>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com>
Subject: Re: Key management with tests
Date: 2021-02-03 18:16:32
Message-ID: 20210203181632.GA11069@momjian.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Wed, Feb 3, 2021 at 10:33:57AM -0500, Stephen Frost wrote:
> > I am thinking group read-access might be a requirement for cluster file
> > encryption to be effective.
>
> People certainly do use group read-access, but I don't see that as being
> a requirement for cluster file encryption to be effective, it's just one
> thing TDE can address, among others, as discussed.

Agreed.

> > This also does not protect against users who have read access to
> > database process memory &mdash; all in-memory data pages and data
> > encryption keys are stored unencrypted in memory, so an attacker who
> > --> is able to read memory can decrypt the entire cluster. The Postgres
> > --> operating system user and the operating system administrator, e.g.,
> > --> the <literal>root</literal> user, have such access.
>
> That's helpful, +1.

Good.

> > > Uh, well, they could modify postgresql.conf to change the script to save
> > > the secret returned by the script before returning it to the PG server.
> > > We could require postgresql.conf to be somewhere secure, but then how do
> > > we know that is secure? I just don't see a clean solution here, but the
> > > idea that you write and then wait for the key to show up seems like a
> > > very valid way of attack, and it took me a while to be able to
> > > articulate it.
>
> postgresql.conf isn't always writable by the postgres user, though
> postgresql.auto.conf is likely to always be. I'm not sure how much of a
> concern that is, but it we wanted to take steps to explicitly address
> this issue, we could have some kind of 'secure' postgresql.conf file
> which we would encourage users to make owned by root and whose values
> wouldn't be allowed to be overridden once set.

Well, I think there is a lot more than postgresql.conf to worry about ---
see below.

> > Let's suppose you lock down your cluster --- the non-PGDATA files are
> > owned by root, postgresql.conf and pg_hba.conf are moved out of PGDATA
> > and are not writable by the database OS user, or we have the PGDATA
> > directory on another server, so the adversary can only write to the
> > remote PGDATA directory.
> >
> > What can they do? Well, they can't modify pg_proc to add a shared
> > library since pg_proc is encrypted, so we have to focus on files needed
> > before encryption starts or files that can't be easily encrypted.
>
> This isn't accurate- just because it's encrypted doesn't mean they can't
> modify it. That's exactly why integrity is important, because an
> attacker absolutely could modify the files directly and potentially
> exploit the system through those modifications.

They can't easily modify it to inject a shared object referenced into a
system column, was my point --- also see below.

> > They could create postgresql.conf.auto in PGDATA, and modify
> > cluster_key_command to capture the key, or they could modify preload
> > libraries or archive command to call a command to read memory as the PG
> > OS user and write the key out somewhere, or use the key to rewrite the
> > database files --- those wouldn't even need a database restart, just a
> > reload.
>
> They would need to actually be able to effect that reload though. This
> is where the question comes up as to just what attack vector we're
> trying to address. It's certainly possible that an attacker has only
> access to the stored data in an off-line fashion (eg: a hard drive that
> was mistakenly thrown away without being properly wiped) and that's one
> of the cases which is addressed by cluster encryption. An attacker
> might have access to the LUN that PG is running on but not to the
> running server itself, which it seems like is what you're contemplating
> here. That's a much harder attack vector to fully protect against and
> we might need to do more than we're currently contemplating to address
> it- but I don't think we necessarily must solve for all cases in the
> first pass at this.

See below.

> > They could also modify pg_xact files so that, even though the heap/index
> > files are encrypted, how the contents of those files are interpreted
> > would change.
>
> Yes, ideally, we'd encrypt/integrity check just about every part of the
> running system and that's one area the patch doesn't address- things
> like temporary files and other parts.

It is worse than that --- see below.

> > In summary, to detect malicious user writes, you would need to protect
> > the files used before encryption starts (root owned or owned by another
> > user?), and encrypt all files after encryption starts --- any other
> > approach would probably leave open attack vectors, and I don't think
> > there is sufficient community desire to add such boundaries.
>
> There's going to be some attack vectors that TDE doesn't address. We
> should identify and document those where we're able to. We could offer
> up some mitigations (eg: strongly suggest monitoring of key utilization
> such that if the KEK is used without a reboot of the system or similar
> happening that it is reported and someone goes to look into it). While
> such mitigations aren't perfect, they can be enough to allow approval of
> a system to go operational (ultimately it comes down to what the
> relevant security officer is willing to accept).

I ended up adding to the feature description in the docs to clearly
outline what this feature provides, and what it does not:

The purpose of cluster file encryption is to prevent users with read
access on the directories used to store database files and write-ahead
log files from being able to access the data stored in those files.
For example, when using cluster file encryption, users who have read
access to the cluster directories for backup purposes will not be able
to decrypt the data stored in these files. Read-only access for a group
of users can be enabled using the <application>initdb</application>
<option>--allow-group-access</option> option. Cluster file encryption
also provides data-at-rest security, protecting users from data loss
should the physical storage media be stolen or improperly erased before
disposal.

Cluster file encryption does not protect against unauthorized file
system writes. Such writes can allow data decryption if used to weaken
the system's security and the weakened system is later supplied with
the externally-stored cluster encryption key. This also does not always
detect if users with write access remove or modify database files.

This also does not protect against users who have read access to database
process memory because all in-memory data pages and data encryption keys
are stored unencrypted in memory. Therefore, an attacker who is able
to read memory can read the data encryption keys and decrypt the entire
cluster. The Postgres operating system user and the operating system
administrator, e.g., the <literal>root</literal> user, have such access.

> > How do other database systems guarantee to detect malicious writes?
>
> 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.

My summary point is that we should clearly spell out exactly what
protections we are offering, and an estimate of the code impact, before
moving forward so the community can agree it is worthwhile to add this.

Also, looking at the PCI DSS 3.2.1 spec from May 2018 (click-through
required):

https://www.pcisecuritystandards.org/document_library?category=pcidss&document=pci_dss#agreement

or open PDF link here:

https://commerce.uwo.ca/pdf/PCI_DSS_v3-2-1.pdf

Page 41 covers what they expect from an encrypted file system, and from
key encryption key and data encryption keys. There is a v4.0 spec in
draft but I can't find a PDF available online.

--
Bruce Momjian <bruce(at)momjian(dot)us> https://momjian.us
EDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Eisentraut 2021-02-03 19:18:33 Re: Dumping/restoring fails on inherited generated column
Previous Message Tom Lane 2021-02-03 17:53:12 Re: Removing support for COPY FROM STDIN in protocol version 2