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 <[email protected]>
> *Sent:* 27 July 2019 00:10:14
> *To:* Abhishek Vijeev <[email protected]>; [email protected]
> <[email protected]>
> *Cc:* Rakesh Rajan Beck <[email protected]>
> *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.
--
AppArmor mailing list
[email protected]
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
