* Re: [PATCH 05/22] audit: Fix sleep in atomic
From: Paul Moore @ 2017-01-05 2:14 UTC (permalink / raw)
To: Jan Kara
Cc: linux-fsdevel, Amir Goldstein, Lino Sanfilippo, Miklos Szeredi,
linux-audit
In-Reply-To: <20170104085002.GI3780@quack2.suse.cz>
On Wed, Jan 4, 2017 at 3:50 AM, Jan Kara <jack@suse.cz> wrote:
> On Tue 03-01-17 16:11:16, Paul Moore wrote:
>> On Mon, Jan 2, 2017 at 1:21 PM, Jan Kara <jack@suse.cz> wrote:
>> > So I found where the problem was. Attached is a new version of the patch.
>> > Tests from audit-testsuite fail for me but do not hang anymore. I guess the
>> > failing is because I don't have audit or selinux configured in any way and
>> > I'm using SUSE I guess (if there's some easy way to do that, I'd be
>> > interested) - runtests.pl complains that I have to be root although I am...
>>
>> I've never tried running the tests on SUSE, but if audit and SELinux
>> are in some undetermined state, then I can only imagine what wierd
>> test results you would get.
>
> Well, the state is well determined - nothing is installed ;) I was kind of
> hoping kernel support would be enough but apparently the testsuite needs
> some userspace installed & configured as well.
Heh, yes :) If nothing is installed you likely missing all the audit
userspace tools and auditd probably isn't running; either case would
cause massive (complete?) failures in the testsuite.
>> I'm building a test kernel as I type this, I'll report back when I have
>> some results.
>
> Thanks!
Good news - I looped the testsuite a couple thousand times this
afternoon and the VM was still standing afterwards (on a 4.10-rc2 base
too!).
Let me take a closer look at your revised patch tomorrow (it is
getting late for me) and if all is well I'll send it up to Linus.
>> Also, while I'm sure you've heard this before (and likely already know
>> better), please send patches inline, it makes review/commenting much
>> easier.
>
> Actually, I haven't heard this for quite a long time :) and I myself
> prefer attached patches (with text/plain attachment type) when they are
> in a reply to another email - they are easier to extract and at least my
> mail client automatically inlines them when I hit reply... Arguably the
> best of both worlds is to use git-send-email with properly set threading
> but I tend to forget about that option.
You probably haven't heard this in some time because everyone else
isn't as cranky and stubborn as me ;) It isn't a big deal for one
small patch with only a few interested parties, but when you get
several people discussing the patch it can be very handy to have it
inline.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 06/22] audit: Abstract hash key handling
From: Paul Moore @ 2017-01-05 2:06 UTC (permalink / raw)
To: Jan Kara
Cc: linux-fsdevel, Amir Goldstein, Lino Sanfilippo, Miklos Szeredi,
linux-audit
In-Reply-To: <20170103173419.GE3780@quack2.suse.cz>
On Tue, Jan 3, 2017 at 12:34 PM, Jan Kara <jack@suse.cz> wrote:
> On Fri 23-12-16 09:13:55, Paul Moore wrote:
>> On Fri, Dec 23, 2016 at 8:27 AM, Jan Kara <jack@suse.cz> wrote:
>> > On Thu 22-12-16 18:27:40, Paul Moore wrote:
>> >> On Thu, Dec 22, 2016 at 4:15 AM, Jan Kara <jack@suse.cz> wrote:
>> >> > Audit tree currently uses inode pointer as a key into the hash table.
>> >> > Getting that from notification mark will be somewhat more difficult with
>> >> > coming fsnotify changes and there's no reason we really have to use the
>> >> > inode pointer. So abstract getting of hash key from the audit chunk and
>> >> > inode so that we can switch to a different key easily later.
>> >> >
>> >> > CC: Paul Moore <paul@paul-moore.com>
>> >> > Signed-off-by: Jan Kara <jack@suse.cz>
>> >> > ---
>> >> > kernel/audit_tree.c | 39 ++++++++++++++++++++++++++++-----------
>> >> > 1 file changed, 28 insertions(+), 11 deletions(-)
>> >>
>> >> I have no objections with this patch in particular, but in patch 8,
>> >> are you certain that inode_to_key() and chunk_to_key() will continue
>> >> to return the same key value?
>> >
>> > Yes, that's the intention. Or better in that patch the key will no longer
>> > be inode pointer but instead the fsnotify_list pointer. But still it would
>> > match for chunks attached to an inode and inode itself so comparison
>> > results should stay the same.
>>
>> My apologies, I probably should have been more clear.
>>
>> Yes, I think we are all in agreement that the *_to_key() functions
>> need to return a consistent value for the same object. My concern is
>> that in patch 8 these functions are using different variables (granted
>> they may contain the same value, and therefore evaluate to the same
>> key) and I worry that there is a possibility of the two variables
>> taking on different values and breaking the hash. What guarantees
>> exist that these values will be the same? Are there any safeguards to
>> prevent future patches from accidentally sidestepping these
>> guarantees?
>
> Ah, OK, so this is more about patch 8 than patch 6. So far audit uses inode
> pointer as a key - now with patch 8, there is a fsnotify_mark_list attached
> to an inode if and only if there is any fsnotify_mark for that inode and
> both inode->i_fsnotify_marks (used as a key in inode_to_key()) and
> mark->obj_list_head (used as a key in chunk_to_key()) point to it. So keys
> for an inode and chunk match if and only if the fsnotify mark in the chunk
> is attached to the inode - the same as before patch 8.
>
> The only reason why I changed audit to use a different pointer for the key
> is that you need some lock protection to do mark->obj_list_head->inode
> dereference and this seemed the easiest. Actually now that all the lifetime
> rules have worked out, I can see we can actually use inode pointer as a key
> relatively easily since mark->obj_list_head is stable once you hold a mark
> reference so locking would be only intermediate step until this gets
> established in the series. Would you prefer me to do that?
Unless you can think of any reason why that would be dangerous, I
think it would be more obvious and easier to maintain as a result.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 05/22] audit: Fix sleep in atomic
From: Jan Kara @ 2017-01-04 8:50 UTC (permalink / raw)
To: Paul Moore
Cc: Jan Kara, linux-fsdevel, Amir Goldstein, Lino Sanfilippo,
Miklos Szeredi, linux-audit
In-Reply-To: <CAHC9VhTarSPE2QZ8xMdwmQ=r_2f0KL8Mum6PtrSEyEVPuq_gjg@mail.gmail.com>
On Tue 03-01-17 16:11:16, Paul Moore wrote:
> On Mon, Jan 2, 2017 at 1:21 PM, Jan Kara <jack@suse.cz> wrote:
> > So I found where the problem was. Attached is a new version of the patch.
> > Tests from audit-testsuite fail for me but do not hang anymore. I guess the
> > failing is because I don't have audit or selinux configured in any way and
> > I'm using SUSE I guess (if there's some easy way to do that, I'd be
> > interested) - runtests.pl complains that I have to be root although I am...
>
> I've never tried running the tests on SUSE, but if audit and SELinux
> are in some undetermined state, then I can only imagine what wierd
> test results you would get.
Well, the state is well determined - nothing is installed ;) I was kind of
hoping kernel support would be enough but apparently the testsuite needs
some userspace installed & configured as well.
> I'm building a test kernel as I type this, I'll report back when I have
> some results.
Thanks!
> Also, while I'm sure you've heard this before (and likely already know
> better), please send patches inline, it makes review/commenting much
> easier.
Actually, I haven't heard this for quite a long time :) and I myself
prefer attached patches (with text/plain attachment type) when they are
in a reply to another email - they are easier to extract and at least my
mail client automatically inlines them when I hit reply... Arguably the
best of both worlds is to use git-send-email with properly set threading
but I tend to forget about that option.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions
From: Kees Cook @ 2017-01-04 6:31 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Tyler Hicks, Paul Moore, linux-audit, Will Drewry, LKML,
Andy Lutomirski
In-Reply-To: <20170104044335.GA20124@madcap2.tricolour.ca>
On Tue, Jan 3, 2017 at 8:43 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2017-01-04 08:58, Tyler Hicks wrote:
>> On 01/04/2017 04:44 AM, Kees Cook wrote:
>> > On Tue, Jan 3, 2017 at 1:31 PM, Paul Moore <paul@paul-moore.com> wrote:
>> >> On Tue, Jan 3, 2017 at 4:21 PM, Kees Cook <keescook@chromium.org> wrote:
>> >>> On Tue, Jan 3, 2017 at 1:13 PM, Paul Moore <paul@paul-moore.com> wrote:
>> >>>> On Tue, Jan 3, 2017 at 4:03 PM, Kees Cook <keescook@chromium.org> wrote:
>> >>>>> On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul@paul-moore.com> wrote:
>> >>>>>> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook@chromium.org> wrote:
>> >>>>>>> I still wonder, though, isn't there a way to use auditctl to get all
>> >>>>>>> the seccomp messages you need?
>> >>>>>>
>> >>>>>> Not all of the seccomp actions are currently logged, that's one of the
>> >>>>>> problems (and the biggest at the moment).
>> >>>>>
>> >>>>> Well... sort of. It all gets passed around, but the logic isn't very
>> >>>>> obvious (or at least I always have to go look it up).
>> >>>>
>> >>>> Last time I checked SECCOMP_RET_ALLOW wasn't logged (as well as at
>> >>>> least one other action, but I can't remember which off the top of my
>> >>>> head)?
>> >>>
>> >>> Sure, but if you're using audit, you don't need RET_ALLOW to be logged
>> >>> because you'll get a full syscall log entry. Logging RET_ALLOW is
>> >>> redundant and provides no new information, it seems to me.
>> >>
>> >> I only bring this up as it might be a way to help solve the
>> >> SECCOMP_RET_AUDIT problem that Tyler mentioned.
>> >
>> > So, I guess I want to understand why something like this doesn't work,
>> > with no changes at all to the kernel:
>> >
>> > Imaginary "seccomp-audit.c":
>> >
>> > ...
>> > pid = fork();
>> > if (pid) {
>> > char cmd[80];
>> >
>> > sprintf(cmd, "auditctl -a always,exit -S all -F pid=%d", pid);
>> > system(cmd);
>> > release...
>> > } else {
>> > wait for release...
>> > execv(argv[1], argv + 1);
>> > }
>> > ...
>> >
>> > This should dump all syscalls (both RET_ALLOW and RET_ERRNO), as well
>> > as all seccomp actions of any kind. (Down side is the need for root to
>> > launch auditctl...)
>>
>> Hey Kees - Thanks for the suggestion!
>>
>> Here are some of the reasons that it doesn't quite work:
>>
>> 1) We don't install/run auditd by default and would continue to prefer
>> not to in some situations where resources are tight.
Strictly speaking, auditd isn't needed for auditctl, IIUC.
>> 2) We block a relatively small number of syscalls as compared to what
>> are allowed so auditing all syscalls is a really heavyweight solution.
>> That could be fixed with a better -S argument, though.
Yeah, it seems like there needs to be some kind of improvement there
on the audit side (I was thinking a better -F). The all-or-nothing
approach is way too big a hammer.
>> 3) We sometimes only block certain arguments for a given syscall and
>> auditing all instances of the syscall could still be a heavyweight solution.
>>
>> 4) If the application spawns children processes, that rule doesn't audit
>> their syscalls. That can be fixed with ppid=%d but then grandchildren
>> pids are a problem.
>
> This patch that wasn't accepted upstream might be useful:
> https://www.redhat.com/archives/linux-audit/2015-August/msg00067.html
> https://www.redhat.com/archives/linux-audit/2015-August/msg00068.html
I'd like this regardless. It's really difficult to audit trees of
processes before they launch. :)
>
>> 5) Cleanup of the audit rule for an old pid, before the pid is reused,
>> could be difficult.
>>
>> Tyler
>>
>> > Perhaps an improvement to this could be enabling audit when seccomp
>> > syscall is seen? I can't tell if auditctl already has something to do
>> > this ("start auditing this process and all children when syscall X is
>> > performed").
>> >
>> > -Kees
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@redhat.com>
> Kernel Security Engineering, Base Operating Systems, Red Hat
> Remote, Ottawa, Canada
> Voice: +1.647.777.2635, Internal: (81) 32635
-Kees
--
Kees Cook
Nexus Security
^ permalink raw reply
* Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions
From: Richard Guy Briggs @ 2017-01-04 4:43 UTC (permalink / raw)
To: Tyler Hicks; +Cc: Will Drewry, LKML, Andy Lutomirski, linux-audit
In-Reply-To: <3d1890e7-bef4-91e3-5d4c-cc5d4786d472@canonical.com>
On 2017-01-04 08:58, Tyler Hicks wrote:
> On 01/04/2017 04:44 AM, Kees Cook wrote:
> > On Tue, Jan 3, 2017 at 1:31 PM, Paul Moore <paul@paul-moore.com> wrote:
> >> On Tue, Jan 3, 2017 at 4:21 PM, Kees Cook <keescook@chromium.org> wrote:
> >>> On Tue, Jan 3, 2017 at 1:13 PM, Paul Moore <paul@paul-moore.com> wrote:
> >>>> On Tue, Jan 3, 2017 at 4:03 PM, Kees Cook <keescook@chromium.org> wrote:
> >>>>> On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul@paul-moore.com> wrote:
> >>>>>> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook@chromium.org> wrote:
> >>>>>>> I still wonder, though, isn't there a way to use auditctl to get all
> >>>>>>> the seccomp messages you need?
> >>>>>>
> >>>>>> Not all of the seccomp actions are currently logged, that's one of the
> >>>>>> problems (and the biggest at the moment).
> >>>>>
> >>>>> Well... sort of. It all gets passed around, but the logic isn't very
> >>>>> obvious (or at least I always have to go look it up).
> >>>>
> >>>> Last time I checked SECCOMP_RET_ALLOW wasn't logged (as well as at
> >>>> least one other action, but I can't remember which off the top of my
> >>>> head)?
> >>>
> >>> Sure, but if you're using audit, you don't need RET_ALLOW to be logged
> >>> because you'll get a full syscall log entry. Logging RET_ALLOW is
> >>> redundant and provides no new information, it seems to me.
> >>
> >> I only bring this up as it might be a way to help solve the
> >> SECCOMP_RET_AUDIT problem that Tyler mentioned.
> >
> > So, I guess I want to understand why something like this doesn't work,
> > with no changes at all to the kernel:
> >
> > Imaginary "seccomp-audit.c":
> >
> > ...
> > pid = fork();
> > if (pid) {
> > char cmd[80];
> >
> > sprintf(cmd, "auditctl -a always,exit -S all -F pid=%d", pid);
> > system(cmd);
> > release...
> > } else {
> > wait for release...
> > execv(argv[1], argv + 1);
> > }
> > ...
> >
> > This should dump all syscalls (both RET_ALLOW and RET_ERRNO), as well
> > as all seccomp actions of any kind. (Down side is the need for root to
> > launch auditctl...)
>
> Hey Kees - Thanks for the suggestion!
>
> Here are some of the reasons that it doesn't quite work:
>
> 1) We don't install/run auditd by default and would continue to prefer
> not to in some situations where resources are tight.
>
> 2) We block a relatively small number of syscalls as compared to what
> are allowed so auditing all syscalls is a really heavyweight solution.
> That could be fixed with a better -S argument, though.
>
> 3) We sometimes only block certain arguments for a given syscall and
> auditing all instances of the syscall could still be a heavyweight solution.
>
> 4) If the application spawns children processes, that rule doesn't audit
> their syscalls. That can be fixed with ppid=%d but then grandchildren
> pids are a problem.
This patch that wasn't accepted upstream might be useful:
https://www.redhat.com/archives/linux-audit/2015-August/msg00067.html
https://www.redhat.com/archives/linux-audit/2015-August/msg00068.html
> 5) Cleanup of the audit rule for an old pid, before the pid is reused,
> could be difficult.
>
> Tyler
>
> > Perhaps an improvement to this could be enabling audit when seccomp
> > syscall is seen? I can't tell if auditctl already has something to do
> > this ("start auditing this process and all children when syscall X is
> > performed").
> >
> > -Kees
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions
From: Tyler Hicks @ 2017-01-04 2:04 UTC (permalink / raw)
To: Paul Moore
Cc: Eric Paris, Kees Cook, Andy Lutomirski, Will Drewry, linux-audit,
linux-kernel
In-Reply-To: <CAHC9VhTzxCZAMWS+peTBV7ZssxUFeErngiSpZJ9AFe5wKC5rEA@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 4445 bytes --]
On 01/04/2017 02:42 AM, Paul Moore wrote:
> On Tue, Jan 3, 2017 at 8:31 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> On 01/02/2017 04:47 PM, Paul Moore wrote:
>>> On Mon, Jan 2, 2017 at 11:53 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
>>>> This patch set creates the basis for auditing information specific to a given
>>>> seccomp return action and then starts auditing SECCOMP_RET_ERRNO return
>>>> actions. The audit messages for SECCOMP_RET_ERRNO return actions include the
>>>> errno value that will be returned to userspace.
>>>
>>> I'm replying to this patchset posting because it his my inbox first,
>>> but my comments here apply to both this patchset and the other
>>> seccomp/audit patchset you posted.
>>>
>>> In my experience, we have two or three problems (the count varies
>>> depending on perspective) when it comes to seccomp filter reporting:
>>>
>>> 1. Inability to log all filter actions.
>>> 2. Inability to selectively enable filtering; e.g. devs want noisy
>>> logging, users want relative quiet.
>>> 3. Consistent behavior with audit enabled and disabled.
>>
>> Agreed. Those three logging issues are what have been nagging me the most.
>
> /me nods
>
>>> My current thinking - forgive me, this has been kicking around in my
>>> head for the better part of six months (longer?) and I haven't
>>> attempted to code it up - is to create a sysctl knob for a system wide
>>> seccomp logging threshold that would be applied to the high 16-bits of
>>> *every* triggered action: if the action was at/below the threshold a
>>> record would be emitted, otherwise silence. This should resolve
>>> problems #1 and #2, and the code should be relatively straightforward
>>> and small.
>>
>> I like that idea quite a bit. To be completely honest, for #1, I
>> personally only care about logging SECCOMP_RET_ERRNO actions but this
>> idea solves it in a nice and general way.
>
> Yeah, I'd much rather solve this problem generally; everybody has
> their favorite action and I'd like to avoid solving the same problem
> multiple times.
>
> Sooo ... you want to take a whack at coding this up? ;)
Yes, I can do that.
>
>>> As part of the code above, I expect that all seccomp logging would get
>>> routed through a single logging function (sort of like a better
>>> implementation of the existing audit_seccomp()) that would check the
>>> threshold and trigger the logging if needed. This function could be
>>> augmented to check for CONFIG_AUDIT and in the case where audit was
>>> not built into the kernel, a simple printk could be used to log the
>>> seccomp event; solving problem #3.
>>
>> That doesn't fully solve #3 for me. In Ubuntu (and I think Debian), we
>> build with CONFIG_AUDIT enabled but don't ship auditd by default so
>> audit_enabled is false. In that default configuration, we still want
>> seccomp audit messages to be printk'ed. I'll need to figure out how to
>> cleanly allow opting into seccomp audit messages when CONFIG_AUDIT is
>> enabled and audit_enabled is false.
>
> Heh, so you've got audit built into the kernel but you're not using
> it; that sounds "fun".
Users that need full auditing functionality can simply install auditd
but most users don't require it. Ubuntu has done it this way for many
years and the lack of seccomp auditing when auditd isn't running has
been the only problem that I can remember.
> Anyway, I think the logging consolidation could still help you, if for
> no other reason than everything is going through the same function at
> that point. We could do some other stuff there to handle the case
> where audit is compiled, but auditd is not running ... we already have
> some code in place to handle that for other reasons, check
> kernel/audit.c for more information. I'd still work on the other
> stuff first and then we can add this in at the end of the patchset.
/me nods. It is easy to continue to distro patch out the recently added
check for audit_enabled until a better solution for all distros can be
identified/upstreamed.
Thanks again!
Tyler
>
>>> We could also add a SECCOMP_RET_AUDIT, or similar, if we still feel
>>> that is important (I personally waffle on this), but I think that is
>>> independent of the ideas above.
>>
>> I agree that it is independent but SECCOMP_RET_AUDIT would still be
>> important to Ubuntu.
>>
>> Tyler
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions
From: Tyler Hicks @ 2017-01-04 1:58 UTC (permalink / raw)
To: Kees Cook, Paul Moore; +Cc: linux-audit, Will Drewry, LKML, Andy Lutomirski
In-Reply-To: <CAGXu5j+t1jp0TqrfHNOsKsNpHV2KnopKQvBML+DkokDFLzXxhw@mail.gmail.com>
[-- Attachment #1.1.1: Type: text/plain, Size: 3155 bytes --]
On 01/04/2017 04:44 AM, Kees Cook wrote:
> On Tue, Jan 3, 2017 at 1:31 PM, Paul Moore <paul@paul-moore.com> wrote:
>> On Tue, Jan 3, 2017 at 4:21 PM, Kees Cook <keescook@chromium.org> wrote:
>>> On Tue, Jan 3, 2017 at 1:13 PM, Paul Moore <paul@paul-moore.com> wrote:
>>>> On Tue, Jan 3, 2017 at 4:03 PM, Kees Cook <keescook@chromium.org> wrote:
>>>>> On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul@paul-moore.com> wrote:
>>>>>> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook@chromium.org> wrote:
>>>>>>> I still wonder, though, isn't there a way to use auditctl to get all
>>>>>>> the seccomp messages you need?
>>>>>>
>>>>>> Not all of the seccomp actions are currently logged, that's one of the
>>>>>> problems (and the biggest at the moment).
>>>>>
>>>>> Well... sort of. It all gets passed around, but the logic isn't very
>>>>> obvious (or at least I always have to go look it up).
>>>>
>>>> Last time I checked SECCOMP_RET_ALLOW wasn't logged (as well as at
>>>> least one other action, but I can't remember which off the top of my
>>>> head)?
>>>
>>> Sure, but if you're using audit, you don't need RET_ALLOW to be logged
>>> because you'll get a full syscall log entry. Logging RET_ALLOW is
>>> redundant and provides no new information, it seems to me.
>>
>> I only bring this up as it might be a way to help solve the
>> SECCOMP_RET_AUDIT problem that Tyler mentioned.
>
> So, I guess I want to understand why something like this doesn't work,
> with no changes at all to the kernel:
>
> Imaginary "seccomp-audit.c":
>
> ...
> pid = fork();
> if (pid) {
> char cmd[80];
>
> sprintf(cmd, "auditctl -a always,exit -S all -F pid=%d", pid);
> system(cmd);
> release...
> } else {
> wait for release...
> execv(argv[1], argv + 1);
> }
> ...
>
> This should dump all syscalls (both RET_ALLOW and RET_ERRNO), as well
> as all seccomp actions of any kind. (Down side is the need for root to
> launch auditctl...)
Hey Kees - Thanks for the suggestion!
Here are some of the reasons that it doesn't quite work:
1) We don't install/run auditd by default and would continue to prefer
not to in some situations where resources are tight.
2) We block a relatively small number of syscalls as compared to what
are allowed so auditing all syscalls is a really heavyweight solution.
That could be fixed with a better -S argument, though.
3) We sometimes only block certain arguments for a given syscall and
auditing all instances of the syscall could still be a heavyweight solution.
4) If the application spawns children processes, that rule doesn't audit
their syscalls. That can be fixed with ppid=%d but then grandchildren
pids are a problem.
5) Cleanup of the audit rule for an old pid, before the pid is reused,
could be difficult.
Tyler
>
> Perhaps an improvement to this could be enabling audit when seccomp
> syscall is seen? I can't tell if auditctl already has something to do
> this ("start auditing this process and all children when syscall X is
> performed").
>
> -Kees
>
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions
From: Kees Cook @ 2017-01-03 21:44 UTC (permalink / raw)
To: Paul Moore; +Cc: Will Drewry, LKML, Andy Lutomirski, linux-audit
In-Reply-To: <CAHC9VhS8MUE4nfNoKBU9khe3kc9QbN7vOY3aWFSNzU=GjZQsmA@mail.gmail.com>
On Tue, Jan 3, 2017 at 1:31 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jan 3, 2017 at 4:21 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Tue, Jan 3, 2017 at 1:13 PM, Paul Moore <paul@paul-moore.com> wrote:
>>> On Tue, Jan 3, 2017 at 4:03 PM, Kees Cook <keescook@chromium.org> wrote:
>>>> On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul@paul-moore.com> wrote:
>>>>> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook@chromium.org> wrote:
>>>>>> I still wonder, though, isn't there a way to use auditctl to get all
>>>>>> the seccomp messages you need?
>>>>>
>>>>> Not all of the seccomp actions are currently logged, that's one of the
>>>>> problems (and the biggest at the moment).
>>>>
>>>> Well... sort of. It all gets passed around, but the logic isn't very
>>>> obvious (or at least I always have to go look it up).
>>>
>>> Last time I checked SECCOMP_RET_ALLOW wasn't logged (as well as at
>>> least one other action, but I can't remember which off the top of my
>>> head)?
>>
>> Sure, but if you're using audit, you don't need RET_ALLOW to be logged
>> because you'll get a full syscall log entry. Logging RET_ALLOW is
>> redundant and provides no new information, it seems to me.
>
> I only bring this up as it might be a way to help solve the
> SECCOMP_RET_AUDIT problem that Tyler mentioned.
So, I guess I want to understand why something like this doesn't work,
with no changes at all to the kernel:
Imaginary "seccomp-audit.c":
...
pid = fork();
if (pid) {
char cmd[80];
sprintf(cmd, "auditctl -a always,exit -S all -F pid=%d", pid);
system(cmd);
release...
} else {
wait for release...
execv(argv[1], argv + 1);
}
...
This should dump all syscalls (both RET_ALLOW and RET_ERRNO), as well
as all seccomp actions of any kind. (Down side is the need for root to
launch auditctl...)
Perhaps an improvement to this could be enabling audit when seccomp
syscall is seen? I can't tell if auditctl already has something to do
this ("start auditing this process and all children when syscall X is
performed").
-Kees
--
Kees Cook
Nexus Security
^ permalink raw reply
* Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions
From: Paul Moore @ 2017-01-03 21:31 UTC (permalink / raw)
To: Kees Cook
Cc: Tyler Hicks, Eric Paris, Andy Lutomirski, Will Drewry,
linux-audit, LKML
In-Reply-To: <CAGXu5jK2MNHJf2uVo0UeWTDUMp=p6a9YXK3UAEnJ51pZnzF7Bg@mail.gmail.com>
On Tue, Jan 3, 2017 at 4:21 PM, Kees Cook <keescook@chromium.org> wrote:
> On Tue, Jan 3, 2017 at 1:13 PM, Paul Moore <paul@paul-moore.com> wrote:
>> On Tue, Jan 3, 2017 at 4:03 PM, Kees Cook <keescook@chromium.org> wrote:
>>> On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul@paul-moore.com> wrote:
>>>> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook@chromium.org> wrote:
>>>>> I still wonder, though, isn't there a way to use auditctl to get all
>>>>> the seccomp messages you need?
>>>>
>>>> Not all of the seccomp actions are currently logged, that's one of the
>>>> problems (and the biggest at the moment).
>>>
>>> Well... sort of. It all gets passed around, but the logic isn't very
>>> obvious (or at least I always have to go look it up).
>>
>> Last time I checked SECCOMP_RET_ALLOW wasn't logged (as well as at
>> least one other action, but I can't remember which off the top of my
>> head)?
>
> Sure, but if you're using audit, you don't need RET_ALLOW to be logged
> because you'll get a full syscall log entry. Logging RET_ALLOW is
> redundant and provides no new information, it seems to me.
I only bring this up as it might be a way to help solve the
SECCOMP_RET_AUDIT problem that Tyler mentioned.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions
From: Kees Cook @ 2017-01-03 21:21 UTC (permalink / raw)
To: Paul Moore
Cc: Tyler Hicks, Eric Paris, Andy Lutomirski, Will Drewry,
linux-audit, LKML
In-Reply-To: <CAHC9VhR1M=LgYF0esbHKqQJnS7s8Wqj4UYRa14hyf_7HuJP5PA@mail.gmail.com>
On Tue, Jan 3, 2017 at 1:13 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jan 3, 2017 at 4:03 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul@paul-moore.com> wrote:
>>> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook@chromium.org> wrote:
>>>> I still wonder, though, isn't there a way to use auditctl to get all
>>>> the seccomp messages you need?
>>>
>>> Not all of the seccomp actions are currently logged, that's one of the
>>> problems (and the biggest at the moment).
>>
>> Well... sort of. It all gets passed around, but the logic isn't very
>> obvious (or at least I always have to go look it up).
>
> Last time I checked SECCOMP_RET_ALLOW wasn't logged (as well as at
> least one other action, but I can't remember which off the top of my
> head)?
Sure, but if you're using audit, you don't need RET_ALLOW to be logged
because you'll get a full syscall log entry. Logging RET_ALLOW is
redundant and provides no new information, it seems to me.
-Kees
--
Kees Cook
Nexus Security
^ permalink raw reply
* Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions
From: Paul Moore @ 2017-01-03 21:13 UTC (permalink / raw)
To: Kees Cook; +Cc: Will Drewry, LKML, Andy Lutomirski, linux-audit
In-Reply-To: <CAGXu5j+ZkOzJ4vkKzFtwa7+6AWvP3h+2zF39Fta9aM0T8zJMaQ@mail.gmail.com>
On Tue, Jan 3, 2017 at 4:03 PM, Kees Cook <keescook@chromium.org> wrote:
> On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul@paul-moore.com> wrote:
>> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook@chromium.org> wrote:
>>> I still wonder, though, isn't there a way to use auditctl to get all
>>> the seccomp messages you need?
>>
>> Not all of the seccomp actions are currently logged, that's one of the
>> problems (and the biggest at the moment).
>
> Well... sort of. It all gets passed around, but the logic isn't very
> obvious (or at least I always have to go look it up).
Last time I checked SECCOMP_RET_ALLOW wasn't logged (as well as at
least one other action, but I can't remember which off the top of my
head)?
> include/linux/audit.h:
>
> #ifdef CONFIG_AUDITSYSCALL
> ...
> static inline void audit_seccomp(unsigned long syscall, long signr, int code)
> {
> if (!audit_enabled)
> return;
>
> /* Force a record to be reported if a signal was delivered. */
> if (signr || unlikely(!audit_dummy_context()))
> __audit_seccomp(syscall, signr, code);
> }
> ...
> #else /* CONFIG_AUDITSYSCALL */
>
> static inline void audit_seccomp(unsigned long syscall, long signr, int code)
> { }
> ...
> #endif
>
> kernel/seccomp.c:
>
> switch (action) {
> case SECCOMP_RET_ERRNO:
> /* Set low-order bits as an errno, capped at MAX_ERRNO. */
> if (data > MAX_ERRNO)
> data = MAX_ERRNO;
> syscall_set_return_value(current, task_pt_regs(current),
> -data, 0);
> goto skip;
> ...
> case SECCOMP_RET_KILL:
> default:
> audit_seccomp(this_syscall, SIGSYS, action);
> do_exit(SIGSYS);
> }
>
> unreachable();
>
> skip:
> audit_seccomp(this_syscall, 0, action);
>
>
> Current state:
>
> - if CONFIG_AUDITSYSCALL=n, then nothing is ever reported.
>
> - if audit is disabled, nothing is ever reported.
>
> - if a process isn't being specifically audited
> (!audit_dummy_context()), only signals (RET_KILL) are reported.
>
> - when being specifically audited, everything is reported.
>
>
> So, shouldn't it be possible to specifically audit a process and
> examine the resulting logs for the RET_* level one is interested in
> ("code=0x%x" in __audit_seccomp())?
>
> -Kees
>
> --
> Kees Cook
> Nexus Security
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 05/22] audit: Fix sleep in atomic
From: Paul Moore @ 2017-01-03 21:11 UTC (permalink / raw)
To: Jan Kara
Cc: linux-fsdevel, linux-audit, Amir Goldstein, Lino Sanfilippo,
Miklos Szeredi
In-Reply-To: <20170102182126.GB23612@quack2.suse.cz>
On Mon, Jan 2, 2017 at 1:21 PM, Jan Kara <jack@suse.cz> wrote:
> So I found where the problem was. Attached is a new version of the patch.
> Tests from audit-testsuite fail for me but do not hang anymore. I guess the
> failing is because I don't have audit or selinux configured in any way and
> I'm using SUSE I guess (if there's some easy way to do that, I'd be
> interested) - runtests.pl complains that I have to be root although I am...
I've never tried running the tests on SUSE, but if audit and SELinux
are in some undetermined state, then I can only imagine what wierd
test results you would get. I'm building a test kernel as I type
this, I'll report back when I have some results.
Also, while I'm sure you've heard this before (and likely already know
better), please send patches inline, it makes review/commenting much
easier.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions
From: Kees Cook @ 2017-01-03 21:03 UTC (permalink / raw)
To: Paul Moore; +Cc: Will Drewry, LKML, Andy Lutomirski, linux-audit
In-Reply-To: <CAHC9VhTynMLhZzqMckJGhVpTVZNk7VRNpC3gEZvSHvbx9j9vAw@mail.gmail.com>
On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook@chromium.org> wrote:
>> I still wonder, though, isn't there a way to use auditctl to get all
>> the seccomp messages you need?
>
> Not all of the seccomp actions are currently logged, that's one of the
> problems (and the biggest at the moment).
Well... sort of. It all gets passed around, but the logic isn't very
obvious (or at least I always have to go look it up).
include/linux/audit.h:
#ifdef CONFIG_AUDITSYSCALL
...
static inline void audit_seccomp(unsigned long syscall, long signr, int code)
{
if (!audit_enabled)
return;
/* Force a record to be reported if a signal was delivered. */
if (signr || unlikely(!audit_dummy_context()))
__audit_seccomp(syscall, signr, code);
}
...
#else /* CONFIG_AUDITSYSCALL */
static inline void audit_seccomp(unsigned long syscall, long signr, int code)
{ }
...
#endif
kernel/seccomp.c:
switch (action) {
case SECCOMP_RET_ERRNO:
/* Set low-order bits as an errno, capped at MAX_ERRNO. */
if (data > MAX_ERRNO)
data = MAX_ERRNO;
syscall_set_return_value(current, task_pt_regs(current),
-data, 0);
goto skip;
...
case SECCOMP_RET_KILL:
default:
audit_seccomp(this_syscall, SIGSYS, action);
do_exit(SIGSYS);
}
unreachable();
skip:
audit_seccomp(this_syscall, 0, action);
Current state:
- if CONFIG_AUDITSYSCALL=n, then nothing is ever reported.
- if audit is disabled, nothing is ever reported.
- if a process isn't being specifically audited
(!audit_dummy_context()), only signals (RET_KILL) are reported.
- when being specifically audited, everything is reported.
So, shouldn't it be possible to specifically audit a process and
examine the resulting logs for the RET_* level one is interested in
("code=0x%x" in __audit_seccomp())?
-Kees
--
Kees Cook
Nexus Security
^ permalink raw reply
* Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions
From: Paul Moore @ 2017-01-03 20:54 UTC (permalink / raw)
To: Kees Cook; +Cc: Will Drewry, LKML, Andy Lutomirski, linux-audit
In-Reply-To: <CAGXu5jLJd+JufiKd3azXgg1C-7or50BP_ShNq6VzR67J2PQe7w@mail.gmail.com>
On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook@chromium.org> wrote:
> I still wonder, though, isn't there a way to use auditctl to get all
> the seccomp messages you need?
Not all of the seccomp actions are currently logged, that's one of the
problems (and the biggest at the moment).
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions
From: Steve Grubb @ 2017-01-03 20:53 UTC (permalink / raw)
To: linux-audit; +Cc: Kees Cook, Paul Moore, Will Drewry, LKML, Andy Lutomirski
In-Reply-To: <CAGXu5jLJd+JufiKd3azXgg1C-7or50BP_ShNq6VzR67J2PQe7w@mail.gmail.com>
On Tuesday, January 3, 2017 12:44:41 PM EST Kees Cook wrote:
> >> That doesn't fully solve #3 for me. In Ubuntu (and I think Debian), we
> >> build with CONFIG_AUDIT enabled but don't ship auditd by default so
> >> audit_enabled is false. In that default configuration, we still want
> >> seccomp audit messages to be printk'ed. I'll need to figure out how to
> >> cleanly allow opting into seccomp audit messages when CONFIG_AUDIT is
> >> enabled and audit_enabled is false.
> >
> > Heh, so you've got audit built into the kernel but you're not using
> > it; that sounds "fun".
> >
> > Anyway, I think the logging consolidation could still help you, if for
> > no other reason than everything is going through the same function at
> > that point. We could do some other stuff there to handle the case
> > where audit is compiled, but auditd is not running ... we already have
> > some code in place to handle that for other reasons, check
> > kernel/audit.c for more information. I'd still work on the other
> > stuff first and then we can add this in at the end of the patchset.
>
> Yeah, I think the "should I report it?" threshold sysctl could just
> check if audit is enabled...
>
> I still wonder, though, isn't there a way to use auditctl to get all
> the seccomp messages you need?
If you do "auditctl -e 1" then auditing will be enabled and it will send
events to syslog if the audit daemon is not running.
-Steve
^ permalink raw reply
* Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions
From: Kees Cook @ 2017-01-03 20:44 UTC (permalink / raw)
To: Paul Moore; +Cc: Will Drewry, LKML, Andy Lutomirski, linux-audit
In-Reply-To: <CAHC9VhTzxCZAMWS+peTBV7ZssxUFeErngiSpZJ9AFe5wKC5rEA@mail.gmail.com>
On Tue, Jan 3, 2017 at 11:42 AM, Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jan 3, 2017 at 8:31 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> On 01/02/2017 04:47 PM, Paul Moore wrote:
>>> On Mon, Jan 2, 2017 at 11:53 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
>>>> This patch set creates the basis for auditing information specific to a given
>>>> seccomp return action and then starts auditing SECCOMP_RET_ERRNO return
>>>> actions. The audit messages for SECCOMP_RET_ERRNO return actions include the
>>>> errno value that will be returned to userspace.
>>>
>>> I'm replying to this patchset posting because it his my inbox first,
>>> but my comments here apply to both this patchset and the other
>>> seccomp/audit patchset you posted.
>>>
>>> In my experience, we have two or three problems (the count varies
>>> depending on perspective) when it comes to seccomp filter reporting:
>>>
>>> 1. Inability to log all filter actions.
>>> 2. Inability to selectively enable filtering; e.g. devs want noisy
>>> logging, users want relative quiet.
>>> 3. Consistent behavior with audit enabled and disabled.
>>
>> Agreed. Those three logging issues are what have been nagging me the most.
>
> /me nods
I think this sounds fine too.
>>> My current thinking - forgive me, this has been kicking around in my
>>> head for the better part of six months (longer?) and I haven't
>>> attempted to code it up - is to create a sysctl knob for a system wide
>>> seccomp logging threshold that would be applied to the high 16-bits of
>>> *every* triggered action: if the action was at/below the threshold a
>>> record would be emitted, otherwise silence. This should resolve
>>> problems #1 and #2, and the code should be relatively straightforward
>>> and small.
>>
>> I like that idea quite a bit. To be completely honest, for #1, I
>> personally only care about logging SECCOMP_RET_ERRNO actions but this
>> idea solves it in a nice and general way.
>
> Yeah, I'd much rather solve this problem generally; everybody has
> their favorite action and I'd like to avoid solving the same problem
> multiple times.
>
> Sooo ... you want to take a whack at coding this up? ;)
>
>>> As part of the code above, I expect that all seccomp logging would get
>>> routed through a single logging function (sort of like a better
>>> implementation of the existing audit_seccomp()) that would check the
>>> threshold and trigger the logging if needed. This function could be
>>> augmented to check for CONFIG_AUDIT and in the case where audit was
>>> not built into the kernel, a simple printk could be used to log the
>>> seccomp event; solving problem #3.
>>
>> That doesn't fully solve #3 for me. In Ubuntu (and I think Debian), we
>> build with CONFIG_AUDIT enabled but don't ship auditd by default so
>> audit_enabled is false. In that default configuration, we still want
>> seccomp audit messages to be printk'ed. I'll need to figure out how to
>> cleanly allow opting into seccomp audit messages when CONFIG_AUDIT is
>> enabled and audit_enabled is false.
>
> Heh, so you've got audit built into the kernel but you're not using
> it; that sounds "fun".
>
> Anyway, I think the logging consolidation could still help you, if for
> no other reason than everything is going through the same function at
> that point. We could do some other stuff there to handle the case
> where audit is compiled, but auditd is not running ... we already have
> some code in place to handle that for other reasons, check
> kernel/audit.c for more information. I'd still work on the other
> stuff first and then we can add this in at the end of the patchset.
Yeah, I think the "should I report it?" threshold sysctl could just
check if audit is enabled...
I still wonder, though, isn't there a way to use auditctl to get all
the seccomp messages you need?
-Kees
>
>>> We could also add a SECCOMP_RET_AUDIT, or similar, if we still feel
>>> that is important (I personally waffle on this), but I think that is
>>> independent of the ideas above.
>>
>> I agree that it is independent but SECCOMP_RET_AUDIT would still be
>> important to Ubuntu.
>>
>> Tyler
>
> --
> paul moore
> www.paul-moore.com
--
Kees Cook
Nexus Security
^ permalink raw reply
* Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions
From: Paul Moore @ 2017-01-03 19:42 UTC (permalink / raw)
To: Tyler Hicks; +Cc: Will Drewry, linux-kernel, Andy Lutomirski, linux-audit
In-Reply-To: <8748cee7-efe3-a603-ef2e-dc9077b6ead4@canonical.com>
On Tue, Jan 3, 2017 at 8:31 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
> On 01/02/2017 04:47 PM, Paul Moore wrote:
>> On Mon, Jan 2, 2017 at 11:53 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
>>> This patch set creates the basis for auditing information specific to a given
>>> seccomp return action and then starts auditing SECCOMP_RET_ERRNO return
>>> actions. The audit messages for SECCOMP_RET_ERRNO return actions include the
>>> errno value that will be returned to userspace.
>>
>> I'm replying to this patchset posting because it his my inbox first,
>> but my comments here apply to both this patchset and the other
>> seccomp/audit patchset you posted.
>>
>> In my experience, we have two or three problems (the count varies
>> depending on perspective) when it comes to seccomp filter reporting:
>>
>> 1. Inability to log all filter actions.
>> 2. Inability to selectively enable filtering; e.g. devs want noisy
>> logging, users want relative quiet.
>> 3. Consistent behavior with audit enabled and disabled.
>
> Agreed. Those three logging issues are what have been nagging me the most.
/me nods
>> My current thinking - forgive me, this has been kicking around in my
>> head for the better part of six months (longer?) and I haven't
>> attempted to code it up - is to create a sysctl knob for a system wide
>> seccomp logging threshold that would be applied to the high 16-bits of
>> *every* triggered action: if the action was at/below the threshold a
>> record would be emitted, otherwise silence. This should resolve
>> problems #1 and #2, and the code should be relatively straightforward
>> and small.
>
> I like that idea quite a bit. To be completely honest, for #1, I
> personally only care about logging SECCOMP_RET_ERRNO actions but this
> idea solves it in a nice and general way.
Yeah, I'd much rather solve this problem generally; everybody has
their favorite action and I'd like to avoid solving the same problem
multiple times.
Sooo ... you want to take a whack at coding this up? ;)
>> As part of the code above, I expect that all seccomp logging would get
>> routed through a single logging function (sort of like a better
>> implementation of the existing audit_seccomp()) that would check the
>> threshold and trigger the logging if needed. This function could be
>> augmented to check for CONFIG_AUDIT and in the case where audit was
>> not built into the kernel, a simple printk could be used to log the
>> seccomp event; solving problem #3.
>
> That doesn't fully solve #3 for me. In Ubuntu (and I think Debian), we
> build with CONFIG_AUDIT enabled but don't ship auditd by default so
> audit_enabled is false. In that default configuration, we still want
> seccomp audit messages to be printk'ed. I'll need to figure out how to
> cleanly allow opting into seccomp audit messages when CONFIG_AUDIT is
> enabled and audit_enabled is false.
Heh, so you've got audit built into the kernel but you're not using
it; that sounds "fun".
Anyway, I think the logging consolidation could still help you, if for
no other reason than everything is going through the same function at
that point. We could do some other stuff there to handle the case
where audit is compiled, but auditd is not running ... we already have
some code in place to handle that for other reasons, check
kernel/audit.c for more information. I'd still work on the other
stuff first and then we can add this in at the end of the patchset.
>> We could also add a SECCOMP_RET_AUDIT, or similar, if we still feel
>> that is important (I personally waffle on this), but I think that is
>> independent of the ideas above.
>
> I agree that it is independent but SECCOMP_RET_AUDIT would still be
> important to Ubuntu.
>
> Tyler
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions
From: Paul Moore @ 2017-01-03 19:31 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Tyler Hicks, Eric Paris, Kees Cook, Will Drewry, linux-audit,
linux-kernel@vger.kernel.org
In-Reply-To: <CALCETrViORew2PzXSPrCS+aqUnVTsatr85b05DPr9eG7RSGT+Q@mail.gmail.com>
On Tue, Jan 3, 2017 at 12:56 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Mon, Jan 2, 2017 at 2:47 PM, Paul Moore <paul@paul-moore.com> wrote:
>> On Mon, Jan 2, 2017 at 11:53 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
>>> This patch set creates the basis for auditing information specific to a given
>>> seccomp return action and then starts auditing SECCOMP_RET_ERRNO return
>>> actions. The audit messages for SECCOMP_RET_ERRNO return actions include the
>>> errno value that will be returned to userspace.
>>
>> I'm replying to this patchset posting because it his my inbox first,
>> but my comments here apply to both this patchset and the other
>> seccomp/audit patchset you posted.
>>
>> In my experience, we have two or three problems (the count varies
>> depending on perspective) when it comes to seccomp filter reporting:
>>
>> 1. Inability to log all filter actions.
>> 2. Inability to selectively enable filtering; e.g. devs want noisy
>> logging, users want relative quiet.
>> 3. Consistent behavior with audit enabled and disabled.
>>
>> My current thinking - forgive me, this has been kicking around in my
>> head for the better part of six months (longer?) and I haven't
>> attempted to code it up - is to create a sysctl knob for a system wide
>> seccomp logging threshold that would be applied to the high 16-bits of
>> *every* triggered action: if the action was at/below the threshold a
>> record would be emitted, otherwise silence. This should resolve
>> problems #1 and #2, and the code should be relatively straightforward
>> and small.
>>
>> As part of the code above, I expect that all seccomp logging would get
>> routed through a single logging function (sort of like a better
>> implementation of the existing audit_seccomp()) that would check the
>> threshold and trigger the logging if needed. This function could be
>> augmented to check for CONFIG_AUDIT and in the case where audit was
>> not built into the kernel, a simple printk could be used to log the
>> seccomp event; solving problem #3.
>
> Would this not be doable with a seccomp tracepoint and a BPF filter?
One of the motivations for the above idea is to make it easier for
admins/users to customize the seccomp logging on their own systems,
it's not just to make devs lives easier. I feel okay providing
guidance to an admin/user the involves setting a sysctl variable, I
can't say the same about asking them to write their own BPF ;)
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 06/22] audit: Abstract hash key handling
From: Jan Kara @ 2017-01-03 17:34 UTC (permalink / raw)
To: Paul Moore
Cc: Jan Kara, linux-fsdevel, Amir Goldstein, Lino Sanfilippo,
Miklos Szeredi, linux-audit
In-Reply-To: <CAHC9VhT3EbWgfR_G39ZV2UUm21+CUDnemNDUPgwYP_vj556D-g@mail.gmail.com>
On Fri 23-12-16 09:13:55, Paul Moore wrote:
> On Fri, Dec 23, 2016 at 8:27 AM, Jan Kara <jack@suse.cz> wrote:
> > On Thu 22-12-16 18:27:40, Paul Moore wrote:
> >> On Thu, Dec 22, 2016 at 4:15 AM, Jan Kara <jack@suse.cz> wrote:
> >> > Audit tree currently uses inode pointer as a key into the hash table.
> >> > Getting that from notification mark will be somewhat more difficult with
> >> > coming fsnotify changes and there's no reason we really have to use the
> >> > inode pointer. So abstract getting of hash key from the audit chunk and
> >> > inode so that we can switch to a different key easily later.
> >> >
> >> > CC: Paul Moore <paul@paul-moore.com>
> >> > Signed-off-by: Jan Kara <jack@suse.cz>
> >> > ---
> >> > kernel/audit_tree.c | 39 ++++++++++++++++++++++++++++-----------
> >> > 1 file changed, 28 insertions(+), 11 deletions(-)
> >>
> >> I have no objections with this patch in particular, but in patch 8,
> >> are you certain that inode_to_key() and chunk_to_key() will continue
> >> to return the same key value?
> >
> > Yes, that's the intention. Or better in that patch the key will no longer
> > be inode pointer but instead the fsnotify_list pointer. But still it would
> > match for chunks attached to an inode and inode itself so comparison
> > results should stay the same.
>
> My apologies, I probably should have been more clear.
>
> Yes, I think we are all in agreement that the *_to_key() functions
> need to return a consistent value for the same object. My concern is
> that in patch 8 these functions are using different variables (granted
> they may contain the same value, and therefore evaluate to the same
> key) and I worry that there is a possibility of the two variables
> taking on different values and breaking the hash. What guarantees
> exist that these values will be the same? Are there any safeguards to
> prevent future patches from accidentally sidestepping these
> guarantees?
Ah, OK, so this is more about patch 8 than patch 6. So far audit uses inode
pointer as a key - now with patch 8, there is a fsnotify_mark_list attached
to an inode if and only if there is any fsnotify_mark for that inode and
both inode->i_fsnotify_marks (used as a key in inode_to_key()) and
mark->obj_list_head (used as a key in chunk_to_key()) point to it. So keys
for an inode and chunk match if and only if the fsnotify mark in the chunk
is attached to the inode - the same as before patch 8.
The only reason why I changed audit to use a different pointer for the key
is that you need some lock protection to do mark->obj_list_head->inode
dereference and this seemed the easiest. Actually now that all the lifetime
rules have worked out, I can see we can actually use inode pointer as a key
relatively easily since mark->obj_list_head is stable once you hold a mark
reference so locking would be only intermediate step until this gets
established in the series. Would you prefer me to do that?
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions
From: Tyler Hicks @ 2017-01-03 13:53 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Paul Moore, Eric Paris, Kees Cook, Will Drewry, linux-audit,
linux-kernel@vger.kernel.org
In-Reply-To: <CALCETrW7sYuUG12DQzgj1pui16KufdBU_3kMw7s3g5oaPpebDg@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 1888 bytes --]
On 01/02/2017 11:57 PM, Andy Lutomirski wrote:
> On Mon, Jan 2, 2017 at 8:53 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> This patch set creates the basis for auditing information specific to a given
>> seccomp return action and then starts auditing SECCOMP_RET_ERRNO return
>> actions. The audit messages for SECCOMP_RET_ERRNO return actions include the
>> errno value that will be returned to userspace.
>>
>
> Not that I'm opposed to the idea, but what's the intended purpose?
Ubuntu has a security sandbox, which includes seccomp as a part of the
confinement strategy, that we're using to confine untrusted third-party
applications. Today, we're using SECCOMP_RET_KILL as the default action
when the applications make a call to a syscall that is not allowed by
the sandbox. It is great from a security perspective but not so great
from the perspective of the application developer as their application
(or in some cases, an interpretor) may work fine without the illegal
syscall but it doesn't get the chance to because it is killed.
In the near future, we want to switch over to using SECCOMP_RET_ERRNO
(the errno is still TBD) as the default action to improve the
application developer experience. The largest remaining blocker is that
there are no audit messages when a SECCOMP_RET_ERRNO action is taken.
Therefore, we can't suggest (to the application developer or to the
user) which sandbox knobs need to be turned to better suite their
application, we can't let the application developer know that a syscall
they're using is illegal outside of them having to debug an odd errno
value, and we can't let the user know of a potentially subverted process
that's under confinement of the sandbox. All of that could be addressed
if SECCOMP_RET_ERRNO actions generated audit messages.
I hope that helps to understand the use case.
Tyler
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions
From: Tyler Hicks @ 2017-01-03 13:31 UTC (permalink / raw)
To: Paul Moore; +Cc: Will Drewry, linux-kernel, Andy Lutomirski, linux-audit
In-Reply-To: <CAHC9VhSOwbY9WEOKZGsx4mf=MAXeudTnQF9nXmKu+OoAs0SDsQ@mail.gmail.com>
[-- Attachment #1.1.1: Type: text/plain, Size: 2872 bytes --]
On 01/02/2017 04:47 PM, Paul Moore wrote:
> On Mon, Jan 2, 2017 at 11:53 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> This patch set creates the basis for auditing information specific to a given
>> seccomp return action and then starts auditing SECCOMP_RET_ERRNO return
>> actions. The audit messages for SECCOMP_RET_ERRNO return actions include the
>> errno value that will be returned to userspace.
>
> I'm replying to this patchset posting because it his my inbox first,
> but my comments here apply to both this patchset and the other
> seccomp/audit patchset you posted.
>
> In my experience, we have two or three problems (the count varies
> depending on perspective) when it comes to seccomp filter reporting:
>
> 1. Inability to log all filter actions.
> 2. Inability to selectively enable filtering; e.g. devs want noisy
> logging, users want relative quiet.
> 3. Consistent behavior with audit enabled and disabled.
Agreed. Those three logging issues are what have been nagging me the most.
> My current thinking - forgive me, this has been kicking around in my
> head for the better part of six months (longer?) and I haven't
> attempted to code it up - is to create a sysctl knob for a system wide
> seccomp logging threshold that would be applied to the high 16-bits of
> *every* triggered action: if the action was at/below the threshold a
> record would be emitted, otherwise silence. This should resolve
> problems #1 and #2, and the code should be relatively straightforward
> and small.
I like that idea quite a bit. To be completely honest, for #1, I
personally only care about logging SECCOMP_RET_ERRNO actions but this
idea solves it in a nice and general way.
> As part of the code above, I expect that all seccomp logging would get
> routed through a single logging function (sort of like a better
> implementation of the existing audit_seccomp()) that would check the
> threshold and trigger the logging if needed. This function could be
> augmented to check for CONFIG_AUDIT and in the case where audit was
> not built into the kernel, a simple printk could be used to log the
> seccomp event; solving problem #3.
That doesn't fully solve #3 for me. In Ubuntu (and I think Debian), we
build with CONFIG_AUDIT enabled but don't ship auditd by default so
audit_enabled is false. In that default configuration, we still want
seccomp audit messages to be printk'ed. I'll need to figure out how to
cleanly allow opting into seccomp audit messages when CONFIG_AUDIT is
enabled and audit_enabled is false.
> We could also add a SECCOMP_RET_AUDIT, or similar, if we still feel
> that is important (I personally waffle on this), but I think that is
> independent of the ideas above.
I agree that it is independent but SECCOMP_RET_AUDIT would still be
important to Ubuntu.
Tyler
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions
From: Andy Lutomirski @ 2017-01-03 5:57 UTC (permalink / raw)
To: Tyler Hicks
Cc: Paul Moore, Eric Paris, Kees Cook, Will Drewry, linux-audit,
linux-kernel@vger.kernel.org
In-Reply-To: <1483375990-14948-1-git-send-email-tyhicks@canonical.com>
On Mon, Jan 2, 2017 at 8:53 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
> This patch set creates the basis for auditing information specific to a given
> seccomp return action and then starts auditing SECCOMP_RET_ERRNO return
> actions. The audit messages for SECCOMP_RET_ERRNO return actions include the
> errno value that will be returned to userspace.
>
Not that I'm opposed to the idea, but what's the intended purpose?
^ permalink raw reply
* Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions
From: Andy Lutomirski @ 2017-01-03 5:56 UTC (permalink / raw)
To: Paul Moore
Cc: Tyler Hicks, Eric Paris, Kees Cook, Will Drewry, linux-audit,
linux-kernel@vger.kernel.org
In-Reply-To: <CAHC9VhSOwbY9WEOKZGsx4mf=MAXeudTnQF9nXmKu+OoAs0SDsQ@mail.gmail.com>
On Mon, Jan 2, 2017 at 2:47 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Mon, Jan 2, 2017 at 11:53 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> This patch set creates the basis for auditing information specific to a given
>> seccomp return action and then starts auditing SECCOMP_RET_ERRNO return
>> actions. The audit messages for SECCOMP_RET_ERRNO return actions include the
>> errno value that will be returned to userspace.
>
> I'm replying to this patchset posting because it his my inbox first,
> but my comments here apply to both this patchset and the other
> seccomp/audit patchset you posted.
>
> In my experience, we have two or three problems (the count varies
> depending on perspective) when it comes to seccomp filter reporting:
>
> 1. Inability to log all filter actions.
> 2. Inability to selectively enable filtering; e.g. devs want noisy
> logging, users want relative quiet.
> 3. Consistent behavior with audit enabled and disabled.
>
> My current thinking - forgive me, this has been kicking around in my
> head for the better part of six months (longer?) and I haven't
> attempted to code it up - is to create a sysctl knob for a system wide
> seccomp logging threshold that would be applied to the high 16-bits of
> *every* triggered action: if the action was at/below the threshold a
> record would be emitted, otherwise silence. This should resolve
> problems #1 and #2, and the code should be relatively straightforward
> and small.
>
> As part of the code above, I expect that all seccomp logging would get
> routed through a single logging function (sort of like a better
> implementation of the existing audit_seccomp()) that would check the
> threshold and trigger the logging if needed. This function could be
> augmented to check for CONFIG_AUDIT and in the case where audit was
> not built into the kernel, a simple printk could be used to log the
> seccomp event; solving problem #3.
Would this not be doable with a seccomp tracepoint and a BPF filter?
--Andy
^ permalink raw reply
* Re: [PATCH 2/2] seccomp: Audit SECCOMP_RET_ERRNO actions with errno values
From: Paul Moore @ 2017-01-02 22:55 UTC (permalink / raw)
To: Steve Grubb
Cc: Tyler Hicks, linux-audit, Eric Paris, Kees Cook, Andy Lutomirski,
Will Drewry, linux-kernel
In-Reply-To: <1540151.sullFKCz8n@x2>
On Mon, Jan 2, 2017 at 1:49 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Monday, January 2, 2017 5:42:47 PM EST Tyler Hicks wrote:
>> On 2017-01-02 12:20:53, Steve Grubb wrote:
>> > On Monday, January 2, 2017 4:53:10 PM EST Tyler Hicks wrote:
...
>> Thanks for having a look at the field name I was using. Although I
>> prefer "errno" over "exit" in terms of clarity, I agree that it makes
>> sense to be consistent with the field names across record types. "exit"
>> works for me.
FWIW, we have a nice (searchable due to GitHub CSV magic) audit field
database at the link below. I will admit that it may be a bit crusty
in places, but we are making a new effort to keep it updated, if you
notice anything wrong, send email and/or a PR.
* https://github.com/linux-audit/audit-documentation/blob/master/specs/fields/field-dictionary.csv
>> > http://people.redhat.com/sgrubb/files/auformat.tar.gz
>> >
>> > $ ausearch --start today --just-one -m syscall -sv no --raw | ./auformat
>> > "%EXIT\n"
>> >
>> > Also, I am working to normalize all the records. That mean every event
>> > record of the same type has the same fields, in the same order, with the
>> > same representation. I would think "exit" could be added to the current
>> > record after syscall so that its ordered similarly to a syscall record.
>>
>> This patch goes against your normalization efforts in more ways than
>> just the placement of the "exit" field. If the action is
>> SECCOMP_RET_KILL, a "sig" field is present but if the action is
>> SECCOMP_RET_ERRNO, the "sig" field will not be present but the "errno"
>> field will be present. This happens all within the AUDIT_SECCOMP record
>> type. How would you suggest normalizing AUDIT_SECCOMP records for
>> different seccomp return actions?
>
> Typically when the layout has to change, we just give it a new record type.
I'm going to be very loathe to accept any new record types that *only*
reorder fields; if you need to add a new field, simply add it to the
end of the record. From my perspective new record types are really
only an option if we need to remove a field that is bogus/confusing or
some other similar case that is not easily solved. New record types
are a last resort.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions
From: Paul Moore @ 2017-01-02 22:47 UTC (permalink / raw)
To: Tyler Hicks
Cc: Eric Paris, Kees Cook, Andy Lutomirski, Will Drewry, linux-audit,
linux-kernel
In-Reply-To: <1483375990-14948-1-git-send-email-tyhicks@canonical.com>
On Mon, Jan 2, 2017 at 11:53 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
> This patch set creates the basis for auditing information specific to a given
> seccomp return action and then starts auditing SECCOMP_RET_ERRNO return
> actions. The audit messages for SECCOMP_RET_ERRNO return actions include the
> errno value that will be returned to userspace.
I'm replying to this patchset posting because it his my inbox first,
but my comments here apply to both this patchset and the other
seccomp/audit patchset you posted.
In my experience, we have two or three problems (the count varies
depending on perspective) when it comes to seccomp filter reporting:
1. Inability to log all filter actions.
2. Inability to selectively enable filtering; e.g. devs want noisy
logging, users want relative quiet.
3. Consistent behavior with audit enabled and disabled.
My current thinking - forgive me, this has been kicking around in my
head for the better part of six months (longer?) and I haven't
attempted to code it up - is to create a sysctl knob for a system wide
seccomp logging threshold that would be applied to the high 16-bits of
*every* triggered action: if the action was at/below the threshold a
record would be emitted, otherwise silence. This should resolve
problems #1 and #2, and the code should be relatively straightforward
and small.
As part of the code above, I expect that all seccomp logging would get
routed through a single logging function (sort of like a better
implementation of the existing audit_seccomp()) that would check the
threshold and trigger the logging if needed. This function could be
augmented to check for CONFIG_AUDIT and in the case where audit was
not built into the kernel, a simple printk could be used to log the
seccomp event; solving problem #3.
We could also add a SECCOMP_RET_AUDIT, or similar, if we still feel
that is important (I personally waffle on this), but I think that is
independent of the ideas above.
Thoughts?
--
paul moore
www.paul-moore.com
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox