lkml.org 
[lkml]   [2008]   [Dec]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 3/6] integrity: IMA as an integrity service provider
    From
    Date
    On Tue, 2008-12-02 at 15:35 -0800, Dave Hansen wrote: 
    > On Tue, 2008-12-02 at 16:47 -0500, Mimi Zohar wrote:
    > > index 0000000..6c6fcd9
    > > --- /dev/null
    > > +++ b/security/integrity/ima/Kconfig
    > > @@ -0,0 +1,34 @@
    > > +#
    > > +# IBM Integrity Measurement Architecture
    > > +#
    > > +config IMA
    > > + bool "Integrity Measurement Architecture(IMA)"
    > > + depends on INTEGRITY
    > > + depends on ACPI
    > > + select SECURITYFS
    > > + select CRYPTO
    > > + select CRYPTO_HMAC
    > > + select CRYPTO_MD5
    > > + select CRYPTO_SHA1
    > > + select TCG_TPM
    > > + select TCG_TIS
    > > + help
    > > + The Trusted Computing Group(TCG) runtime Integrity
    > > + Measurement Architecture(IMA) maintains a list of hash
    > > + values of executables and other sensitive system files
    > > + loaded into the run-time of this system. If your system
    > > + has a TPM chip, then IMA also maintains an aggregate
    > > + integrity value over this list inside the TPM hardware.
    > > + These measurements and the aggregate (signed inside the
    > > + TPM) can be retrieved and presented to remote parties to
    > > + establish system properties. If unsure, say N.
    >
    > This still doesn't tell me how it helps me. "If an attacker managed to
    > change the contents of an important system file being measured, we can
    > tell." Right?

    Yes, that is a clearer description. Thanks.

    > > +config IMA_MEASURE_PCR_IDX
    > > + int "PCR for Aggregate (8 <= Index <= 14)"
    > > + depends on IMA
    > > + range 8 14
    > > + default 10
    > > + help
    > > + IMA_MEASURE_PCR_IDX determines the TPM PCR register index
    > > + that IMA uses to maintain the integrity aggregate of the
    > > + measurement list. If unsure, use the default 10.
    >
    > Why would you want to change this? Can it be done at runtime instead of
    > compile time? I don't know what a PCR is.

    The only reason to change it would be if in the future, TCG decides on a
    standard PCR for IMA, other than 10, or if they pick 10 for something
    else. We really don't need a runtime variable for this, but kconfig
    makes it easy to change once if necessary in the future.

    > > +#define ima_printk(level, format, arg...) \
    > > + printk(level "ima (%s): " format, __func__, ## arg)
    > > +
    > > +#define ima_error(format, arg...) \
    > > + ima_printk(KERN_ERR, format, ## arg)
    > > +
    > > +#define ima_info(format, arg...) \
    > > + ima_printk(KERN_INFO, format, ## arg)
    >
    > Please don't. Can you use pr_debug() and friends?

    will clean this up.

    > > +/* digest size for IMA, fits SHA1 or MD5 */
    > > +#define IMA_DIGEST_SIZE 20
    >
    > When another algorithm (with a longer digest) is added, will we detect
    > that without this just plain breaking?
    >

    As the kernel command line option "ima_hash=" verifies the crypto
    algorithm specified, changes to the code would be required to support a
    new algorithm anyway.

    > > +struct ima_h_table {
    > > + atomic_long_t len; /* number of stored measurements in the list */
    > > + atomic_long_t violations;
    > > + unsigned int max_htable_size;
    > > + struct hlist_head queue[IMA_MEASURE_HTABLE_SIZE];
    > > + atomic_t queue_len[IMA_MEASURE_HTABLE_SIZE];
    > > +};
    > > +extern struct ima_h_table ima_htable;
    > > +
    > > +static inline unsigned long IMA_HASH_KEY(u8 *digest)
    > > +{
    > > + return (hash_long(*digest, IMA_HASH_BITS));
    > > +}
    >
    > 'return' isn't a function. :)

    > Also, please lower-case this sucker so we know it isn't a macro.

    Ok

    > > +void ima_add_violation(struct inode *inode, const unsigned char *filename,
    > > + char *op, char *cause)
    > > +{
    > > + int result, namelen;
    > > + struct ima_inode_measure_entry measure_entry;
    > > + struct ima_store_template_data template = {
    > > + .name = "ima",
    > > + .len = sizeof(measure_entry),
    > > + .data = (char *)&measure_entry,
    > > + .violation = 1,
    > > + };
    >
    > If '.data' is a char*, perhaps it should be a void*. If it already is a
    > void*, you don't need a cast.
    >
    > > +int ima_must_measure(void *data)
    > > +{
    > > + struct ima_measure_data *mdata = (struct ima_measure_data *)data;
    >
    > No need to cast a void*. You have several of these. Please find all of
    > them and fix them up.

    Thank you. will be in the next set patches.

    > > + struct ima_iint_cache *iint;
    > > + int must_measure = -EACCES;
    > > +
    > > + if (!S_ISREG(mdata->inode->i_mode))
    > > + return -EPERM;
    > > + if ((mdata->mask & MAY_WRITE) || (mdata->mask & MAY_APPEND))
    > > + return -EPERM;
    > > +
    > > + rcu_read_lock();
    > > + iint = ima_iint_lookup(mdata->inode);
    > > + if (iint)
    > > + kref_get(&iint->refcount);
    > > + rcu_read_unlock();
    >
    > All of ima_iint_lookup()'s callers do the exact same thing. Please just
    > make it ima_iint_find_get(), and do the RCU and kref_get() internally
    > and once.

    cleaner, thanks.

    > > + if (!iint) {
    > > + int rc;
    > > +
    > > + /* Most insertions are done at inode_alloc,
    > > + * except those allocated before late_initcall.
    > > + * Can't initialize at security_initcall,
    > > + * got to wait at least until proc_init.
    > > + */
    > > + rc = ima_iint_insert(mdata->inode);
    > > + if (rc < 0)
    > > + return rc;
    > > +
    > > + rcu_read_lock();
    > > + iint = ima_iint_lookup(mdata->inode);
    > > + if (!iint) {
    > > + rcu_read_unlock();
    > > + return -ENOMEM;
    > > + }
    > > + kref_get(&iint->refcount);
    > > + rcu_read_unlock();
    > > + }
    >
    > How about a retry goto instead of just copying the code again? Better
    > yet, can you just stick all of this in a helper function?

    After the ima_iint_find_get() recommendation above, this becomes a lot
    smaller.

    > > +int ima_iint_insert(struct inode *inode)
    > > +{
    > > + struct ima_iint_cache *iint;
    > > + int rc = 0;
    > > +
    > > + iint = kzalloc(sizeof(*iint), GFP_KERNEL);
    >
    > Does this basically get done for every inode, or only special ones? I
    > just wonder if having a dedicated slab with a constructor to do
    > redundant things like mutex_init() would be helpful.

    every inode, except those allocated before init_latecall.

    > > +static void ima_add_boot_aggregate(void)
    > > +{
    > > + struct ima_inode_measure_entry measure_entry;
    > > + struct ima_store_template_data template = {
    > > + .name = "ima",
    > > + .len = sizeof(measure_entry),
    > > + .data = (char *)&measure_entry,
    > > + };
    > > + int namelen, result;
    > > +
    > > + memset(&measure_entry, 0, sizeof measure_entry);
    > > + namelen = strlen(boot_aggregate_name);
    > > + if (namelen > IMA_EVENT_NAME_LEN_MAX)
    > > + namelen = IMA_EVENT_NAME_LEN_MAX;
    > > + memcpy(measure_entry.file_name, boot_aggregate_name, namelen);
    > > +
    > > + if (ima_used_chip) {
    > > + int i;
    > > + u8 pcr_i[IMA_DIGEST_SIZE];
    > > + struct hash_desc desc;
    > > + struct crypto_hash *tfm;
    > > + struct scatterlist sg;
    >
    > All of this stack stuff with very important, large sounding names makes
    > me nervous. Can you reassure me?

    The crypto code here will be moved to ima_crypto.c and will be
    refactored, cleaning up the code. Both measure_entry and template could
    be allocated/freed each time, but does that make sense?

    > > + tfm = crypto_alloc_hash(ima_hash, 0, CRYPTO_ALG_ASYNC);
    > > + if (!tfm || IS_ERR(tfm)) {
    > > + ima_error("error initializing digest.\n");
    > > + return;
    > > + }
    > > + desc.tfm = tfm;
    > > + desc.flags = 0;
    > > + crypto_hash_init(&desc);
    > > +
    > > + /* cumulative sha1 over tpm registers 0-7 */
    > > + for (i = 0; i < 8; i++) {
    >
    > Surely there's a NR_TPM_REGISTERS or similar somewhere.

    Will look.

    > > +static void ima_file_free(struct file *file)
    > > +{
    > > + struct inode *inode = file->f_dentry->d_inode;
    > > + struct ima_iint_cache *iint;
    > > +
    > > + if (S_ISDIR(inode->i_mode))
    > > + return;
    > > + if ((file->f_mode & FMODE_WRITE) &&
    > > + (atomic_read(&inode->i_writecount) == 1)) {
    > > + rcu_read_lock();
    > > + iint = ima_iint_lookup(inode);
    > > + if (!iint) {
    > > + rcu_read_unlock();
    > > + return;
    > > + }
    > > + kref_get(&iint->refcount);
    > > + rcu_read_unlock();
    > > +
    > > + mutex_lock(&iint->mutex);
    > > + if (iint->version != inode->i_version)
    > > + iint->flags &= ~IMA_MEASURED;
    > > + mutex_unlock(&iint->mutex);
    > > + kref_put(&iint->refcount, iint_free);
    > > + }
    > > +}
    >
    > I'm also wondering if there's a way to wrap up the mutex operations
    > since this seems to be done the exact same way every time. Dunno, maybe
    > it is too much locking obfuscation for just a few lines saved.

    Unlike the ima_iint_lookup(), the code within the mutex locking differs
    between calls.

    > > +static int ima_path_check_integrity(struct path *path, int mask)
    > > +{
    > > + struct ima_measure_data mdata;
    > > +
    > > + memset(&mdata, 0, sizeof mdata);
    > > + mdata.inode = path->dentry->d_inode;
    > > + mdata.mask = mask;
    > > + mdata.function = PATH_CHECK;
    > > +
    > > + /* Invalidate PCR, if a measured file is already open for read */
    > > + if ((mdata.mask & (MAY_WRITE | MAY_READ)) == MAY_WRITE) {
    >
    > It would warm my heart to see something like this:
    >
    > int mdata_is_write_only(struct ima_measure_data *mdata)
    > {
    > if (mdata.mask & MAY_READ)
    > return 0;
    > return mdata.mask & MAY_WRITE;
    > }
    >
    > I don't know about you, but I find that immeasurably nicer. Is it even
    > right?

    In addition to MAY_READ/MAY_WRITE, there might be other flags.

    Mimi



    \
     
     \ /
      Last update: 2008-12-03 19:19    [W:0.039 / U:58.992 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site