On Tue, 15 Nov 2005, David Safford wrote:
> integrity of a file˙˙s data and metadata, and allows or denies access
> based on the measured integrity. Integrity measurements for each file
> are stored in configured extended attributes. EVM has one built-in
> extended attribute, security.evm.hmac, which contains an HMAC-sha1
> across all configured extended attributes, and which is keyed by the
> kernel master key obtained from the TPM (Trusted Platform Module)
> device driver, based on a trusted boot.
You are building policy into the kernel. Please don't hardcode this
algorithm into the code. Users may want to use other algorithms for
many reasons such as specific certification requirements, local crypto
policy etc. Make this user-configurable.
> + if ((istatus->xattr_id == 0) /* unlabeled HMAC */
No magic numbers. Why use comment when you can make it self describing as
EVM_UNLABELED_HMAC or similar?
> +void evm_file_install_free(struct file *file)
This is almost identical to evm_file_install(). Refactor into a core
function with wrappers.
> +{
> + struct inode *inode = NULL;
> + char hashValue[SHA1_DIGEST_SIZE + 1];
> + char hashStr[SHA1_STR_SIZE + 11];
> + rc = evm_calc_hash(file, hashValue, EVM_TYPE_MD5);
> + len = mem2hex(hashValue, hashStr + sizeof(time_t),
> + MD5_DIGEST_SIZE);
Why are the buffers sized for SHA1 when you're using MD5?
Again, also, don't hardcode algorithims into the code. Parameterize it.
Make it user configurable.
> +static void evm_verify_md5(struct evm_isec_xattr *istatus,
> + struct nameidata *nd, char *xattr_value,
> + int xattr_size, char *hashValue)
Almost identical to evm_verify_sha1(). Refactor and parameterize.
> +static int evm_verify_hmac(struct dentry *dentry, struct inode *inode)
> +{
> + int rc = 0;
> + struct evm_isec_cache *isec;
> + struct evm_isec_xattr *istatus;
> + char hmacVal[SHA1_DIGEST_SIZE];
> + char trustedHMAC[SHA1_DIGEST_SIZE];
> + struct timespec now;
> + const unsigned char *debug_name = "???";
> +
> + if (!inode)
> + return -EINVAL;
This is a kernel bug, use BUG_ON() if you need it.
> + istatus->xattr_id = 0; /* HMAC */
No magic numbers. Use self-describing macros.
> + switch (isec->cache_flag) {
> + case EVM_CACHE_DIRTY:
Broken indenting.
> + isec =
> + (struct evm_isec_cache *) kmalloc(evm_isec_size, GFP_KERNEL);
> + if (isec) {
> + memset(isec, 0, evm_isec_size);
Use kzalloc() (and no need for return cast).
> + {"security.evm.antivirus", 300, 7776000,
What is this doing in the kernel?
> + if ((rbuf = (char *) kmalloc(RBUF_SIZE, GFP_KERNEL)) == NULL) {
Don't cast the results of kmalloc() to anything. Needs fixing in many
places. Avoid using side-effects in the kernel.
> + for (offset = 0; offset < file->f_dentry->d_inode->i_size;
> + offset += RBUF_SIZE) {
> + rbuf_len = kernel_read(file, offset, rbuf, RBUF_SIZE);
Increment by rbuf_len, bit RBUF_SIZE. What if the kernel performs a short
read?
> +int evm_calc_hash(struct file *file, char *digest, int xattr_type)
This and evm_calc_dhash() are almost identical. Refactor into a core
function with wrappers.
> + char *rbuf;
> + int rbuf_len;
> + int offset = 0;
> +
> + if (!file)
> + return -1;
> +
> + /* Load either MD5/SHA1, if not already loaded */
> + down(&evm_crypto_sem);
> + if (xattr_type == EVM_TYPE_MD5)
> + tfm = crypto_alloc_tfm("md5", 0);
> + else if (xattr_type == EVM_TYPE_SHA1)
> + tfm = crypto_alloc_tfm("sha1", 0);
> +
> + if (!tfm) {
> + dprintk(EVM_CRYPTO,
> + "%s: failed to load MD5/SHA1 transfrom\n",
> + __FUNCTION__);
> + up(&evm_crypto_sem);
> + return -1;
> + }
> +
> + if ((rbuf = (char *) kmalloc(RBUF_SIZE, GFP_KERNEL)) == NULL) {
> + up(&evm_crypto_sem);
> + return -ENOMEM;
> + }
Sometimes this returns -1 and other times, a specific -Evalue. Be
consistent and make it always return zero or an -Evalue.
> + * Calculate the SHA1 HMAC across all the extended attributes included
> + * in the HMAC policy as defined in /etc/evm.conf.
> + */
> +int evm_calc_hmac(struct dentry *dentry, char *digest)
> +{
> + ssize_t error;
> + struct crypto_tfm *tfm;
> + struct scatterlist sg[1];
> + const unsigned char *debug_name = "???";
> +
> + struct evm_xattr_config *config_p, *config_data;
> + int xattrSize = 0;
> + char *xattrValue = NULL;
> + int config_datasize = 0; /* number of extended attributes */
Give variables descriptive names instead of abstract ones with comments
explaining them. This happens in quite a few places.
> + if (size > xattrSize) {
> + if (xattrValue)
> + kfree(xattrValue);
Don't test for non-NULL before calling kfree(). Needs to be fixed in many
places.
> +int evm_init_crypto(void)
> +{
> + init_MUTEX(&evm_crypto_sem);
> +
> + return (tpm_get_key(tpmKey, &tpmKeylen));
> +}
Remove extra braces.
> +#define evm_get_isec(head) \
> + security_get_value_type(head, EVM_LSM_ID, struct evm_isec_cache,\
> + evm_idx);
> +#define evm_set_isec(head, isec) \
> + security_set_value_type(head, EVM_LSM_ID, isec, evm_idx);
> +#define evm_del_isec(head) \
> + security_del_value_type(head, EVM_LSM_ID, struct evm_isec_cache, \
> + evm_idx);
Use static inlines instead of macros where possible, as you get type
checking.
> + * Dummy getprocattr defined here, otherwise
> + * either security_getprocattr() or dummy_getprocattr() return -EINVAL,
> + * preventing other getprocattr definitions from being executed.
> + */
> +static inline int evm_getprocattr(struct task_struct *p,
> + char *name, void *value, size_t size)
> +{
> + return 0;
> +}
There must be a better way to do this.
> +void evm_init_mode(char *evm_mode, int *enforce, int *install)
> +{
> + if (memcmp(evm_mode, "INSTALL", 7) == 0) {
> + *install = 1;
> + *enforce = 0;
> + } else if (memcmp(evm_mode, "LOGONLY", 7) == 0)
> + *enforce = 0;
> +}
Use strcmp().
> +static int __init init_evm(void)
> +{
> + int error;
> +
> + if ((error = evm_init_crypto()) < 0) {
Please avoid using side-effects in the kernel.
Standard form is:
error = foo();
if (error < 0)
something();
> +static void evm_disable_config(void)
> +{
> + securityfs_remove(evm_config);
> +}
What is the purpose of this?
And what if something is referencing it?
> +static ssize_t evm_write_secfs(struct file *file, const char __user * buf,
> + size_t buflen, loff_t * ppos)
> +{
> + size_t rc = 0, datalen;
> + char *data;
> +
> + struct evm_xattr_config *evm_new_data = NULL;
> + int evm_new_datasize = 0;
> +
> + datalen = buflen >= 4095 ? 4095 : buflen;
No magic numbers, and why these values?
> + evm_new_data = evm_parse_config(data, datalen, &evm_new_datasize);
> + if (!evm_new_data
> + || ((evm_update_config(evm_new_data, evm_new_datasize)) < 0)) {
> + printk(KERN_INFO "%s: invalid config file\n",
> + __FUNCTION__);
> + rc = -ENOMEM;
-ENOMEM for invalid configuration data?
> +void evm_init_secfs(void)
This should be marked __init, along with anything else in that call chain
which is not reused and contains no global state. Similar for a bunch of
functions which should be marked __exit.
> +{
> + if ((evm_dir = securityfs_create_dir("evm", NULL)) == NULL)
> + return;
> + evm_config = securityfs_create_file("config", S_IRUSR | S_IRGRP
> + | S_IWUSR, evm_dir, NULL, &evm_ops);
> +
> + if ((evm_debug_dir =
> + securityfs_create_dir("debug", evm_dir)) == NULL)
> + return;
> + evm_cache = securityfs_create_file("cache", S_IRUSR | S_IRGRP,
> + evm_debug_dir, "cache",
> + &evm_debug_ops);
> + evm_crypto =
> + securityfs_create_file("crypto", S_IRUSR | S_IRGRP,
> + evm_debug_dir, "crypto",
> + &evm_debug_ops);
> + evm_xattr =
> + securityfs_create_file("xattr", S_IRUSR | S_IRGRP,
> + evm_debug_dir, "xattr", &evm_debug_ops);
> + return;
> +}
There's no error checking or handling here.
> +ssize_t evm_verify_xattr(struct dentry *dentry, char *kVal,
> + char *xattrName, char *xattrVal, int xattrValSize)
> +{
> + ssize_t error;
> + const unsigned char *debug_name = "???";
> +
> + if (!kVal || !xattrName || !xattrVal)
> + return -EINVAL;
These are kernel bugs. Use BUG_ON() if you think they're likely.
> +void evm_cleanup_config(struct evm_xattr_config *evm_data)
> +{
> + if (!evm_data)
> + kfree(evm_data);
> +}
Unecessary and wrong logic.
- James
--
James Morris
<jmorris@private>
This archive was generated by hypermail 2.1.3 : Wed Nov 16 2005 - 10:28:03 PST