[apparmor] RFC: cryptographic profile hashing

[John Johansen]

On 07/22/2013 06:19 PM, Seth Arnold wrote:
> On Fri, Jul 19, 2013 at 10:27:57PM -0700, John Johansen wrote:
>> Below is a patch that has been hanging around for a long time but needs a
>> little more work to be completed. What it does is add a per profile sha1
>> hash to apparmorfs. So that kernel policy can be verified against the
>> policy in userspace.
>>
>> eg.
>>> cat /sys/kernel/security/apparmor/policy/profiles/bin.ping.7/sha1
>> 40463c310088fffff02d10300088ffff00194e3b
>>
>>
>> The hash value added is NOT the hash value that can be attained by running
>> sha1sum on the profile or cache file, because we aren't loading the
>> original profile text into the kernel, and the binary cache file may
>> contain multiple profiles, but the kernel doesn't see the profiles at the
>> file level but as a stream of profiles. Instead it hashes each individual
>> profile within the profile stream. This means we would need a userspace
>> tool capable of hashing the profiles within a binary cache. This would be
>> one of the things that needs to be done so that we can test and verify
>> everything is working as expect.
>>
>> we could move the hash to md5, or another hash or even make it selectable.
>> I think md5 would be sufficient as we are just using it provide a simple
>> way to verify what policy is loaded. And the plan is to make the full
>> binary policy available as well at some point so perhaps this isn't
>> even needed.  Except the kernel export binary may not exactly match
>> what is in userspace because the load stage may transform parts of it.
> 
> sha1 is fine, md5 isn't faster enough to justify switching to it, I
> think.
>
that was my feeling as well

>>
>> ---
>>
>> diff --git a/security/apparmor/Kconfig b/security/apparmor/Kconfig
>> index 9b9013b..5da5e4f 100644
>> --- a/security/apparmor/Kconfig
>> +++ b/security/apparmor/Kconfig
>> @@ -29,3 +29,16 @@ config SECURITY_APPARMOR_BOOTPARAM_VALUE
>>  	  boot.
>>  
>>  	  If you are unsure how to answer this question, answer 1.
>> +
>> +config SECURITY_APPARMOR_HASH
>> +	bool "Cryptographic hash of loaded profiles"
> 
> I think "cryptographic" is overselling the feature. "Hash" alone would
> suit me.
> 
Hash is fine by me too, part of this is that the original patch offered
a choice dialog in menu config and you could choose the has you wanted.

In fact I thin "SHA1 Hash of loaded profiles" works well for the current
patch.

>> +	depends on SECURITY_APPARMOR
>> +	depends on CRYPTO
>> +	select CRYPTO_SHA1
>> +	default n
>> +
>> +	help
>> +	  This option selects whether cryptographic hashing is done
>> +	  against loaded profiles to present to user space via the apparmor
>> +	  filesystem so that userspace can validate the currently loaded
>> +	  profiles.
> 
> ... and I'm a little leery of "validate the currently loaded profiles".
> The hash is computed when the profile is loaded, not at the time the
> sha1 file is read, so it identifes the policy that was loaded, not the
> current policy. (They should be the same, barring bugs, of course...)
> 
Well they had better be the same thing or we have a series bug. Of course
I am fine saying "the loaded policy"

>> diff --git a/security/apparmor/Makefile b/security/apparmor/Makefile
>> index 5706b74..adbef37 100644
>> --- a/security/apparmor/Makefile
>> +++ b/security/apparmor/Makefile
>> @@ -5,6 +5,7 @@ obj-$(CONFIG_SECURITY_APPARMOR) += apparmor.o
>>  apparmor-y := apparmorfs.o audit.o capability.o context.o ipc.o lib.o match.o \
>>                path.o domain.o policy.o policy_unpack.o procattr.o lsm.o \
>>                resource.o sid.o file.o
>> +apparmor-$(CONFIG_SECURITY_APPARMOR_HASH) += crypto.o
>>  
>>  clean-files := capability_names.h rlim_names.h
>>  
>> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
>> index 5325e90..abe6386 100644
>> --- a/security/apparmor/apparmorfs.c
>> +++ b/security/apparmor/apparmorfs.c
>> @@ -26,6 +26,7 @@
>>  #include "include/apparmorfs.h"
>>  #include "include/audit.h"
>>  #include "include/context.h"
>> +#include "include/crypto.h"
>>  #include "include/policy.h"
>>  #include "include/resource.h"
>>  
>> @@ -319,6 +320,33 @@ static const struct file_operations aa_fs_profattach_fops = {
>>  	.release	= aa_fs_seq_profile_release,
>>  };
>>  
>> +static int aa_fs_seq_hash_show(struct seq_file *seq, void *v)
>> +{
>> +	char *string = seq->private;
> 
> If this were "unsigned char" you wouldn't need the cast lower...
> 
yep

>> +	unsigned int i;
>> +
>> +	if (string) {
>> +	  for (i = 0; i < aa_hash_size(); i++)
>> +			seq_printf(seq, "%.2x", (unsigned char) string[i]);
>> +		seq_printf(seq, "\n");
>> +	}
> 
> The indentation of the 'for' line is incorrect.
> 
yep

> Will gcc haul out the aa_hash_size() function call from the for loop? If
> not, we should. (We could even hard-code the length of sha1 in the few
Hrmmm it will for the disabled case but not I think for the enabled case,
at least not without cross link analysis and discovery that the hash fns
don't touch any mem etc.

So yeah we should lift it out

> places it's used and get rid of the function and variable entirely...
> it'll never change at runtime, ksplice aside.)
> 
It rather not, while this patch doesn't provide alternative hashes, its
structured to do so easily, if at some point we choose to do so.

>> +
>> +	return 0;
>> +}
>> +
>> +static int aa_fs_seq_hash_open(struct inode *inode, struct file *file)
>> +{
>> +	return single_open(file, aa_fs_seq_hash_show, inode->i_private);
>> +}
>> +
>> +static const struct file_operations aa_fs_seq_hash_fops = {
>> +	.owner		= THIS_MODULE,
>> +	.open		= aa_fs_seq_hash_open,
>> +	.read		= seq_read,
>> +	.llseek		= seq_lseek,
>> +	.release	= single_release,
>> +};
>> +
>>  /** fns to setup dynamic per profile/namespace files **/
>>  void __aa_fs_profile_rmdir(struct aa_profile *profile)
>>  {
>> @@ -420,6 +448,15 @@ int __aa_fs_profile_mkdir(struct aa_profile *profile, struct dentry *parent)
>>  		goto fail;
>>  	profile->dents[AAFS_PROF_ATTACH] = dent;
>>  
>> +	if (profile->hash) {
>> +		dent = securityfs_create_file("sha1", S_IFREG | 0444, dir,
>> +					      &profile->hash,
>> +					      &aa_fs_seq_hash_fops);
>> +		if (IS_ERR(dent))
>> +			goto fail;
>> +		profile->dents[AAFS_PROF_HASH] = dent;
>> +	}
>> +
>>  	list_for_each_entry(child, &profile->base.profiles, base.list) {
>>  		error = __aa_fs_profile_mkdir(child, prof_child_dir(profile));
>>  		if (error)
>> diff --git a/security/apparmor/crypto.c b/security/apparmor/crypto.c
>> new file mode 100644
>> index 0000000..8c2484d
>> --- /dev/null
>> +++ b/security/apparmor/crypto.c
>> @@ -0,0 +1,90 @@
>> +/*
>> + * AppArmor security module
>> + *
>> + * This file contains AppArmor policy loading interface function definitions.
>> + *
>> + * Copyright 2013 Canonical Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation, version 2 of the
>> + * License.
>> + *
>> + * Fns to provide a cryptographic checksum of policy that has been loaded
>> + * this can be compared to userspace policy compiles to verify loaded
>> + * policy is what it should be.
>> + */
>> +
>> +#include <linux/crypto.h>
>> +
>> +#include "include/apparmor.h"
>> +#include "include/crypto.h"
>> +
>> +static unsigned int apparmor_hash_size;
>> +
>> +static struct crypto_hash *apparmor_tfm;
>> +
>> +unsigned int aa_hash_size(void)
>> +{
>> +	return apparmor_hash_size;
>> +}
>> +
>> +int aa_calc_profile_hash(struct aa_profile *profile, void *start, size_t len)
>> +{
>> +	struct scatterlist sg;
>> +	struct hash_desc desc = {
>> +		.tfm = apparmor_tfm,
>> +		.flags = 0
>> +	};
>> +	int error = -ENOMEM;
>> +
>> +	if (!apparmor_tfm)
>> +		return 0;
>> +
>> +	sg_init_one(&sg, (u8 *) start, len);
>> +
>> +	profile->hash = kzalloc(apparmor_hash_size, GFP_KERNEL);
>> +	if (!profile->hash)
>> +		goto fail;
>> +
>> +	error = crypto_hash_init(&desc);
>> +	if (error)
>> +		goto fail;
>> +	error = crypto_hash_update(&desc, &sg, len);
>> +	if (error)
>> +		goto fail;
>> +	error = crypto_hash_final(&desc, profile->hash);
>> +	if (error)
>> +		goto fail;
>> +
>> +	return 0;
>> +
>> +fail:
>> +	kfree(profile->hash);
>> +	profile->hash = NULL;
>> +
>> +	return error;
>> +}
>> +
>> +static int __init init_profile_hash(void)
>> +{
>> +	struct crypto_hash *tfm;
>> +
>> +	if (!apparmor_initialized)
>> +		return 0;
>> +
>> +	tfm = crypto_alloc_hash("sha1", 0, CRYPTO_ALG_ASYNC);
>> +	if (IS_ERR(tfm)) {
>> +		int error = PTR_ERR(tfm);
>> +		AA_ERROR("failed to setup profile sha1 hashing: %d\n", error);
>> +		return error;
>> +	}
>> +	apparmor_tfm = tfm;
>> +	apparmor_hash_size = crypto_hash_digestsize(apparmor_tfm);
>> +
>> +	aa_info_message("AppArmor cryptographic sha1 policy hashing Enabled");
>> +
>> +	return 0;
>> +}
>> +
>> +late_initcall(init_profile_hash);
>> diff --git a/security/apparmor/include/apparmorfs.h b/security/apparmor/include/apparmorfs.h
>> index f91712c..414e568 100644
>> --- a/security/apparmor/include/apparmorfs.h
>> +++ b/security/apparmor/include/apparmorfs.h
>> @@ -82,6 +82,7 @@ enum aafs_prof_type {
>>  	AAFS_PROF_NAME,
>>  	AAFS_PROF_MODE,
>>  	AAFS_PROF_ATTACH,
>> +	AAFS_PROF_HASH,
>>  	AAFS_PROF_SIZEOF,
>>  };
>>  
>> diff --git a/security/apparmor/include/crypto.h b/security/apparmor/include/crypto.h
>> new file mode 100644
>> index 0000000..613944d
>> --- /dev/null
>> +++ b/security/apparmor/include/crypto.h
>> @@ -0,0 +1,35 @@
>> +/*
>> + * AppArmor security module
>> + *
>> + * This file contains AppArmor policy loading interface function definitions.
>> + *
>> + * Copyright 2013 Canonical Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation, version 2 of the
>> + * License.
>> + */
>> +
>> +#ifndef __APPARMOR_CRYPTO_H
>> +#define __APPARMOR_CRYPTO_H
>> +
>> +#include "policy.h"
>> +
>> +#ifdef CONFIG_SECURITY_APPARMOR_HASH
>> +unsigned int aa_hash_size(void);
>> +int aa_calc_profile_hash(struct aa_profile *profile, void *start, size_t len);
>> +#else
>> +static inline int aa_calc_profile_hash(struct aa_profile *profile, void *start,
>> +				       size_t len)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline unsigned int aa_hash_size(void)
>> +{
>> +	return 0;
>> +}
>> +#endif
>> +
>> +#endif /* __APPARMOR_CRYPTO_H */
>> diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h
>> index 6283caf..e8460e4 100644
>> --- a/security/apparmor/include/policy.h
>> +++ b/security/apparmor/include/policy.h
>> @@ -218,6 +218,7 @@ struct aa_profile {
>>  	struct aa_caps caps;
>>  	struct aa_rlimit rlimits;
>>  
>> +	char *hash;
> 
> unsigned char instead? It stores a binary blob, not human-aimed
> characters...
> 
yep

>>  	char *dirname;
>>  	struct dentry *dents[AAFS_PROF_SIZEOF];
>>  };
>> diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
>> index f5b9977..49fff70 100644
>> --- a/security/apparmor/policy_unpack.c
>> +++ b/security/apparmor/policy_unpack.c
>> @@ -24,6 +24,7 @@
>>  #include "include/apparmor.h"
>>  #include "include/audit.h"
>>  #include "include/context.h"
>> +#include "include/crypto.h"
>>  #include "include/match.h"
>>  #include "include/policy.h"
>>  #include "include/policy_unpack.h"
>> @@ -66,6 +67,7 @@ struct aa_ext {
>>  	u32 version;
>>  };
>>  
>> +
>>  /* audit callback for unpack fields */
>>  static void audit_cb(struct audit_buffer *ab, void *va)
>>  {
>> @@ -756,12 +758,14 @@ int aa_unpack(void *udata, size_t size, struct list_head *lh, const char **ns)
>>  
>>  	*ns = NULL;
>>  	while (e.pos < e.end) {
>> +		void *start;
>>  		error = verify_header(&e, e.pos == e.start, ns);
>>  		if (error)
>>  			goto fail;
>>  
>>  		/* alignment adjust needed by dfa unpack */
>>  		e.adj = (e.pos - e.start) & 7;
>> +		start = e.pos;
>>  		profile = unpack_profile(&e);
>>  		if (IS_ERR(profile)) {
>>  			error = PTR_ERR(profile);
>> @@ -769,16 +773,17 @@ int aa_unpack(void *udata, size_t size, struct list_head *lh, const char **ns)
>>  		}
>>  
>>  		error = verify_profile(profile);
>> -		if (error) {
>> -			aa_free_profile(profile);
>> -			goto fail;
>> -		}
>> +		if (error)
>> +			goto fail_profile;
> 
> I don't understand this change. Is this the correct thing to do before
> the aa_load_ent_alloc() call below? (Which appeared to be the turning
> point between free/fail vs put/fail...)
> 
So yes if we are going to fail there is no point in allocating the
load_ent.  In fact at this point we don't really need put_profile and
we could change these to calls to free_profile.

We know at this point we have the single reference count, either on
the load ent, or the ref we are about to create a load ent for

put profile really only becomes necessary as we start getting more
references in the replace_profiles routine that calls this.

>> +
>> +		error = aa_calc_profile_hash(profile, start, e.pos - start);
>> +		if (error)
>> +			goto fail_profile;
>>  
>>  		ent = aa_load_ent_alloc();
>>  		if (!ent) {
>>  			error = -ENOMEM;
>> -			aa_put_profile(profile);
>> -			goto fail;
>> +			goto fail_profile;
>>  		}
>>  
>>  		ent->new = profile;
>> @@ -787,6 +792,9 @@ int aa_unpack(void *udata, size_t size, struct list_head *lh, const char **ns)
>>  
>>  	return 0;
>>  
>> +fail_profile:
>> +	aa_put_profile(profile);
>> +
>>  fail:
>>  	list_for_each_entry_safe(ent, tmp, lh, list) {
>>  		list_del_init(&ent->list);
> 
> 
> Thanks
> 
> 
>