[apparmor] Questions about AppArmor's Kernel Code
John Johansen
john.johansen at canonical.com
Mon Jul 29 22:40:00 UTC 2019
I haven't tested if this is the cause of your failure but it could very well be
+ // Custom code begin
+
+ if (unpack_nameX(e, AA_STRUCT, "custom_label"))
+ {
+ profile->clabel = kzalloc (sizeof(struct custom_label), GFP_KERNEL);
+ if (!profile->clabel)
+ goto fail;
this block of code
+ if (!unpack_str(e, &name, NULL))
+ goto fail;
+ profile->clabel->label_name = kzalloc (strlen(name), GFP_KERNEL);
+ if (!profile->clabel->label_name)
+ goto fail;
+
+ strcpy (profile->clabel->label_name, name);
+
you are allocation a string that is not long enough to include the null terminating \0 so when
you use strcpy it is writing one byte beyond your allocated data.
replace this with
+ if (!unpack_strdup(e, &profile->clabel->label_name, NULL))
+ goto fail;
+ if (!unpack_u32(e, &(profile->clabel->allow_cnt), NULL))
+ goto fail;
+
+
+ if (unpack_nameX(e, AA_STRUCT, "data_list"))
+ {
+ profile->clabel->allow_list = kzalloc(sizeof(struct data_list), GFP_KERNEL);
+ if (!profile->clabel->allow_list)
+ goto fail;
+ INIT_LIST_HEAD(&(profile->clabel->allow_list->lh));
+
+ for (i = 0; i < profile->clabel->allow_cnt; i++)
+ {
+ if (!unpack_str(e, &name, NULL))
+ goto fail;
+ struct data_list *new_node = kzalloc(sizeof(struct data_list), GFP_KERNEL);
+ if (!new_node)
+ goto fail;
same thing for this block of code
+ new_node->data = kzalloc(strlen(name), GFP_KERNEL);
+ if (!new_node->data)
+ goto fail;
+ strcpy(new_node->data, name);
its writing 1 past the end of your allocation. You can either
- increase allocation size by one to account for terminating \0
- switch to unpack_strdup() like above and add a free on failure
+ INIT_LIST_HEAD(&(new_node->lh));
+ list_add(&(new_node->lh), &(profile->clabel->allow_list->lh));
+ }
+ if (!unpack_nameX(e, AA_STRUCTEND, NULL))
+ goto fail;
+ }
+ if (!unpack_nameX(e, AA_STRUCTEND, NULL))
+ goto fail;
+ }
+
+ // Custom code end
On 7/29/19 12:58 AM, Abhishek Vijeev wrote:
> Thank you for the explanation John.
>
> I have attached the files we have modified. Every piece of code that we inserted is enclosed
> within comment lines 'Custom code begin' and 'Custom code end' so that it's easy for you to find. Here
> is a brief description of the changes made:
>
> AppArmor Parser (user-space) - We modified the grammar of AppArmor's parser to include additional
> grammar rules. These rules store data in class Profile
>
> a) profile.h - 2 new structure definitions to store our custom data
> - class Profile now contains a member 'clabel'
> b) parser_interface.c - Added code to sd_serialize_profile( ) that serializes the additional custom
> data we added to class Profile during the parsing phase
> AppArmor LSM (kernel) :
>
>
> a) include/policy.h - 2 new structure definitions
> - struct aa_profile now contains a member 'clabel'
>
> b) policy_unpack.c- Added code to unpack_profile( ) that unpacks the serialized object sent from
> user-space, and allocates kernel memory for the security structures added to
> aa_profile - namely, a label string and a linked list containing allow permissions
> c) policy.c - Added code to function aa_free_profile( ) that frees the allocated memory
>
> ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> *From:* John Johansen <john.johansen at canonical.com>
> *Sent:* 27 July 2019 00:10:14
> *To:* Abhishek Vijeev <abhishekvijeev at iisc.ac.in>; apparmor at lists.ubuntu.com <apparmor at lists.ubuntu.com>
> *Cc:* Rakesh Rajan Beck <rakeshbeck at iisc.ac.in>
> *Subject:* Re: [apparmor] Questions about AppArmor's Kernel Code
>
> On 7/26/19 5:56 AM, Abhishek Vijeev wrote:
>> Hi,
>>
>>
>> I have a few questions about AppArmor's kernel code and would be grateful if you could kindly answer them.
>>
>>
>> 1) Why does AppArmor maintain two separate security blobs in cred->security as well as task-security for processes? For a simple project that requires associating a security context with every task, would it suffice to use just one of these?
>>
> the task->security field is used to store task specific information, that is not used for general mediation. Currently the information stored their is for the change_hat and change_onexec apis and some info to track what the confinement was when no-newprivs was applied to the task.
>
> cred->security is used to store the subjects label (type) for mediation.
>
> Before the task->security field was reintroduce all the information was stored off the cred in a intermediate structure. Doing so would cause use of the change_hat and change_onexec api to change the cred of the task even when the confinement had not changed. The switch to using the task->security field was pre 4.18
>
>>
>> 2) There has been a change in the way security blobs are accessed from kernel version 4.18 to 5.2. I see that in v5.2, the security blob's address is obtained by adding the size of the blob to the start address. Why has this change been made? (For reference: https://github.com/torvalds/linux/blob/master/security/apparmor/include/cred.h#L24)
>>
> see Casey's answer
>
>>
>> 3) I tried adding a custom field (pointer to a custom structure) to struct aa_profile, at exactly this point - https://github.com/torvalds/linux/blob/master/security/apparmor/include/policy.h#L144. I have taken care to allocate and free memory for the pointer at the appropriate places (allocation is performed here - https://github.com/torvalds/linux/blob/master/security/apparmor/policy_unpack.c#L671 and freeing is performed here - https://github.com/torvalds/linux/blob/master/security/apparmor/policy.c#L205). However, while booting the kernel, it crashes at the function 'security_prepare_creds( )', which I presume invokes 'apparmor_cred_prepare( )'. If I was, to assume for a moment that there are no bugs with my memory allocation code, is there any other reason why such a crash might have occurred? I have attached the kernel crash log file with this email for your kind reference.
>>
>
> I know the code points but to be able to comment beyond vague guesses I need to see your changes. I can give you the warning to not add your field after the current last field,
>
> struct aa_label label;
>
> as it has a variable length field. While that is always 2 entries when its embedded in the profile the compiler will end up treating it as zero length over lapping your new field with the start of the variable length array.
>
> I do have a patch to address this using a union but I haven't landed it yet.
More information about the AppArmor
mailing list