RE: [RFC PATCH v7 12/16] fsverity|security: add security hooks to fsverity digest and signature

[Roberto Sassu <roberto.sassu@huawei.com>]

> From: Deven Bowers [mailto:deven.desai@linux.microsoft.com]
> Sent: Friday, October 15, 2021 9:26 PM
> On 10/13/2021 12:24 PM, Eric Biggers wrote:
> > On Wed, Oct 13, 2021 at 12:06:31PM -0700,
> deven.desai@linux.microsoft.com wrote:
> >> From: Fan Wu <wufan@linux.microsoft.com>
> >>
> >> Add security_inode_setsecurity to fsverity signature verification.
> >> This can let LSMs save the signature data and digest hashes provided
> >> by fsverity.
> > Can you elaborate on why LSMs need this information?
> 
> The proposed LSM (IPE) of this series will be the only one to need
> this information at the  moment. IPE’s goal is to have provide
> trust-based access control. Trust and Integrity are tied together,
> as you cannot prove trust without proving integrity.

I wanted to go back on this question.

It seems, at least for fsverity, that you could obtain the
root digest at run-time, without storing it in a security blob.

I thought I should use fsverity_get_info() but the fsverity_info
structure is not exported (it is defined in fs/verity/fsverity_private.h).

Then, I defined a new function, fsverity_get_file_digest() to copy
the file_digest member of fsverity_info to a buffer and to pass
the associated hash algorithm.

With that, the code of evaluate() for DIGLIM becomes:

        info = fsverity_get_info(file_inode(ctx->file));
        if (info)
                ret = fsverity_get_file_digest(info, buffer, sizeof(buffer), &algo);

        if (!strcmp(expect->data, "diglim") && ret > 0) {
                ret = diglim_digest_get_info(buffer, algo, COMPACT_FILE, &modifiers, &actions);
                if (!ret)
                        return true;
        }

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua

> IPE needs the digest information to be able to compare a digest
> provided by the policy author, against the digest calculated by
> fsverity to make a decision on whether that specific file, represented
> by the digest is authorized for the actions specified in the policy.
> 
> A more concrete example, if an IPE policy author writes:
> 
>      op=EXECUTE fsverity_digest=<HexDigest > action=DENY
> 
> IPE takes the digest provided by this security hook, stores it
> in IPE's security blob on the inode. If this file is later
> executed, IPE compares the digest stored in the LSM blob,
> provided by this hook, against <HexDigest> in the policy, if
> it matches, it denies the access, performing a revocation
> of that file.
> 
> This brings me to your next comment:
> 
>  > The digest isn't meaningful without knowing the hash algorithm it uses.
> It's available here, but you aren't passing it to this function.
> 
> The digest is meaningful without the algorithm in this case.
> IPE does not want to recalculate a digest, that’s expensive and
> doesn’t provide any value. IPE, in this case, treats this as a
> buffer to compare the policy-provided one above to make a
> policy decision about access to the resource.
> 
> >> Also changes the implementaion inside the hook function to let
> >> multiple LSMs can add hooks.
> > Please split fs/verity/ changes and security/ changes into separate patches, if
> > possible.
> 
> Sorry, will do, not a problem.
> 
> >> @@ -177,6 +178,17 @@ struct fsverity_info *fsverity_create_info(const
> struct inode *inode,
> >>   		fsverity_err(inode, "Error %d computing file digest", err);
> >>   		goto out;
> >>   	}
> >> +
> >> +	err = security_inode_setsecurity((struct inode *)inode,
> > If a non-const inode is needed, please propagate that into the callers rather
> > than randomly casting away the const.
> >
> >> +					 FS_VERITY_DIGEST_SEC_NAME,
> >> +					 vi->file_digest,
> >> +					 vi->tree_params.hash_alg-
> >digest_size,
> >> +					 0);
> >> @@ -84,7 +85,9 @@ int fsverity_verify_signature(const struct fsverity_info
> *vi,
> >>
> >>   	pr_debug("Valid signature for file digest %s:%*phN\n",
> >>   		 hash_alg->name, hash_alg->digest_size, vi->file_digest);
> >> -	return 0;
> >> +	return security_inode_setsecurity((struct inode *)inode,
> >>
> > Likewise, please don't cast away const.
> 
> Sorry, I should've caught these myself. I'll change
> fsverity_create_info to accept the non-const inode, and
> change fsverity_verify_signature to accept an additional inode
> struct as the first arg instead of changing the fsverity_info
> structure to have a non-const inode field.
> 
> >> +					FS_VERITY_SIGNATURE_SEC_NAME,
> >> +					signature, sig_size, 0);
> > This is only for fs-verity built-in signatures which aren't the only way to do
> > signatures with fs-verity.  Are you sure this is what you're looking for?
> 
> Could you elaborate on the other signature types that can be used
> with fs-verity? I’m 99% sure this is what I’m looking for as this
> is a signature validated in the kernel against the fs-verity keyring
> as part of the “fsverity enable” utility.
> 
> It's important that the signature is validated in the kernel, as
> userspace is considered untrusted until the signature is validated
> for this case.
> 
> > Can you elaborate on your use case for fs-verity built-in signatures,
> Sure, signatures, like digests, also provide a way to prove integrity,
> and the trust component comes from the validation against the keyring,
> as opposed to a fixed value in IPE’s policy. The use case for fs-verity
> built-in signatures is that we have a rw ext4 filesystem that has some
> executable files, and we want to have a execution policy (through IPE)
> that only _trusted_ executables can run. Perf is important here, hence
> fs-verity.
> 
> > and what the LSM hook will do with them?
> 
> At the moment, this will just signal to IPE that these fs-verity files were
> enabled with a built-in signature as opposed to enabled without a signature.
> In v7, it copies the signature data into IPE's LSM blob attached to the
> inode.
> In v8+, I'm changing this to store “true” in IPE's LSM blob instead, as
> copying
> the signature data is an unnecessary waste of space and point of
> failure. This
> has a _slightly_ different functionality then fs.verity.require_signatures,
> because even if someone were to disable the require signatures option, IPE
> would still know if these files were signed or not and be able to make the
> access control decision based IPE's policy.
> 
> Very concretely, this powers this kind of rule in IPE:
> 
>    op=EXECUTE fsverity_signature=TRUE action=ALLOW
> 
> if that fsverity_signature value in IPE’s LSM blob attached to the inode is
> true, then fsverity_signature in IPE’s policy will evaluate to true and
> match
> this rule. The inverse is also applicable.