Re: [PATCH] IPC_SET_PERM cleanup
Hi Dustin,
Thanks for the comments.
Dustin Kirkland wrote:
On 5/5/06, Linda Knippers <linda.knippers(a)hp.com> wrote:
> The following patch addresses most of the issues with the IPC_SET_PERM
> records as described in:
> https://www.redhat.com/archives/linux-audit/2006-May/msg00010.html
Hi Linda-
I apologize for the delay in my response. I'm pretty much permanently
away from Audit-related work, and just now got a chance to respond to
this.
First, let me point you to a thread wherein I explained why I made the
audit ipc changes that I did (in case you missed it the first time
around). It starts here:
https://www.redhat.com/archives/linux-audit/2006-March/msg00088.html
That said, thanks for testing some of this out more thoroughly and
posting your findings.
Thanks, I did find that mail as I was looking through the changes and it was
helpful.
> To summarize, I made the following changes:
>
> 1. Changed sys_msgctl() and semctl_down() so that an IPC_SET_PERM
> record is emitted in the failure case as well as the success case.
> This matches the behavior in sys_shmctl(). I could simplify the
> code in sys_msgctl() and semctl_down() slightly but it would mean
> that in some error cases we could get an IPC_SET_PERM record
> without an IPC record and that seemed odd.
I think this is ok.
> 2. No change to the IPC record type, given no feedback on the backward
> compatibility question.
I'm not one to speak authoritatively about compatibility issues... But I
do prefer the more descriptive AUDIT_IPC_SET_PERM type, as it
more accurately explains what the record is. Someone might complain
at some point about changing the record type, but there will be
considerably more invasive API changes elsewhere between the 2.6.5 and
the 2.6.16 kernels (and the RHEL4/RHEL5 kernels).
> 3. Removed the qbytes field from the IPC record. It wasn't being
> set and when audit_ipc_obj() is called from ipcperms(), the
> information isn't available. If we want the information in the IPC
> record, more extensive changes will be necessary. Since it only
> applies to message queues and it isn't really permission related, it
> doesn't seem worth it.
I agree with you here. However, I resisted the urge to remove the
qbytes field when I reworked the ipc audit code as I did not know
__why__ this was being saved. I assumed this was required by CAPP for
one reason or another. I personally don't find it a very interesting
field to capture. But I encourage you to review this with Klaus (or
another of the CAPP/LSPP experts).
I left it in the AUDIT_IPC_SET_PERM record but removed it from the IPC
record. The reason I can see for having it in the AUDIT_IPC_SET_PERM
record is that depending on the user and what the user is trying to set it to,
it can cause an EPERM. In that case it might be helpful to see the value
the users was attempting to set it to in the AUDIT_IPC_SET_PERM record, but
the current value seems less interesting. For CAPP, we only recorded the
new values. They were captured in the IPC record. In any case, I have asked
for our evaluator's opinion as well.
> 4. Removed the obj field from the IPC_SET_PERM record. This means that
> the kern_ipc_perm argument is no longer needed.
Hmm... This is probably ok, as long as you can __guarantee__ that an
IPC record will always closely follow an IPC_SET_PERM (and can be
associated together). The object label is needed for LSPP. If that
can be found in an associated record, so be it. Looking at your
example results, it seems ok.
I believe that's true. I could have cleaned up the code slightly but that
would have introduced cases where we could get an IPC_SET_PERM without
the IPC record and I wanted to avoid that.
> 5. Replaced the spaces in the IPC_SET_PERM field names with underscores.
Thanks, that was my oversight. There should definitely be underscores
rather than spaces.
I believe an early iteration of your patch did have underscores but somehow
they got turned into spaces during some rework.
Thanks again for the comments.
-- ljk
:-Dustin