On Tue, 2005-11-15 at 09:08 -0500, David Safford wrote:
> diff -rpuN linux-2.6.14.2.orig/security/evm/evm_cache.c linux-2.6.14.2.tc/security/evm/evm_cache.c
> --- linux-2.6.14.2.orig/security/evm/evm_cache.c 1969-12-31 19:00:00.000000000 -0500
> +++ linux-2.6.14.2.tc/security/evm/evm_cache.c 2005-11-11 13:48:30.000000000 -0500
<snip>
> +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.
> +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)?
> +int evm_inode_permission(struct inode *inode, int mask,
> + struct nameidata *nd)
> +{
> + struct evm_isec_cache *isec;
> + struct evm_isec_xattr *istatus;
> + struct dentry *dentry;
> + ssize_t error = 0;
> +
> + int xattr_size = 0;
> + char *xattr_value = NULL;
> + struct evm_xattr_config *config_data, *config_p;
> + time_t xattr_refresh = 0;
> +
> + if (!nd || !nd->dentry || !inode)
> + return 0;
> +
> + dentry = nd->dentry;
> + if ((error = evm_valid_security(dentry, inode)) == 0) /* Valid? */
> + return 0;
And what if error != 0 here?
> + isec = evm_get_isec(inode->i_security);
> + if (!isec) {
> + isec = evm_alloc_security();
> + if (!isec)
> + return (-ENOMEM);
> + evm_set_isec(inode->i_security, isec);
> + }
Race? On entry, isec is NULL, and you go to allocate the security
structure, and another thread hits inode_permission on the same inode,
and it allocates its own security structure, and someone loses at
evm_set_isec time. Note that evm_set_isec is defined to be
security_set_value_type(), and the stacker documentation notes that you
cannot use it safely outside of the object's alloc_security hook
function.
> + /*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.
> 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?
> diff -rpuN linux-2.6.14.2.orig/security/evm/evm_crypto.c linux-2.6.14.2.tc/security/evm/evm_crypto.c
> --- linux-2.6.14.2.orig/security/evm/evm_crypto.c 1969-12-31 19:00:00.000000000 -0500
> +++ linux-2.6.14.2.tc/security/evm/evm_crypto.c 2005-11-11 13:44:19.000000000 -0500
> + crypto_digest_init(tfm);
> + for (offset = 0; offset < file->f_dentry->d_inode->i_size;
What happens if i_size is changing while you are doing this processing?
> +/*
> + * Calculate the hmac, update it, and mark the cache dirty.
> + */
> +int evm_update_hmac(struct dentry *dentry, int flags)
> +{
> + char hmacVal[SHA1_DIGEST_SIZE];
> + int rc = -1;
> +
> + if (!dentry || !dentry->d_inode || !dentry->d_inode->i_op
> + || !dentry->d_inode->i_op->setxattr)
> + return rc;
> +
> + memset(hmacVal, 0, SHA1_DIGEST_SIZE);
> + if ((rc = evm_calc_hmac(dentry, hmacVal)) == 0) {
> + struct evm_isec_cache *isec;
> + struct inode *inode;
> +
> + rc = dentry->d_inode->i_op->setxattr(dentry,
> + "security.evm.hmac", hmacVal,
> + SHA1_DIGEST_SIZE, flags);
What serializes such updates to the HMAC attribute? Ditto for all of
the other attributes set by EVM. Both in terms of multiple updates by
EVM itself and in terms of userspace-initiated updates that may end up
running concurrent with an EVM-initiated update.
> diff -rpuN linux-2.6.14.2.orig/security/evm/evm_init.c linux-2.6.14.2.tc/security/evm/evm_init.c
> --- linux-2.6.14.2.orig/security/evm/evm_init.c 1969-12-31 19:00:00.000000000 -0500
> +++ linux-2.6.14.2.tc/security/evm/evm_init.c 2005-11-11 13:45:38.000000000 -0500
<snip>
> +/*
> + * Rigid enforcement of signed executables,
> + * when a sandboxing module is not present.
> + */
> +static int evm_bprm_check_security(struct linux_binprm *bprm)
Not very rigid. Look at digsig again and explain why you don't need
file_mmap/mprotect hooks. Or look at SELinux (not signed executables in
that case, but access control is applied there).
> diff -rpuN linux-2.6.14.2.orig/security/evm/evm_xattr.c linux-2.6.14.2.tc/security/evm/evm_xattr.c
> --- linux-2.6.14.2.orig/security/evm/evm_xattr.c 1969-12-31 19:00:00.000000000 -0500
> +++ linux-2.6.14.2.tc/security/evm/evm_xattr.c 2005-11-11 13:46:26.000000000 -0500
> +int evm_inode_setxattr(struct dentry *dentry, char *name, void *value,
> + size_t size, int flags)
> +{
> + int error = 0;
> + struct evm_xattr_config *config_p;
> +
> + struct timespec now;
> + time_t secVal;
> + const unsigned char *debug_name = "???";
> + struct evm_xattr_config *config_data;
> + int config_datasize = 0; /* number of extended attributes */
> + time_t config_version;
> +
> + if (!dentry || !name || !value)
> + return -EINVAL;
> +
> + now = CURRENT_TIME;
> + if (dentry && dentry->d_name.name)
> + debug_name = dentry->d_name.name;
> + evm_get_config_info(&config_data, &config_datasize,
> + &config_version);
> + for_each_xattr(config_p, config_data, config_datasize) {
> + if (memcmp(name, config_p->xattr_name, strlen(name)) == 0) {
> + secVal = ntohl(*((time_t *) value));
What verifies that size >= sizeof(time_t)? Alignment issue?
--
Stephen Smalley
National Security Agency
This archive was generated by hypermail 2.1.3 : Tue Nov 15 2005 - 10:03:07 PST