Re: [PATCH v3 21/24] Audit: Store LSM audit information in an lsmblob

From: Casey Schaufler <casey@schaufler-ca.com>
To: Paul Moore <paul@paul-moore.com>,
	John Johansen <john.johansen@canonical.com>
Cc: casey.schaufler@intel.com, James Morris <jmorris@namei.org>,
	linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
	keescook@chromium.org, penguin-kernel@i-love.sakura.ne.jp,
	Stephen Smalley <sds@tycho.nsa.gov>,
	Eric Paris <eparis@redhat.com>,
	linux-audit@redhat.com, casey@schaufler-ca.com
Subject: Re: [PATCH v3 21/24] Audit: Store LSM audit information in an lsmblob
Date: Tue, 25 Jun 2019 08:30:35 -0700	[thread overview]
Message-ID: <c231726e-5235-1dae-8fb9-6f89db406260@schaufler-ca.com> (raw)
In-Reply-To: <CAHC9VhSSMjCdT8gfB03gh8VvJngTHnsUsDCpseMUiEPQM9z_FQ@mail.gmail.com>

On 6/24/2019 7:42 PM, Paul Moore wrote:
> On Mon, Jun 24, 2019 at 10:15 PM John Johansen
> <john.johansen@canonical.com> wrote:
>> On 6/24/19 6:46 PM, Paul Moore wrote:
>>> On Mon, Jun 24, 2019 at 9:01 PM Casey Schaufler <casey@schaufler-ca.com> 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@schaufler-ca.com>
>>>>> 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.
> Hmm.  Actually, let me change my comments slightly ... perhaps what we
> should do here is keep the audit_log_format(), but change it from
> audit_log_format("osid=%u",...) to audit_log_format("obj=?").  The "?"
> is used in audit when we can't determine a piece of information, but
> we normally log it.  It wasn't used very widely originally, which is
> probably why it isn't in this piece of code.

Works for me. I'll make the change. Thank you.
??