Re: [PATCH v4 8/8] fscrypt: Add HCTR2 support for filename encryption

Ard Biesheuvel
2022-04-13T06:29:21Z

On Wed, 13 Apr 2022 at 08:10, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Apr 12, 2022 at 05:28:16PM +0000, Nathan Huckleberry wrote:
> > HCTR2 is a tweakable, length-preserving encryption mode. It has the
> > same security guarantees as Adiantum, but is intended for use on CPUs
> > with dedicated crypto instructions. It fixes a known weakness with
> > filename encryption: when two filenames in the same directory share a
> > prefix of >= 16 bytes, with CTS-CBC their encrypted filenames share a
> > common substring, leaking information. HCTR2 does not have this
> > problem.
> >
> > More information on HCTR2 can be found here: Length-preserving
> > encryption with HCTR2: https://eprint.iacr.org/2021/1441.pdf
>
> Please quote titles to distinguish them from the surrounding text. E.g.
>
> More information on HCTR2 can be found in the paper "Length-preserving
> encryption with HCTR2" (https://eprint.iacr.org/2021/1441.pdf)
>
>
> >
> > Signed-off-by: Nathan Huckleberry <nhuck@google.com>
> > ---
> > Documentation/filesystems/fscrypt.rst | 19 ++++++++++++++-----
> > fs/crypto/fscrypt_private.h | 2 +-
> > fs/crypto/keysetup.c | 7 +++++++
> > fs/crypto/policy.c | 4 ++++
> > include/uapi/linux/fscrypt.h | 3 ++-
> > tools/include/uapi/linux/fscrypt.h | 3 ++-
> > 6 files changed, 30 insertions(+), 8 deletions(-)
>
> Can you make sure that all fscrypt patches are Cc'ed to the linux-fscrypt
> mailing list? In this case, just Cc the whole series to there.
>
> >
> > diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst
> > index 4d5d50dca65c..09915086abd8 100644
> > --- a/Documentation/filesystems/fscrypt.rst
> > +++ b/Documentation/filesystems/fscrypt.rst
> > @@ -337,6 +337,7 @@ Currently, the following pairs of encryption modes are supported:
> > - AES-256-XTS for contents and AES-256-CTS-CBC for filenames
> > - AES-128-CBC for contents and AES-128-CTS-CBC for filenames
> > - Adiantum for both contents and filenames
> > +- AES-256-XTS for contents and AES-256-HCTR2 for filenames
> >
> > If unsure, you should use the (AES-256-XTS, AES-256-CTS-CBC) pair.
> >
> > @@ -357,6 +358,14 @@ To use Adiantum, CONFIG_CRYPTO_ADIANTUM must be enabled. Also, fast
> > implementations of ChaCha and NHPoly1305 should be enabled, e.g.
> > CONFIG_CRYPTO_CHACHA20_NEON and CONFIG_CRYPTO_NHPOLY1305_NEON for ARM.
> >
> > +AES-256-HCTR2 is another true wide-block encryption mode. It has the same
> > +security guarantees as Adiantum, but is intended for use on CPUs with dedicated
> > +crypto instructions. See the paper "Length-preserving encryption with HCTR2"
> > +(https://eprint.iacr.org/2021/1441.pdf) for more details. To use HCTR2,
> > +CONFIG_CRYPTO_HCTR2 must be enabled. Also, fast implementations of XCTR and
> > +POLYVAL should be enabled, e.g. CRYPTO_POLYVAL_ARM64_CE and
> > +CRYPTO_AES_ARM64_CE_BLK for ARM64.
>
> "same security guarantees as Adiantum" is not really correct. Both Adiantum and
> HCTR2 are secure super-pseudorandom permutations if their underlying primitives
> are secure. So their security guarantees are pretty similar, but not literally
> the same. Can you reword this? This potentially-misleading claim also showed
> up in the commit message.
>
> > diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
> > index ed3d623724cd..fa8bdb8c76b7 100644
> > --- a/fs/crypto/policy.c
> > +++ b/fs/crypto/policy.c
> > @@ -54,6 +54,10 @@ static bool fscrypt_valid_enc_modes(u32 contents_mode, u32 filenames_mode)
> > filenames_mode == FSCRYPT_MODE_ADIANTUM)
> > return true;
> >
> > + if (contents_mode == FSCRYPT_MODE_AES_256_XTS &&
> > + filenames_mode == FSCRYPT_MODE_AES_256_HCTR2)
> > + return true;
> > +
> > return false;
> > }
>
> This is allowing HCTR2 for both v1 and v2 encryption policies. I don't think we
> should add any new features to v1 encryption policies, as they are deprecated.
> How about allowing HCTR2 for v2 encryption policies only? This is the first new
> encryption mode where this issue has come up, but this could be handled easily
> by splitting fscrypt_valid_enc_modes() into fscrypt_valid_enc_modes_v1() and
> fscrypt_valid_enc_modes_v2(). The v2 one can call the v1 one to share code.
>
> > diff --git a/tools/include/uapi/linux/fscrypt.h b/tools/include/uapi/linux/fscrypt.h
> > index 9f4428be3e36..a756b29afcc2 100644
> > --- a/tools/include/uapi/linux/fscrypt.h
> > +++ b/tools/include/uapi/linux/fscrypt.h
> > @@ -27,7 +27,8 @@
> > #define FSCRYPT_MODE_AES_128_CBC 5
> > #define FSCRYPT_MODE_AES_128_CTS 6
> > #define FSCRYPT_MODE_ADIANTUM 9
> > -/* If adding a mode number > 9, update FSCRYPT_MODE_MAX in fscrypt_private.h */
> > +#define FSCRYPT_MODE_AES_256_HCTR2 10
> > +/* If adding a mode number > 10, update FSCRYPT_MODE_MAX in fscrypt_private.h */
> >
>
> As far as I know, you don't actually need to update the copy of UAPI headers in
> tools/. The people who maintain those files handle that. It doesn't make sense
> to have copies of files in the source tree anyway.
>

Doesn't the x86 build emit a warning if these go out of sync?