We appreciate these excellent comments and questions.
Some we are still studying. Here are some initial followups:
On Tue, 2005-11-15 at 13:09 -0500, Stephen Smalley wrote:
> > +static int evm_getxattr(struct dentry *dentry, char *xattrName,
> > + char **xattr_value, int xattr_size)
> > +{
> > + char *value = *xattr_value;
> > + int size;
> > + int error;
> > +
> > + if (!dentry || !dentry->d_inode || !dentry->d_inode->i_op
> > + || !dentry->d_inode->i_op->getxattr)
> > + return xattr_size;
>
> You should only apply this kind of checking for those pointers that can
> be legitimately NULL here. Otherwise, you are just covering up more
> serious bugs. Same issue occurs throughout the code, so please fix it
> everywhere. Also, returning xattr_size here seems likely to confuse the
> caller, as it will then try to use the value buffer which hasn't been
> initialized.
agreed - will fix these.
>
> > +static int evm_verify_hmac(struct dentry *dentry, struct inode *inode)
> > +{
> <snip>
> > + rc = evm_calc_hmac(dentry, hmacVal);
> > + if (rc < 0)
> > + return rc;
> <snip>
> > + rc = evm_verify_xattr(dentry, hmacVal, "security.evm.hmac",
> > + trustedHMAC, SHA1_DIGEST_SIZE);
>
> The attributes over which the HMAC is being computed can change prior to
> or during the verification. And this is ok? I see that you attempt to
> provide access control over such changes partially via EVM's own
> inode_setxattr hook and partially via the other LSM's inode_setxattr
> hook, but if we are relying on the access control mechanism at runtime
> for such protection, then what precisely do we gain from performing such
> verification on inode_permission and file_permission (versus a one-time
> verification when the inode is first d_instantiated and then using the
> access control mechanism subsequently)?
We did not understand that d_instantiate is the preferred approach,
partially because it is not well documented in security.h. If this
is the better method, it would certainly seem possible to move to this
hook.
> > + /*Numerous reasons for no hmac, errors cached in istatus->xattr_flags.*/
> > + error = evm_verify_hmac(dentry, inode);
> > + if (error < 0)
> > + return 0;
>
> I see the comment, but I'm not clear on why you think it is safe to return 0 here.
yes this is still too confusing. We will rewrite it for clarity.
>
> > diff -rpuN linux-2.6.14.2.orig/security/evm/evm_config.c linux-2.6.14.2.tc/security/evm/evm_config.c
> > --- linux-2.6.14.2.orig/security/evm/evm_config.c 1969-12-31 19:00:00.000000000 -0500
> > +++ linux-2.6.14.2.tc/security/evm/evm_config.c 2005-11-11 13:43:18.000000000 -0500
> <snip>
> > +int evm_get_config_info(struct evm_xattr_config **head, int *size,
> > + time_t * version)
> > +{
> > + *head = evm_config_data;
> > + *size = evm_config_datasize;
> > + *version = evm_config_version;
> > + return 0;
> > +}
> > +
> > +/*
> > + * Initialize the Extended Verification module
> > + */
> > +int evm_update_config(struct evm_xattr_config *evm_new_data,
> > + int evm_new_datasize)
> > +{
> <snip>
> > + if (evm_new_datasize > 0) {
> > + struct timespec now;
> > +
> > + evm_old_data = evm_config_data;
> > + evm_old_datasize = evm_config_datasize;
> > +
> > + evm_config_data = evm_new_data;
> > + evm_config_datasize = evm_new_datasize;
> > + now = CURRENT_TIME;
> > + evm_config_version = now.tv_sec;
> > +
> > + evm_isec_size = sizeof(struct evm_isec_cache) +
> > + ((evm_config_datasize + 1)
> > + * sizeof(struct evm_isec_xattr));
>
> What serializes multiple updates to the config via securityfs to prevent
> clobbering one another, and what serializes the update with
> evm_get_config_info to prevent the config from being visible in an
> inconsistent state?
After conversion of the configuration code to securityfs,
we allow only one initial configuration (which we do in
the initrd, when there is only the one init process), and
then we remove the securityfs config file.
dave safford
This archive was generated by hypermail 2.1.3 : Thu Nov 17 2005 - 14:30:20 PST