[apparmor] Help with extending struct aa_profile

[Abhishek Vijeev]

On Jun 7 2019, at 12:48 am, John Johansen <john.johansen at canonical.com> wrote:
On 6/6/19 1:21 AM, Abhishek Vijeev wrote:
Hi,

I'm looking for some help with modifying AppArmor's kernel code. Kindly let me know whether this is the right forum for such discussions (as I didn't think it would be appropriate to ask for help via the 'Issues' tab on GitLab).

Onto my problem. Basically, I'm trying to add a custom field to 'struct aa_profile' found in <linux_kernel_path>/security/apparmor/include/policy.h and set this field to  a value of my choice. To accomplish this, I have added a single line of code to the 'unpack_profile( )' function found in <linux_kernel_path>/security/apparmor/policy_unpack.c.  However, a kernel that has been compiled with this single extra line of code fails to boot. The boot process halts at 'A start job is running for AppArmor initialization'.

the is odd, the addition of a field to aa_profile should not stop apparmor from functioning. There are several fields in there that are kernel only and have nothing to do with loaded policy. Also unless the field is to do with loaded policy/can or will be set be loaded policy a better place to initialize fields is in aa_alloc_profile in policy.c


For greater clarity, here is the structure after adding my custom field,

struct aa_profile {
struct aa_policy base;
struct aa_profile __rcu *parent;

struct aa_ns *ns;
const char *rename;

const char *attach;
struct aa_dfa *xmatch;
int xmatch_len;
enum audit_mode audit;
long mode;
u32 path_flags;
const char *disconnected;
int size;

struct aa_policydb policy;
struct aa_file_rules file;
struct aa_caps caps;

int xattr_count;
char **xattrs;

struct aa_rlimit rlimits;

struct aa_loaddata *rawdata;
unsigned char *hash;
char *dirname;
struct dentry *dents[AAFS_PROF_SIZEOF];
struct rhashtable *data;
struct aa_label label;
/*
* Custom field:
*/
int custom_field;
};

I would avoid putting data fields after label. The label structure is technically variable length, though its use in profile is always fixed to a single entry + the null sentinel, so you should be safe but its bad form knowing that field is naturally variable length.

Thanks for this extremely valuable information John. It has fixed the problem. Adding the custom field before 'label' has resulted in the kernel successfully booting, and the field being set to the required value. Honestly, we would haven't been able to resolve the issue, had we not known about this.



and here is the line of code to set this field (added at the end of 'unpack_profile( )'),

          profile -> custom_field = 10;


where are you making the assignment in unpack_profile? Does profile exist yet? unpack_profile() will allocate after unpacking the profiles name. Assigning profile->custom_field before the profile is allocated would cause the kernel to oops and could cause things to hang.

Yes, I am making the assignment only after the profile has been allocated (which I presume occurs at line 677 of this file for reference - https://github.com/torvalds/linux/blob/master/security/apparmor/policy_unpack.c<https://link.getmailspring.com/link/77F13BD1-F8FB-41D4-A853-D9FF2221AEC4@getmailspring.com/0?redirect=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fsecurity%2Fapparmor%2Fpolicy_unpack.c&recipient=YXBwYXJtb3JAbGlzdHMudWJ1bnR1LmNvbQ%3D%3D> )

I'm not sure if I'm doing something fundamentally wrong with trying to modify the structure. I do understand that AppArmor verifies each policy's cryptographic hash, and suspect that a hash mismatch renders the kernel un-bootable. However, if the code that generates the hash and the code that calculates and verifies the hash at kernel boot are oblivious of the custom field, why would a mismatch occur?


It does compute a policy hash for the loaded profile but that is not used by the kernel to verify what it loads, the hash values are used for policy dedup and made available to userspace so userspace can do quick verifications of what is in the kernel (dumping large amounts of policy back out is much slower than reading a few hashes). Their is no mismatch on hashes to render the kernel unbootable, apparmor does not do secure boot, integrity is done through the ima subsystem, and secure boot to other parts of the kernel. AppArmor does validate policy that is being loaded and ensures that the data can not be used to crash the kernel, but a failure in a validation check will result in the policy being rejected from loaded not a failure to boot. It is possible you could configure boot to abort on failure to load policy but I haven't seen that done for a long time.

Thank you for this detailed explanation. I'm sure it's going to come in handy soon.

So to be clear the policy verification is not causing a mismatch that is resulting in the hang. My immediate guess is you are assigning profile->custom_field before profile exists.


I'd be grateful if you could kindly provide me with some insight into the root cause of this problem as well as how to resolve it. Do let me know if I can provide any additional information to help clarify the problem.

Thank you,
Abhishek.
Sent from Mailspring

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20190607/75b5ddf3/attachment.html>