Re: [PATCH 5/6] ovl: Validate verity xattr when resolving lowerdata

[Eric Biggers <ebiggers@kernel.org>]

On Thu, Apr 27, 2023 at 09:22:57AM +0200, Alexander Larsson wrote:
> On Wed, Apr 26, 2023 at 11:47 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Thu, Apr 20, 2023 at 09:44:04AM +0200, Alexander Larsson wrote:
> > > +     err = fsverity_get_digest(d_inode(datapath->dentry), actual_digest, &verity_algo);
> > > +     if (err < 0) {
> > > +             pr_warn_ratelimited("lower file '%pd' has no fs-verity digest\n", datapath->dentry);
> > > +             return -EIO;
> > > +     }
> > > +
> > > +     if (digest_len != hash_digest_size[verity_algo] ||
> > > +         memcmp(required_digest, actual_digest, digest_len) != 0) {
> > > +             pr_warn_ratelimited("lower file '%pd' has the wrong fs-verity digest\n",
> > > +                                 datapath->dentry);
> > > +             return -EIO;
> > > +     }
> > > +
> > > +     return 0;
> >
> > This is incorrect because the digest algorithm is not being compared.
> 
> This is actually an interesting question. How much are things weakened
> by comparing the digest size, but not comparing the digest type. Like,
> suppose the xattr has a sha256 digest (32 bytes), how likely is there
> to be another new supported verity algorithm of the same digest size
> where you can force it to produce matching digests?

It might actually be fairly likely, considering that whenever a system includes
a choice of cryptographic algorithm, it tends to grow to include many different
algorithms.  Some of the reasons for this include:

  - Algorithms can become outdated and broken, yet systems often have to
    continue to support such algorithms for backwards compatibility.

  - People sometimes insist on using "national pride" algorithms, e.g. due to
    government regulations.  For example, in China people can be required to use
    Chinese algorithms instead of the U.S. / NIST algorithms.  See e.g. the
    existing support for SM3, SM4, Streebog, and Aria in the kernel crypto API
    and various other kernel subsystems.

  - Non-cryptographic algorithms might be added for use cases that don't
    actually require cryptographic security, e.g. integrity-only.

I'd strongly discourage you from building something whose security critically
depends on every algorithm that may ever exist being cryptographically secure.

Also, two hash algorithms that are each secure individually are not necessarily
secure when used as alternatives sharing the same output space.  E.g. consider
algorithm1 = SHA-256(data) and algorithm2 = SHA-256(data with all bits flipped).

> 
> I ask because ideally we want to minimize the size of the xattrs,
> since they are stored for each file, and not having to specify the
> type for each saves space. Currently the only two supported algorithms
> (sha256 and sha512) are different sizes, so we essentially compare
> type by comparing the size.
> 
> I see three options here:
> 1) Only compare digest + size (like now)
> 2) Assume size 32 means sha256, and 64 means sha512 and validate that
> 3) Use more space in the xattr to store an algorithm type

Just store the algorithm alongside the digest.  It's just 1 extra byte.

- Eric