On 6/24/19 6:46 PM, Paul Moore wrote:
> On Mon, Jun 24, 2019 at 9:01 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
>> On 6/24/2019 2:33 PM, John Johansen wrote:
>>> On 6/21/19 11:52 AM, Casey Schaufler wrote:
>>>> Change the audit code to store full lsmblob data instead of
>>>> a single u32 secid. This allows for multiple security modules
>>>> to use the audit system at the same time. It also allows the
>>>> removal of scaffolding code that was included during the
>>>> revision of LSM interfaces.
>>>>
>>>> Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
>>> I know Kees raised this too, but I haven't seen a reply
>>>
>>> Eric (Paul is already CCed): I have directly added you because of
>>> the question below.
>>>
>>> In summary there isn't necessarily a single secid any more, and
>>> we need to know whether dropping the logging of the secid or
>>> logging all secids is the correct action.
>>
>> It is to be considered that this is an error case. If
>> everything is working normally you should have produced
>> a secctx previously, which you'll have included in the
>> audit record. Including the secid in the record ought to
>> be pointless, as the secid is strictly an internal token
>> with no meaning outside the running kernel. You are providing
>> no security relevant information by providing the secid.
>> I will grant the possibility that the secid might be useful
>> in debugging, but for that a pr_warn is more appropriate
>> than a field in the audit record.
>
> FWIW, this probably should have been CC'd to the audit list.
>
hrmm indeed, sorry
> I agree that this is an error case (security_secid_to_secctx() failed
> to resolve the secid) and further that logging the secid, or a
> collection of secids, has little value the way things currently work.
> Since secids are a private kernel implementation detail, we don't
> really display them outside the context of the kernel, including in
> the audit logs. Recording a secid in this case doesn't provide
> anything meaningful since secids aren't recorded in the audit record
> stream, only the secctxs, and there is no "magic decoder ring" to go
> between the two in the audit logs, or anywhere else in userspace for
> that matter.
>
Okay, thanks. Casey I am good with just a pr_warn here. I just didn't
have context of why it was going to the audit_log and didn't want
to change that without some more input.
>>>> ---
>>>> kernel/audit.h | 6 +++---
>>>> kernel/auditsc.c | 38 +++++++++++---------------------------
>>>> 2 files changed, 14 insertions(+), 30 deletions(-)
>
> ...
>
>>>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>>>> index 0478680cd0a8..d3ad13f11788 100644
>>>> --- a/kernel/auditsc.c
>>>> +++ b/kernel/auditsc.c
>>>> @@ -1187,21 +1184,18 @@ static void show_special(struct audit_context *context, int *call_panic)
>>>> context->socketcall.args[i]);
>>>> break; }
>>>> case AUDIT_IPC: {
>>>> - u32 osid = context->ipc.osid;
>>>> + struct lsmblob *olsm = &context->ipc.olsm;
>>>>
>>>> audit_log_format(ab, "ouid=%u ogid=%u mode=%#ho",
>>>> from_kuid(&init_user_ns, context->ipc.uid),
>>>> from_kgid(&init_user_ns, context->ipc.gid),
>>>> context->ipc.mode);
>>>> - if (osid) {
>>>> + if (lsmblob_is_set(olsm)) {
>>>> struct lsmcontext lsmcxt;
>>>> - struct lsmblob blob;
>>>>
>>>> - lsmblob_init(&blob, osid);
>>>> - if (security_secid_to_secctx(&blob, &lsmcxt)) {
>>>> - audit_log_format(ab, " osid=%u", osid);
>>> I am not comfortable just dropping this I would think logging all secids is the
>>> correct action here.
>>>
>>>
>>>> + if (security_secid_to_secctx(olsm, &lsmcxt))
>>>> *call_panic = 1;
>>>> - } else {
>>>> + else {
>>>> audit_log_format(ab, " obj=%s", lsmcxt.context);
>>>> security_release_secctx(&lsmcxt);
>>>> }
>
