Subject: XTS cipher mode for cluster file encryption

From: Bruce Momjian <[email-masked]>
Date: Wed, 13 Oct 2021 18:26:48 -0400

As you might have seen from my email in another thread, thanks to
Stephen and Cybertec staff, I am back working on cluster file
encryption/TDE.

Stephen was going to research if XTS cipher mode would be a good fit for
this since it was determined that CTR cipher mode was too vulnerable to
IV reuse, and the LSN provides insufficient uniqueness.  Stephen
reported having trouble finding a definitive answer, so I figured I
would research it myself.

Of course, I found the same lack of information that Stephen did.  ;-)
None of my classic cryptographic books cover XTS or the XEX encryption
mode it is based on, since XTS was only standardized in 2007 and
recommended in 2010.  (Yeah, don't get me started on poor cryptographic
documentation.)

Therefore, I decide to go backward and look at CTR and CBC to see how
the nonce is used there, and if XTS fixes problems with nonce reuse.

First, I originally chose CTR mode since it was a streaming cipher, and
we therefore could skip certain page fields like the LSN.  However, CTR
is very sensitive to LSN reuse since the input bits generate encrypted
bits in exactly the same locations on the page. (It uses a simple XOR
against a cipher).  Since sometimes pages with different page contents
are encrypted with the same LSN, especially on replicas, this method
failed.

Second is CBC mode. which is a block cipher.  I thought that meant that
you could only encrypt 16-byte chunks, meaning you couldn't skip
encryption of certain page fields unless they are 16-byte chunks. 
However, there is something called ciphertext stealing
(https://en.wikipedia.org/wiki/Ciphertext_stealing#CBC_ciphertext_stealing)
which allows that.  I am not sure if OpenSSL supports this, but looking
at my OpenSSL  1.1.1d manual entry for EVP_aes, cipher stealing is only
mentioned for XTS.

Anyway, CBC mode still needs a nonce for the first 16-byte block, and
then feeds the encrypted output of the first block as a IV to the second
block, etc. This gives us the same problem with finding a nonce per
page.  However, since it is a block cipher, the bits don't output in the
same locations they have on input, so that is less of a problem.  There
is also the problem that the encrypted output from one 16-byte block
could repeat, causing leakage.

So, let's look how XTS is designed.  First, it uses two keys.  If you
are using AES128, you need _two_ 128-bit keys.  If using AES256, you
need two 256-bit keys.  The first of the two keys is used like normal,
to encrypt the data.  The second key, which is also secret, is used to
encrypt the values used for the IV for the first 16-byte block (in our
case dboid, relfilenode, blocknum, maybe LSN).  This is most clearly
explained here:

	https://www.kingston.com/unitedstates/en/solutions/data-security/xts-encryption

That IV is XOR'ed against both the input value and the encryption output
value, as explained here as key tweaking:

	https://crossbowerbt.github.io/xts_mode_tweaking.html

The purpose of using it before and after encryption is explained here:

	https://crypto.stackexchange.com/questions/24431/what-is-the-benefit-of-applying-the-tweak-a-second-time-using-xts

The second 16-byte block gets an IV that is the multiplication of the
first IV and an alpha value raised to the second power but mapped to a
finite field (Galois field, modulus a prime).  This effectively means an
attacker has _no_ idea what the IV is since it involves a secret key,
and each 16-byte block uses a different, unpredictable IV value. XTS
also supports ciphertext stealing by default so we can use the LSN if we
want, but we aren't sure we need to.

Finally, there is an interesting web page about when not to use XTS:

	https://sockpuppet.org/blog/2014/04/30/you-dont-want-xts/

Basically, what XTS does is to make the IV unknown to attackers and
non-repeating except for multiple writes to a specific 16-byte block
(with no LSN change).  What isn't clear is if repeated encryption of
different data in the same 16-byte block can leak data.

This probably needs more research and maybe we need to write something
up like the above and let security researchers review it since there
doesn't seem to be enough documentation for us to decide ourselves.

-- 
  Bruce Momjian  <[email-masked]>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

---

From: Stephen Frost <[email-masked]>
Date: Fri, 15 Oct 2021 15:22:48 -0400

Greetings,

> want, but we aren't sure we need to.

Yeah, this all seems to be about where I got to too.

> 	https://sockpuppet.org/blog/2014/04/30/you-dont-want-xts/

This particular article always struck me as more of a reason for us, at
least, to use XTS than to not- in particular the very first comment it
makes, which seems to be pretty well supported, is: "XTS is the de-facto
standard disk encryption mode."  Much of the rest of it is the well
trodden discussion we've had about how FDE (or TDE in our case) doesn't
protect against all the attack vectors that sometimes people think it
does.  Another point is that XTS isn't authenticated- something else we
know quite well around here and isn't news.

> different data in the same 16-byte block can leak data.

Any time a subset of the data is changed but the rest of it isn't,
there's a leak of information.  This is a really good example of exactly
what that looks like:

https://github.com/robertdavidgraham/ecb-penguin

In our case, if/when this happens (no LSN change, repeated encryption
of the same block), someone might be able to deduce that hint bits were
being updated/changed, and where some of those are in the block.

That said, I don't think that's really a huge issue or something that's
a show stopper or a reason to hold off on using XTS.  Note that what
those bits actually *are* isn't leaked, just that they changed in some
fashion inside of that 16-byte cipher text block.  That they're directly
leaked with CTR is why there was concern raised about using that method,
as discussed above and previously.

> doesn't seem to be enough documentation for us to decide ourselves.

The one issue identified here is hopefully answered above and given that
what you've found matches what I found, I'd argue that moving forward
with XTS makes sense.

The other bit of research that I wanted to do, and thanks for sending
this and prodding me to go do so, was to look at other implementations
and see what they do for the IV when it comes to using XTS, and this is
what I found:

https://wiki.gentoo.org/wiki/Dm-crypt_full_disk_encryption

Specifically: The default cipher for LUKS is nowadays aes-xts-plain64

and then this:

https://gitlab.com/cryptsetup/cryptsetup/-/wikis/DMCrypt

where plain64 is defined as:

plain64: the initial vector is the 64-bit little-endian version of the
sector number, padded with zeros if necessary

That is, the default for LUKS is AES, XTS, with a simple IV.  That
strikes me as a pretty ringing endorsement.

Now, to address the concern around re-encrypting a block with the same
key+IV but different data and leaking what parts of the page changed, I
do think we should use the LSN and have it change regularly (including
unlogged tables) but that's just because it's relatively easy for us to
do and means an attacker wouldn't be able to tell what part of the page
changed when the LSN was also changed.  That was also recommended by
NIST and that's a pretty strong endorsement also.

I'm all for getting security folks and whomever else to come and review
this thread and chime in with their thoughts, but I don't think it's a
reason to hold off on moving forward with the approach that we've been
converging towards.

Thanks!

Stephen

---

From: Tomas Vondra <[email-masked]>
Date: Fri, 15 Oct 2021 22:57:03 +0200

> as discussed above and previously.

Yeah. With CTR you pretty learn where the hint bits are exactly, while 
with XTS the whole ciphertext changes.

This also means CTR is much more malleable, i.e. you can tweak the 
ciphertext bits to flip the plaintext, while with XTS that's not really 
possible - it's pretty much guaranteed to break the block structure. Not 
sure if that's an issue for our use case, but if it is then neither of 
the two modes is a solution.

> what you've found matches what I found, I'd argue that moving forward

+1

> strikes me as a pretty ringing endorsement.

Seems reasonable, on the assumption the threat models are the same.

> NIST and that's a pretty strong endorsement also.

Not sure - it seems a bit weird to force LSN change even in cases that 
don't generate any WAL. I was not following the encryption thread and 
maybe it was discussed/rejected there, but I've always imagined we'd 
have a global nonce generator (similar to a sequence) and we'd store it 
at the end of each block, or something like that.

> reason to hold off on moving forward with the approach that we've been

+1

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Robert Haas <[email-masked]>
Date: Fri, 15 Oct 2021 17:02:38 -0400

> strikes me as a pretty ringing endorsement.

Yes, that sounds promising. It might not hurt to check for other
precedents as well, but that seems like a pretty good one.

I'm not very convinced that using the LSN for any of this is a good
idea. Something that changes most of the time but not all the time
seems more like it could hurt by masking fuzzy thinking more than it
helps anything.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Andres Freund <[email-masked]>
Date: Fri, 15 Oct 2021 14:21:09 -0700

Hi,

On 2021-10-15 15:22:48 -0400, Stephen Frost wrote:
> standard disk encryption mode."

I don't find that line of argument *that* convincing. The reason XTS is the
de-facto standard is that for generic block layer encryption is that you can't
add additional data for each block without very significant overhead
(basically needing journaling to ensure that the data doesn't get out of
sync). But we don't really face the same situation - we *can* add additional
data.

With something like AES-GCM-SIV we can use the additional data to get IV reuse
resistance *and* authentication. And while perhaps we are ok with the IV reuse
guarantees XTS has, it seems pretty clear that we'll want want guaranteed
authenticity at some point. And then we'll need extra data anyway.

Thus, to me, it doesn't seem worth going down the XTS route, just to
temporarily save a bit of implementation effort. We'll have to endure that
pain anyway.

Greetings,

Andres Freund

---

From: Tomas Vondra <[email-masked]>
Date: Fri, 15 Oct 2021 23:26:01 +0200

> precedents as well, but that seems like a pretty good one.

TrueCrypt/VeraCrypt uses XTS too, I think. There's an overview of other 
FDE products at [1], and some of them use XTS, but I would take that 
with a grain of salt - some of the products are somewhat obscure, very 
old, or both.

What is probably more interesting is that there's an IEEE standard [2] 
dealing with encrypted shared storage, and that uses XTS too. I'd bet 
there's a bunch of smart cryptographers involved.

[1] https://en.wikipedia.org/wiki/Comparison_of_disk_encryption_software

[2] https://en.wikipedia.org/wiki/IEEE_P1619

> seems more like it could hurt by masking fuzzy thinking more than it

I haven't been following the discussion about using LSN, but I agree 
that while using it seems convenient, the consequences of some changes 
not incrementing LSN seem potentially disastrous, depending on the 
encryption mode.

regards

---

From: Bruce Momjian <[email-masked]>
Date: Sat, 16 Oct 2021 10:16:25 -0400

On Fri, Oct 15, 2021 at 10:57:03PM +0200, Tomas Vondra wrote:
> Yeah. With CTR you pretty learn where the hint bits are exactly, while with
> XTS the whole ciphertext changes.
> 
> sure if that's an issue for our use case, but if it is then neither of the
> two modes is a solution.

Yes, this is a vary good point.  Let's look at the impact of _not_ using
the LSN.  For CTR (already rejected) bit changes would be visible by
comparing old/new page contents.  For CBC (also not under consideration)
the first 16-byte block would show a change, and all later 16-byte
blocks would show a change.  For CBC, you see the 16-byte blocks change,
but you have no idea how many bits were changed, and in what locations
in the 16-byte block (AES uses substitution and diffusion).  For XTS,
because earlier blocks don't change the IV used by later blocks like
CBC, you would be able to see each 16-byte block that changed in the 8k
page.  Again, you would not know the number of bits changed or their
locations.

Do we think knowing which 16-byte blocks on an 8k page change would leak
useful information?  If so, we should use the LSN and just accept that
some cases might leak as described above.  If we don't care, then we can
skip the use of the LSN and simplify the patch.

> Not sure - it seems a bit weird to force LSN change even in cases that don't
> generate any WAL. I was not following the encryption thread and maybe it was
> discussed/rejected there, but I've always imagined we'd have a global nonce
> generator (similar to a sequence) and we'd store it at the end of each
> block, or something like that.

Storing the nonce in the page means more code complexity, possible
performance impact, and the inability to create standbys via binary
replication that use cluster file encryption.

As a final comment to Andres's email, adding a GCM has the problems
above, plus it wouldn't detect changes to pg_xact, fsm, vm, etc, which
could also affect the integrity of the data.  Someone could also restore
and old copy of a patch to revert a change, and that would not be
detected even by GCM.

I consider this a checkbox feature and making it too complex will cause
it to be rightly rejected.

---

From: Andres Freund <[email-masked]>
Date: Sat, 16 Oct 2021 09:15:05 -0700

Hi,

> it to be rightly rejected.

You're just deferring / hiding the complexity. For one, we'll need integrity
before long if we add encryption support. Then we'll deal with a more complex
on-disk format because there will be two different ways of encrypting. For
another, you're spreading out the security analysis to a lot of places in the
code and more importantly to future changes affecting on-disk data.

If it's really just a checkbox feature without a real use case, then we should
just reject requests for it and use our energy for useful things.

Greetings,

Andres Freund

---

From: Bruce Momjian <[email-masked]>
Date: Sat, 16 Oct 2021 12:28:51 -0400

> just reject requests for it and use our energy for useful things.

Agreed.  That is the conclusion I came to in May:

	https://www.postgresql.org/message-id/20210526210201.GZ3048%40momjian.us
	https://www.postgresql.org/message-id/20210527160003.GF5646%40momjian.us

---

From: Tomas Vondra <[email-masked]>
Date: Sun, 17 Oct 2021 23:11:49 +0200

> replication that use cluster file encryption.

Would it really be that complex? Reserving a bunch of bytes at the end 
of each encrypted page (a bit like the "special" space, but after 
encryption) seems fairly straightforward. And I don't quite see why 
would this have a measurable impact, given the nonce is 16B at most. The 
encryption is likely way more expensive.

Moreover, it seems fairly reasonable to trade a bit of code complexity 
for something LSN-based which seems simpler but apparently has various 
weak points and is much harder to reason about.

regards

---

From: Tomas Vondra <[email-masked]>
Date: Sun, 17 Oct 2021 23:23:49 +0200

>> code and more importantly to future changes affecting on-disk data.

I've argued for storing the nonce, but I don't quite see why would we 
need integrity guarantees?

AFAICS the threat model the patch aims to address is an attacker who can 
observe the data (e.g. a low-privileged OS user), but can't modify the 
files. Which seems like a reasonable model for shared environments.

IMO extending this to cases where the attacker can modify the data moves 
the goalposts quite significantly. And it's quite possible authenticated 
encryption would not be enough to prevent that, because that still works 
only at block level, and you can probably do a lot of harm with replay 
attacks (e.g. replacing blocks with older versions). And if you can 
modify the data directory / config files, what are the chances you can't 
just get access to the database, trace the processes or whatever?

We already have a way to check integrity by storing page checksum, but 
I'm not sure if that's good enough (there's a lot of subtle issues with 
building proper AEAD scheme).

regards

---

From: Sasasu <[email-masked]>
Date: Mon, 18 Oct 2021 10:19:48 +0800

Just a mention. the HMAC (or AE/AD) can be disabled in AES-GCM. HMAC in 
AES-GCM is an encrypt-then-hash MAC.

CRC-32 is not a crypto-safe hash (technically CRC-32 is not a hash 
function). Cryptographers may unhappy with CRC-32.

I think CRC or SHA is not such important. If IV can be stored, I believe 
there should have enough space to store HMAC.

---

From: Sasasu <[email-masked]>
Date: Mon, 18 Oct 2021 10:35:48 +0800

On 2021/10/16 03:22, Stephen Frost wrote:
> strikes me as a pretty ringing endorsement
 > files. Which seems like a reasonable model for shared environments.

I agree this threat model.

And if PostgreSQL is using XTS, there is no different with dm-encrypt.
The user can use dm-encrypt directly.

---

From: Stephen Frost <[email-masked]>
Date: Mon, 18 Oct 2021 11:56:03 -0400

Greetings,

> block, or something like that.

The 'LSN' being referred to here isn't the regular LSN that is
associated with the WAL but rather the separate FakeLSN counter which we
already have.  I wasn't suggesting having the regular LSN change in
cases that don't generate WAL.

> seems more like it could hurt by masking fuzzy thinking more than it

This argument doesn't come across as very strong at all to me,
particularly when we have explicit recommendations from NIST that having
the IV vary more is beneficial.  While this would be using the LSN, the
fact that the LSN changes most of the time but not all of the time isn't
new and is something we already have to deal with.  I'd think we'd
address the concern about mis-thinking around how this works by
providing a README and/or an appropriate set of comments around what's
being done and why.

> sync). But we don't really face the same situation - we *can* add additional

No, we can't always add additional data, and that's part of the
consideration for an XTS option- there are things we can do if we use
XTS that we can't with GCM or another solution.  Specifically, being
able to perform physical replication from an unencrypted cluster to an
encrypted one is a worthwhile use-case that we shouldn't be just tossing
out.

> authenticity at some point. And then we'll need extra data anyway.

I agree that it'd be useful to have an authenticated encryption option.
Implementing XTS doesn't preclude us from adding that capability down
the road and it's simpler with fewer dependencies.  These all strike me
as good reasons to add XTS first.

> temporarily save a bit of implementation effort. We'll have to endure that

This isn't a valid argument as it isn't just about implementation but
about the capabilities we will have once it's done.

* Tomas Vondra ([email-masked]) wrote:
> TrueCrypt/VeraCrypt uses XTS too, I think. There's an overview of other FDE
> products at [1], and some of them use XTS, but I would take that with a
> grain of salt - some of the products are somewhat obscure, very old, or
> both.
> 

Thanks for finding those and linking to them, that's helpful.

> I haven't been following the discussion about using LSN, but I agree that
> while using it seems convenient, the consequences of some changes not
> incrementing LSN seem potentially disastrous, depending on the encryption
> mode.

Yes, this depends on the encryption mode, and is why we are specifically
talking about XTS here as it's an encryption mode that doesn't suffer
from this risk and therefore it's perfectly fine to use the LSN/FakeLSN
with XTS (and would also be alright for AES-GCM-SIV as it's specifically
designed to be resistant to IV reuse).

> skip the use of the LSN and simplify the patch.

While there may not be an active attack against PG that leverages such a
leak, I have a hard time seeing why we would intentionally design this
in when we have a great option that's directly available to us and
doesn't cause such a leak with nearly such regularity as not using the
LSN would, and also follows recommendations of using XTS from NIST.
Further, not using the LSN wouldn't really be an option if we did
eventually implement AES-GCM-SIV, so why not have the two cases be
consistent?

> replication that use cluster file encryption.

Right- the point of XTS is that we can do things that we can't with GCM
and that it's simpler.

> and old copy of a patch to revert a change, and that would not be

That's an argument based on how things stand today.  I appreciate that
it's no small thing to consider changes to those other systems but I
would argue that having authentication of the heap is still better than
not (but also agree that XTS is simpler to implement and therefore makes
sense to do first and see how things stand after that's done).  Surely,
we would want progress made here to be done so incrementally as a patch
that attempted to change all of those other systems to be encrypted and
authenticated with AES-GCM-SIV would be far too large to consider in one
shot anyway.

> it to be rightly rejected.

Presuming that 'checkbox feature' here means "we need it to please
$someone but no one will ever use it" or something along those lines,
this is very clearly not the case and therefore we shouldn't be
describing it or treating it as such.  Even if the meaning here is
"there's other ways people could get this capability" the reality is
that those other methods are simply not always available and in those
cases, people will choose to not use PostgreSQL.  Nearly every other
database system which we might compare ourselves to has a solution in
this area and people actively use those solutions in a lot of
deployments.

> code and more importantly to future changes affecting on-disk data.

I don't follow this argument.  The XTS approach is explicitly the same
on-disk format as what we have unencrypted today, just encrypted, and
that's the whole point of going with that approach.  If we were to
implement AES-GCM-SIV, then that would introduce a new on-disk format
and then we'd have two- one which has space on each page for the
authentication information, and one which doesn't.

> just reject requests for it and use our energy for useful things.

This capability certainly has a real use-case and it's one that a lot of
organizations are looking for PG to provide a solution for.  That we
don't today is keeping those organizations from using PG in at least
some cases, and for some organizations, it prevents them from using PG
at all, as they understandably would rather not deal with a hybrid of
using PG for some things and having to use another solution for other
things.

* Tomas Vondra ([email-masked]) wrote:
> Would it really be that complex? Reserving a bunch of bytes at the end of
> each encrypted page (a bit like the "special" space, but after encryption)
> seems fairly straightforward. And I don't quite see why would this have a
> measurable impact, given the nonce is 16B at most. The encryption is likely
> way more expensive.

There's a patch to do exactly this- make space available towards the end
of the page.  If we go down the route of using a different page format
then we lose the ability to do physical replication between an
unencrypted cluster and an encrypted one.  That's certainly a nice
capability to have and it will help people migrate to an encrypted PG
instance, plus it's overall simpler to work with, which is also an
advantge.

> Moreover, it seems fairly reasonable to trade a bit of code complexity for
> something LSN-based which seems simpler but apparently has various weak
> points and is much harder to reason about.

This isn't just about code complexity but is also about the resulting
capabilities from these different approaches.

* Tomas Vondra ([email-masked]) wrote:
> I've argued for storing the nonce, but I don't quite see why would we need
> integrity guarantees?
> 

There are multiple threat models which we should be considering and
that's why we may want to eventually add integrity.

> IMO extending this to cases where the attacker can modify the data moves the
> goalposts quite significantly. And it's quite possible authenticated
> attacks (e.g. replacing blocks with older versions). And if you can modify
> the data directory / config files, what are the chances you can't just get
> access to the database, trace the processes or whatever?

I agree that working towards an authenticated solution is a larger task.
I don't agree that we should throw out the possibility that we may want
to implement it eventually as there are certainly threat models where an
attacker might have access to the storage but not to the database or the
system on which the database is running.  Implementing a system to
address such an attack vector would take more consideration than just
having authenticated encryption provided by PG, but it certainly
couldn't be done without that either.

> We already have a way to check integrity by storing page checksum, but I'm
> not sure if that's good enough (there's a lot of subtle issues with building
> proper AEAD scheme).

No, it isn't good enough.

> AES-GCM is an encrypt-then-hash MAC.

Not sure why you would though.

> function). Cryptographers may unhappy with CRC-32.

Yes, that's correct (and it isn't even CRC-32 that we have, heh).

> there should have enough space to store HMAC.

This would be the case, yes.  If we can find a way to make room for an
IV then we could make room to store the tag too, and we certainly should
(and we should include a variety of additional data in the AEAD- block
number, relfileno, etc).

> The user can use dm-encrypt directly.

dm-encrypt is not always an option and it doesn't actually address the
threat-model that Tomas brought up here anyway, as it would be below the
level that the low-privileged OS user would be looking at.  That's not
the only threat model to consider, but it is one which could potentially
be addressed by either XTS or AES-GCM-SIV.  There are threat models
which dm-crypt would address, of course, such as data-at-rest (hard
drive theft, improper disposal of storage media, backups which don't
have their own encryption, etc), but, again, dm-crypt isn't always an
option that is available and so I don't agree that we should throw this
out just because dm-crypt exists and may be useable in some cases.

Thanks,

Stephen

---

From: Robert Haas <[email-masked]>
Date: Mon, 18 Oct 2021 12:37:39 -0400

> sync). But we don't really face the same situation - we *can* add additional

Yes. The downside is that there is some code complexity, and also some
runtime overhead even for cases that don't use encryption, because
some things that now are compile time constants might need to be
computed at runtime. That can probably be made pretty small, though.

> temporarily save a bit of implementation effort. We'll have to endure that

I agree up to a point, but I do also kind of feel like we should be
leaving it up to whoever is working on an implementation to decide
what they want to implement. I don't particularly like this discussion
where it feels like people are trying to tell other people what they
have to do because "the community has decided X." It's pretty clear
that there are multiple opinions here, and I don't really see any of
them to be without merit, nor do I see why Bruce or Stephen or you or
anyone else should get to say "what the community has decided" in the
absence of a clear consensus.

I do really like the idea of using AES-GCM-SIV not because I know
anything about it, but because the integrity checking seems cool, and
storing the nonce seems like it would improve security. However, based
on what I know now, I would not vote to reject an XTS-based patch and,
as Stephen and Bruce have said, maybe with the right design it permits
upgrades from non-encrypted clusters to encrypted clusters. I'm
actually kind of doubtful about that, because that seems to require
some pretty specific and somewhat painful implementation decisions.
For example, I think if your solution for rotating keys is to shut
down the standby, re-encrypt it with a new key, start it up again, and
fail over to it, then you probably ever can't do key rotation in any
other way. The keys now have to be non-WAL-logged so that the standby
can be different, which means you can't add a new key on the master
and run around re-encrypting everything with it, WAL-logging those
changes as you go. Now I realize that implementing that is really
challenging anyway so maybe some people wouldn't like to go that way,
but then maybe other people would. Another thing you probably can't do
in this model is encrypt different parts of the database with
different keys, because how would you keep track of that? Certainly
not in the system catalogs, if none of that can show up in the WAL
stream.

But, you know, still: if somebody showed up with a fully-working XTS
patch with everything in good working order, I don't see that we have
enough evidence to reject it just because it's XTS. And I would hope
that the people favoring XTS would not vote to reject a fully working
GCM patch just because it's GCM. I think what we ought to be doing at
this point is combining our efforts to try to get some things
committed which make future work in this area committed - like that
patch to preserve relfilenode and database OID, or maybe some patches
to drive all of our I/O through a smaller number of code paths instead
of having every different type of temporary file we write reinvent the
wheel. These discussions about what encryption type we ought to use
are useful for ruling out options that we know are bad, but beyond
that I'm not sure they have much value. AES in any mode could seem
like a much less safe choice by the time we get a committed feature
here than it does today - even if somehow that were to happen for v15.
I expect there are people out there trying to break it even as I write
these words, and it seems likely that they will eventually succeed,
but as to when, who can say?

---

From: Tomas Vondra <[email-masked]>
Date: Mon, 18 Oct 2021 21:02:56 +0200

> function). Cryptographers may unhappy with CRC-32.

True. If you can flip enough bits in the page, it probably is not very 
hard to generate a page with the desired checksum. It's probably harder 
with XTS, but likely not much more.

> there should have enough space to store HMAC.

Right, I agree.

regards

---

From: Tomas Vondra <[email-masked]>
Date: Tue, 19 Oct 2021 01:54:58 +0200

> cases that don't generate WAL.

I'm not very familiar with FakeLSN, but isn't that just about unlogged 
tables? How does that help cases like setting hint bits, which may not 
generate WAL?

> providing a README and/or an appropriate set of comments around what's

I don't think anyone objects to varying IV more, as recommended by NIST. 
AFAICS the issue at hand is exactly the opposite - maybe not varying it 
enough, in some cases. It might be enough for MVCC purposes yet it might 
result in fatal failure of the encryption scheme. That's my concern, at 
least, and I assume it's what Robert meant by "fuzzy thinking" too.

FWIW I think we seem to be mixing nonces, IVs and tweak values. Although 
various encryption schemes place different requirements on those anyway.

> encrypted one is a worthwhile use-case that we shouldn't be just tossing

Yeah, XTS seems like a reasonable first step, both because it doesn't 
require storing extra data and it's widespread use in FDE software (of 
course, there's a link between those). And I suspect replication between 
encrypted and unencrypted clusters is going to be a huge can of worms, 
even with XTS.

It's probably a silly / ugly idea, but can't we simply store a special 
"page format" flag in controldat - when set to 'true' during initdb, 
each page would have a bit of space (at the end) reserved for additional 
encryption data. Say, ~64B should be enough. On the encrypted cluster 
this would store the nonce/IV/... and on the unencrypted cluster it'd be 
simply unused. 64B seems like a negligible amount of data. And when set 
to 'false' the cluster would not allow encryption.

> as good reasons to add XTS first.

True. If XTS addresses the threat model we aimed to solve ...

> designed to be resistant to IV reuse).

I'm not quite sure about the "perfectly fine" bit, as it's making XTS 
vulnerable to traffic analysis attacks (comparing multiple copies of an 
encrypted block). It may be a reasonable trade-off, of course.

> eventually implement AES-GCM-SIV, so why not have the two cases be

I'm a bit confused, because the question was what happens if we encrypt 
the page twice with the same LSN or any tweak value in general. It 
certainly does not matter when it comes to malleability or replay 
attacks, because in that case the attacker is the one who modifies the 
block (and obviously won't change the LSN).

regards

---

From: Tomas Vondra <[email-masked]>
Date: Tue, 19 Oct 2021 02:07:27 +0200

On 10/18/21 17:56, Stephen Frost wrote:
 >> ...

So what are these threat models? If we should be considering them it'd 
be nice to have a description, explaining what capabilities must the 
attacker have ...

My (perhaps naive) understanding is that the authentication / integrity 
provides (partial) protection against attackers that may modify instance 
data - modify files, etc. But I'd guess an attacker with such capability 
can do various other (simpler) things to extract data. Say, modify the 
config to load an extension that dumps keys from memory, or whatever.

So what's a plausible / practical threat model that would be mitigated 
by the authenticated encryption?

It'd be a bit silly to add complexity to allow AEAD, only to find out 
there are ways around it.

regards

---

From: Sasasu <[email-masked]>
Date: Tue, 19 Oct 2021 23:46:12 +0800

On 2021/10/19 00:37, Robert Haas wrote:
> I think what we ought to be doing at
> wheel.

A unified block-based I/O API sounds great. Has anyone tried to do this 
before? It would be nice if the front-end tools could also use these API.

As there are so many threat models, I propose to do the TDE feature by a 
set of hooks. those hooks are on the critical path of IO operation, add 
the ability to let extension replace the IO API. and also load extension 
when initdb, single-mode, and in front-end tools.
This sounds Like using $LD_PRELOAD to replace pread(2) and pwrite(2), 
which widely used in folder based encryption. but the hook will pass 
more context (filenode, tableoid, blocksize, and many) to the under 
layer, this hook API will look like object_access_hook.
then implement the simplest AES-XTS. and put it to contrib. provide a 
tool to deactivate AES-XTS to make PostgreSQL upgradeable.

I think this is the most peaceful method. GCM people will not reject 
this just because XTS. and XTS people will satisfied(maybe?) with the 
complexity. for performance, just one more long-jump compare with 
current AES-XTS code.

---

From: Robert Haas <[email-masked]>
Date: Tue, 19 Oct 2021 12:35:39 -0400

On Tue, Oct 19, 2021 at 11:46 AM Sasasu <[email-masked]> wrote:
> set of hooks.

This is too invasive to do using hooks. We are inevitably going to
need to make significant changes in core.

---

From: Stephen Frost <[email-masked]>
Date: Tue, 19 Oct 2021 14:44:26 -0400

Greetings,

> tables? How does that help cases like setting hint bits, which may not

Errr, there seems to have been some confusion in this thread between the
different ideas being tossed around.

The point of using FakeLSN for unlogged tables consistently is to
provide variability in the value used as the IV.  I wasn't suggesting to
use FakeLSN to provide a uniqueness guarantee- the reason we're talking
about using XTS is specifically because, unlike CTR, unique IVs are not
required.  Further, we don't need to find any additional space on the
page if we use XTS, meaning we can do things like go from an unencrypted
to an encrypted system with a basebackup+physical replication.

> I don't think anyone objects to varying IV more, as recommended by NIST.

Great.

> least, and I assume it's what Robert meant by "fuzzy thinking" too.

XTS does not require the IV to be unique for every invocation, and
indeed other systems like dm-crypt don't use a unique IV for XTS.  We
really can't divorce the encryption methodology from the parameters that
are being used.  CTR and GCM are the methods that require a unique IV
(or, at least, a very very very likely unique one if you can't actually
implement a proper counter) but that's *not* what we're talking about
here.  The methods being discussed are XTS and GCM-SIV, the former
explicitly doesn't require a unique IV and the latter is specifically
designed to reduce the impact of an IV being re-used.  Both are, as NIST
points out, better off with a varying IV, but having the IV be reused
from time to time in either XTS or GCM-SIV does not result in a fatal
failure of the encryption scheme.

> various encryption schemes place different requirements on those anyway.

The differences between those are pretty subtle and it gets a bit
confusing when things like OpenSSL accept an IV but then use it as the
'tweak', such as with XTS.  In general though, most methods accept a
key, a nonce/IV/tweak, and then produce ciphertext and possbily a tag,
so I don't think we've been incorrect in usage but rather perhaps a bit
sloppy by not using the right term for the specific methodology.

> encrypted and unencrypted clusters is going to be a huge can of worms, even
> with XTS.

Glad you agree with XTS being a reasonable first step.  XTS will at
least make physical replication possible- other methods (such as below)
simply won't work.

> It's probably a silly / ugly idea, but can't we simply store a special "page
> format" flag in controldat - when set to 'true' during initdb, each page
> would have a bit of space (at the end) reserved for additional encryption
> data. Say, ~64B should be enough. On the encrypted cluster this would store
> the nonce/IV/... and on the unencrypted cluster it'd be simply unused. 64B
> seems like a negligible amount of data. And when set to 'false' the cluster
> would not allow encryption.

This is essentially what the patch that was posted does, but the problem
is that you can't do physical replication when 64B have been stolen off
of a page because the page in the unencrypted database might be entirely
full and not able to physically fit those extra 64B, and then what?

> True. If XTS addresses the threat model we aimed to solve ...

It addresses a valid threat model that people are interested in PG
having a solution for, yes.

> encrypted block). It may be a reasonable trade-off, of course.

Considering the default usage in dmcrypt isn't even varying the IV as
much as what we're talking about here, I'd say that, yes, it's quite
reasonable for our use-case and allows us to vary the IV quite a bit
which reduces the attack surface in a meaningful way.  That dmcrypt has
this risk and it isn't considered enough of an issue for them to use
something other than plain64 as the default makes it certainly seem
reasonable to me.

> I'm a bit confused, because the question was what happens if we encrypt the
> page twice with the same LSN or any tweak value in general. It certainly
> does not matter when it comes to malleability or replay attacks, because in
> that case the attacker is the one who modifies the block (and obviously
> won't change the LSN).

The question that I was responding to above was specifically about if
knowing which 16-byte blocks on an 8K page changes was an issue or not
and that's what I was addressing.  As for if we encrypt the page twice
with the same LSN/tweak/IV or not- that depends on the specific
encryption methodology being used as to how much of an issue that is, as
discussed above.

* Tomas Vondra ([email-masked]) wrote:
> So what are these threat models? If we should be considering them it'd be
> nice to have a description, explaining what capabilities must the attacker
> have ...

The first and simplest to consider is the basic "data at rest" threat
model, where a hard drive is not properly wiped or a backup isn't
properly encrypted, or a laptop or other removable media is stolen, etc.
These are well addressed through XTS as in those cases what is needed is
confidentiality, not integrity, as the attacker isn't able to modify the
existing system in such cases.

Another threat model to consider is if the attacker has read-only access
to the data directory through, say, unix group read privileges or maybe
the ability to monitor the traffic on the SAN, or the ability to
read-only mount the LUN on to another system.  This might be obtained by
attacking a backup process where the system was configured to run
physical backups using an unprivileged OS user who only has group read
access to the cluster (and the necessary but non-superuser privleges in
the database system to start/stop the backup), or various potential
attacks at the storage layer.  This is similar to the "data at rest"
case above in that XTS works well to address this, but because the
attacker would have ongoing access (rather than just one-time, such as
in the first case), information such as which blocks are being changed
inside of a given 8k page might be able to be determined and that could
be useful information, though a point here: they would already be able
to see clearly which 8k pages are being changed and which aren't, and
there's not really any way for us to prevent that reasonably.  As such,
I'd argue that using XTS is reasonable and we can mitigate some of this
concern by using the LSN in the tweak instead of just the block number
as the 'plain64' option in dmcrypt does.  That doing so would mean that
we'd also be able to reasonably use the same IV for both XTS and
AES-GCM-SIV, should we choose to implement that at some point down the
road, is a nice perk to this approach.

> data - modify files, etc. But I'd guess an attacker with such capability can
> do various other (simpler) things to extract data. Say, modify the config to
> load an extension that dumps keys from memory, or whatever.

Another attack vector to consider is an attacker who is able to actively
modify files on the system in some way.  The specific files they are
able to modify matter in such a case.  There's no doubt that it's more
difficult to address such an attack vector and it's unlikely we'd be
able to provide a 100% solution in all cases, but that doesn't mean we
should throw out the idea entirely.

> So what's a plausible / practical threat model that would be mitigated by
> the authenticated encryption?

In a similar vein to above- consider a storage-level attacker who is
able to gain read/write access to a particular volume.  If that volume
is used only for a tablespace, a TDE implementation which uses
AES-GCM-SIV would go a long way towards protecting the system.  We may
even go so far as to encourage users to ensure that their 'main' PG data
directory, where configuration, transaction log, et al, are stored be
kept on a more secure (perhaps local) volume.  This isn't a 100%
solution, of course, due to the visibility map and the free space map
being stored with the tables in the tablespaces, but attacks on those
would be much more coarse and difficult to mount effectively.

We should also be thinking about ways to address those other subsystems
and improve the situation around them (and not just for encryption but
also for detection of corruption) but trying to get all of that done in
one patch or even one major release would make this a massive change
that would almost certainly be rejected as too destablizing.

> It'd be a bit silly to add complexity to allow AEAD, only to find out there
> are ways around it.

There are ways around it.  There likely always will be.  We need to be
clear about what it provides and what it doesn't.  We need to stop
telling ourselves that the only answer is a 100% solution and therefore
it's impossible to do.  Users who care about these capabilities will
understand that it's not 100% and they will still happily use it because
it's better than 0% which is where we are today and is why they are
going with other solutions.  Yes, if it's trivial to get around then
perhaps it's not much better than 0% and if that's the case then it
doesn't make sense to do it, but none of what has been discussed here
thus far has made me feel like either the XTS or the GCM-SIV approaches
would be trivial to to circumvent for the threat models they're intended
to address, though it certainly takes more care and more thought when
we're trying to address someone who has write access to part of the
system and that we need to be clear what is addressed and what isn't in
all of these cases.

Thanks,

Stephen

---

From: Stephen Frost <[email-masked]>
Date: Tue, 19 Oct 2021 14:54:56 -0400

Greetings,

> before? It would be nice if the front-end tools could also use these API.

The TDE patch from Cybertec did go down this route, but the API ended up
being rather different which menat a lot of changes in other parts of
the system.  If we can get a block-based temporary file method that
maintains more-or-less the same API, that'd be great, but I'm not sure
that we can really do so and I am not entirely convinced that we should
make the TDE effort depend on an otherwise quite independent effort of
making all temp files usage be block based.

> As there are so many threat models, I propose to do the TDE feature by a set
> of hooks. those hooks are on the critical path of IO operation, add the
> ability to let extension replace the IO API. and also load extension when
> initdb, single-mode, and in front-end tools.
> This sounds Like using $LD_PRELOAD to replace pread(2) and pwrite(2), which
> widely used in folder based encryption. but the hook will pass more context
> (filenode, tableoid, blocksize, and many) to the under layer, this hook API
> will look like object_access_hook.
> then implement the simplest AES-XTS. and put it to contrib. provide a tool
> to deactivate AES-XTS to make PostgreSQL upgradeable.
> 
> I think this is the most peaceful method. GCM people will not reject this
> just because XTS. and XTS people will satisfied(maybe?) with the complexity.
> for performance, just one more long-jump compare with current AES-XTS code.

I agree with Robert- using hooks for this really isn't realistic.  Where
would you store the tag for GCM without changes in core, for starters..?
Certainly wouldn't make sense to provide GCM only to throw the tag away.
Even between XTS and GCM, to say nothing of other possible methods,
there's going to be some serious differences that a single hook-based
API wouldn't be able to address.

Thanks,

Stephen

---

From: Sasasu <[email-masked]>
Date: Wed, 20 Oct 2021 15:05:14 +0800

On 2021/10/20 02:54, Stephen Frost wrote:
 > I agree with Robert- using hooks for this really isn't realistic.

OK, I agree use hooks is too invasive. Just a whim, never mind.

But If PG has a clear block-based IO API, TDE is much easier to 
understand. security people may lack database knowledge but they can 
understand block IO.
This will allow more people to join PG community.

On 2021/10/20 02:54, Stephen Frost wrote:
 > Where would you store the tag for GCM without changes in core?

If can add 32bit reserve field (in CGM is 28bits) will be best.
data file size will increase 0.048% (if BLCKSZ = 8KiB), I think it is 
acceptable even for the user who does not use TDE. but need ondisk 
format change.
If without of modify anything in core and doing GCM, the under-layer can 
write out a key fork, fsync(2) key fork with the same strategy for main 
fork. this is crash-safe. The consistency is ensured by WAL. (means 
wal_log_hints need set to on)
Or the underlayer can re-struct the IO request. insert one IV block per 
2730(=BLKSZ/IV_SIZE) data blocks. this method like the _mdfd_getseg() in 
md.c which split file by 1GiB. No perception in the upper layers.
Both of them can use cache to reduce performance downgrade.

for WAL encryption, the CybertecDB implement is correct. we can not 
write any extra data without adding a reserved field in core. because 
can not guarantee consistency. If use GCM for WAL encryption must 
disable HMAC verification.

* only shows the possibility, not mean anyone should implement TDE in 
that way.
* blahblah

---

From: Stephen Frost <[email-masked]>
Date: Wed, 20 Oct 2021 08:24:08 -0400

Greetings,

* Sasasu ([email-masked]) wrote:
> But If PG has a clear block-based IO API, TDE is much easier to understand.

PG does have a block-based IO API, it's just not exposed as hooks.  In
particular, take a look at md.c, though perhaps you'd be more interested
in the higher level bufmgr.c routines.  For the specific places where
encryption may hook in, looking at the DataChecksumsEnabled() call sites
may be informative (there aren't that many of them).

> security people may lack database knowledge but they can understand block
> IO.

We'd certainly welcome them.  I don't think we're going to try to
redesign our entire IO subsystem in the hopes that they'll show up
though.

> If can add 32bit reserve field (in CGM is 28bits) will be best.

That's the idea that's been discussed, but the approach put forward is
to do it in a manner that allows the same binaries to work with a
TDE-enabled cluster and a non-TDE cluster which means two different
formats on disk.  This is still a pretty big deal and would require
logical replication or pg_dump/restore to go from unencrypted to
encrypted.

> acceptable even for the user who does not use TDE. but need ondisk format
> change.

Breaking our ondisk format explicitly means that pg_upgrade won't work
any longer and folks won't be able to do in-place upgrades.  That's a
pretty huge deal and it's something we've not done in over a decade.
I doubt that's going to fly.

> Both of them can use cache to reduce performance downgrade.

Yes, using another fork for this is something that's been considered but
it's not without its own drawbacks, in particular having to do another
write and later fsync when a page changes.

Further, none of this is necessary for XTS, but only for GCM.  This is
why it was put forward that GCM involves a lot more changes to the
system and means that we won't be able to do things like binary
replication to switch from an unencrypted to encrypted cluster.  Those
are good reasons to consider an XTS implementation first and then later,
perhaps, implement GCM.

> for WAL encryption, the CybertecDB implement is correct. we can not write
> any extra data without adding a reserved field in core. because can not
> guarantee consistency. If use GCM for WAL encryption must disable HMAC
> verification.

What's the point of using GCM if we aren't going to actually verify the
tag?  Also, the Cybertec patch didn't add an extra reserved field to the
page format, and it used CTR anyway..

Thanks,

Stephen

---

From: Sasasu <[email-masked]>
Date: Thu, 21 Oct 2021 12:18:59 +0800

> may be informative (there aren't that many of them).

md.c is great, easy to understand. but PG does not have a unified API. 
There has many unexpected pread()/pwrite() in many corners. md.c only 
for heap table, bufmgr.c only for a buffered heap table.

eg: XLogWrite() looks like a block API, but is a range write. equivalent 
to the append(2)
eg: ALTER DATABASE SET TABLESPACE , the movedb() call. use copy_file() 
on heap table. which is just pread() pwrite() with 8*BLCKSZ.
eg: all front-end tools use pread() to read heap table. in particular, 
pg_rewind write heap table by offset.
eg: in contrib, pg_standby use system("cp") to copy WAL.

 > I doubt that's going to fly.

I completely agree.

 > are good reasons to consider an XTS implementation first and then later,

same as Robert Haas. I wish PG can do some infrastructure first. add 
more abstract layers like md.c (maybe a block-based API with ondisk 
format version field). so people can dive in without understanding the 
things which isolated by the abstract layer.

 > page format, and it used CTR anyway..

Oh, I am wrong, Cybertec patch can not use XTS, because WAL may not be 
aligned to 16bytes. for WAL need a stream cipher. The CTR implement is 
still correct.

CTR with hash(offset) as IV is basically equal to XTS. if use another 
AES key to encrypt the hash(offset), and block size is 16bytes it is XTS.
The point is that can not save random IV for WAL without adding a 
reserved field, no matter use GCM or CTR.

Because WAL only does append to the end, using CTR for WAL is safer than 
using XTS for heap table. If you want more security for WAL encryption, 
add HKDF[1].

[1]: https://en.wikipedia.org/wiki/HKDF

---

From: Stephen Frost <[email-masked]>
Date: Thu, 21 Oct 2021 13:28:12 -0400

Greetings,

* Sasasu ([email-masked]) wrote:
> md.c is great, easy to understand. but PG does not have a unified API. There
> has many unexpected pread()/pwrite() in many corners. md.c only for heap
> table, bufmgr.c only for a buffered heap table.

There's certainly other calls out there, yes.

> eg: XLogWrite() looks like a block API, but is a range write. equivalent to
> the append(2)

XLog is certainly another thing that has to be dealt with, of course,
but I don't see us trying to shoehorn that into using md.c somehow.

> eg: ALTER DATABASE SET TABLESPACE , the movedb() call. use copy_file() on
> heap table. which is just pread() pwrite() with 8*BLCKSZ.

None of these are actually working with or changing the data though,
they're just copying it.  I don't think we'd actually want these to
decrypt and reencrypt the data.

Great.

> same as Robert Haas. I wish PG can do some infrastructure first. add more
> abstract layers like md.c (maybe a block-based API with ondisk format
> version field). so people can dive in without understanding the things which
> isolated by the abstract layer.

I really don't think this is necessary.  Similar to PageSetChecksumCopy
and PageSetChecksumInplace, I'm sure we would have functions which are
called in the appropriate spots to do encryption (such as 'encrypt_page'
and 'encrypt_block' in the Cybertec patch) and folks could review those
in relative isolation to the rest.  Dealing with blocks in PG is already
pretty well handled, the infrastructure that needs to be added is around
handling temporary files and is being actively worked on ... if we could
move past this debate around if we should be adding support for XTS or
if only GCM-SIV would be accepted.

> aligned to 16bytes. for WAL need a stream cipher. The CTR implement is still
> correct.

No, the CTR approach isn't great because, as has been discussed quite a
bit already, using the LSN as the IV means that different data might be
re-encrypted with the same LSN and that's not an acceptable thing to
have happen with CTR.

> CTR with hash(offset) as IV is basically equal to XTS. if use another AES
> key to encrypt the hash(offset), and block size is 16bytes it is XTS.

I don't understand why we're talking about CTR+other-stuff.  Maybe if
you use CTR and then do other things then it's equivilant in some
fashion to XTS ... but then it's not CTR anymore and we shouldn't be
calling it that.  Saying that we should do "CTR+other stuff" (which
happens to make it equivilant to XTS) instead of just saying we should
use "XTS" is very confusing and further means that we're starting down
the path of trying to come up with our own hack on existing encryption
schemes, and regardless of who shows up on this list to claim that doing
so makes sense and is "right", I'm going to be extremely skeptical.

> The point is that can not save random IV for WAL without adding a reserved
> field, no matter use GCM or CTR.

Yes, it's correct that we can't use a random IV for the WAL without
figuring out how to keep track of that random IV.  Thankfully, for WAL
(unlike heap and index blocks) we don't really have that issue- we
hopefully aren't going to write different WAL records at the same LSN
and so using the LSN there should be alright.  There's some odd edge
cases around this too (split-brain situations in particular where one of
the systems does crash recovery and doesn't actually promote and
therefore stays on the same timeline), but that kind of thing ends up
breaking other things too (WAL archiving, as an example) and isn't
really something we can support.  Further, I figure we'll tell people to
use different keys for different systems anyway to avoid this risk.

> using XTS for heap table. If you want more security for WAL encryption, add
> HKDF[1].

I don't follow how you're making these comparisons as to what is "safer"
for which when talking about two different encryption methodologies and
two very different systems (the heap vs. WAL) or if you're actually
suggesting something here.

We've discussed at length how using CTR for heap isn't a good idea even
if we're using the LSN for the IV, while if we use XTS then we don't
have the issues that CTR has with IV re-use and using the LSN (plus
block number and perhaps other things).  Nothing in what has been
discussed here has really changed anything there that I can see and so
it's unclear to me why we continue to go round and round with it.

Thanks,

Stephen

---

From: Sasasu <[email-masked]>
Date: Fri, 22 Oct 2021 11:35:38 +0800

> decrypt and reencrypt the data.

OK, but why ALTER TABLE SET TABLESPACE use smgrread() and smgrextend() 
instead of copy_file().
TDE needs to modify these code paths, and make the patch bigger.

On 2021/10/22 01:28, Stephen Frost wrote:
 > Thankfully, for WAL
 > and so using the LSN there should be alright.
 > it's unclear to me why we continue to go round and round with it.

I am not re-discuss using CTR for heap table. I mean use some CTR-like 
algorithm *only* for WAL encryption. My idea is exactly the same when 
you are typing "we hopefully aren't going to write different WAL records 
at the same LSN and so using the LSN there should be alright."

The point of disagreement between you and me is only on the block-based API.

 > it's unclear to me why we continue to go round and round with it.

same to me. I am monitoring this thread about 9 months, watching yours 
discuss key management/CBC/CRT/GCM round and round.

---

From: Stephen Frost <[email-masked]>
Date: Fri, 22 Oct 2021 11:36:37 -0400

Greetings,

> TDE needs to modify these code paths, and make the patch bigger.

Tables and databases are handled differently, yes.

With ALTER TABLE SET TABLESPACE, we're allocating a new refilenode and
WAL'ing the table as FPIs.  What happens with databases is fundamentally
different- no one is allowed to be connected to the database being moved
and we write a single 'database changed tablespace' record in the WAL
for this case.  When it comes to TDE, this probably is actually helpful
as we're going to likely want the relfilenode to be included as part of
the IV.

> algorithm *only* for WAL encryption. My idea is exactly the same when you
> are typing "we hopefully aren't going to write different WAL records at the
> same LSN and so using the LSN there should be alright."

I don't like the idea of "CTR-like".  What's wrong with using CTR for
WAL encryption?  Based on the available information, that seems like the
exact use-case for CTR.

> The point of disagreement between you and me is only on the block-based API.

I'm glad to hear that, at least.

Thanks,

Stephen

---

From: Bruce Momjian <[email-masked]>
Date: Fri, 22 Oct 2021 19:51:05 -0400

> cases that don't generate WAL.

Yes, my original patch created dummy WAL records for dummy LSNs but that
is no longer needed with XTS.

> providing a README and/or an appropriate set of comments around what's

Agreed.  I think we would need to document when we reencrypt a page
with the same LSN, and of course write-based attacks.

> LSN would, and also follows recommendations of using XTS from NIST.

Agreed.

> this area and people actively use those solutions in a lot of

I think people will use this feature, but I called it a 'checkbox
feature' because they usually are not looking for a complex or flexible
feature, but rather something that is simple to setup and effective.

> out just because dm-crypt exists and may be useable in some cases.

I actually think a Postgres integrity-check feature would need to create
an abstraction layer on top of all writes to PGDATA and tablespaces so
the filesystem would look unencrypted to Postgres.

---

From: Bruce Momjian <[email-masked]>
Date: Fri, 22 Oct 2021 19:57:02 -0400

> system and that we need to be clear what is addressed and what isn't in

Stephen, your emails on this thread have been very helpful and on-topic.
I think the distinction above is that it is useful to fully protect
against some attack types, even if we don't protect against all attack
types.  For example, if we protect 100% against read attacks, it doesn't
mean that gets reduced to 50% because we don't protect against write
attacks --- we are still 100% read-protected and 0% write protected.

---

From: Bruce Momjian <[email-masked]>
Date: Fri, 22 Oct 2021 20:04:50 -0400

> what they want to implement. I don't particularly like this discussion

Uh, our TODO has this list:

	Desirability -> Design -> Implement -> Test -> Review -> Commit

I think we have to agree on Desirability and Design before anyone starts
work since it is more likely a patch will be rejected without this.

> absence of a clear consensus.

I don't see anyone saying we have agreed on anything, but I do hear
people say they are willing to work on some things, and not others.

> not in the system catalogs, if none of that can show up in the WAL

The design is to have a heap/index key and a WAL key.  You create a
binary replica that uses a different heap/index key but the same WAL
key, switch-over to it, and then change the WAL key.

`> But, you know, still: if somebody showed up with a fully-working XTS
> GCM patch just because it's GCM. I think what we ought to be doing at

I don't think that would happen, but I do think patch size, code
maintenance, and upgradability would be reasonable considerations.

> but as to when, who can say?

Yes, we should start on things we know we need, but we will have to have
these discussions until we have desirability and design most people
agree on.

---

From: Bruce Momjian <[email-masked]>
Date: Fri, 22 Oct 2021 20:06:46 -0400

Agreed.

---

From: Bruce Momjian <[email-masked]>
Date: Sat, 23 Oct 2021 11:29:05 -0400

> as the 'plain64' option in dmcrypt does.  That doing so would mean that

That is an excellent point, and something we should mention in our
documentation --- the fact that a change of 8k granularity will be
visible, and in certain specified cases, 16-byte change granularity will
also be visible.

---

From: Bruce Momjian <[email-masked]>
Date: Sat, 23 Oct 2021 11:49:44 -0400

> anything about it, but because the integrity checking seems cool, and
                                                        ----------
> storing the nonce seems like it would improve security. However, based

Frankly, I think we need to be cautious about doing anything related to
security for "cool" motivations.  (This might be how OpenSSL became such
a mess.)  For non-security features, you can often add a few lines of
code to enable some cool use-case.  For security features, you have to
block its targeted attack methods fully or it is useless.  (It doesn't
need to block all attack methods.)  To fully block attack methods,
security features must be thoroughly designed and all potential
interactions must be researched.

When adding non-security Postgres features, cool features can be more
easily implemented because they are built on the sold foundation of
Postgres.  For security features, you have to assume that attacks can
come from anywhere, so the foundation is unclear and caution is wise.

---

From: Bruce Momjian <[email-masked]>
Date: Sat, 23 Oct 2021 12:03:36 -0400

> making all temp files usage be block based.

Uh, I thought people felt the Cybertec patch was too large and that a
unified API for temporary file I/O-encryption was a requirement.  Would
a CTR-steaming-encryption API for temporary tables be easier to
implement?

---

From: Yura Sokolov <[email-masked]>
Date: Mon, 25 Oct 2021 09:59:17 +0300

В Чт, 21/10/2021 в 13:28 -0400, Stephen Frost пишет:
> .....
> .....

Instead of debatting XTS vs GCM-SIV I'd suggest Google's Adiantum [1][2]
[3][4].

It is explicitely created to solve large block encryption issue - disk
encryption. It is used to encrypt 4kb at whole, but in fact has no
(practical) limit on block size: it is near-zero modified to encrypt 1kb
or 8kb or 32kb.

It has benefits of both XTS and GCM-SIV:
- like GCM-SIV every bit of cipher text depends on every bit of plain text
- therefore like GCM-SIV it is resistant to IV reuse: it is safe to reuse
  LSN+reloid+blocknumber tuple as IV even for hint-bit changes since every
  block's bit will change.
- like XTS it doesn't need to change plain text format and doesn't need in
  additional Nonce/Auth Code.

Adiantum stands on "giant's shoulders": AES, Chacha and Poly1305.
It is included into Linux kernel since 5.0 .

Adiantum/HPolyC approach (hash+cipher+stream-cipher+hash) could be used
with other primitives as well. For example, Chacha12 could be replaces
with AES-GCM or AES-XTS with IV derived from hash+cipher.

[1] https://security.googleblog.com/2019/02/introducing-adiantum-encryption-for.html
[2] https://en.wikipedia.org/wiki/Adiantum_(cipher)
[3] https://tosc.iacr.org/index.php/ToSC/article/view/7360
[4] https://github.com/google/adiantum
[5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=059c2a4d8e164dccc3078e49e7f286023b019a98

-------

regards
Yura Sokolov
[email-masked]
[email-masked]

---

From: Stephen Frost <[email-masked]>
Date: Mon, 25 Oct 2021 11:58:14 -0400

Greetings,

> a CTR-steaming-encryption API for temporary tables be easier to

Having a unified API for temporary file I/O (that could then be extended
to provide encryption) would definitely help with reducing the size of a
TDE patch.  The approach used in the Cybertec patch was to make
temporary file access block based, but the way that was implemented was
with an API different from pread/pwrite and that meant changing pretty
much all of the call sites for temporary file access, which naturally
resulted in changes in a lot of otherwise unrelated code.

There was an argument put forth that a block-based API for temporary
file access would generally be good as it would mean fewer syscalls.  If
we can get behind that and make it happen in (relatively) short order
then we'd certainly be better off when it comes to implementing TDE
which also deals with temporary files.  I'm a bit concerned about that
approach due to the changes needed but I'm also not against it.  I do
think that an API which was more-or-less the same as what's used today
would be a smaller change and therefore might be easier to get in and
that it'd also make a TDE patch smaller.  Perhaps both could be
accomplished (an API that's similar to today, but the actual file access
being block-based).

Either way, we should get that unification done in the core code first
as an independent effort.  That's what I hope the Cybertec folks have
had a chance to work on.

As for the specific encryption method to use, using CTR would be simpler
as it doesn't require access to be block-based, though we would need to
make sure to not re-use the IV across any of the temporary files being
created (potentially concurrently).  Probably not that hard to do but
just something to make sure we do.  Of course, if we arrange for
block-based access then we could use XTS or perhaps GCM/GCM-SIV if we
wanted to.

Thanks,

Stephen

---

From: Stephen Frost <[email-masked]>
Date: Mon, 25 Oct 2021 12:12:27 -0400

Greetings,

> Instead of debatting XTS vs GCM-SIV I'd suggest Google's Adiantum [1][2]

That sounds like a great thing to think about adding ... after we get
something in that's based on XTS.

>   LSN+reloid+blocknumber tuple as IV even for hint-bit changes since every

The advantage of GCM-SIV is that it provides integrity as well as
confidentiality.

>   additional Nonce/Auth Code.

Sure, in which case it's something that could potentially be added later
as another option in the future.  I don't think we'll always have just
one encryption method and it's good to generally think about what it
might look like to have others but I don't think it makes sense to try
and get everything in all at once.

Thanks,

Stephen

---

From: Bruce Momjian <[email-masked]>
Date: Mon, 25 Oct 2021 16:06:50 -0400

> block-based access then we could use XTS or perhaps GCM/GCM-SIV if we

Agreed on all points.

---

From: Yura Sokolov <[email-masked]>
Date: Mon, 25 Oct 2021 23:32:43 +0300

> something in that's based on XTS.

Why? I see no points to do it after. Why not XTS after Adiantum?

Ok, I see one: XTS is standartized.

> The advantage of GCM-SIV is that it provides integrity as well as

Integrity could be based on simple non-cryptographic checksum, and it could
be checked after decryption. It would be imposible to intentionally change
encrypted page in a way it will pass checksum after decription. 

Currently we have 16bit checksum, and it is very small. But having larger
checksum is orthogonal (ie doesn't bound) to having encryption.

In fact, Adiantum is easily made close to SIV construction:
- just leave last 8/16 bytes zero. If after decription they are zero,
  then integrity check passed.
That is because SIV and Adiantum are very similar in its structure:
- SIV:
-- hash
-- then stream cipher
- Adiantum:
-- hash (except last 16bytes)
-- then encrypt last 16bytes with hash,
-- then stream cipher
-- then hash.
If last N (N>16) bytes is nonce + zero bytes, then "hash, then encrypt last
16bytes with hash" become equivalent to just "hash", and Adiantum became
logical equivalent to SIV.

> and get everything in all at once.

And among others Adiantum looks best: it is fast even without hardware
acceleration, it provides whole block encryption (ie every bit depends
on every bit) and it doesn't bound to plain-text format.

> Stephen

regards,

Yura

PS. Construction beside SIV and Adiantum could be used with XTS as well.
I.e. instead of AES-GCM-SIV it could be AES-XTS-SIV.
And same way AES-XTS could be used instead of Chacha12 in Adiantum.

---

From: Sasasu <[email-masked]>
Date: Tue, 26 Oct 2021 11:08:38 +0800

On 2021/10/26 04:32, Yura Sokolov wrote:
> acceleration,

No, AES is fast on modern high-end hardware.

on X86 AMD 3700X
type              1024 bytes  8192 bytes   16384 bytes
aes-128-ctr       8963982.50k 11124613.88k 11509149.42k
aes-128-gcm       3978860.44k 4669417.10k  4732070.64k
aes-128-xts       7776628.39k 9073664.63k  9264617.74k
chacha20-poly1305 2043729.73k 2131296.36k  2141002.10k

on ARM RK3399, A53 middle-end with AES-NI
type              1024 bytes   8192 bytes   16384 bytes
aes-128-ctr       1663857.66k  1860930.22k  1872991.57k
aes-128-xts       685086.38k   712906.07k   716073.64k
aes-128-gcm       985578.84k   1054818.30k  1056768.00k
chacha20-poly1305 309012.82k   318889.98k   319711.91k

I think the baseline is the speed when using read(2) syscall on 
/dev/zero (which is 3.6GiB/s, on ARM is 980MiB/s)
chacha is fast on the low-end arm, but I haven't seen any HTTPS sites 
using chacha, including Cloudflare and Google.

 > Ok, I see one: XTS is standartized.
:>
PostgreSQL even not discuss single-table key rotation or remote KMS.
I think it's too hard to use an encryption algorithm which openssl 
doesn't implement.

---

From: Yura Sokolov <[email-masked]>
Date: Tue, 26 Oct 2021 12:33:47 +0300

> using chacha, including Cloudflare and Google.

1. Chacha20-poly1305 includes authentication code (poly1305),
   aes-gcm also includes (GCM).
   But aes-128-(ctr,xts) doesn't.
   Therefore, Chacha should be compared with ctr,xts, not Chacha-Poly1305.
2. Chacha20 has security margin x2.8: only 7 rounds from 20 are broken.
   AES-128 has security margin x1.4: broken 7 rounds from 10.
   That is why Adiantum uses Chacha12: it is still "more secure" than AES-128.

Yes, AES with AES-NI is fastest. But not so much.

And, AES-CTR could be easily used instead of ChaCha12 in Adiantum.
Adiantum uses ChaCha12 as a stream cipher, and any other stream cipher will
be ok as well with minor modifications to algorithm. 

> I think it's too hard to use an encryption algorithm which openssl 

That is argument. But, again, openssl could be used for primitives:
AES + AES-CTR + Poly/GCM. And Adiantum like construction could be
composed from them quite easily.

---

From: Sasasu <[email-masked]>
Date: Tue, 26 Oct 2021 23:47:10 +0800

> be ok as well with minor modifications to algorithm.

not so much ~= half speed.

I prefer to use AES on all devices because AES is faster and more power 
efficient. using chacha only on low-end arm devices running complex 
program which most people do not have this device.

I reserve my opinion on this point, but I agree with you on the rest.
And I also agree and think it should add more algorithms. The current 
implementation does not have any reserved fields, which makes any 
upgrade like adding a new algorithm unfeasible.

 > composed from them quite easily.

implement Adiantum is a small problem (which doesn't look good, lack 
security audits). the real problem is there are too many code path can 
trigger disk IO.
TDE need modify them. each code path has different behavior (align or 
unaligned, once or more than once). and front-end tools even not use VF 
layer, they use pread with offset. TDE need fix them all. at the same 
time, keep the patch small enough.

I still think there need a unified IO API, not only modify 
BufFileDumpBuffer() and BufFileLoadBuffer(). the front-end tools also 
use this API will be great. with that API, TDE can focus on block IO. 
then give out a small patch.

Other works can also benefit from this API, like PostgreSQL with AIO, 
PostgreSQL on S3 (BLKSZ=4M), PostgreSQL on PMEM(no FS) and many.

---

From: Stephen Frost <[email-masked]>
Date: Tue, 26 Oct 2021 15:43:30 -0400

Greetings,

> Ok, I see one: XTS is standartized.

That's certainly one aspect of it.  It's also more easily available to
us, and frankly the people working on this and writing the patches have
a better understanding of XTS from having looked into it.  That it's
also more widely used is another reason.

> encrypted page in a way it will pass checksum after decription. 

No, it wouldn't be impossible when we're talking about non-cryptographic
checksums.  That is, in fact, why you'd call them that.  If it were
impossible (or at least utterly impractical) then you'd be able to claim
that it's cryptographic-level integrity validation.

> checksum is orthogonal (ie doesn't bound) to having encryption.

Sure, but that would also require a page-format change.  We've pointed
out the downsides of that and what it would prevent in terms of
use-cases.  That's still something that might happen but it would be a
different effort from this.

> logical equivalent to SIV.

While I appreciate your interest in this, I don't think it makes sense
for us to try and implement something of our own- we're not
cryptographers.  Best is to look at published guideance and what other
projects have had success doing, and that's what this thread has been
about.

> on every bit) and it doesn't bound to plain-text format.

And it could still be added later as another option if folks really want
it to be.  I've outlined why it makes sense to go with XTS first but I
don't mean that to imply that we'll only ever have that.  Indeed, once
we've actually got something, adding other methods will almost certainly
be simpler.  Trying to do everything from the start will make this very
difficult to accomplish though.

Thanks,

Stephen

---

From: Tomas Vondra <[email-masked]>
Date: Tue, 26 Oct 2021 23:11:39 +0200

On 10/26/21 21:43, Stephen Frost wrote:
>> ... >>

Yeah, our checksums are probabilistic protection against rare and random 
bitflips cause by hardware, not against an attacker in the crypto sense.

To explain why it's not enough, consider our checksum is uint16, i.e. 
there are only 64k possible values. In other words, you can try flipping 
bits in the encrypted page, and after generating 64k you're guaranteed 
to have at least one collision. Yes, it's harder to get collision with 
the existing checksum, and compression methods that diffuse bits better 
makes it harder to get a valid page after decryption, but it's simply 
not the same thing as a crypto integrity.

Let's not try inventing something custom, there's been enough crypto 
failures due to smart custom stuff in the past already.

BTW I'm not sure what the existing patches do, but I wonder if we should 
calculate the checksum before or after encryption. I'd say it should be 
after encryption, because checksums were meant as a protection against 
issues at the storage level, so the checksum should be on what's written 
to storage, and it'd also allow offline verification of checksums etc. 
(Of course, that'd make the whole idea of relying on our checksums even 
more futile.)

Note: Maybe there are reasons why the checksum needs to be calculated 
before encryption, not sure.

> different effort from this.

... and if such page format ends up happening, it'd be fairly easy to 
just add some extra crypto data into the page header and not rely on the 
data checksums at all.

> projects have had success doing, and that's what this thread has been

Yeah, I personally don't see much difference between XTS and Adiantum.

There are a bunch of benefits, but the main reason why Google developed 
it seems to be performance on low-end ARM machines (i.e. phones). Which 
is nice, but it's probably not hugely important - very few people run Pg 
on such machines, especially in performance-sensitive context.

It's true Adiantum is probably more resilient to IV reuse etc. but it's 
not like XTS is suddenly obsolete, and it certainly doesn't solve the 
integrity issue etc.

> difficult to accomplish though.

Yeah.

So maybe the best thing is simply to roll with both - design the whole 
feature in a way that allows selecting the encryption scheme, with two 
options. That's generally a good engineering practice, as it ensures 
things are not coupled too much. And it's not like the encryption 
methods are expected to be super difficult.

regards

---

From: Bruce Momjian <[email-masked]>
Date: Tue, 26 Oct 2021 17:39:30 -0400

On Tue, Oct 26, 2021 at 11:11:39PM +0200, Tomas Vondra wrote:
> issues at the storage level, so the checksum should be on what's written to
> storage, and it'd also allow offline verification of checksums etc. (Of
> course, that'd make the whole idea of relying on our checksums even more
> futile.)
> 
> Note: Maybe there are reasons why the checksum needs to be calculated before
> encryption, not sure.

Yes, these are the tradeoffs --- allowing offline checksum checking
without requiring the key vs. giving _some_ integrity checking and
requiring the key.

> There are a bunch of benefits, but the main reason why Google developed it
> seems to be performance on low-end ARM machines (i.e. phones). Which is
> nice, but it's probably not hugely important - very few people run Pg on
> such machines, especially in performance-sensitive context.
> 
> It's true Adiantum is probably more resilient to IV reuse etc. but it's not
> like XTS is suddenly obsolete, and it certainly doesn't solve the integrity
> issue etc.
> 
...
> options. That's generally a good engineering practice, as it ensures things
> are not coupled too much. And it's not like the encryption methods are
> expected to be super difficult.

I am not in favor of adding additional options to this feature unless we
can explain why users should choose one over the other.  There is also
the problem of OpenSSL not supporting Adiantum.

---

From: Stephen Frost <[email-masked]>
Date: Mon, 1 Nov 2021 14:24:36 -0400

Greetings,

> without requiring the key vs. giving _some_ integrity checking and

I'm in favor of calculating the checksum before encrypting as that will
still catch the storage level bit-flips that it was implemented to
address in the first place and will also make it so that we're very
likely to realize we have an incorrect key before trying to do anything
with the page.  That it might also serve as a deterrent against
attackers trying to randomly flip bits in a page has perhaps some value
but without a cryptographic-level hash it isn't really enough to prevent
against an attacker who has write access to a page.

Any tools which include checking the checksum on pages already have to
deal with clusters where checksums aren't enabled anyway and I wouldn't
expect it to generally be an issue for tools which want to validate
checksums on an encrypted cluster to be able to have the appropriate
key(s) necessary for doing so and to be able to perform the decryption
to do the check.  We can certainly make pg_checksums do this and I don't
see it as an issue for pgbackrest, as two examples that I've
specifically thought about.

> the problem of OpenSSL not supporting Adiantum.

I can understand the general idea that we should be sure to engineer
this in a way that multiple methods can be used, as surely one day folks
will say that AES128 isn't acceptable any more.  In terms of what we'll
do from the start, I would think providing the options of AES128 and
AES256 would be good to ensure that we have the bits covered to support
multiple methods and I don't think that would put us into a situation of
having to really explain which to use to users (we don't for pgcrypto
anyway, as an example).  I agree that we shouldn't be looking at adding
in a whole new crypto library for this though, that's a large and
independent effort (see the work on NSS happening nearby).

Thanks,

Stephen

---

From: Sasasu <[email-masked]>
Date: Tue, 2 Nov 2021 14:22:39 +0800

On 2021/11/2 02:24, Stephen Frost wrote:
> will say that AES128 isn't acceptable any more.
Cheers!

---

From: Bruce Momjian <[email-masked]>
Date: Tue, 2 Nov 2021 17:49:03 -0400

> multiple methods and I don't think that would put us into a situation of

My patch supports AES128, AES192, and AES256.

---

From: Stephen Frost <[email-masked]>
Date: Wed, 3 Nov 2021 14:45:22 -0400

Greetings,

> My patch supports AES128, AES192, and AES256.

Right, so we're already showing that it's flexible to allow for multiple
encryption methods.  If folks want more then it's on them to research
how they'd work exactly and explain why they'd be useful to add and how
users might make an informed choice (though, again, I don't think we
need to go *too* deep into that as we don't for, eg, pgcrypto, and I
don't believe we've ever heard people complain about that).

Thanks,

Stephen

---

From: Bruce Momjian <[email-masked]>
Date: Fri, 12 Nov 2021 13:13:07 -0500

> independent effort (see the work on NSS happening nearby).

Since it has been two weeks since the last activity on this thread, I
have updated the TDE wiki to reflect the conclusions and discussions:

	https://wiki.postgresql.org/wiki/Transparent_Data_Encryption

---

From: Antonin Houska <[email-masked]>
Date: Mon, 29 Nov 2021 08:37:31 +0100

> resulted in changes in a lot of otherwise unrelated code.

The changes to buffile.c are not trivial, but we haven't really changed the
API, as long as you mean BufFileCreateTemp(), BufFileWrite(), BufFileRead().

What our patch affects on the caller side is that BufFileOpenTransient(),
BufFileCloseTransient(), BufFileWriteTransient() and BufFileReadTransient()
replace OpenTransientFile(), CloseTransientFile(), write()/fwrite() and
read()/fread() respectively in reorderbuffer.c and in pgstat.c. These changes
become a little bit less invasive in TDE 1.1 than they were in 1.0, see [1],
see the diffs attached.

(I expect that [2] will get committed someday so that the TDE feature won't
affect pgstat.c in the future at all.)

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

[1] https://github.com/cybertec-postgresql/postgres/tree/PG_14_TDE_1_1

[2] https://commitfest.postgresql.org/34/1708/

diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index d474ea1d0a..9cb0708e41 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -99,6 +99,7 @@
 #include "replication/slot.h"
 #include "replication/snapbuild.h"	/* just for SnapBuildSnapDecRefcount */
 #include "storage/bufmgr.h"
+#include "storage/buffile.h"
 #include "storage/fd.h"
 #include "storage/sinval.h"
 #include "utils/builtins.h"
@@ -131,21 +132,13 @@ typedef struct ReorderBufferTupleCidEnt
 	CommandId	combocid;		/* just for debugging */
 } ReorderBufferTupleCidEnt;
 
-/* Virtual file descriptor with file offset tracking */
-typedef struct TXNEntryFile
-{
-	File		vfd;			/* -1 when the file is closed */
-	off_t		curOffset;		/* offset for next write or read. Reset to 0
-								 * when vfd is opened. */
-} TXNEntryFile;
-
 /* k-way in-order change iteration support structures */
 typedef struct ReorderBufferIterTXNEntry
 {
 	XLogRecPtr	lsn;
 	ReorderBufferChange *change;
 	ReorderBufferTXN *txn;
-	TXNEntryFile file;
+	TransientBufFile *file;
 	XLogSegNo	segno;
 } ReorderBufferIterTXNEntry;
 
@@ -243,14 +236,22 @@ static void ReorderBufferExecuteInvalidations(uint32 nmsgs, SharedInvalidationMe
  * Disk serialization support functions
  * ---------------------------------------
  */
+static void ReorderBufferTweakBase(ReorderBufferTXN *txn,
+								   char tweak_base[TWEAK_BASE_SIZE]);
 static void ReorderBufferCheckMemoryLimit(ReorderBuffer *rb);
 static void ReorderBufferSerializeTXN(ReorderBuffer *rb, ReorderBufferTXN *txn);
 static void ReorderBufferSerializeChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
-										 int fd, ReorderBufferChange *change);
+							 TransientBufFile *file, ReorderBufferChange *change);
+static void ReorderBufferWriteData(TransientBufFile *file, void *ptr, size_t size,
+					   ReorderBufferTXN *txn);
 static Size ReorderBufferRestoreChanges(ReorderBuffer *rb, ReorderBufferTXN *txn,
-										TXNEntryFile *file, XLogSegNo *segno);
+							TransientBufFile **file, XLogSegNo *segno);
 static void ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
-									   char *change);
+						   TransientBufFile **file);
+static ReorderBufferTupleBuf *ReorderBufferRestoreTuple(ReorderBuffer *rb,
+						  TransientBufFile *file);
+static void ReorderBufferReadData(TransientBufFile *file, void *ptr, size_t size,
+					  bool *no_data_p);
 static void ReorderBufferRestoreCleanup(ReorderBuffer *rb, ReorderBufferTXN *txn);
 static void ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
 									 bool txn_prepared);
@@ -342,8 +343,6 @@ ReorderBufferAllocate(void)
 	buffer->by_txn_last_xid = InvalidTransactionId;
 	buffer->by_txn_last_txn = NULL;
 
-	buffer->outbuf = NULL;
-	buffer->outbufsize = 0;
 	buffer->size = 0;
 
 	buffer->spillTxns = 0;
@@ -1241,7 +1240,7 @@ ReorderBufferIterTXNInit(ReorderBuffer *rb, ReorderBufferTXN *txn,
 
 	for (off = 0; off < state->nr_txns; off++)
 	{
-		state->entries[off].file.vfd = -1;
+		state->entries[off].file = NULL;
 		state->entries[off].segno = 0;
 	}
 
@@ -1423,8 +1422,8 @@ ReorderBufferIterTXNFinish(ReorderBuffer *rb,
 
 	for (off = 0; off < state->nr_txns; off++)
 	{
-		if (state->entries[off].file.vfd != -1)
-			FileClose(state->entries[off].file.vfd);
+		if (state->entries[off].file)
+			BufFileCloseTransient(state->entries[off].file);
 	}
 
 	/* free memory we might have "leaked" in the last *Next call */
@@ -3332,21 +3331,39 @@ ReorderBufferXidHasBaseSnapshot(ReorderBuffer *rb, TransactionId xid)
  */
 
 /*
- * Ensure the IO buffer is >= sz.
+ * Initialize the common part of the encryption tweak.
  */
 static void
-ReorderBufferSerializeReserve(ReorderBuffer *rb, Size sz)
+ReorderBufferTweakBase(ReorderBufferTXN *txn,
+					   char tweak_base[TWEAK_BASE_SIZE])
 {
-	if (!rb->outbufsize)
-	{
-		rb->outbuf = MemoryContextAlloc(rb->context, sz);
-		rb->outbufsize = sz;
-	}
-	else if (rb->outbufsize < sz)
-	{
-		rb->outbuf = repalloc(rb->outbuf, sz);
-		rb->outbufsize = sz;
-	}
+	char	*c = tweak_base;
+	pid_t	pid = MyProcPid;
+	int	pid_bytes;
+
+/* Only this part of the PID fits into the tweak. */
+#define PID_BYTES_USABLE	3
+
+	StaticAssertStmt(1 + sizeof(TransactionId) + PID_BYTES_USABLE
+					 <= TWEAK_BASE_SIZE,
+					 "tweak components do not fit into TWEAK_BASE_SIZE");
+
+	memset(tweak_base, 0, TWEAK_BASE_SIZE);
+	*c = TRANS_BUF_FILE_REORDERBUFFER;
+	c++;
+	memcpy(c, &txn->xid, sizeof(TransactionId));
+	c += sizeof(TransactionId);
+
+	/*
+	 * There's only room for PID_BYTES_USABLE bytes of the PID. Use the less
+	 * significant part so that PID increment always causes tweak change.
+	 */
+	pid_bytes = Min(sizeof(pid), PID_BYTES_USABLE);
+#ifdef WORDS_BIGENDIAN
+	memcpy(c, ((char *) &pid) + sizeof(pid) - pid_bytes, pid_bytes);
+#else
+	memcpy(c, &pid, pid_bytes);
+#endif
 }
 
 /*
@@ -3517,9 +3534,10 @@ ReorderBufferSerializeTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
 {
 	dlist_iter	subtxn_i;
 	dlist_mutable_iter change_i;
-	int			fd = -1;
+	TransientBufFile *file = NULL;
 	XLogSegNo	curOpenSegNo = 0;
 	Size		spilled = 0;
+
 	Size		size = txn->size;
 
 	elog(DEBUG2, "spill %u changes in XID %u to disk",
@@ -3545,13 +3563,14 @@ ReorderBufferSerializeTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
 		 * store in segment in which it belongs by start lsn, don't split over
 		 * multiple segments tho
 		 */
-		if (fd == -1 ||
+		if (file == NULL ||
 			!XLByteInSeg(change->lsn, curOpenSegNo, wal_segment_size))
 		{
 			char		path[MAXPGPATH];
+			char		tweak_base[TWEAK_BASE_SIZE];
 
-			if (fd != -1)
-				CloseTransientFile(fd);
+			if (file)
+				BufFileCloseTransient(file);
 
 			XLByteToSeg(change->lsn, curOpenSegNo, wal_segment_size);
 
@@ -3562,17 +3581,15 @@ ReorderBufferSerializeTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
 			ReorderBufferSerializedPath(path, MyReplicationSlot, txn->xid,
 										curOpenSegNo);
 
+			if (data_encrypted)
+				ReorderBufferTweakBase(txn, tweak_base);
 			/* open segment, create it if necessary */
-			fd = OpenTransientFile(path,
-								   O_CREAT | O_WRONLY | O_APPEND | PG_BINARY);
-
-			if (fd < 0)
-				ereport(ERROR,
-						(errcode_for_file_access(),
-						 errmsg("could not open file \"%s\": %m", path)));
+			file = BufFileOpenTransient(path,
+										O_CREAT | O_WRONLY | O_APPEND | PG_BINARY,
+										tweak_base);
 		}
 
-		ReorderBufferSerializeChange(rb, txn, fd, change);
+		ReorderBufferSerializeChange(rb, txn, file, change);
 		dlist_delete(&change->node);
 		ReorderBufferReturnChange(rb, change, true);
 
@@ -3597,8 +3614,8 @@ ReorderBufferSerializeTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
 	txn->nentries_mem = 0;
 	txn->txn_flags |= RBTXN_IS_SERIALIZED;
 
-	if (fd != -1)
-		CloseTransientFile(fd);
+	if (file)
+		BufFileCloseTransient(file);
 }
 
 /*
@@ -3606,15 +3623,13 @@ ReorderBufferSerializeTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
  */
 static void
 ReorderBufferSerializeChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
-							 int fd, ReorderBufferChange *change)
+							 TransientBufFile *file, ReorderBufferChange *change)
 {
-	ReorderBufferDiskChange *ondisk;
+	ReorderBufferDiskChange hdr;
 	Size		sz = sizeof(ReorderBufferDiskChange);
 
-	ReorderBufferSerializeReserve(rb, sz);
-
-	ondisk = (ReorderBufferDiskChange *) rb->outbuf;
-	memcpy(&ondisk->change, change, sizeof(ReorderBufferChange));
+	memcpy((char *) &hdr + offsetof(ReorderBufferDiskChange, change),
+		   change, sizeof(ReorderBufferChange));
 
 	switch (change->action)
 	{
@@ -3624,7 +3639,6 @@ ReorderBufferSerializeChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
 		case REORDER_BUFFER_CHANGE_DELETE:
 		case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_INSERT:
 			{
-				char	   *data;
 				ReorderBufferTupleBuf *oldtup,
 						   *newtup;
 				Size		oldlen = 0;
@@ -3647,84 +3661,71 @@ ReorderBufferSerializeChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
 					sz += newlen;
 				}
 
-				/* make sure we have enough space */
-				ReorderBufferSerializeReserve(rb, sz);
-
-				data = ((char *) rb->outbuf) + sizeof(ReorderBufferDiskChange);
-				/* might have been reallocated above */
-				ondisk = (ReorderBufferDiskChange *) rb->outbuf;
+				hdr.size = sz;
+				ReorderBufferWriteData(file, &hdr, sizeof(ReorderBufferDiskChange),
+									   txn);
 
 				if (oldlen)
 				{
-					memcpy(data, &oldtup->tuple, sizeof(HeapTupleData));
-					data += sizeof(HeapTupleData);
-
-					memcpy(data, oldtup->tuple.t_data, oldlen);
-					data += oldlen;
+					ReorderBufferWriteData(file, &oldtup->tuple,
+										   sizeof(HeapTupleData), txn);
+					ReorderBufferWriteData(file, oldtup->tuple.t_data, oldlen,
+										   txn);
 				}
 
 				if (newlen)
 				{
-					memcpy(data, &newtup->tuple, sizeof(HeapTupleData));
-					data += sizeof(HeapTupleData);
-
-					memcpy(data, newtup->tuple.t_data, newlen);
-					data += newlen;
+					ReorderBufferWriteData(file, &newtup->tuple,
+										   sizeof(HeapTupleData), txn);
+					ReorderBufferWriteData(file, newtup->tuple.t_data, newlen,
+										   txn);
 				}
 				break;
 			}
 		case REORDER_BUFFER_CHANGE_MESSAGE:
 			{
-				char	   *data;
 				Size		prefix_size = strlen(change->data.msg.prefix) + 1;
 
 				sz += prefix_size + change->data.msg.message_size +
 					sizeof(Size) + sizeof(Size);
-				ReorderBufferSerializeReserve(rb, sz);
 
-				data = ((char *) rb->outbuf) + sizeof(ReorderBufferDiskChange);
-
-				/* might have been reallocated above */
-				ondisk = (ReorderBufferDiskChange *) rb->outbuf;
+				hdr.size = sz;
+				ReorderBufferWriteData(file, &hdr,
+									   sizeof(ReorderBufferDiskChange),
+									   txn);
 
 				/* write the prefix including the size */
-				memcpy(data, &prefix_size, sizeof(Size));
-				data += sizeof(Size);
-				memcpy(data, change->data.msg.prefix,
-					   prefix_size);
-				data += prefix_size;
+				ReorderBufferWriteData(file, &prefix_size, sizeof(Size), txn);
+				ReorderBufferWriteData(file, change->data.msg.prefix,
+									   prefix_size, txn);
 
 				/* write the message including the size */
-				memcpy(data, &change->data.msg.message_size, sizeof(Size));
-				data += sizeof(Size);
-				memcpy(data, change->data.msg.message,
-					   change->data.msg.message_size);
-				data += change->data.msg.message_size;
+				ReorderBufferWriteData(file, &change->data.msg.message_size,
+									   sizeof(Size), txn);
+				ReorderBufferWriteData(file, change->data.msg.message,
+									   change->data.msg.message_size, txn);
 
 				break;
 			}
 		case REORDER_BUFFER_CHANGE_INVALIDATION:
 			{
-				char	   *data;
 				Size		inval_size = sizeof(SharedInvalidationMessage) *
 				change->data.inval.ninvalidations;
 
 				sz += inval_size;
 
-				ReorderBufferSerializeReserve(rb, sz);
-				data = ((char *) rb->outbuf) + sizeof(ReorderBufferDiskChange);
-
-				/* might have been reallocated above */
-				ondisk = (ReorderBufferDiskChange *) rb->outbuf;
-				memcpy(data, change->data.inval.invalidations, inval_size);
-				data += inval_size;
-
+				hdr.size = sz;
+				ReorderBufferWriteData(file, &hdr,
+									   sizeof(ReorderBufferDiskChange),
+									   txn);
+				ReorderBufferWriteData(file, change->data.inval.invalidations,
+									   inval_size,
+									   txn);
 				break;
 			}
 		case REORDER_BUFFER_CHANGE_INTERNAL_SNAPSHOT:
 			{
 				Snapshot	snap;
-				char	   *data;
 
 				snap = change->data.snapshot;
 
@@ -3732,78 +3733,50 @@ ReorderBufferSerializeChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
 					sizeof(TransactionId) * snap->xcnt +
 					sizeof(TransactionId) * snap->subxcnt;
 
-				/* make sure we have enough space */
-				ReorderBufferSerializeReserve(rb, sz);
-				data = ((char *) rb->outbuf) + sizeof(ReorderBufferDiskChange);
-				/* might have been reallocated above */
-				ondisk = (ReorderBufferDiskChange *) rb->outbuf;
+				hdr.size = sz;
+				ReorderBufferWriteData(file, &hdr,
+									   sizeof(ReorderBufferDiskChange), txn);
 
-				memcpy(data, snap, sizeof(SnapshotData));
-				data += sizeof(SnapshotData);
+				ReorderBufferWriteData(file, snap, sizeof(SnapshotData), txn);
 
 				if (snap->xcnt)
-				{
-					memcpy(data, snap->xip,
-						   sizeof(TransactionId) * snap->xcnt);
-					data += sizeof(TransactionId) * snap->xcnt;
-				}
+					ReorderBufferWriteData(file, snap->xip,
+										   sizeof(TransactionId) * snap->xcnt,
+										   txn);
 
 				if (snap->subxcnt)
-				{
-					memcpy(data, snap->subxip,
-						   sizeof(TransactionId) * snap->subxcnt);
-					data += sizeof(TransactionId) * snap->subxcnt;
-				}
+					ReorderBufferWriteData(file, snap->subxip,
+										   sizeof(TransactionId) * snap->subxcnt,
+										   txn);
 				break;
 			}
 		case REORDER_BUFFER_CHANGE_TRUNCATE:
 			{
 				Size		size;
-				char	   *data;
 
 				/* account for the OIDs of truncated relations */
 				size = sizeof(Oid) * change->data.truncate.nrelids;
 				sz += size;
 
-				/* make sure we have enough space */
-				ReorderBufferSerializeReserve(rb, sz);
-
-				data = ((char *) rb->outbuf) + sizeof(ReorderBufferDiskChange);
-				/* might have been reallocated above */
-				ondisk = (ReorderBufferDiskChange *) rb->outbuf;
-
-				memcpy(data, change->data.truncate.relids, size);
-				data += size;
+				hdr.size = sz;
+				ReorderBufferWriteData(file, &hdr, sizeof(ReorderBufferDiskChange),
+									   txn);
 
+				ReorderBufferWriteData(file, change->data.truncate.relids, size,
+									   txn);
 				break;
 			}
 		case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_CONFIRM:
 		case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_ABORT:
 		case REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID:
 		case REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID:
+			hdr.size = sz;
+			ReorderBufferWriteData(file, &hdr, sizeof(ReorderBufferDiskChange),
+								   txn);
 			/* ReorderBufferChange contains everything important */
 			break;
 	}
 
-	ondisk->size = sz;
-
-	errno = 0;
-	pgstat_report_wait_start(WAIT_EVENT_REORDER_BUFFER_WRITE);
-	if (write(fd, rb->outbuf, ondisk->size) != ondisk->size)
-	{
-		int			save_errno = errno;
-
-		CloseTransientFile(fd);
-
-		/* if write didn't set errno, assume problem is no disk space */
-		errno = save_errno ? save_errno : ENOSPC;
-		ereport(ERROR,
-				(errcode_for_file_access(),
-				 errmsg("could not write to data file for XID %u: %m",
-						txn->xid)));
-	}
-	pgstat_report_wait_end();
-
 	/*
 	 * Keep the transaction's final_lsn up to date with each change we send to
 	 * disk, so that ReorderBufferRestoreCleanup works correctly.  (We used to
@@ -3814,8 +3787,21 @@ ReorderBufferSerializeChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
 	 */
 	if (txn->final_lsn < change->lsn)
 		txn->final_lsn = change->lsn;
+}
 
-	Assert(ondisk->change.action == change->action);
+/*
+ * Wrapper for BufFileWriteTransient() that raises ERROR if the whole chunk
+ * was not written. XXX Should this be a macro?
+ */
+static void
+ReorderBufferWriteData(TransientBufFile *file, void *ptr, size_t size,
+					   ReorderBufferTXN *txn)
+{
+	if (BufFileWriteTransient(file, ptr, size) != size)
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not write to data file for XID %u: %m",
+						txn->xid)));
 }
 
 /* Returns true, if the output plugin supports streaming, false, otherwise. */
@@ -4058,12 +4044,11 @@ ReorderBufferChangeSize(ReorderBufferChange *change)
  */
 static Size
 ReorderBufferRestoreChanges(ReorderBuffer *rb, ReorderBufferTXN *txn,
-							TXNEntryFile *file, XLogSegNo *segno)
+							TransientBufFile **file, XLogSegNo *segno)
 {
 	Size		restored = 0;
 	XLogSegNo	last_segno;
 	dlist_mutable_iter cleanup_iter;
-	File	   *fd = &file->vfd;
 
 	Assert(txn->first_lsn != InvalidXLogRecPtr);
 	Assert(txn->final_lsn != InvalidXLogRecPtr);
@@ -4084,12 +4069,10 @@ ReorderBufferRestoreChanges(ReorderBuffer *rb, ReorderBufferTXN *txn,
 
 	while (restored < max_changes_in_memory && *segno <= last_segno)
 	{
-		int			readBytes;
-		ReorderBufferDiskChange *ondisk;
-
-		if (*fd == -1)
+		if (*file == NULL)
 		{
 			char		path[MAXPGPATH];
+			char		tweak_base[TWEAK_BASE_SIZE];
 
 			/* first time in */
 			if (*segno == 0)
@@ -4104,86 +4087,27 @@ ReorderBufferRestoreChanges(ReorderBuffer *rb, ReorderBufferTXN *txn,
 			ReorderBufferSerializedPath(path, MyReplicationSlot, txn->xid,
 										*segno);
 
-			*fd = PathNameOpenFile(path, O_RDONLY | PG_BINARY);
-
-			/* No harm in resetting the offset even in case of failure */
-			file->curOffset = 0;
-
-			if (*fd < 0 && errno == ENOENT)
+			if (data_encrypted)
+				ReorderBufferTweakBase(txn, tweak_base);
+			*file = BufFileOpenTransient(path, O_RDONLY | PG_BINARY,
+										 tweak_base);
+			if (*file == NULL)
 			{
-				*fd = -1;
+				Assert(errno == ENOENT);
 				(*segno)++;
 				continue;
 			}
-			else if (*fd < 0)
-				ereport(ERROR,
-						(errcode_for_file_access(),
-						 errmsg("could not open file \"%s\": %m",
-								path)));
 		}
 
-		/*
-		 * Read the statically sized part of a change which has information
-		 * about the total size. If we couldn't read a record, we're at the
-		 * end of this file.
-		 */
-		ReorderBufferSerializeReserve(rb, sizeof(ReorderBufferDiskChange));
-		readBytes = FileRead(file->vfd, rb->outbuf,
-							 sizeof(ReorderBufferDiskChange),
-							 file->curOffset, WAIT_EVENT_REORDER_BUFFER_READ);
-
-		/* eof */
-		if (readBytes == 0)
+		ReorderBufferRestoreChange(rb, txn, file);
+		if (*file)
+			restored++;
+		else
 		{
-			FileClose(*fd);
-			*fd = -1;
+			/* No data could be restored. */
 			(*segno)++;
 			continue;
 		}
-		else if (readBytes < 0)
-			ereport(ERROR,
-					(errcode_for_file_access(),
-					 errmsg("could not read from reorderbuffer spill file: %m")));
-		else if (readBytes != sizeof(ReorderBufferDiskChange))
-			ereport(ERROR,
-					(errcode_for_file_access(),
-					 errmsg("could not read from reorderbuffer spill file: read %d instead of %u bytes",
-							readBytes,
-							(uint32) sizeof(ReorderBufferDiskChange))));
-
-		file->curOffset += readBytes;
-
-		ondisk = (ReorderBufferDiskChange *) rb->outbuf;
-
-		ReorderBufferSerializeReserve(rb,
-									  sizeof(ReorderBufferDiskChange) + ondisk->size);
-		ondisk = (ReorderBufferDiskChange *) rb->outbuf;
-
-		readBytes = FileRead(file->vfd,
-							 rb->outbuf + sizeof(ReorderBufferDiskChange),
-							 ondisk->size - sizeof(ReorderBufferDiskChange),
-							 file->curOffset,
-							 WAIT_EVENT_REORDER_BUFFER_READ);
-
-		if (readBytes < 0)
-			ereport(ERROR,
-					(errcode_for_file_access(),
-					 errmsg("could not read from reorderbuffer spill file: %m")));
-		else if (readBytes != ondisk->size - sizeof(ReorderBufferDiskChange))
-			ereport(ERROR,
-					(errcode_for_file_access(),
-					 errmsg("could not read from reorderbuffer spill file: read %d instead of %u bytes",
-							readBytes,
-							(uint32) (ondisk->size - sizeof(ReorderBufferDiskChange)))));
-
-		file->curOffset += readBytes;
-
-		/*
-		 * ok, read a full change from disk, now restore it into proper
-		 * in-memory format
-		 */
-		ReorderBufferRestoreChange(rb, txn, rb->outbuf);
-		restored++;
 	}
 
 	return restored;
@@ -4193,25 +4117,36 @@ ReorderBufferRestoreChanges(ReorderBuffer *rb, ReorderBufferTXN *txn,
  * Convert change from its on-disk format to in-memory format and queue it onto
  * the TXN's ->changes list.
  *
- * Note: although "data" is declared char*, at entry it points to a
- * maxalign'd buffer, making it safe in most of this function to assume
- * that the pointed-to data is suitably aligned for direct access.
+ * If no data was found in the file, close it and set *file to NULL.
  */
 static void
 ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
-						   char *data)
+						   TransientBufFile **file)
 {
-	ReorderBufferDiskChange *ondisk;
+	ReorderBufferDiskChange ondisk;
+	bool		no_data;
 	ReorderBufferChange *change;
 
-	ondisk = (ReorderBufferDiskChange *) data;
+	/*
+	 * Read the statically sized part of a change which has information about
+	 * the total size. If we couldn't read a record, we're at the end of this
+	 * file.
+	 */
+	ReorderBufferReadData(*file, &ondisk, sizeof(ReorderBufferDiskChange),
+						  &no_data);
+
+	/* eof */
+	if (no_data)
+	{
+		BufFileCloseTransient(*file);
+		*file = NULL;
+		return;
+	}
 
 	change = ReorderBufferGetChange(rb);
 
 	/* copy static part */
-	memcpy(change, &ondisk->change, sizeof(ReorderBufferChange));
-
-	data += sizeof(ReorderBufferDiskChange);
+	memcpy(change, &ondisk.change, sizeof(ReorderBufferChange));
 
 	/* restore individual stuff */
 	switch (change->action)
@@ -4222,50 +4157,10 @@ ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
 		case REORDER_BUFFER_CHANGE_DELETE:
 		case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_INSERT:
 			if (change->data.tp.oldtuple)
-			{
-				uint32		tuplelen = ((HeapTuple) data)->t_len;
-
-				change->data.tp.oldtuple =
-					ReorderBufferGetTupleBuf(rb, tuplelen - SizeofHeapTupleHeader);
-
-				/* restore ->tuple */
-				memcpy(&change->data.tp.oldtuple->tuple, data,
-					   sizeof(HeapTupleData));
-				data += sizeof(HeapTupleData);
-
-				/* reset t_data pointer into the new tuplebuf */
-				change->data.tp.oldtuple->tuple.t_data =
-					ReorderBufferTupleBufData(change->data.tp.oldtuple);
-
-				/* restore tuple data itself */
-				memcpy(change->data.tp.oldtuple->tuple.t_data, data, tuplelen);
-				data += tuplelen;
-			}
+				change->data.tp.oldtuple = ReorderBufferRestoreTuple(rb, *file);
 
 			if (change->data.tp.newtuple)
-			{
-				/* here, data might not be suitably aligned! */
-				uint32		tuplelen;
-
-				memcpy(&tuplelen, data + offsetof(HeapTupleData, t_len),
-					   sizeof(uint32));
-
-				change->data.tp.newtuple =
-					ReorderBufferGetTupleBuf(rb, tuplelen - SizeofHeapTupleHeader);
-
-				/* restore ->tuple */
-				memcpy(&change->data.tp.newtuple->tuple, data,
-					   sizeof(HeapTupleData));
-				data += sizeof(HeapTupleData);
-
-				/* reset t_data pointer into the new tuplebuf */
-				change->data.tp.newtuple->tuple.t_data =
-					ReorderBufferTupleBufData(change->data.tp.newtuple);
-
-				/* restore tuple data itself */
-				memcpy(change->data.tp.newtuple->tuple.t_data, data, tuplelen);
-				data += tuplelen;
-			}
+				change->data.tp.newtuple = ReorderBufferRestoreTuple(rb, *file);
 
 			break;
 		case REORDER_BUFFER_CHANGE_MESSAGE:
@@ -4273,22 +4168,20 @@ ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
 				Size		prefix_size;
 
 				/* read prefix */
-				memcpy(&prefix_size, data, sizeof(Size));
-				data += sizeof(Size);
+				ReorderBufferReadData(*file, &prefix_size, sizeof(Size), NULL);
 				change->data.msg.prefix = MemoryContextAlloc(rb->context,
 															 prefix_size);
-				memcpy(change->data.msg.prefix, data, prefix_size);
+				ReorderBufferReadData(*file, change->data.msg.prefix,
+									  prefix_size, NULL);
 				Assert(change->data.msg.prefix[prefix_size - 1] == '\0');
-				data += prefix_size;
 
 				/* read the message */
-				memcpy(&change->data.msg.message_size, data, sizeof(Size));
-				data += sizeof(Size);
+				ReorderBufferReadData(*file, &change->data.msg.message_size,
+									  sizeof(Size), NULL);
 				change->data.msg.message = MemoryContextAlloc(rb->context,
 															  change->data.msg.message_size);
-				memcpy(change->data.msg.message, data,
-					   change->data.msg.message_size);
-				data += change->data.msg.message_size;
+				ReorderBufferReadData(*file, change->data.msg.message,
+									  change->data.msg.message_size, NULL);
 
 				break;
 			}
@@ -4301,29 +4194,32 @@ ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
 					MemoryContextAlloc(rb->context, inval_size);
 
 				/* read the message */
-				memcpy(change->data.inval.invalidations, data, inval_size);
+				ReorderBufferReadData(*file, change->data.inval.invalidations,
+									  inval_size, NULL);
 
 				break;
 			}
 		case REORDER_BUFFER_CHANGE_INTERNAL_SNAPSHOT:
 			{
-				Snapshot	oldsnap;
+				SnapshotData oldsnap;
 				Snapshot	newsnap;
 				Size		size;
 
-				oldsnap = (Snapshot) data;
+				ReorderBufferReadData(*file, &oldsnap, sizeof(SnapshotData), NULL);
 
 				size = sizeof(SnapshotData) +
-					sizeof(TransactionId) * oldsnap->xcnt +
-					sizeof(TransactionId) * (oldsnap->subxcnt + 0);
+					sizeof(TransactionId) * oldsnap.xcnt +
+					sizeof(TransactionId) * (oldsnap.subxcnt + 0);
 
 				change->data.snapshot = MemoryContextAllocZero(rb->context, size);
 
 				newsnap = change->data.snapshot;
 
-				memcpy(newsnap, data, size);
+				memcpy(newsnap, &oldsnap, sizeof(SnapshotData));
 				newsnap->xip = (TransactionId *)
 					(((char *) newsnap) + sizeof(SnapshotData));
+				ReorderBufferReadData(*file, newsnap->xip,
+									  size - sizeof(SnapshotData), NULL);
 				newsnap->subxip = newsnap->xip + newsnap->xcnt;
 				newsnap->copied = true;
 				break;
@@ -4335,7 +4231,9 @@ ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
 
 				relids = ReorderBufferGetRelids(rb,
 												change->data.truncate.nrelids);
-				memcpy(relids, data, change->data.truncate.nrelids * sizeof(Oid));
+				ReorderBufferReadData(*file, relids,
+									  change->data.truncate.nrelids * sizeof(Oid),
+									  NULL);
 				change->data.truncate.relids = relids;
 
 				break;
@@ -4362,6 +4260,77 @@ ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
 									ReorderBufferChangeSize(change));
 }
 
+/*
+ * Convert heap tuple from its on-disk format to in-memory format.
+ */
+static ReorderBufferTupleBuf *
+ReorderBufferRestoreTuple(ReorderBuffer *rb, TransientBufFile *file)
+{
+	HeapTupleData tupdata;
+	uint32		tuplelen;
+	ReorderBufferTupleBuf *result;
+
+	ReorderBufferReadData(file, &tupdata, sizeof(HeapTupleData), NULL);
+	tuplelen = tupdata.t_len;
+
+	result = ReorderBufferGetTupleBuf(rb, tuplelen - SizeofHeapTupleHeader);
+
+	/* restore ->tuple */
+	memcpy(&result->tuple, &tupdata, sizeof(HeapTupleData));
+
+	/* reset t_data pointer into the new tuplebuf */
+	result->tuple.t_data = ReorderBufferTupleBufData(result);
+
+	/* restore tuple data itself */
+	ReorderBufferReadData(file, result->tuple.t_data, tuplelen, NULL);
+
+	return result;
+}
+
+/*
+ * Wrapper for BufFileReadTransient() that raises ERROR if the expected amount
+ * of bytes was not read.
+ *
+ * If valid pointer is passed for no_data_p, set *no_data_p to indicate
+ * whether zero bytes was read. If NULL is passed, do not tolerate missing
+ * data.
+ */
+static void
+ReorderBufferReadData(TransientBufFile *file, void *ptr, size_t size,
+					  bool *no_data_p)
+{
+	int			readBytes;
+
+	/*
+	 * Caller should not request zero bytes. This assumption simplifies
+	 * setting of *no_data_p below.
+	 */
+	Assert(size > 0);
+
+	if ((readBytes = BufFileReadTransient(file, ptr, size)) != size)
+	{
+		if (no_data_p)
+			*no_data_p = readBytes == 0;
+
+		/*
+		 * It is o.k. to receive exactly zero bytes if caller passed valid
+		 * no_data_p.
+		 */
+		if (no_data_p && *no_data_p)
+			return;
+
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not read from reorderbuffer spill file: read %d instead of %u bytes",
+						readBytes, (uint32) size)));
+	}
+	else if (no_data_p)
+	{
+		/* Given that size is non-zero, readBytes must be non-zero too. */
+		*no_data_p = false;
+	}
+}
+
 /*
  * Remove all on-disk stored for the passed in transaction.
  */
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index a2f75b23b8..af5b329f8a 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -11,6 +11,7 @@
  *			- Add a pgstat config column to pg_database, so this
  *			  entire thing can be enabled/disabled on a per db basis.
  *
+ *	Portions Copyright (c) 2019-2021, CYBERTEC PostgreSQL International GmbH
  *	Copyright (c) 2001-2021, PostgreSQL Global Development Group
  *
  *	src/backend/postmaster/pgstat.c
@@ -54,6 +55,7 @@
 #include "replication/slot.h"
 #include "replication/walsender.h"
 #include "storage/backendid.h"
+#include "storage/buffile.h"
 #include "storage/dsm.h"
 #include "storage/fd.h"
 #include "storage/ipc.h"
@@ -308,10 +310,16 @@ NON_EXEC_STATIC void PgstatCollectorMain(int argc, char *argv[]) pg_attribute_no
 static PgStat_StatDBEntry *pgstat_get_db_entry(Oid databaseid, bool create);
 static PgStat_StatTabEntry *pgstat_get_tab_entry(PgStat_StatDBEntry *dbentry,
 												 Oid tableoid, bool create);
+static void pgstat_tweak_base(bool permanent, bool global, Oid database,
+							  char tweak_base[TWEAK_BASE_SIZE]);
 static void pgstat_write_statsfiles(bool permanent, bool allDbs);
 static void pgstat_write_db_statsfile(PgStat_StatDBEntry *dbentry, bool permanent);
+static void pgstat_write_bytes(TransientBufFile *file, void *ptr, size_t size,
+							   bool *failed);
 static HTAB *pgstat_read_statsfiles(Oid onlydb, bool permanent, bool deep);
 static void pgstat_read_db_statsfile(Oid databaseid, HTAB *tabhash, HTAB *funchash, bool permanent);
+static void pgstat_read_bytes(TransientBufFile *file, void *ptr, size_t size,
+							  bool *failed);
 static void backend_read_statsfile(void);
 
 static bool pgstat_write_statsfile_needed(void);
@@ -3202,6 +3210,9 @@ PgstatCollectorMain(int argc, char *argv[])
 	MyBackendType = B_STATS_COLLECTOR;
 	init_ps_display(NULL);
 
+	/* BufFileOpenTransient() and friends do use VFD. */
+	InitFileAccess();
+
 	/*
 	 * Read in existing stats files or initialize the stats to zero.
 	 */
@@ -3595,6 +3606,26 @@ pgstat_get_tab_entry(PgStat_StatDBEntry *dbentry, Oid tableoid, bool create)
 	return result;
 }
 
+/*
+ * Initialize the common part of the encryption tweak.
+ */
+static void
+pgstat_tweak_base(bool permanent, bool global, Oid database,
+				  char tweak_base[TWEAK_BASE_SIZE])
+{
+	char	*c = tweak_base;
+
+	StaticAssertStmt(3 + sizeof(Oid) <= TWEAK_BASE_SIZE,
+					 "tweak components do not fit into TWEAK_BASE_SIZE");
+	memset(tweak_base, 0, TWEAK_BASE_SIZE);
+	*c = TRANS_BUF_FILE_PGSTATS;
+	c++;
+	*c = permanent;
+	c++;
+	*c = global;
+	c++;
+	memcpy(c, &database, sizeof(Oid));
+}
 
 /* ----------
  * pgstat_write_statsfiles() -
@@ -3615,18 +3646,24 @@ pgstat_write_statsfiles(bool permanent, bool allDbs)
 {
 	HASH_SEQ_STATUS hstat;
 	PgStat_StatDBEntry *dbentry;
-	FILE	   *fpout;
+	TransientBufFile	   *fpout;
+	File	vfd;
 	int32		format_id;
 	const char *tmpfile = permanent ? PGSTAT_STAT_PERMANENT_TMPFILE : pgstat_stat_tmpname;
 	const char *statfile = permanent ? PGSTAT_STAT_PERMANENT_FILENAME : pgstat_stat_filename;
-	int			rc;
+	bool	failed = false;
+	char tweak_base[TWEAK_BASE_SIZE];
 
 	elog(DEBUG2, "writing stats file \"%s\"", statfile);
 
 	/*
 	 * Open the statistics temp file to write out the current values.
 	 */
-	fpout = AllocateFile(tmpfile, PG_BINARY_W);
+	if (data_encrypted)
+		pgstat_tweak_base(permanent, true, InvalidOid, tweak_base);
+	fpout = BufFileOpenTransient(tmpfile,
+								 O_CREAT | O_WRONLY | O_APPEND | PG_BINARY,
+								 tweak_base);
 	if (fpout == NULL)
 	{
 		ereport(LOG,
@@ -3645,32 +3682,27 @@ pgstat_write_statsfiles(bool permanent, bool allDbs)
 	 * Write the file header --- currently just a format ID.
 	 */
 	format_id = PGSTAT_FILE_FORMAT_ID;
-	rc = fwrite(&format_id, sizeof(format_id), 1, fpout);
-	(void) rc;					/* we'll check for error with ferror */
+	pgstat_write_bytes(fpout, &format_id, sizeof(format_id), &failed);
 
 	/*
 	 * Write global stats struct
 	 */
-	rc = fwrite(&globalStats, sizeof(globalStats), 1, fpout);
-	(void) rc;					/* we'll check for error with ferror */
+	pgstat_write_bytes(fpout, &globalStats, sizeof(globalStats), &failed);
 
 	/*
 	 * Write archiver stats struct
 	 */
-	rc = fwrite(&archiverStats, sizeof(archiverStats), 1, fpout);
-	(void) rc;					/* we'll check for error with ferror */
+	pgstat_write_bytes(fpout, &archiverStats, sizeof(archiverStats), &failed);
 
 	/*
 	 * Write WAL stats struct
 	 */
-	rc = fwrite(&walStats, sizeof(walStats), 1, fpout);
-	(void) rc;					/* we'll check for error with ferror */
+	pgstat_write_bytes(fpout, &walStats, sizeof(walStats), &failed);
 
 	/*
 	 * Write SLRU stats struct
 	 */
-	rc = fwrite(slruStats, sizeof(slruStats), 1, fpout);
-	(void) rc;					/* we'll check for error with ferror */
+	pgstat_write_bytes(fpout, slruStats, sizeof(slruStats), &failed);
 
 	/*
 	 * Walk through the database table.
@@ -3694,9 +3726,11 @@ pgstat_write_statsfiles(bool permanent, bool allDbs)
 		 * Write out the DB entry. We don't write the tables or functions
 		 * pointers, since they're of no use to any other process.
 		 */
-		fputc('D', fpout);
-		rc = fwrite(dbentry, offsetof(PgStat_StatDBEntry, tables), 1, fpout);
-		(void) rc;				/* we'll check for error with ferror */
+		pgstat_write_bytes(fpout, "D", 1, &failed);
+		pgstat_write_bytes(fpout,
+						   dbentry,
+						   offsetof(PgStat_StatDBEntry, tables),
+						   &failed);
 	}
 
 	/*
@@ -3709,29 +3743,36 @@ pgstat_write_statsfiles(bool permanent, bool allDbs)
 		hash_seq_init(&hstat, replSlotStatHash);
 		while ((slotent = (PgStat_StatReplSlotEntry *) hash_seq_search(&hstat)) != NULL)
 		{
-			fputc('R', fpout);
-			rc = fwrite(slotent, sizeof(PgStat_StatReplSlotEntry), 1, fpout);
-			(void) rc;			/* we'll check for error with ferror */
+			pgstat_write_bytes(fpout, "R", 1, &failed);
+			pgstat_write_bytes(fpout, slotent, sizeof(PgStat_StatReplSlotEntry),
+							   &failed);
 		}
 	}
 
 	/*
 	 * No more output to be done. Close the temp file and replace the old
-	 * pgstat.stat with it.  The ferror() check replaces testing for error
-	 * after each individual fputc or fwrite above.
+	 * pgstat.stat with it.
 	 */
-	fputc('E', fpout);
+	pgstat_write_bytes(fpout, "E", 1, &failed);
 
-	if (ferror(fpout))
+	if (failed)
 	{
 		ereport(LOG,
 				(errcode_for_file_access(),
 				 errmsg("could not write temporary statistics file \"%s\": %m",
 						tmpfile)));
-		FreeFile(fpout);
+		BufFileCloseTransient(fpout);
 		unlink(tmpfile);
+		return;
 	}
-	else if (FreeFile(fpout) < 0)
+
+	/*
+	 * XXX This might PANIC, see FileClose(). Don't we need special behaviour
+	 * for statistics?
+	 */
+	vfd = BufFileTransientGetVfd(fpout);
+	BufFileCloseTransient(fpout);
+	if (!FileIsClosed(vfd))
 	{
 		ereport(LOG,
 				(errcode_for_file_access(),
@@ -3796,12 +3837,14 @@ pgstat_write_db_statsfile(PgStat_StatDBEntry *dbentry, bool permanent)
 	HASH_SEQ_STATUS fstat;
 	PgStat_StatTabEntry *tabentry;
 	PgStat_StatFuncEntry *funcentry;
-	FILE	   *fpout;
+	TransientBufFile	   *fpout;
+	File	vfd;
 	int32		format_id;
 	Oid			dbid = dbentry->databaseid;
-	int			rc;
 	char		tmpfile[MAXPGPATH];
 	char		statfile[MAXPGPATH];
+	bool	failed = false;
+	char tweak_base[TWEAK_BASE_SIZE];
 
 	get_dbstat_filename(permanent, true, dbid, tmpfile, MAXPGPATH);
 	get_dbstat_filename(permanent, false, dbid, statfile, MAXPGPATH);
@@ -3811,7 +3854,11 @@ pgstat_write_db_statsfile(PgStat_StatDBEntry *dbentry, bool permanent)
 	/*
 	 * Open the statistics temp file to write out the current values.
 	 */
-	fpout = AllocateFile(tmpfile, PG_BINARY_W);
+	if (data_encrypted)
+		pgstat_tweak_base(permanent, false, dbid, tweak_base);
+	fpout = BufFileOpenTransient(tmpfile,
+								 O_CREAT | O_WRONLY | O_APPEND | PG_BINARY,
+								 tweak_base);
 	if (fpout == NULL)
 	{
 		ereport(LOG,
@@ -3825,8 +3872,7 @@ pgstat_write_db_statsfile(PgStat_StatDBEntry *dbentry, bool permanent)
 	 * Write the file header --- currently just a format ID.
 	 */
 	format_id = PGSTAT_FILE_FORMAT_ID;
-	rc = fwrite(&format_id, sizeof(format_id), 1, fpout);
-	(void) rc;					/* we'll check for error with ferror */
+	pgstat_write_bytes(fpout, &format_id, sizeof(format_id), &failed);
 
 	/*
 	 * Walk through the database's access stats per table.
@@ -3834,9 +3880,14 @@ pgstat_write_db_statsfile(PgStat_StatDBEntry *dbentry, bool permanent)
 	hash_seq_init(&tstat, dbentry->tables);
 	while ((tabentry = (PgStat_StatTabEntry *) hash_seq_search(&tstat)) != NULL)
 	{
-		fputc('T', fpout);
-		rc = fwrite(tabentry, sizeof(PgStat_StatTabEntry), 1, fpout);
-		(void) rc;				/* we'll check for error with ferror */
+		pgstat_write_bytes(fpout, "T", 1, &failed);
+		if (failed)
+			break;
+
+		pgstat_write_bytes(fpout, tabentry, sizeof(PgStat_StatTabEntry),
+						   &failed);
+		if (failed)
+			break;
 	}
 
 	/*
@@ -3845,28 +3896,42 @@ pgstat_write_db_statsfile(PgStat_StatDBEntry *dbentry, bool permanent)
 	hash_seq_init(&fstat, dbentry->functions);
 	while ((funcentry = (PgStat_StatFuncEntry *) hash_seq_search(&fstat)) != NULL)
 	{
-		fputc('F', fpout);
-		rc = fwrite(funcentry, sizeof(PgStat_StatFuncEntry), 1, fpout);
-		(void) rc;				/* we'll check for error with ferror */
+		pgstat_write_bytes(fpout, "F", 1, &failed);
+		if (failed)
+			break;
+
+		pgstat_write_bytes(fpout,
+						   funcentry,
+						   sizeof(PgStat_StatFuncEntry),
+						   &failed);
+		if (failed)
+			break;
 	}
 
 	/*
 	 * No more output to be done. Close the temp file and replace the old
-	 * pgstat.stat with it.  The ferror() check replaces testing for error
-	 * after each individual fputc or fwrite above.
+	 * pgstat.stat with it.
 	 */
-	fputc('E', fpout);
+	pgstat_write_bytes(fpout, "E", 1, &failed);
 
-	if (ferror(fpout))
+	if (failed)
 	{
 		ereport(LOG,
 				(errcode_for_file_access(),
 				 errmsg("could not write temporary statistics file \"%s\": %m",
 						tmpfile)));
-		FreeFile(fpout);
+		BufFileCloseTransient(fpout);
 		unlink(tmpfile);
+		return;
 	}
-	else if (FreeFile(fpout) < 0)
+
+	/*
+	 * XXX This might PANIC, see FileClose(). Don't we need special behaviour
+	 * for statistics?
+	 */
+	vfd = BufFileTransientGetVfd(fpout);
+	BufFileCloseTransient(fpout);
+	if (!FileIsClosed(vfd))
 	{
 		ereport(LOG,
 				(errcode_for_file_access(),
@@ -3892,6 +3957,25 @@ pgstat_write_db_statsfile(PgStat_StatDBEntry *dbentry, bool permanent)
 	}
 }
 
+/*
+ * Convenience routine to write data to file and check for errors.
+ */
+static void
+pgstat_write_bytes(TransientBufFile *file, void *ptr, size_t size,
+	bool *failed)
+{
+	/* Do nothing if any previous write failed. */
+	if (*failed)
+		return;
+
+	/*
+	 * Use BufFileWriteTransient() because it handles encryption
+	 * transparently.
+	 */
+	if (BufFileWriteTransient(file, ptr, size) != size)
+		*failed = true;
+}
+
 /* ----------
  * pgstat_read_statsfiles() -
  *
@@ -3919,10 +4003,12 @@ pgstat_read_statsfiles(Oid onlydb, bool permanent, bool deep)
 	PgStat_StatDBEntry dbbuf;
 	HASHCTL		hash_ctl;
 	HTAB	   *dbhash;
-	FILE	   *fpin;
+	TransientBufFile *fpin;
 	int32		format_id;
 	bool		found;
 	const char *statfile = permanent ? PGSTAT_STAT_PERMANENT_FILENAME : pgstat_stat_filename;
+	bool	failed = false;
+	char tweak_base[TWEAK_BASE_SIZE];
 	int			i;
 
 	/*
@@ -3971,7 +4057,11 @@ pgstat_read_statsfiles(Oid onlydb, bool permanent, bool deep)
 	 * not yet written the stats file the first time.  Any other failure
 	 * condition is suspicious.
 	 */
-	if ((fpin = AllocateFile(statfile, PG_BINARY_R)) == NULL)
+	if (data_encrypted)
+		pgstat_tweak_base(permanent, true, InvalidOid, tweak_base);
+	if ((fpin = BufFileOpenTransient(statfile,
+									 O_RDONLY | PG_BINARY,
+									 tweak_base)) == NULL)
 	{
 		if (errno != ENOENT)
 			ereport(pgStatRunningInCollector ? LOG : WARNING,
@@ -3984,8 +4074,8 @@ pgstat_read_statsfiles(Oid onlydb, bool permanent, bool deep)
 	/*
 	 * Verify it's of the expected format.
 	 */
-	if (fread(&format_id, 1, sizeof(format_id), fpin) != sizeof(format_id) ||
-		format_id != PGSTAT_FILE_FORMAT_ID)
+	pgstat_read_bytes(fpin, &format_id, sizeof(format_id), &failed);
+	if (failed || format_id != PGSTAT_FILE_FORMAT_ID)
 	{
 		ereport(pgStatRunningInCollector ? LOG : WARNING,
 				(errmsg("corrupted statistics file \"%s\"", statfile)));
@@ -3995,7 +4085,8 @@ pgstat_read_statsfiles(Oid onlydb, bool permanent, bool deep)
 	/*
 	 * Read global stats struct
 	 */
-	if (fread(&globalStats, 1, sizeof(globalStats), fpin) != sizeof(globalStats))
+	pgstat_read_bytes(fpin, &globalStats, sizeof(globalStats), &failed);
+	if (failed)
 	{
 		ereport(pgStatRunningInCollector ? LOG : WARNING,
 				(errmsg("corrupted statistics file \"%s\"", statfile)));
@@ -4016,7 +4107,8 @@ pgstat_read_statsfiles(Oid onlydb, bool permanent, bool deep)
 	/*
 	 * Read archiver stats struct
 	 */
-	if (fread(&archiverStats, 1, sizeof(archiverStats), fpin) != sizeof(archiverStats))
+	pgstat_read_bytes(fpin, &archiverStats, sizeof(archiverStats), &failed);
+	if (failed)
 	{
 		ereport(pgStatRunningInCollector ? LOG : WARNING,
 				(errmsg("corrupted statistics file \"%s\"", statfile)));
@@ -4027,7 +4119,8 @@ pgstat_read_statsfiles(Oid onlydb, bool permanent, bool deep)
 	/*
 	 * Read WAL stats struct
 	 */
-	if (fread(&walStats, 1, sizeof(walStats), fpin) != sizeof(walStats))
+	pgstat_read_bytes(fpin, &walStats, sizeof(walStats), &failed);
+	if (failed)
 	{
 		ereport(pgStatRunningInCollector ? LOG : WARNING,
 				(errmsg("corrupted statistics file \"%s\"", statfile)));
@@ -4038,7 +4131,8 @@ pgstat_read_statsfiles(Oid onlydb, bool permanent, bool deep)
 	/*
 	 * Read SLRU stats struct
 	 */
-	if (fread(slruStats, 1, sizeof(slruStats), fpin) != sizeof(slruStats))
+	pgstat_read_bytes(fpin, &slruStats, sizeof(slruStats), &failed);
+	if (failed)
 	{
 		ereport(pgStatRunningInCollector ? LOG : WARNING,
 				(errmsg("corrupted statistics file \"%s\"", statfile)));
@@ -4052,15 +4146,22 @@ pgstat_read_statsfiles(Oid onlydb, bool permanent, bool deep)
 	 */
 	for (;;)
 	{
-		switch (fgetc(fpin))
+		char	c;
+
+		pgstat_read_bytes(fpin, &c, 1, &failed);
+
+		switch (c)
 		{
 				/*
 				 * 'D'	A PgStat_StatDBEntry struct describing a database
 				 * follows.
 				 */
 			case 'D':
-				if (fread(&dbbuf, 1, offsetof(PgStat_StatDBEntry, tables),
-						  fpin) != offsetof(PgStat_StatDBEntry, tables))
+				pgstat_read_bytes(fpin,
+								  &dbbuf,
+								  offsetof(PgStat_StatDBEntry, tables),
+								  &failed);
+				if (failed)
 				{
 					ereport(pgStatRunningInCollector ? LOG : WARNING,
 							(errmsg("corrupted statistics file \"%s\"",
@@ -4144,8 +4245,10 @@ pgstat_read_statsfiles(Oid onlydb, bool permanent, bool deep)
 					PgStat_StatReplSlotEntry slotbuf;
 					PgStat_StatReplSlotEntry *slotent;
 
-					if (fread(&slotbuf, 1, sizeof(PgStat_StatReplSlotEntry), fpin)
-						!= sizeof(PgStat_StatReplSlotEntry))
+					pgstat_read_bytes(fpin, &slotbuf,
+									  sizeof(PgStat_StatReplSlotEntry),
+									  &failed);
+					if (failed)
 					{
 						ereport(pgStatRunningInCollector ? LOG : WARNING,
 								(errmsg("corrupted statistics file \"%s\"",
@@ -4186,7 +4289,7 @@ pgstat_read_statsfiles(Oid onlydb, bool permanent, bool deep)
 	}
 
 done:
-	FreeFile(fpin);
+	BufFileCloseTransient(fpin);
 
 	/* If requested to read the permanent file, also get rid of it. */
 	if (permanent)
@@ -4221,10 +4324,12 @@ pgstat_read_db_statsfile(Oid databaseid, HTAB *tabhash, HTAB *funchash,
 	PgStat_StatTabEntry tabbuf;
 	PgStat_StatFuncEntry funcbuf;
 	PgStat_StatFuncEntry *funcentry;
-	FILE	   *fpin;
+	TransientBufFile	   *fpin;
 	int32		format_id;
 	bool		found;
 	char		statfile[MAXPGPATH];
+	bool	failed = false;
+	char tweak_base[TWEAK_BASE_SIZE];
 
 	get_dbstat_filename(permanent, false, databaseid, statfile, MAXPGPATH);
 
@@ -4237,7 +4342,11 @@ pgstat_read_db_statsfile(Oid databaseid, HTAB *tabhash, HTAB *funchash,
 	 * not yet written the stats file the first time.  Any other failure
 	 * condition is suspicious.
 	 */
-	if ((fpin = AllocateFile(statfile, PG_BINARY_R)) == NULL)
+	if (data_encrypted)
+		pgstat_tweak_base(permanent, false, databaseid, tweak_base);
+	if ((fpin = BufFileOpenTransient(statfile,
+									 O_RDONLY | PG_BINARY,
+									 tweak_base)) == NULL)
 	{
 		if (errno != ENOENT)
 			ereport(pgStatRunningInCollector ? LOG : WARNING,
@@ -4250,8 +4359,8 @@ pgstat_read_db_statsfile(Oid databaseid, HTAB *tabhash, HTAB *funchash,
 	/*
 	 * Verify it's of the expected format.
 	 */
-	if (fread(&format_id, 1, sizeof(format_id), fpin) != sizeof(format_id) ||
-		format_id != PGSTAT_FILE_FORMAT_ID)
+	pgstat_read_bytes(fpin, &format_id, sizeof(format_id), &failed);
+	if (failed || format_id != PGSTAT_FILE_FORMAT_ID)
 	{
 		ereport(pgStatRunningInCollector ? LOG : WARNING,
 				(errmsg("corrupted statistics file \"%s\"", statfile)));
@@ -4264,14 +4373,19 @@ pgstat_read_db_statsfile(Oid databaseid, HTAB *tabhash, HTAB *funchash,
 	 */
 	for (;;)
 	{
-		switch (fgetc(fpin))
+		char	c;
+
+		pgstat_read_bytes(fpin, &c, 1, &failed);
+
+		switch (c)
 		{
 				/*
 				 * 'T'	A PgStat_StatTabEntry follows.
 				 */
 			case 'T':
-				if (fread(&tabbuf, 1, sizeof(PgStat_StatTabEntry),
-						  fpin) != sizeof(PgStat_StatTabEntry))
+				pgstat_read_bytes(fpin, &tabbuf, sizeof(PgStat_StatTabEntry),
+								  &failed);
+				if (failed)
 				{
 					ereport(pgStatRunningInCollector ? LOG : WARNING,
 							(errmsg("corrupted statistics file \"%s\"",
@@ -4304,8 +4418,11 @@ pgstat_read_db_statsfile(Oid databaseid, HTAB *tabhash, HTAB *funchash,
 				 * 'F'	A PgStat_StatFuncEntry follows.
 				 */
 			case 'F':
-				if (fread(&funcbuf, 1, sizeof(PgStat_StatFuncEntry),
-						  fpin) != sizeof(PgStat_StatFuncEntry))
+				pgstat_read_bytes(fpin,
+								  &funcbuf,
+								  sizeof(PgStat_StatFuncEntry),
+								  &failed);
+				if (failed)
 				{
 					ereport(pgStatRunningInCollector ? LOG : WARNING,
 							(errmsg("corrupted statistics file \"%s\"",
@@ -4349,7 +4466,7 @@ pgstat_read_db_statsfile(Oid databaseid, HTAB *tabhash, HTAB *funchash,
 	}
 
 done:
-	FreeFile(fpin);
+	BufFileCloseTransient(fpin);
 
 	if (permanent)
 	{
@@ -4386,15 +4503,21 @@ pgstat_read_db_statsfile_timestamp(Oid databaseid, bool permanent,
 	PgStat_WalStats myWalStats;
 	PgStat_SLRUStats mySLRUStats[SLRU_NUM_ELEMENTS];
 	PgStat_StatReplSlotEntry myReplSlotStats;
-	FILE	   *fpin;
+	TransientBufFile	   *fpin;
 	int32		format_id;
 	const char *statfile = permanent ? PGSTAT_STAT_PERMANENT_FILENAME : pgstat_stat_filename;
+	bool	failed = false;
+	char tweak_base[TWEAK_BASE_SIZE];
 
 	/*
 	 * Try to open the stats file.  As above, anything but ENOENT is worthy of
 	 * complaining about.
 	 */
-	if ((fpin = AllocateFile(statfile, PG_BINARY_R)) == NULL)
+	if (data_encrypted)
+		pgstat_tweak_base(permanent, true, InvalidOid, tweak_base);
+	if ((fpin = BufFileOpenTransient(statfile,
+									 O_RDONLY | PG_BINARY,
+									 tweak_base)) == NULL)
 	{
 		if (errno != ENOENT)
 			ereport(pgStatRunningInCollector ? LOG : WARNING,
@@ -4407,58 +4530,62 @@ pgstat_read_db_statsfile_timestamp(Oid databaseid, bool permanent,
 	/*
 	 * Verify it's of the expected format.
 	 */
-	if (fread(&format_id, 1, sizeof(format_id), fpin) != sizeof(format_id) ||
-		format_id != PGSTAT_FILE_FORMAT_ID)
+	pgstat_read_bytes(fpin, &format_id, sizeof(format_id), &failed);
+	if (failed || format_id != PGSTAT_FILE_FORMAT_ID)
 	{
 		ereport(pgStatRunningInCollector ? LOG : WARNING,
 				(errmsg("corrupted statistics file \"%s\"", statfile)));
-		FreeFile(fpin);
+		BufFileCloseTransient(fpin);
 		return false;
 	}
 
 	/*
 	 * Read global stats struct
 	 */
-	if (fread(&myGlobalStats, 1, sizeof(myGlobalStats),
-			  fpin) != sizeof(myGlobalStats))
+	pgstat_read_bytes(fpin, &myGlobalStats, sizeof(myGlobalStats), &failed);
+	if (failed)
 	{
 		ereport(pgStatRunningInCollector ? LOG : WARNING,
 				(errmsg("corrupted statistics file \"%s\"", statfile)));
-		FreeFile(fpin);
+		BufFileCloseTransient(fpin);
 		return false;
 	}
 
 	/*
 	 * Read archiver stats struct
 	 */
-	if (fread(&myArchiverStats, 1, sizeof(myArchiverStats),
-			  fpin) != sizeof(myArchiverStats))
+	pgstat_read_bytes(fpin, &myArchiverStats, sizeof(myArchiverStats),
+					  &failed);
+	if (failed)
 	{
 		ereport(pgStatRunningInCollector ? LOG : WARNING,
 				(errmsg("corrupted statistics file \"%s\"", statfile)));
-		FreeFile(fpin);
+		BufFileCloseTransient(fpin);
 		return false;
 	}
 
 	/*
 	 * Read WAL stats struct
 	 */
-	if (fread(&myWalStats, 1, sizeof(myWalStats), fpin) != sizeof(myWalStats))
+	pgstat_read_bytes(fpin, &myWalStats, sizeof(myWalStats), &failed);
+	if (failed)
 	{
 		ereport(pgStatRunningInCollector ? LOG : WARNING,
 				(errmsg("corrupted statistics file \"%s\"", statfile)));
-		FreeFile(fpin);
+		BufFileCloseTransient(fpin);
 		return false;
 	}
 
 	/*
 	 * Read SLRU stats struct
 	 */
-	if (fread(mySLRUStats, 1, sizeof(mySLRUStats), fpin) != sizeof(mySLRUStats))
+	pgstat_read_bytes(fpin, mySLRUStats, sizeof(mySLRUStats),
+					  &failed);
+	if (failed)
 	{
 		ereport(pgStatRunningInCollector ? LOG : WARNING,
 				(errmsg("corrupted statistics file \"%s\"", statfile)));
-		FreeFile(fpin);
+		BufFileCloseTransient(fpin);
 		return false;
 	}
 
@@ -4471,20 +4598,27 @@ pgstat_read_db_statsfile_timestamp(Oid databaseid, bool permanent,
 	 */
 	for (;;)
 	{
-		switch (fgetc(fpin))
+		char	c;
+
+		pgstat_read_bytes(fpin, &c, 1, &failed);
+
+		switch (c)
 		{
 				/*
 				 * 'D'	A PgStat_StatDBEntry struct describing a database
 				 * follows.
 				 */
 			case 'D':
-				if (fread(&dbentry, 1, offsetof(PgStat_StatDBEntry, tables),
-						  fpin) != offsetof(PgStat_StatDBEntry, tables))
+				pgstat_read_bytes(fpin,
+								  &dbentry,
+								  offsetof(PgStat_StatDBEntry, tables),
+								  &failed);
+				if (failed)
 				{
 					ereport(pgStatRunningInCollector ? LOG : WARNING,
 							(errmsg("corrupted statistics file \"%s\"",
 									statfile)));
-					FreeFile(fpin);
+					BufFileCloseTransient(fpin);
 					return false;
 				}
 
@@ -4505,13 +4639,14 @@ pgstat_read_db_statsfile_timestamp(Oid databaseid, bool permanent,
 				 * replication slot follows.
 				 */
 			case 'R':
-				if (fread(&myReplSlotStats, 1, sizeof(PgStat_StatReplSlotEntry), fpin)
-					!= sizeof(PgStat_StatReplSlotEntry))
+				pgstat_read_bytes(fpin, &myReplSlotStats,
+								  sizeof(PgStat_StatReplSlotEntry), &failed);
+				if (failed)
 				{
 					ereport(pgStatRunningInCollector ? LOG : WARNING,
 							(errmsg("corrupted statistics file \"%s\"",
 									statfile)));
-					FreeFile(fpin);
+					BufFileCloseTransient(fpin);
 					return false;
 				}
 				break;
@@ -4524,17 +4659,35 @@ pgstat_read_db_statsfile_timestamp(Oid databaseid, bool permanent,
 					ereport(pgStatRunningInCollector ? LOG : WARNING,
 							(errmsg("corrupted statistics file \"%s\"",
 									statfile)));
-					FreeFile(fpin);
+					BufFileCloseTransient(fpin);
 					return false;
 				}
 		}
 	}
 
 done:
-	FreeFile(fpin);
+	BufFileCloseTransient(fpin);
 	return true;
 }
 
+/*
+ * Convenience routine to read data from file and check for errors.
+ */
+static void
+pgstat_read_bytes(TransientBufFile *file, void *ptr, size_t size,
+	bool *failed)
+{
+	/* Do nothing if any previous read failed. */
+	if (*failed)
+		return;
+
+	/*
+	 * Use BufFileReadTransient() because it handles encryption transparently.
+	 */
+	if (BufFileReadTransient(file, ptr, size) != size)
+		*failed = true;
+}
+
 /*
  * If not already done, read the statistics collector stats file into
  * some hash tables.  The results will be kept until pgstat_clear_snapshot()