Re: Linux Audit Mail List
On Tue, Oct 31, 2023 at 5:24 PM Steve Grubb wrote: > > Hello, > > I think the linux-audit mail list will be shutdown at midnight tonight ... Whoa, that seems rather abrupt, is the mail server being shut down? Or something else? > There are mail archives such as > > https://marc.info/?l=linux-audit=1=2 The linux-audit@redhat list is also archived on lore.kernel.org, a link to the archive is below: * https://lore.kernel.org/linux-audit If those interested in upstream userspace development wanted to migrate to the audit@vger list I see no problem with that, link below: * http://vger.kernel.org/vger-lists.html#audit -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [pcmoore-audit:main 1/1] README.orig: warning: ignored by one of the .gitignore files
On Thu, Aug 31, 2023 at 5:33 AM Liu, Yujie wrote: > On Wed, 2023-08-30 at 10:29 -0400, Paul Moore wrote: > > I have no idea if there is a person attached to any of the addresses > > on the To line above, but please send audit kernel issues to > > au...@vger.kernel.org and not linux-audit@redhat.com. > > Sorry for the inconvenience. We've updated the bot's email > configuration to send audit-related reports to au...@vger.kernel.org. Great, thank you! -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [nf PATCH 2/2] netfilter: nf_tables: Audit log rule reset
On Tue, Aug 29, 2023 at 2:24 PM Phil Sutter wrote:
>
> Resetting rules' stateful data happens outside of the transaction logic,
> so 'get' and 'dump' handlers have to emit audit log entries themselves.
>
> Cc: Richard Guy Briggs
> Fixes: 8daa8fde3fc3f ("netfilter: nf_tables: Introduce NFT_MSG_GETRULE_RESET")
> Signed-off-by: Phil Sutter
> ---
> include/linux/audit.h | 1 +
> kernel/auditsc.c | 1 +
> net/netfilter/nf_tables_api.c | 18 ++
> 3 files changed, 20 insertions(+)
See my comments in patch 1/2.
Acked-by: Paul Moore
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [nf PATCH 1/2] netfilter: nf_tables: Audit log setelem reset
On Wed, Aug 30, 2023 at 4:54 PM Richard Guy Briggs wrote: > On 2023-08-30 17:46, Pablo Neira Ayuso wrote: > > On Tue, Aug 29, 2023 at 07:51:57PM +0200, Phil Sutter wrote: > > > Since set element reset is not integrated into nf_tables' transaction > > > logic, an explicit log call is needed, similar to NFT_MSG_GETOBJ_RESET > > > handling. > > > > > > For the sake of simplicity, catchall element reset will always generate > > > a dedicated log entry. This relieves nf_tables_dump_set() from having to > > > adjust the logged element count depending on whether a catchall element > > > was found or not. > > > > Applied, thanks Phil > > Thanks Phil, Pablo. If it isn't too late, please add my > Reviewed-by: Richard Guy Briggs Similarly, you can add my ACK. FWIW, if you're sending patches out during the first few days of the merge window it might be advisable to wait more than a day or two to give the relevant maintainers a chance to review the patch. Also, as a note for future submissions, we've moved the audit kernel mailing list to au...@vger.kernel.org. Acked-by: Paul Moore -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [pcmoore-audit:main 1/1] README.orig: warning: ignored by one of the .gitignore files
On Wed, Aug 30, 2023 at 12:17 AM kernel test robot wrote: > tree: https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git main > head: 94e5ef9b157c6c3779352a8d4121542f71de52a1 > commit: 94e5ef9b157c6c3779352a8d4121542f71de52a1 [1/1] audit: add a Linux > Audit specific README.md and SECURITY.md > config: parisc-randconfig-r012-20230830 > (https://download.01.org/0day-ci/archive/20230830/202308301248.hewyevel-...@intel.com/config) > compiler: hppa-linux-gcc (GCC) 13.2.0 > reproduce (this is a W=1 build): > (https://download.01.org/0day-ci/archive/20230830/202308301248.hewyevel-...@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version > of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot > | Closes: > https://lore.kernel.org/oe-kbuild-all/202308301248.hewyevel-...@intel.com/ > > All warnings (new ones prefixed by >>): > > >> README.orig: warning: ignored by one of the .gitignore files >tools/perf/util/bpf_skel/vmlinux/vmlinux.h: warning: ignored by one of the > .gitignore files >tools/testing/selftests/arm64/tags/.gitignore: warning: ignored by one of > the .gitignore files >tools/testing/selftests/arm64/tags/Makefile: warning: ignored by one of > the .gitignore files >tools/testing/selftests/arm64/tags/run_tags_test.sh: warning: ignored by > one of the .gitignore files >tools/testing/selftests/arm64/tags/tags_test.c: warning: ignored by one of > the .gitignore files >tools/testing/selftests/kvm/.gitignore: warning: ignored by one of the > .gitignore files >tools/testing/selftests/kvm/Makefile: warning: ignored by one of the > .gitignore files >tools/testing/selftests/kvm/config: warning: ignored by one of the > .gitignore files >tools/testing/selftests/kvm/settings: warning: ignored by one of the > .gitignore files I have no idea if there is a person attached to any of the addresses on the To line above, but please send audit kernel issues to au...@vger.kernel.org and not linux-audit@redhat.com. Thanks. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: add task history record
On August 26, 2023 2:38:55 AM Tetsuo Handa wrote: On 2023/08/25 12:36, Paul Moore wrote: It is unfortunate that you continue ignoring the How can auditd generate logs that are not triggered via syscalls? line. I know how to configure syscall rules using "-S" option. But I do not know how to configure non syscall rules (such as process creation via kthread_create(), process termination due to tty hangup or OOM killer). At this point you've exhausted my goodwill so I would suggest simply reading the audit code, manages, and experimenting with a running system to understand how things work, especially for non-syscall records. Are we on the same page that non-syscall records include process creation via kthread_create() and process termination via send_sig() ? I'm not confident that we are in agreement on many of the issues in this thread, some of that is likely due to disagreements in what functionality is worthwhile, but I believe some of that is due to misunderstandings. I'm not going to merge the patch you posted at the start of this thread, but as I've mentioned *several* times now, if you find a kernel code path that is missing an audit logging call submit a patch with an explanation of why the audit call is needed and we can discuss it. My initial thinking is that we do not need, or want, an audit call in kthread_create(). I'm less sure about send_sig(), and as my network access is limited at the moment I don't have the ability to look into it further right now. If you feel we do need an additional audit call, create a patch, test it, write a good commit description explaining why it is necessary and we can review it. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: add task history record
On August 24, 2023 6:24:47 PM Tetsuo Handa wrote: On 2023/08/24 23:26, Paul Moore wrote: On Thu, Aug 24, 2023 at 9:47 AM Tetsuo Handa wrote: On 2023/08/24 22:39, Tetsuo Handa wrote: (1) Catch _all_ process creations (both via fork()/clone() system calls and kthread_create() from the kernel), and duplicate the history upon process creation. Create an audit filter rule to record the syscalls you are interested in logging. I can't interpret what you are talking about. Please show me using command line. I'm not interested in logging the syscalls just for maintaining process history information. That's unfortunate because I'm not interested in merging your patch when we already have an audit log which can be used to trace process history information. It is unfortunate that you continue ignoring the How can auditd generate logs that are not triggered via syscalls? line. I know how to configure syscall rules using "-S" option. But I do not know how to configure non syscall rules (such as process creation via kthread_create(), process termination due to tty hangup or OOM killer). At this point you've exhausted my goodwill so I would suggest simply reading the audit code, manages, and experimenting with a running system to understand how things work, especially for non-syscall records. I repeat: The auditd is not capable of generating _all_ records needed for maintaining this information. The logs generated via system call auditing is just an example user of this information. I repeat: If you find a place in the code where you believe there should be an audit record, post a patch and we can discuss it. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: add task history record
On Thu, Aug 24, 2023 at 11:55 AM Steve Grubb wrote: > On Thursday, August 24, 2023 9:30:10 AM EDT Paul Moore wrote: > > On Thu, Aug 24, 2023 at 9:21 AM Tetsuo Handa > > wrote: > > > On 2023/08/23 23:48, Paul Moore wrote: > > > > We've already discussed this both from a kernel load perspective (it > > > > should be able to handle the load, if not that is a separate problem > > > > to address) as well as the human perspective (if you want auditing, > > > > you need to be able to handle auditing). > > > > > > No. You haven't shown us audit rules that can satisfy requirements shown > > > below.> > > > (1) Catch _all_ process creations (both via fork()/clone() system calls > > > and kthread_create() from the kernel), and duplicate the history upon > > > process creation. > > > > Create an audit filter rule to record the syscalls you are interested > > in logging. > > > > > (2) Catch _all_ execve(), and update the history upon successful > > > execve(). > > > > Create an audit filter rule to record the syscalls you are interested > > in logging. > > > > > (3) Catch _all_ process terminations (both exit()/exit_group()/kill() > > > system calls and internal reasons such as OOM killer), and erase the > > > history upon process termination. > > > > Create an audit filter rule to record the events you are interested in > > logging, if there is an event which isn't being recorded feel free to > > submit a patch to generate an audit record. > > I'm not for or against this or a similar patch. That was my impression based on your previous comments, my opinion remains unchanged. > The information Tetsuo is > looking for cannot be recreated from logs. What if it were a daemon that's > been running for a year? With the amount of data you are suggesting to log, > it would have rotated away months ago. Just because it requires configuration and/or a way of maintaining log information over a period of time does not mean it "cannot" be done. I also suspect that the number of well managed, and properly updated systems that have uptimes over a year are increasingly rare. Yes, there are systems with uptimes much longer than that, but my argument is that those systems are not likely as security focused as they may claim. > To log all of the system calls you > mention would be abusive of the audit system, hurt performance, wear out SSD > drives, and ultimately fail. Thank you for your input. It is clear that we have different opinions on this matter. > There may be other reasons you don't like the patch and that's fine. But > saying it can be done from user space after the fact is not helpful. Arguably your choice to reintroduce arguments you have previously made, which I believe I've answered, is also not helpful, yet here we are. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: add task history record
On Thu, Aug 24, 2023 at 9:47 AM Tetsuo Handa wrote: > On 2023/08/24 22:39, Tetsuo Handa wrote: > >>> (1) Catch _all_ process creations (both via fork()/clone() system calls > >>> and > >>> kthread_create() from the kernel), and duplicate the history upon > >>> process > >>> creation. > >> > >> Create an audit filter rule to record the syscalls you are interested > >> in logging. > > > > I can't interpret what you are talking about. Please show me using command > > line. > > I'm not interested in logging the syscalls just for maintaining process > history > information. That's unfortunate because I'm not interested in merging your patch when we already have an audit log which can be used to trace process history information. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: add task history record
On Thu, Aug 24, 2023 at 9:39 AM Tetsuo Handa wrote: > On 2023/08/24 22:30, Paul Moore wrote: > > On Thu, Aug 24, 2023 at 9:21 AM Tetsuo Handa > > wrote: > >> > >> On 2023/08/23 23:48, Paul Moore wrote: > >>> We've already discussed this both from a kernel load perspective (it > >>> should be able to handle the load, if not that is a separate problem > >>> to address) as well as the human perspective (if you want auditing, > >>> you need to be able to handle auditing). > >> > >> No. You haven't shown us audit rules that can satisfy requirements shown > >> below. > >> > >> (1) Catch _all_ process creations (both via fork()/clone() system calls > >> and > >> kthread_create() from the kernel), and duplicate the history upon > >> process > >> creation. > > > > Create an audit filter rule to record the syscalls you are interested > > in logging. > > I can't interpret what you are talking about. Please show me using command > line. I'm sorry Tetsuo, but I've already spent far too much time going in circles with you on this topic. As you are capable of submitting kernel patches, you should be capable of reading a manpage and experimenting yourself: https://man7.org/linux/man-pages/man8/auditctl.8.html -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: add task history record
On Thu, Aug 24, 2023 at 9:21 AM Tetsuo Handa wrote: > > On 2023/08/23 23:48, Paul Moore wrote: > > We've already discussed this both from a kernel load perspective (it > > should be able to handle the load, if not that is a separate problem > > to address) as well as the human perspective (if you want auditing, > > you need to be able to handle auditing). > > No. You haven't shown us audit rules that can satisfy requirements shown > below. > > (1) Catch _all_ process creations (both via fork()/clone() system calls and > kthread_create() from the kernel), and duplicate the history upon > process > creation. Create an audit filter rule to record the syscalls you are interested in logging. > (2) Catch _all_ execve(), and update the history upon successful execve(). Create an audit filter rule to record the syscalls you are interested in logging. > (3) Catch _all_ process terminations (both exit()/exit_group()/kill() system > calls and internal reasons such as OOM killer), and erase the history > upon > process termination. Create an audit filter rule to record the events you are interested in logging, if there is an event which isn't being recorded feel free to submit a patch to generate an audit record. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: add task history record
On Wed, Aug 23, 2023 at 10:18 AM Tetsuo Handa wrote: > On 2023/08/22 1:35, Paul Moore wrote: > >> "auditctl -D" must not clear rules for tracing fork()/execve()/exit() > >> system calls. This is impossible because this change will break userspace > >> programs expecting that "auditctl -D" clears all rules. > > > > It's a good thing that 'audtictl -d ...' exists so that one can > > selectively delete audit rules from the kernel. If someone wants to > > preserve specific audit rules, that is the way to do it; 'auditctl -D' > > is a very coarse tool and not something that is likely very useful for > > users with strict auditing requirements. > > In most systems, "auditctl -D" is the first command done via > /etc/audit/audit.rules file. > You are asking all administrators who want to emulate this patch's > functionality using > auditd to customize that line. We can't afford asking such administrators to > become > specialist of strict auditing configurations, as well as we can't afford > asking > every administrator to become specialist of strict SELinux policy > configurations. If an administrator/user needs to configure the audit subsystem to do something, removing one line in a configuration file seems like a very reasonable thing to do. You can expect the default configuration of every Linux distribution to fit every conceivable use case. > Like Steve Grubb mentions, technically possible and practically affordable are > different. The audit subsystem could handle the load, but the system > administrator > can't handle the load. If an administrator/user wants this type of information in their audit logs they need to be prepared to handle that information. > That's why I said > > That is a "No LSM modules other than SELinux is needed because SELinux can > do > everything" assertion. What? That doesn't make any sense following what you said above. You're starting to cherry pick quotes and apply them out of context, which only hurts your argument further. > and your response > > Except we are not talking SELinux or LSMs here, we are talking about > audit and the audit subsystem is very different from the LSM layer. > The LSM layer is designed to be pluggable with support for multiple > individual LSMs, whereas the audit subsystem is designed to support a > single audit implementation. > > is totally missing the point. It makes perfect sense in the original context, see my comment above, and perhaps go re-read that original email. > For example, doing > > auditctl -a exit,always -F arch=b64 -S exit,exit_group > > (in order to allow userspace daemon which tries to emulate this patch's > functionality) will let auditd to generate process termination logs via exit() > system call. This command alone can generate too much stress on a busy system > (performance DoS and storage DoS). However, in this very same email, a few paragraphs above you conceded that "The audit subsystem could handle the load". > And moreover, this command will not let > auditd to generate process termination logs via kill() system call. > > kill -9 $$ > > Auditing kill system call may generate more stress than auditing exit system > call. > Too much noisy logs for catching the exact one event we want to know. We've already discussed this both from a kernel load perspective (it should be able to handle the load, if not that is a separate problem to address) as well as the human perspective (if you want auditing, you need to be able to handle auditing). Tetsuo, as should be apparent at this point, I'm finding your arguments unconvincing at best, and confusing at worst. If you or someone else wants to take a different approach towards arguing for this patch I'll entertain the discussion further, but please do not reply back with the same approach; it simply isn't constructive at this point. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: add task history record
On Tue, Aug 22, 2023 at 12:29 PM Steve Grubb wrote: > On Wednesday, August 16, 2023 9:53:58 AM EDT Paul Moore wrote: > > On Wed, Aug 16, 2023 at 6:10 AM Tetsuo Handa > > wrote: > > > On 2023/08/16 3:44, Paul Moore wrote: > > > > On Fri, Aug 11, 2023 at 6:58 AM Tetsuo Handa > > > > wrote: > > > >> When an unexpected system event occurs, the administrator may want to > > > >> identify which application triggered the event. For example, > > > >> unexpected process termination is still a real concern enough to write > > > >> articles like https://access.redhat.com/solutions/165993 . > > > >> > > > >> This patch adds a record which emits TOMOYO-like task history > > > >> information into the audit logs for better understanding of unexpected > > > >> system events. > > > >> > > > >> type=UNKNOWN[1340] msg=audit(1691750738.271:108): > > > >> history="name=swapper/0;pid=1;start=20230811194329=>name=init;pid=1;s > > > >> tart=20230811194343=>name=systemd;pid=1;start=20230811194439=>name=ssh > > > >> d;pid=3660;start=20230811104504=>name=sshd;pid=3767;start=202308111045 > > > >> 35" > > > > > > > > While I respect your persistence, we've talked about this quite a bit > > > > already in other threads. What you are trying to do is already > > > > possible with audit > > > > > > How? > > > > If you configure audit to record exec() and friends you should have a > > proper history of the processes started on the system. > > This is not a practical solution. Yes, technically this could be done. But it > would be a huge burden on the system to keep up with this. And it would bury > events you truly wanted to see effectively DoS'ing the audit system. If the audit subsystem can't handle the load, that is a separate issue. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: add task history record
On Sat, Aug 19, 2023 at 3:09 AM Tetsuo Handa wrote: > On 2023/08/18 23:59, Paul Moore wrote: > > Except we are not talking SELinux or LSMs here, we are talking about > > audit and the audit subsystem is very different from the LSM layer. > > The LSM layer is designed to be pluggable with support for multiple > > individual LSMs, whereas the audit subsystem is designed to support a > > single audit implementation. It is my opinion that the audit patch > > you have proposed here does not provide an audit administrator with > > any new capabilities that they do not currently have as an option. > > Before explaining why an audit administrator cannot afford emulating > this patch, I explain what this patch will do. > > There are three system calls for managing a process: fork()/execve()/exit(). > > https://I-love.SAKURA.ne.jp/tomoyo/fork.gif > https://I-love.SAKURA.ne.jp/tomoyo/execve.gif > https://I-love.SAKURA.ne.jp/tomoyo/exit.gif > > As a result, history of a process can be represented as a tree, where the > root of the tree is the kernel thread which is started by the boot loader. > > https://I-love.SAKURA.ne.jp/tomoyo/railway.gif > > This fundamental mechanism cannot be changed as long as Linux remains as a > Unix-like OS. That is, adding this information will not cause what you call > "the support burden" ... Any new functionality added to the kernel, especially user visible functionality or some sort of interface, adds a support burden. Nothing is "free". > > There are also concerns around field formatting, record length, etc., > > but those are secondary issues compared to the more important issue of > > redundant functionality. > > If someone tries to emulate this patch, we need to be able to trace all > fork()/execve()/exit() system calls. Or, the history tree will be broken. > > If an audit administrator tries to emulate this patch using system call > auditing functionality, we need to make sure that > > "auditctl -D" must not clear rules for tracing fork()/execve()/exit() > system calls. This is impossible because this change will break userspace > programs expecting that "auditctl -D" clears all rules. It's a good thing that 'audtictl -d ...' exists so that one can selectively delete audit rules from the kernel. If someone wants to preserve specific audit rules, that is the way to do it; 'auditctl -D' is a very coarse tool and not something that is likely very useful for users with strict auditing requirements. > Rules for tracing fork()/execve()/exit() system calls must be enabled > when the kernel thread which is started by the boot loader starts. > How can we embed such system call auditing rules into the kernel and > tell whether to enable these rules using the kernel command line options? I would boot the system with 'audit=1' on the kernel command line and ensure that your desired audit rules are loaded as early in the boot process as possible, before any long-running processes/daemons/logins are started. Honestly, that's simply a good best practice for anyone who cares about maintaining a proper audit log, independent of the specific use case here. > In order to avoid possibility of loosing fork()/execve()/exit() records, > auditd must not be stopped even temporarily. Who wants to enforce such > requirement in order to be able to obtain process history information? A silly amount of work has gone into ensuring that the audit subsystem in the kernel doesn't lose records when properly configured. If you haven't already, I would encourage you to read the auditctl(8) man page and look for the parameters that adjust the audit backlog configuration. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: add task history record
On Fri, Aug 18, 2023 at 6:30 AM Tetsuo Handa wrote: > On 2023/08/16 22:53, Paul Moore wrote: > > On Wed, Aug 16, 2023 at 6:10 AM Tetsuo Handa > > wrote: > >> On 2023/08/16 3:44, Paul Moore wrote: > >>> On Fri, Aug 11, 2023 at 6:58 AM Tetsuo Handa > >>> wrote: > >>>> > >>>> When an unexpected system event occurs, the administrator may want to > >>>> identify which application triggered the event. For example, unexpected > >>>> process termination is still a real concern enough to write articles > >>>> like https://access.redhat.com/solutions/165993 . > >>>> > >>>> This patch adds a record which emits TOMOYO-like task history information > >>>> into the audit logs for better understanding of unexpected system events. > >>>> > >>>> type=UNKNOWN[1340] msg=audit(1691750738.271:108): > >>>> history="name=swapper/0;pid=1;start=20230811194329=>name=init;pid=1;start=20230811194343=>name=systemd;pid=1;start=20230811194439=>name=sshd;pid=3660;start=20230811104504=>name=sshd;pid=3767;start=20230811104535" > >>> > >>> While I respect your persistence, we've talked about this quite a bit > >>> already in other threads. What you are trying to do is already > >>> possible with audit > >> > >> How? > > > > If you configure audit to record exec() and friends you should have a > > proper history of the processes started on the system. > > That is a "No LSM modules other than SELinux is needed because SELinux can do > everything" assertion. Except we are not talking SELinux or LSMs here, we are talking about audit and the audit subsystem is very different from the LSM layer. The LSM layer is designed to be pluggable with support for multiple individual LSMs, whereas the audit subsystem is designed to support a single audit implementation. It is my opinion that the audit patch you have proposed here does not provide an audit administrator with any new capabilities that they do not currently have as an option. There are also concerns around field formatting, record length, etc., but those are secondary issues compared to the more important issue of redundant functionality. > People propose different approaches/implementations because > they can't afford utilizing/configuring existing approaches/implementations. From what I've seen, both in this thread as well as the other related threads from you, these recent efforts are due to a lack of TOMOYO support in mainstream Linux distributions. My advice is to stop trying to duplicate the TOMOYO functionality in other subsystems/LSMs and start working with the distributions to better understand why they are not supporting TOMOYO. I believe that if you can determine why the distributions are not enabling TOMOYO, you should be able to develop a plan to address those issues and eventually gain distribution support for TOMOYO. I understand that such an approach will likely be time consuming and difficult, but I think that is your best option for success. > Your assertion is a fatal problem for merging "Re: [PATCH v13 00/11] LSM: > Three basic syscalls" > at > https://lkml.kernel.org/r/cahc9vhq4ttksltbcrxnzsbr1fp9uz_guhmo0bs37lcdybmu...@mail.gmail.com > . > > Please please allow LSM modules like > https://lkml.kernel.org/r/41d03271-ff8a-9888-11de-a7f53da47...@i-love.sakura.ne.jp > to obtain a stable LSM ID We've already discussed that in the TaskTracker thread. > if you don't want to support something that possibly have an alternative. We've already upstreamed an alternative approach to TaskTracker: TOMOYO. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: add task history record
On Wed, Aug 16, 2023 at 6:10 AM Tetsuo Handa wrote: > On 2023/08/16 3:44, Paul Moore wrote: > > On Fri, Aug 11, 2023 at 6:58 AM Tetsuo Handa > > wrote: > >> > >> When an unexpected system event occurs, the administrator may want to > >> identify which application triggered the event. For example, unexpected > >> process termination is still a real concern enough to write articles > >> like https://access.redhat.com/solutions/165993 . > >> > >> This patch adds a record which emits TOMOYO-like task history information > >> into the audit logs for better understanding of unexpected system events. > >> > >> type=UNKNOWN[1340] msg=audit(1691750738.271:108): > >> history="name=swapper/0;pid=1;start=20230811194329=>name=init;pid=1;start=20230811194343=>name=systemd;pid=1;start=20230811194439=>name=sshd;pid=3660;start=20230811104504=>name=sshd;pid=3767;start=20230811104535" > > > > While I respect your persistence, we've talked about this quite a bit > > already in other threads. What you are trying to do is already > > possible with audit > > How? If you configure audit to record exec() and friends you should have a proper history of the processes started on the system. > > and/or TOMOYO enabled and configured > > Wrong. Since not all LSM hooks allow sleeping, TOMOYO is unable to > check sending signals. Also, TOMOYO is not using audit interface. I said "audit and/or TOMOYO"; I believe the "and/or" is important. If I recall correctly, and perhaps I misunderstood you, you conceded that a combination of audit *and/or* TOMOYO would solve this issue. > > so I see no > > reason why we want to merge this. > > This code makes it possible to record sending signals with TOMOYO-like > context, > and we can avoid assigning LSM ID for this code if we can merge this code as > a part of audit. If you want TOMOYO-like information, run TOMOYO. If your preferred distribution doesn't support TOMOYO, you need to either ask them to support it, find a new distribution that does, or build your own kernel. > >I understand your frustration that > > TOMOYO is not enabled by your prefered distribution, but adding > > additional (and arguably redundant code) code to the upstream kernel > > is not a solution I am willing to support and maintain long term. > > Never a redundant code. Absolutely no reason we don't want to merge. At this point in time, I obviously disagree. > The only choice is which approach (a standalone LSM module or a part of audit) > to go. Casey suggests this code as a part of audit. You must persuade Casey > if you don't want this code as a part of audit. To be very clear, it isn't my duty to persuade Casey about anything (although if you've followed the LSM stacking saga you know I've definitely tried on occasion! ). My role here is to maintain the audit subsystem and LSM layer (along with others which aren't relevant here) to the best of my ability. A big part of that is ensuring we make "smart decisions" with respect to what code we merge as things like new LSMs and new audit records are things that we have to support *forever*. Because of this rather extreme support burden I need to make sure that we aren't making our jobs (current developers, current maintainers, and those that will follow us) more difficult than absolutely necessary. From my current perspective, the benefits of this patch, both in terms of unique functionality and durability of the design/code, are not enough to outweigh the support burden. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: add task history record
On Fri, Aug 11, 2023 at 6:58 AM Tetsuo Handa wrote: > > When an unexpected system event occurs, the administrator may want to > identify which application triggered the event. For example, unexpected > process termination is still a real concern enough to write articles > like https://access.redhat.com/solutions/165993 . > > This patch adds a record which emits TOMOYO-like task history information > into the audit logs for better understanding of unexpected system events. > > type=UNKNOWN[1340] msg=audit(1691750738.271:108): > history="name=swapper/0;pid=1;start=20230811194329=>name=init;pid=1;start=20230811194343=>name=systemd;pid=1;start=20230811194439=>name=sshd;pid=3660;start=20230811104504=>name=sshd;pid=3767;start=20230811104535" While I respect your persistence, we've talked about this quite a bit already in other threads. What you are trying to do is already possible with audit and/or TOMOYO enabled and configured so I see no reason why we want to merge this. I understand your frustration that TOMOYO is not enabled by your prefered distribution, but adding additional (and arguably redundant code) code to the upstream kernel is not a solution I am willing to support and maintain long term. > To be able to avoid bloating audit log files due to this information, this > patch uses audit_history= kernel command line parameter that controls max > length of history in bytes (default is 1024, and setting to 0 disables > recording and emitting). > > Unlike execve()'s argv record, records in this history information is > emitted as one string in order to reduce bloat of the audit log files. > This information can be split into an array using => as the tokenizer. > But don't expect that you can compare array elements throughout the whole > audit logs by splitting into an array, for old records get removed from > history when history became too long to append the newest record. This > history information is meant to be interpreted by humans rather than be > analyzed by programs. > > Signed-off-by: Tetsuo Handa > --- > fs/exec.c | 1 + > include/linux/audit.h | 5 ++ > include/linux/sched.h | 1 + > include/uapi/linux/audit.h | 1 + > init/init_task.c | 7 +++ > kernel/audit.c | 1 + > kernel/auditsc.c | 108 + > 7 files changed, 124 insertions(+) -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2] TaskTracker : Simplified thread information tracker.
On Tue, Aug 8, 2023 at 6:07 AM Tetsuo Handa wrote: > On 2023/08/08 5:13, Paul Moore wrote: > > On Mon, Aug 7, 2023 at 3:03 PM Steve Grubb wrote: > >> On Monday, August 7, 2023 2:53:40 PM EDT Paul Moore wrote: > >>> On Sun, Aug 6, 2023 at 9:05 AM Tetsuo Handa > >>> > >>> wrote: > >>>> When an unexpected system event occurs, the administrator may want to > >>>> identify which application triggered the event. For example, unexpected > >>>> process termination is still a real concern enough to write articles > >>>> like https://access.redhat.com/solutions/165993 . TaskTracker is a > >>>> trivial LSM module which emits TOMOYO-like information into the audit > >>>> logs for better understanding of unexpected system events. > >>> > >>> Help me understand why all of this information isn't already available > >>> via some combination of Audit and TOMOYO, or simply audit itself? > >> > >> Usually when you want this kind of information, you are investigating an > >> incident. You wouldn't place a syscall audit for every execve and then > >> reconstruct the call chain from that. In the case of long running daemons, > >> the information could have been rotated away. But typically you want to see > >> what the entry point is. A sudden shell from bind would be suspicious > >> while a > >> shell from sshd is not. > > > > Once again, why not use the existing audit and/or TOMOYO capabilities. > > > > Can't, for Fedora/RHEL does not enable TOMOYO. > I need a way that can be used by RHEL users running with selinux=0. What makes you think your distribution of choice would enable this new LSM? I'm sorry, but this sounds like more of an issue with the choices made by a distro rather than something missing upstream. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2] TaskTracker : Simplified thread information tracker.
On Mon, Aug 7, 2023 at 3:03 PM Steve Grubb wrote: > On Monday, August 7, 2023 2:53:40 PM EDT Paul Moore wrote: > > On Sun, Aug 6, 2023 at 9:05 AM Tetsuo Handa > > > > wrote: > > > When an unexpected system event occurs, the administrator may want to > > > identify which application triggered the event. For example, unexpected > > > process termination is still a real concern enough to write articles > > > like https://access.redhat.com/solutions/165993 . TaskTracker is a > > > trivial LSM module which emits TOMOYO-like information into the audit > > > logs for better understanding of unexpected system events. > > > > Help me understand why all of this information isn't already available > > via some combination of Audit and TOMOYO, or simply audit itself? > > Usually when you want this kind of information, you are investigating an > incident. You wouldn't place a syscall audit for every execve and then > reconstruct the call chain from that. In the case of long running daemons, > the information could have been rotated away. But typically you want to see > what the entry point is. A sudden shell from bind would be suspicious while a > shell from sshd is not. Once again, why not use the existing audit and/or TOMOYO capabilities. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2] TaskTracker : Simplified thread information tracker.
On Sun, Aug 6, 2023 at 9:05 AM Tetsuo Handa wrote: > > When an unexpected system event occurs, the administrator may want to > identify which application triggered the event. For example, unexpected > process termination is still a real concern enough to write articles > like https://access.redhat.com/solutions/165993 . TaskTracker is a > trivial LSM module which emits TOMOYO-like information into the audit > logs for better understanding of unexpected system events. Help me understand why all of this information isn't already available via some combination of Audit and TOMOYO, or simply audit itself? In the case of an audit-only design you would likely need to do some processing of the audit log to determine the full historical process tree of the process being killed, but all of the information should be there if you configure audit properly. I'm less familiar with TOMOYO, but your comment about this LSM recording "TOMOYO-like" information makes me believe that TOMOYO already records this information. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: Comprehensive Documentation on the Linux Audit Framework
On Tue, Jun 6, 2023 at 3:09 PM Steve Grubb wrote: > On Tuesday, June 6, 2023 6:31:55 PM EDT Vincent Abraham wrote: > > Thanks. Could you also point to portions in the codebase where these > > functions are called for monitoring file access? > > I'll let Richard or Paul point to the place in the kernel if that's > necessary. I think there's a fundamental mismatch and it might not matter. The audit subsystem in the Linux Kernel is currently found in the core kernel/ directory: % ls -1 kernel/audit* kernel/audit.c kernel/auditfilter.c kernel/audit_fsnotify.c kernel/audit.h kernel/auditsc.c kernel/audit_tree.c kernel/audit_watch.c > ... would be path, kind of access, who is accessing it, program accessing > it, portions of se linux labeling, and a few other things. FYI for everyone on the thread, the generally accepted way to write to "SELinux" is as one word (no space between the "SE" and "Linux") and with the first three letters capitalized. I know we can be a little lazy with capitalization, I definitely am, but writing it as one word is the important part. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: Auditd doesn't receive syscalls after installation for the current shell.
On Wed, May 24, 2023 at 10:46 AM Steve Grubb wrote: > On Wednesday, May 24, 2023 7:37:27 AM EDT Rinat Gadelshin wrote: > > Hello there. > > > > It seems that the kernel doesn't send messages for syscalls of the shell > > process from which auditd is installed. > > > > Reproducing steps (performed on Ubuntu 22.04 x86_64 on virtual box by > > `root`): > > > > step #1: $ apt install auditd > > step #2: $ auditctl -a always,exit -F arch=b64 -S openat,renameat2,unlinkat > > step #3: $ echo t>delme;echo t2>>delme;cat delme;mv delme d;mv d > > delme;rm delme > > step #4: $ service auditd stop > > step #5: $ ausearch -f delme > > > > There are syscalls from /usr/bin/cat, /usr/bin/mv, /usr/bin/rm but there > > are no any syscalls (openat expected) > > for /usr/bin/bash (current shell process) for the file. > > > > If step #3 is performed from another tty, then openat syscalls > > (CREATE for the first echo and NORMAL for the second one) > > is logged for the /usr/bin/bash process. > > > > `uname -a` returns: Linux grin-vb-ubuntu-22-0-4 5.19.0-41-generic > > #42~22.04.01-Ubuntu SMP PREEMPT_DYNAMIC Tue Apr 18 17:40:00 UTC 2 x86_64 > > x86_64 x86_64 GNU/Linux > > > > Should I open an issue for the case at > > https://github.com/linux-audit/audit-kernel ? > > Are you booting with audit=1 ? If not, the install process and any before it > are not auditable. You will only get events for processes started after audit > enabled = 1. It is also worth noting that some distributions (I'm not sure if this applies to Ubuntu) effectively limit auditing with their default runtime configuration, see the wiki page below for more information: * https://github.com/linux-audit/audit-documentation/wiki/HOWTO-Fedora-Enable-Auditing -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: Can AUDIT_LIST_RULES causes kthreadd-spam?
On Wed, May 3, 2023 at 10:50 PM Tetsuo Handa wrote: > On 2023/05/04 7:12, Rinat Gadelshin wrote: > > On 04.05.2023 00:27, Paul Moore wrote: > >> Can you be more specific about the kernel threads you are seeing, are > >> you seeing multiple "kauditd" threads? > >> > >> % ps -fC kauditd > >> UID PIDPPID C STIME TTY TIME CMD > >> root 89 2 0 Apr28 ?00:00:00 [kauditd] > > I don't think so. > > kernel audit subsystem uses kthread_run() in order to run short-lived kernel > threads. Thanks Tetsuo, I agree that's far more likely. Ever since I took over shepherding the audit code, all of the thread issues have been around the main audit queue thread so it's a bit reflexive to assume that is the case :) -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: Can AUDIT_LIST_RULES causes kthreadd-spam?
On Wed, May 3, 2023 at 5:14 PM Rinat Gadelshin wrote: > Hello there =) > > My name is Rinat. > I'm a newbie here (at Linux kernel developer community). > > My current job is to work with audit subsystem on different > versions of Linux (and different kernel versions from 3.10 to the latest) > with and without auditd. > > My program works behalf of root account and uses netlink > (unicast or multicast depends of the kernel's version) > to communicate with audit subsystem of the kernel. > > If actual audit rule list has been changed > then my program should restore the configured audit rule list. > > To do it the program periodically (with 60 seconds interval) > requests the actual rule list be sending AUDIT_LIST_RULES. > > All rules are receiving perfectly. > > But I've noticed that there are many (2K+ for 5 minutes test) > kthreadd process have been spawned after that request > (I've stubbed the poll code and compare logs). Hi Rinat, First, a quick note that audit discussions involving the upstream Linux Kernel have moved to the au...@vger.kernel.org list (CC'd), please direct future emails there. Can you be more specific about the kernel threads you are seeing, are you seeing multiple "kauditd" threads? % ps -fC kauditd UID PIDPPID C STIME TTY TIME CMD root 89 2 0 Apr28 ?00:00:00 [kauditd] > Please, can you point me, what can I do to avoid this kthreadd-spam. > > Thank you. > > Best regards > Rinath -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: Re: "service auditd start" fails inside a container
On Fri, Apr 28, 2023 at 8:25 AM 江杨 wrote: > > May I ask if Auditd supports Docker? Thank you > https://listman.redhat.com/archives/linux-audit/2018-July/msg00078.html Have you tried the configuration changes suggested in the mailing list post you linked above? -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v9 05/16] ipe: add userspace interface
On Mon, Apr 17, 2023 at 5:18 PM Fan Wu wrote: > On Mon, Apr 17, 2023 at 04:16:29PM -0400, Paul Moore wrote: > > On Mon, Apr 17, 2023 at 2:06???PM Fan Wu wrote: > > > On Thu, Apr 13, 2023 at 02:45:07PM -0400, Paul Moore wrote: > > > > On Wed, Apr 12, 2023 at 7:36???PM Fan Wu > > > > wrote: > > > > > On Tue, Apr 11, 2023 at 05:45:41PM -0400, Paul Moore wrote: > > > > > > On Mon, Apr 10, 2023 at 3:10???PM Fan Wu > > > > > > wrote: > > > > > > > On Thu, Mar 02, 2023 at 02:04:42PM -0500, Paul Moore wrote: > > > > > > > > On Mon, Jan 30, 2023 at 5:58???PM Fan Wu > > > > > > > > wrote: > > > > > > > > ... > > > > > > > > > > I guess this does make me wonder about keeping a non-active policy > > > > > > loaded in the kernel, what purpose does that serve? > > > > > > > > > > > > > > > > The non-active policy doesn't serve anything unless it is activated. > > > > > User can > > > > > even delete a policy if that is no longer needed. Non-active is just > > > > > the default > > > > > state when a new policy is loaded. > > > > > > > > > > If IPE supports namespace, there is another use case where different > > > > > containers > > > > > can select different policies as the active policy from among > > > > > multiple loaded > > > > > policies. Deven has presented a demo of this during LSS 2021. But > > > > > this goes > > > > > beyond the scope of this version. > > > > > > > > Do you plan to add namespace support at some point in the > > > > not-too-distant future? If so, I'm okay with keeping support for > > > > multiple policies, but if you think you're only going to support one > > > > active policy at a time, it might be better to remove support for > > > > multiple (inactive) policies. > > > > > > > > -- > > > > paul-moore.com > > > > > > Another benefit of having multiple policies is that it provides isolation > > > between different policies. For instance, if we have two policies named > > > "policy_a" and "policy_b," we can ensure that only team a can update > > > "policy_a," > > > and only team b can update "policy_b." This way, both teams can update > > > their policy without affecting others. However, if there is only one > > > policy > > > in the system, both teams will have to operate on the same policy, making > > > it > > > less manageable. > > > > That only really matters if both policies are active at the same time; > > if only one policy can be active at one point in time the only > > permission that matters is the one who can load/activate a policy. > > > > Allowing for multiple policies complicates the code. If there is > > another feature that requires multiple policies, e.g. IPE namespaces, > > then that is okay. However, if there is no feature which requires > > multiple active policies, supporting multiple loaded policies only > > increases the risk of an exploitable bug in the IPE code. > > > > > Besides, removing multiple (inactive) policies support will > > > render the policy_name field meaningless, and we should only audit the > > > policy > > > hash. I am fine if we decide to go for the single policy option. > > > > Once again, I think it comes back to: do you still want to support IPE > > namespaces at some point in the future, and if so, when do you expect > > to work on that? > > Yes, absolutely! We definitely have plans to support namespaces in the future. > However, it's worth mentioning that there are other tasks that we may need > to prioritize due to their relatively lower complexity. For example, before > we can fully implement namespaces, we need to address some other important > aspects of the system, such as adding a policy language for integrity > enforcement on configuration files and defining trusted certificates > that can sign the root hash. Therefore, the timeline for implementing > namespaces will depend on the completion time of these tasks. > > I understand your concerns, and we can proceed with a single policy design > for the initial version. I think it's okay to stick with the multi-policy code for the initial submission, you've got the code now, and it's tested. I just wanted to make sure there were plans to make use of it at some point, if not we might as well drop it now. However, it sounds like you've got a plan to utilize the multi-policy support so that's fine with me. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v9 05/16] ipe: add userspace interface
On Mon, Apr 17, 2023 at 2:06 PM Fan Wu wrote: > On Thu, Apr 13, 2023 at 02:45:07PM -0400, Paul Moore wrote: > > On Wed, Apr 12, 2023 at 7:36???PM Fan Wu wrote: > > > On Tue, Apr 11, 2023 at 05:45:41PM -0400, Paul Moore wrote: > > > > On Mon, Apr 10, 2023 at 3:10???PM Fan Wu > > > > wrote: > > > > > On Thu, Mar 02, 2023 at 02:04:42PM -0500, Paul Moore wrote: > > > > > > On Mon, Jan 30, 2023 at 5:58???PM Fan Wu > > > > > > wrote: > > > > ... > > > > > > I guess this does make me wonder about keeping a non-active policy > > > > loaded in the kernel, what purpose does that serve? > > > > > > > > > > The non-active policy doesn't serve anything unless it is activated. User > > > can > > > even delete a policy if that is no longer needed. Non-active is just the > > > default > > > state when a new policy is loaded. > > > > > > If IPE supports namespace, there is another use case where different > > > containers > > > can select different policies as the active policy from among multiple > > > loaded > > > policies. Deven has presented a demo of this during LSS 2021. But this > > > goes > > > beyond the scope of this version. > > > > Do you plan to add namespace support at some point in the > > not-too-distant future? If so, I'm okay with keeping support for > > multiple policies, but if you think you're only going to support one > > active policy at a time, it might be better to remove support for > > multiple (inactive) policies. > > > > -- > > paul-moore.com > > Another benefit of having multiple policies is that it provides isolation > between different policies. For instance, if we have two policies named > "policy_a" and "policy_b," we can ensure that only team a can update > "policy_a," > and only team b can update "policy_b." This way, both teams can update > their policy without affecting others. However, if there is only one policy > in the system, both teams will have to operate on the same policy, making it > less manageable. That only really matters if both policies are active at the same time; if only one policy can be active at one point in time the only permission that matters is the one who can load/activate a policy. Allowing for multiple policies complicates the code. If there is another feature that requires multiple policies, e.g. IPE namespaces, then that is okay. However, if there is no feature which requires multiple active policies, supporting multiple loaded policies only increases the risk of an exploitable bug in the IPE code. > Besides, removing multiple (inactive) policies support will > render the policy_name field meaningless, and we should only audit the policy > hash. I am fine if we decide to go for the single policy option. Once again, I think it comes back to: do you still want to support IPE namespaces at some point in the future, and if so, when do you expect to work on that? -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v9 05/16] ipe: add userspace interface
On Wed, Apr 12, 2023 at 7:36 PM Fan Wu wrote: > On Tue, Apr 11, 2023 at 05:45:41PM -0400, Paul Moore wrote: > > On Mon, Apr 10, 2023 at 3:10???PM Fan Wu wrote: > > > On Thu, Mar 02, 2023 at 02:04:42PM -0500, Paul Moore wrote: > > > > On Mon, Jan 30, 2023 at 5:58???PM Fan Wu > > > > wrote: ... > > I guess this does make me wonder about keeping a non-active policy > > loaded in the kernel, what purpose does that serve? > > > > The non-active policy doesn't serve anything unless it is activated. User can > even delete a policy if that is no longer needed. Non-active is just the > default > state when a new policy is loaded. > > If IPE supports namespace, there is another use case where different > containers > can select different policies as the active policy from among multiple loaded > policies. Deven has presented a demo of this during LSS 2021. But this goes > beyond the scope of this version. Do you plan to add namespace support at some point in the not-too-distant future? If so, I'm okay with keeping support for multiple policies, but if you think you're only going to support one active policy at a time, it might be better to remove support for multiple (inactive) policies. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: small patch for issue with rules that have been (incorrecly) copied from Windows
On Thu, Apr 13, 2023 at 12:25 PM Carlos De Avillez wrote: > > Hello again, > > Just checking is there is interest in the below. I noticed that your email ended up in my spam folder, likely because it was not plaintext, but who knows for sure. You might want to try posting your patch as a GitHub PR since it looks like Steve checks both the mailing list and GitHub for patches. * https://github.com/linux-audit/audit-userspace -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v9 07/16] uapi|audit|ipe: add ipe auditing support
On Thu, Mar 2, 2023 at 2:05 PM Paul Moore wrote: > On Tue, Jan 31, 2023 at 12:11 PM Steve Grubb wrote: > > On Monday, January 30, 2023 5:57:22 PM EST Fan Wu wrote: ... > > > audit: MAC_POLICY_LOAD policy_name="dmverity_roothash" > > > policy_version=0.0.0 sha256=DC67AC19E05894EFB3170A8E55DE529794E248C2 > > > auid=4294967295 ses=4294967295 lsm=ipe res=1 > > > > The MAC_POLICY_LOAD record type simply states the lsm that had it's policy > > loaded. There isn't name, version, and hash information. I'd prefer to see > > all users of this record type decide if it should be extended because they > > also have that information available to record. > > Not all LSMs which load policy have that information; as an example, > SELinux doesn't have the concept of a policy name or version. The > SELinux policy version you might see in the kernel sources refers to > the policy format version and has no bearing on the actual policy > content beyond that dictated by the format. > > If additional information is required by IPE, perhaps an auxiliary IPE > policy load record could be created with those additional fields. The issue of policy load audit records came up in an offline discussion with Fan today and I think it's worth talking about this a bit more to reach some consensus. Currently only SELinux generates MAC_POLICY_LOAD records, and it contains all of the information that is present in the IPE example above with the exception of the 'policy_name', 'policy_version', and the policy digest. I personally don't have a problem extending the MAC_POLICY_LOAD record with these fields, and leaving them unused/"?" in the SELinux generated records. It's possible we may even want to use the policy digest field at some point, as it would be nice to be able to have some policy "key" within SELinux that could be used to help identify the loaded policy. The only catch is that we may want to find a better field name than just 'sha256', in the context of the MAC_POLICY_LOAD record it seems easily understood, but in the larger context of a full audit stream it might be too ambiguous. We would also need to decide if we wanted to encode the digest algorithm in the field name, the field value, or have it as a separate field. I might lean towards encoding it in the field value like this: policy_digest=sha256:X ... however that is something that would need some discussion from the other folks on the To/CC line. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v9 07/16] uapi|audit|ipe: add ipe auditing support
On Thu, Mar 16, 2023 at 6:53 PM Fan Wu wrote: > On Thu, Mar 02, 2023 at 02:05:33PM -0500, Paul Moore wrote: > > On Tue, Jan 31, 2023 at 12:11???PM Steve Grubb wrote: > > > > > > Hello, > > > > > > On Monday, January 30, 2023 5:57:22 PM EST Fan Wu wrote: > > > > From: Deven Bowers > > > > > > > > Users of IPE require a way to identify when and why an operation fails, > > > > allowing them to both respond to violations of policy and be notified > > > > of potentially malicious actions on their systens with respect to IPE > > > > itself. > > > > > > > > The new 1420 audit, AUDIT_IPE_ACCESS indicates the result of a policy > > > > evaulation of a resource. The other two events, AUDIT_MAC_POLICY_LOAD, > > > > and AUDIT_MAC_CONFIG_CHANGE represent a new policy was loaded into the > > > > kernel and the currently active policy changed, respectively. > > > > > > Typically when you reuse an existing record type, it is expected to > > > maintain > > > the same fields in the same order. Also, it is expect that fields that are > > > common across diferent records have the same meaning. To aid in this, we > > > have > > > a field dictionary here: > > > > > > https://github.com/linux-audit/audit-documentation/blob/main/specs/fields/ > > > field-dictionary.csv > > > > > > For example, dev is expected to be 2 hex numbers separated by a colon > > > which > > > are the device major and minor numbers. But down a couple lines from > > > here, we > > > find dev="tmpfs". But isn't that a filesystem type? > > > > What Steve said. > > > > I'll also add an administrative note, we just moved upstream Linux > > audit development to a new mailing list, au...@vger.kernel.org, please > > use that in future patch submissions. As a positive, it's a fully > > open list so you won't run into moderation delays/notifications/etc. > > > Thanks for the info, I will update the address. > > > > > This patch also adds support for success auditing, allowing users to > > > > identify how a resource passed policy. It is recommended to use this > > > > option with caution, as it is quite noisy. > > > > > > > > This patch adds the following audit records: > > > > > > > > audit: AUDIT1420 path="/tmp/tmpwxmam366/deny/bin/hello" dev="tmpfs" > > > > ino=72 rule="DEFAULT op=EXECUTE action=DENY" > > > > > > Do we really need to log the whole rule? > > > > Fan, would it be reasonable to list the properties which caused the > > access denial? That seems like it might be more helpful than the > > specific rule, or am I missing something? > > Audit the whole rule can let the user find the reason of a policy decision. > We need the whole rule because an allow/block is not caused by a specific > property, but the combination of all property conditions in a rule. Okay, that's a reasonable argument for logging the rule along with the decision. I think it helps that the IPE policy rules are not particularly long. > We could also add a verbose switch such that we only audit > the whole rule when a user turned the verbose switch on. I'm not sure that's necessary, and honestly it might be annoying as we would still need to output a 'rule="?"' field in the audit record as it is considered good practice to not have fields magically appear and disappear from the record format. However, if there are concerns about record sizes, that could be a potential mitigation. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v9 05/16] ipe: add userspace interface
On Mon, Apr 10, 2023 at 3:10 PM Fan Wu wrote:
> On Thu, Mar 02, 2023 at 02:04:42PM -0500, Paul Moore wrote:
> > On Mon, Jan 30, 2023 at 5:58???PM Fan Wu wrote:
> > >
> > > From: Deven Bowers
> > >
> > > As is typical with LSMs, IPE uses securityfs as its interface with
> > > userspace. for a complete list of the interfaces and the respective
> > > inputs/outputs, please see the documentation under
> > > admin-guide/LSM/ipe.rst
> > >
> > > Signed-off-by: Deven Bowers
> > > Signed-off-by: Fan Wu
> >
> > ...
> >
> > > ---
> > > security/ipe/Makefile| 2 +
> > > security/ipe/fs.c| 101 +
> > > security/ipe/fs.h| 17 ++
> > > security/ipe/ipe.c | 3 +
> > > security/ipe/ipe.h | 2 +
> > > security/ipe/policy.c| 135
> > > security/ipe/policy.h| 7 +
> > > security/ipe/policy_fs.c | 459 +++
> > > 8 files changed, 726 insertions(+)
> > > create mode 100644 security/ipe/fs.c
> > > create mode 100644 security/ipe/fs.h
> > > create mode 100644 security/ipe/policy_fs.c
...
> > > +/**
> > > + * ipe_update_policy - parse a new policy and replace @old with it.
> > > + * @addr: Supplies a pointer to the i_private for saving policy.
> > > + * @text: Supplies a pointer to the plain text policy.
> > > + * @textlen: Supplies the length of @text.
> > > + * @pkcs7: Supplies a pointer to a buffer containing a pkcs7 message.
> > > + * @pkcs7len: Supplies the length of @pkcs7len.
> > > + *
> > > + * @text/@textlen is mutually exclusive with @pkcs7/@pkcs7len - see
> > > + * ipe_new_policy.
> > > + *
> > > + * Return:
> > > + * * !IS_ERR - OK
> > > + * * -ENOENT - Policy doesn't exist
> > > + * * -EINVAL - New policy is invalid
> > > + */
> > > +struct ipe_policy *ipe_update_policy(struct ipe_policy __rcu **addr,
> > > +const char *text, size_t textlen,
> > > +const char *pkcs7, size_t pkcs7len)
> > > +{
> > > + int rc = 0;
> > > + struct ipe_policy *old, *new;
> > > +
> > > + old = ipe_get_policy_rcu(*addr);
> > > + if (!old) {
> > > + rc = -ENOENT;
> > > + goto err;
> > > + }
> > > +
> > > + new = ipe_new_policy(text, textlen, pkcs7, pkcs7len);
> > > + if (IS_ERR(new)) {
> > > + rc = PTR_ERR(new);
> > > + goto err;
> > > + }
> > > +
> > > + if (strcmp(new->parsed->name, old->parsed->name)) {
> > > + rc = -EINVAL;
> > > + goto err;
> > > + }
> > > +
> > > + if (ver_to_u64(old) > ver_to_u64(new)) {
> > > + rc = -EINVAL;
> > > + goto err;
> > > + }
> > > +
> > > + if (ipe_is_policy_active(old)) {
> >
> > I don't understand the is-active check, you want to make @new the new
> > active policy regardless, right? Could this is-active check ever be
> > false?
>
> Actually this is needed. Policy updates can be applied to any deployed
> policy, which may be saved in two places: the securityfs file node
> and the ipe_active_policy pointer. To update a policy, this function first
> checks if the policy saved in the securityfs file node is currently active.
> If so, it updates the ipe_active_policy pointer to point to the new policy,
> and finally updates the policy pointer in the securityfs to the new policy.
Ah, okay. I must have forgotten, or not realized, that multiple
policies could be loaded and not active.
I guess this does make me wonder about keeping a non-active policy
loaded in the kernel, what purpose does that serve?
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v9 03/16] ipe: add evaluation loop and introduce 'boot_verified' as a trust provider
On Mon, Apr 10, 2023 at 2:53 PM Fan Wu wrote:
> On Thu, Mar 02, 2023 at 02:03:11PM -0500, Paul Moore wrote:
> > On Mon, Jan 30, 2023 at 5:58???PM Fan Wu wrote:
> > >
> > > From: Deven Bowers
> > >
> > > IPE must have a centralized function to evaluate incoming callers
> > > against IPE's policy. This iteration of the policy against the rules
> > > for that specific caller is known as the evaluation loop.
> > >
> > > In addition, IPE is designed to provide system level trust guarantees,
> > > this usually implies that trust starts from bootup with a hardware root
> > > of trust, which validates the bootloader. After this, the bootloader
> > > verifies the kernel and the initramfs.
> > >
> > > As there's no currently supported integrity method for initramfs, and
> > > it's typically already verified by the bootloader, introduce a property
> > > that causes the first superblock to have an execution to be "pinned",
> > > which is typically initramfs.
> > >
> > > Signed-off-by: Deven Bowers
> > > Signed-off-by: Fan Wu
> >
> > ...
> >
> > > ---
> > > security/ipe/Makefile| 1 +
> > > security/ipe/eval.c | 180 +++
> > > security/ipe/eval.h | 28 ++
> > > security/ipe/hooks.c | 25 +
> > > security/ipe/hooks.h | 14 +++
> > > security/ipe/ipe.c | 1 +
> > > security/ipe/policy.c| 20
> > > security/ipe/policy.h| 3 +
> > > security/ipe/policy_parser.c | 8 +-
> > > 9 files changed, 279 insertions(+), 1 deletion(-)
> > > create mode 100644 security/ipe/eval.c
> > > create mode 100644 security/ipe/eval.h
> > > create mode 100644 security/ipe/hooks.c
> > > create mode 100644 security/ipe/hooks.h
...
> > > diff --git a/security/ipe/eval.c b/security/ipe/eval.c
> > > new file mode 100644
> > > index ..48b5104a3463
> > > --- /dev/null
> > > +++ b/security/ipe/eval.c
> > > @@ -0,0 +1,180 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) Microsoft Corporation. All rights reserved.
> > > + */
> > > +
> > > +#include "ipe.h"
> > > +#include "eval.h"
> > > +#include "hooks.h"
> > > +#include "policy.h"
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +struct ipe_policy __rcu *ipe_active_policy;
> > > +
> > > +static struct super_block *pinned_sb;
> > > +static DEFINE_SPINLOCK(pin_lock);
> > > +#define FILE_SUPERBLOCK(f) ((f)->f_path.mnt->mnt_sb)
> > > +
> > > +/**
> > > + * pin_sb - Pin the underlying superblock of @f, marking it as trusted.
> > > + * @f: Supplies a file structure to source the super_block from.
> > > + */
> > > +static void pin_sb(const struct file *f)
> > > +{
> > > + if (!f)
> > > + return;
> > > + spin_lock(_lock);
> > > + if (pinned_sb)
> > > + goto out;
> > > + pinned_sb = FILE_SUPERBLOCK(f);
> > > +out:
> > > + spin_unlock(_lock);
> > > +}
> >
> > Since you don't actually use @f, just the super_block, you might
> > consider passing the super_block as the parameter and not the
> > associated file.
> >
> > I'd probably also flip the if-then to avoid the 'goto', for example:
> >
> > static void pin_sb(const struct super_block *sb)
> > {
> > if (!sb)
> > return;
> > spin_lock(_lock);
> > if (!pinned_sb)
> > pinned_sb = sb;
> > spin_unlock(_lock);
> > }
> >
>
> Sure, I can change the code accordingly.
>
> > Also, do we need to worry about the initramfs' being unmounted and the
> > super_block going away?
>
> If initramfs is being unmounted, the boot_verified property will never be
> TRUE,
> which is an expected behavior. In an actual use case, we can leverage this
> property to only enable files in initramfs during the booting stage, and
> later switch
> to another policy without the boot_verified property after unmounting the
> initramfs.
> This approach helps keep the allowed set of files minimum at each stage.
I think I was worried about not catching when the fs was unmounted and
the superblock disappeared, but you've got a hook defined for that so
it should be okay. I'm not sure what I was thinking here, sorry for
the noise ...
Regardless of the source of my confusion, your policy/boot_verified
description all sounds good to me.
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v9 02/16] ipe: add policy parser
On Thu, Apr 6, 2023 at 4:00 PM Fan Wu wrote:
> On Thu, Mar 02, 2023 at 02:02:32PM -0500, Paul Moore wrote:
> > On Mon, Jan 30, 2023 at 5:58???PM Fan Wu wrote:
> > >
> > > From: Deven Bowers
> > >
> > > IPE's interpretation of the what the user trusts is accomplished through
> > > its policy. IPE's design is to not provide support for a single trust
> > > provider, but to support multiple providers to enable the end-user to
> > > choose the best one to seek their needs.
> > >
> > > This requires the policy to be rather flexible and modular so that
> > > integrity providers, like fs-verity, dm-verity, dm-integrity, or
> > > some other system, can plug into the policy with minimal code changes.
> > >
> > > Signed-off-by: Deven Bowers
> > > Signed-off-by: Fan Wu
> >
> > ...
> >
> > > ---
> > > security/ipe/Makefile| 2 +
> > > security/ipe/policy.c| 99 +++
> > > security/ipe/policy.h| 77 ++
> > > security/ipe/policy_parser.c | 515 +++
> > > security/ipe/policy_parser.h | 11 +
> > > 5 files changed, 704 insertions(+)
> > > create mode 100644 security/ipe/policy.c
> > > create mode 100644 security/ipe/policy.h
> > > create mode 100644 security/ipe/policy_parser.c
> > > create mode 100644 security/ipe/policy_parser.h
...
> > > diff --git a/security/ipe/policy_parser.c b/security/ipe/policy_parser.c
> > > new file mode 100644
> > > index ..c7ba0e865366
> > > --- /dev/null
> > > +++ b/security/ipe/policy_parser.c
> > > @@ -0,0 +1,515 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) Microsoft Corporation. All rights reserved.
> > > + */
> > > +
> > > +#include "policy.h"
> > > +#include "policy_parser.h"
> > > +#include "digest.h"
> > > +
> > > +#include
> > > +
> > > +#define START_COMMENT '#'
> > > +
> > > +/**
> > > + * new_parsed_policy - Allocate and initialize a parsed policy.
> > > + *
> > > + * Return:
> > > + * * !IS_ERR - OK
> > > + * * -ENOMEM - Out of memory
> > > + */
> > > +static struct ipe_parsed_policy *new_parsed_policy(void)
> > > +{
> > > + size_t i = 0;
> > > + struct ipe_parsed_policy *p = NULL;
> > > + struct ipe_op_table *t = NULL;
> > > +
> > > + p = kzalloc(sizeof(*p), GFP_KERNEL);
> > > + if (!p)
> > > + return ERR_PTR(-ENOMEM);
> > > +
> > > + p->global_default_action = ipe_action_max;
> >
> > I'm assuming you're using the "ipe_action_max" as an intentional bogus
> > placeholder value here, yes? If that is the case, have you considered
> > creating an "invalid" enum with an explicit zero value to save you
> > this additional assignment (you are already using kzalloc())? For
> > example:
> >
> > enum ipe_op_type {
> > IPE_OP_INVALID = 0,
> > IPE_OP_EXEC,
> > ...
> > IPE_OP_MAX,
> > };
> >
> > enum ipe_action_type {
> > IPE_ACTION_INVALID = 0,
> > IPE_ACTION_ALLOW,
> > ...
> > IPE_ACTION_MAX,
> > };
> >
>
> Yes, IPE_ACTION_MAX is kind of the INVALID value we are using here.
>
> But I think we might be adding unnecessary complexity by using the
> IPE_OP_INVLIAD enum here. Currently, we are using IPE_OP_MAX to
> represent the number of operations we have, and we have allocated
> an IPE_OP_MAX-sized array to store linked lists that link all rules
> for each operation. If we were to add IPE_OP_INVLIAD to the enum
> definition, then IPE_OP_MAX-1 would become the number of operations,
> and we would need to change the index used to access the linked list
> array.
Gotcha. Thanks for the explanation, that hadn't occurred to me while
I was reviewing the code.
Another option would be to create a macro to help reinforce that the
"max" value is being used as an "invalid" value, for example:
#define IPE_OP_INVALID IPE_OP_MAX
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: Help with setting up Auditd kernel repository
On Mon, Mar 20, 2023 at 11:09 AM Anurag Aggarwal wrote: > > > > Can you be more specific about the exact repository? There is a > > repository which contains the development code for the Linux Kernel's > > audit subsystem, and there is a second repository which contains > > Steve's audit userspace code (which contains auditd as a component). > > There is no combined "auditd-kernel" repository. > > Yes Paul, the repository that I am talking about is: > https://github.com/linux-audit/audit-kernel [NOTE: The upstream Linux Kernel discussion has moved to au...@vger.kernel.org, CC'd on this email] Thanks for the clarification. If you aren't already aware of the Kernel Newbies site, it's a good resource for getting familiar with the Linux Kernel. There are a couple of pages which I believe might be helpful here, the first is the guide on building the Linux Kernel: * https://kernelnewbies.org/KernelBuild ... the second page is a general landing page which has links to other kernel related guides and resources: * https://kernelnewbies.org/KernelHacking These info on these pages should help you get started but if you have any additional questions let us know. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: Help with setting up Auditd kernel repository
On Mon, Mar 20, 2023 at 8:20 AM Anurag Aggarwal wrote: > > Hello All, > > Will anyone help me with some documentation on how to build and test > auditd-kernel repository? Can you be more specific about the exact repository? There is a repository which contains the development code for the Linux Kernel's audit subsystem, and there is a second repository which contains Steve's audit userspace code (which contains auditd as a component). There is no combined "auditd-kernel" repository. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: audit userspace problems with io_uring async ops
On Tue, Mar 7, 2023 at 4:17 PM Steve Grubb wrote: > > Hello Paul, > > On Tuesday, February 28, 2023 5:04:04 PM EST Paul Moore wrote: > > ... if you look closely you'll notice that the #289 event (the async > > URINGOP) is missing from the ausearch output. > > Thanks for the bug report. Let me know if you see anything else. > > Upstream commit 7d35e14 should fix parsing URINGOP and DM_CTRL records. Finally got a chance to try the fix, and it looks like it solves the problem for me. Thanks. In case anyone wants a hacky patched source RPM, I put the copy I'm using at the link below: * https://drop.paul-moore.com/120.OH1C/audit-3.1-2.1.secnext.fc39.src.rpm [The link above should work for the next 120 days] -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: Auditing nftables changes
On Fri, Mar 10, 2023 at 9:36 AM Steve Grubb wrote: > > On Thursday, March 9, 2023 5:52:28 PM EST Bruce Elrick wrote: > > Anyway, I think I need to spend some time playing until that "aha!" > > moment comes. It's feels a lot closer thanks to both of your responses > > and I really apprecaite the time you've taken to read my emails and > > respond to them. > > There are simple events which are one line and compound events which are > multiple lines - called records. The simple events tend to be hardwired and > not optional. For example, logins are hardwired; kernel config changes are > hardwired; authentication is hardwired. Reading Steve's response I'm not sure we use the same terminology, or perhaps we explain it a bit differently. Regardless, here is a quick definition that I stick to when discussing audit: "audit record": An audit record is a single line in the audit log that consists of a timestamp, record type (type=XXX), and a series of fields which are dependent on the record type. Here is an example of a SYSCALL record: type=SYSCALL msg=audit(03/10/2023 10:59:00.797:563) : arch=x86_64 syscall=bpf success=yes exit=12 a0=BPF_PROG_LOAD a1=0x7ffde0efc650 a2=0x80 a3=0x13 items=0 ppid=1 pid=2683 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=(none) ses=10 comm=systemd exe=/usr/lib/systemd/systemd subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null) "audit event": An audit event consists of multiple audit records grouped together by a single timestamp. Single record audit events are allowed and do exist. There is no upper bound on the number of records allowed in an audit event. Here is an example of an audit event consisting of PROCTITLE, SYSCALL, and BPF audit records: type=PROCTITLE msg=audit(03/10/2023 10:59:00.797:563) : proctitle=(systemd) type=SYSCALL msg=audit(03/10/2023 10:59:00.797:563) : arch=x86_64 syscall=bpf success=yes exit=12 a0=BPF_PROG_LOAD a1=0x7ffde0efc650 a2=0x80 a3=0x13 items=0 ppid=1 pid=2683 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=(none) ses=10 comm=systemd exe=/usr/lib/systemd/systemd subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null) type=BPF msg=audit(03/10/2023 10:59:00.797:563) : prog-id=172 op=LOAD I hope that helps. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: Auditing nftables changes
On Thu, Mar 9, 2023 at 11:33 AM Bruce Elrick wrote:
>
> I think I need to clarify where I'm confused ;-)
>
> With iptables you could write a rule that would only catch system
> calls that were for iptables changes. That is, you didn't need to
> capture *all* setsockopt calls (not that there would be lots of
> *those*) but rather you could add the a2=64 to only get the
> op=IPT_SO_SET_REPLACE ones.
>
> With netfilter, however, since the control interface is netlink and
> netlink requires a message to a socket and messages are structs, there
> is no way to have a similarly narrow audit rule as in the case of
> iptables.
>
> That's the first thing I want to confirm: whether my understanding
> above is correct?
Yes, you are correct.
> I'm confused because your answer implies I'm correct
> but you didn't explicitly confirm that my interpretation of how it
> works was correct.
>
> You talk about having an exclude filter on NETFILTER_CFG (or rather
> exclude everything except NETFILTER_CFG??) but my understanding is
> that you can only do that filtering after the fact using ausearch or
> writing some sort of correlation code using the auparse library.
The kernel implements an exclude filter which is described in the
auditctl(8) manpage:
"Add a rule to the event type exclusion filter list.
This list is used to filter events that you do not
want to see. For example, if you do not want to see
any avc messages, you would using this list to
record that. Events can be excluded by process ID,
user ID, group ID, login user ID, message type,
subject context, or executable name. The action is
ignored and uses its default of "never".
Taken from https://man7.org/linux/man-pages/man8/auditctl.8.html
However, in my last reply I wasn't advocating for this use of the
exclude filter, I was simply trying to explain that unless you are
explicitly excluding the creation of NETFILTER_CFG records via the
exclude filter you should be seeing NETFILTER_CFG in your audit stream
with basic auditing enabled.
> It just seemed surprising that there is a non-trivial loss of audit
> functionality but that I could not find any obvious discussion about
> that. By obvious discussion I mean as explicitly as what I'm trying to
> say here.
Unfortunately it is a fairly common practice for kernel features to be
added, and removed, without consulting with the various Linux Kernel
security developers, e.g. audit, SELinux, LSM, etc. Sometimes we are
successful in retrofitting the necessary security and/or auditing
hooks, sometimes we are limited due to design choices.
> The other thing I'm trying to understand is how heavy an audit load
> would it be to have an audit rule that captures *all* sendmsg calls
> (well, all except where auid=-1 or auid=${serviceuser_uid}). I don't
> have a good enough understanding of systems programming to know where
> and how often the sendmsg is called. Of course I know this is highly
> dependent on workload, but my knowledge is limited enough that I I can
> convince myself both that the audit load would be not trivial but
> still manageable in most cases but also I can convince myself that no
> same sysadmin would consider running such an audit rule. With file IO
> it's easy to distinguish that file opens are worth auditing but file
> reads and writes would be insane to audit. It's not so clear for me
> for sockets.
This is going to be dependent on both the workloads and applications
used on the system, there is no one "right" answer here.
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: Auditing nftables changes
On Wed, Mar 8, 2023 at 7:13 PM Bruce Elrick wrote:
> Hello all,
>
> I'm not sure if this list is appropriate for questions so please let
> me know and otherwise ignore if this message is not appropriate.
>
> I'm trying to help someone who is finally migrating from iptables to
> nftables on the back-end and needs to therefore migrate their audit
> capability.
>
> Currently they have a single simple audit rule to detect when there is
> a iptable change from any audit user apart from their service user
> using a rule like the accepted answer given in this[0] StackExchange
> question, although with added filters on the auid (I have to admit I
> don't know the origin of auid=-1 events):
>
> auditctl -a exit,always -F arch=b64 -F a2=64 -F auid!=-1 -F
> auid!=${serviceuser_uid} -S setsockopt -k iptablesChange
>
> They are migrating from Ubuntu bionic to jammy and still using the
> iptables front-end but since the back-end changes from default
> iptables to default nftables they need to change their audit rules
>
> They did strace testing and noted the syscall changing from
>
> setsockopt(4, SOL_IP, IPT_SO_SET_REPLACE,
> "filter\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
> 80952) = 0
>
> to
>
> sendto(3, [{nlmsg_len=20,
> nlmsg_type=NFNL_SUBSYS_NFTABLES<<8|NFT_MSG_GETGEN,
> nlmsg_flags=NLM_F_REQUEST, nlmsg_seq=0, nlmsg_pid=0},
> {nfgen_family=AF_UNSPEC, version=NFNETLINK_V0, res_id=htons(0)}], 20,
> 0, {sa_family=AF_NETLINK, nl_pid=0, nl_groups=}, 12) = 20
>
> between the two versions.
>
> In my own testing, I decided to approach from the audit tools
> perspective so I created a broad rule to capture all system call
> related to a test user:
>
> auditctl -a always,exit -S all -F auid=1001 # 1001 is uid of testuser
>
> Then I tried various operations using my testuser such as
> iptables-restore of either a default-accept rule set with no rules or
> with one or two simple drop rules. I also tested adding just a single
> iptables rule. I then used ausearch to discover what the audit system
> captured:
>
> # ausearch -i -m NETFILTER_CFG
> ...
>
> type=PROCTITLE msg=audit(03/07/2023 17:18:55.152:143044) :
> proctitle=iptables-restore
> type=SYSCALL msg=audit(03/07/2023 17:18:55.152:143044) :
> arch=x86_64 syscall=sendmsg success=yes exit=764 a0=0x3
> a1=0x7ffdb0e98db0 a2=0x0 a3=0x7ffdb0e98d9c items=0 ppid=5673 pid=5676
> auid=testuser uid=root gid=root euid=root suid=root fsuid=root
> egid=root sgid=root fsgid=root tty=pts2 ses=108 comm=iptables-restor
> exe=/usr/sbin/xtables-nft-multi subj=unconfined key=(null)
> type=NETFILTER_CFG msg=audit(03/07/2023 17:18:55.152:143044) :
> table=filter:30 family=ipv4 entries=12 op=nft_unregister_table
> pid=5676 subj=unconfined comm=iptables-restor
> type=NETFILTER_CFG msg=audit(03/07/2023 17:18:55.152:143044) :
> table=filter:30 family=ipv4 entries=7 op=nft_register_chain pid=5676
> subj=unconfined comm=iptables-restor
>
> type=PROCTITLE msg=audit(03/07/2023 17:23:04.390:144459) :
> proctitle=sudo /usr/sbin/iptables -A OUTPUT -d 10.100.249.64 -j DROP
> type=SOCKADDR msg=audit(03/07/2023 17:23:04.390:144459) : saddr={
> saddr_fam=netlink nlnk-fam=16 nlnk-pid=0 }
> type=SYSCALL msg=audit(03/07/2023 17:23:04.390:144459) :
> arch=x86_64 syscall=sendmsg success=yes exit=304 a0=0x3
> a1=0x7ffc80659110 a2=0x0 a3=0x7ffc806590fc items=0 ppid=5703 pid=5704
> auid=testuser uid=root gid=root euid=root suid=root fsuid=root
> egid=root sgid=root fsgid=root tty=pts2 ses=108 comm=iptables
> exe=/usr/sbin/xtables-nft-multi subj=unconfined key=(null)
> type=NETFILTER_CFG msg=audit(03/07/2023 17:23:04.390:144459) :
> table=filter:31 family=ipv4 entries=1 op=nft_register_rule pid=5704
> subj=unconfined comm=iptables
>
> The event sequences seem to make sense with the sockaddr function
> selecting the netlink family which agrees with the strace output.
>
> With the change in the back-end to nftables, I can see in either case
> that the setsockopt system call with a nice, crisp, single argument
> (a2=64/IPT_SO_SET_REPLACE) option with either a sendto or sendmsg
> system call but with a pointer to a message structure. I read that
> audit rules cannot filter using data inside struct arguments.
>
> My naive interpretation of this is that I'd need to have a rule that
> captures all sendmsg syscalls with (auid!=-1 and
> auid!=${serviceuser_uid} but I don't know enough about socket syscall
> usage to know whether this is too much. I see that write(2) to a
> socket is the same as send(2) without the flags so I might assume that
> most socket syscalls that are sending data use write(2) and not
> send/sendto/sendmsg(2) but I worry this would be too much audit data.
>
> Anyone care to comment or point me in the correct direction?
The problem I think you're going to have, and I believe you've already
suspected it, is that auditing socket writes is going to result in a
firehose of records. However, unless you have
Re: Key based rate limiter (audit_set_rate_limit)
On Wed, Mar 8, 2023 at 6:53 AM Anurag Aggarwal wrote: >> Limiting of audit records is actually done in the kernel, and >> currently the rate limit applies equally[1] to all records, there is >> no ability to enforce limits per-key. > > One question Paul, will it be ok, if we contribute something similar to the > Auditd Kernel repository? I don't like telling people *not* to work on improvements to the kernel, I'm happy to see more contributors, especially in the audit space :) However, I am fairly skeptical that we could add per-key rate limiting without introducing a non-trivial amount of overhead to record generation, which would be a show stopper for this feature given its expected limited appeal. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: audit userspace problems with io_uring async ops
On Tue, Mar 7, 2023 at 4:17 PM Steve Grubb wrote: > > Hello Paul, > > On Tuesday, February 28, 2023 5:04:04 PM EST Paul Moore wrote: > > ... if you look closely you'll notice that the #289 event (the async > > URINGOP) is missing from the ausearch output. > > Thanks for the bug report. Let me know if you see anything else. > > Upstream commit 7d35e14 should fix parsing URINGOP and DM_CTRL records. Thanks Steve. I'm working through the post merge window batch of reviews/merging, but I'll give that commit a shot and let you know how it goes. > Btw, has anyone ever seen a DM_CTRL record? I don't think they are following > our > guidelines. They were added back in the v5.16 timeframe: * https://www.paul-moore.com/blog/d/2022/01/linux_v516.html ... with patches first being posted to the linux-audit@redhat list in August 2021: * https://lore.kernel.org/linux-audit/20210812145748.4460-1-michael.we...@aisec.fraunhofer.de -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: audit userspace problems with io_uring async ops
On Mon, Mar 6, 2023 at 3:58 PM Steve Grubb wrote: > > Hello Paul, > > On Monday, March 6, 2023 3:07:37 PM EST Paul Moore wrote: > > On Tue, Feb 28, 2023 at 5:04 PM Paul Moore wrote: > > > Hi all, > > > > > > We just recently started picking up audit-testsuite failures with the > > > latest upstream kernels and I tracked it down to a change in how the > > > IORING_OP_OPENAT operation is handled, and how Steve's audit userspace > > > displays async io_uring ops. It appears that when ausearch is used to > > > look for events it doesn't display async io_uring events (URINGOP > > > records/events without an associated SYSCALL record/event). Take the > > > following snippet from /var/log/audit/audit.log ... > > > > Hi Steve, > > > > Before I start working around this in the audit-testsuite I just > > wanted to check and see if you already had a fix for ausearch? > > Thanks for the bug report. I have been out for the last 8 days and now have > ~3600 unread emails. I will try look into this tomorrow. Great thanks. I'll hold off an a workaround. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: audit userspace problems with io_uring async ops
On Tue, Feb 28, 2023 at 5:04 PM Paul Moore wrote: > > Hi all, > > We just recently started picking up audit-testsuite failures with the > latest upstream kernels and I tracked it down to a change in how the > IORING_OP_OPENAT operation is handled, and how Steve's audit userspace > displays async io_uring ops. It appears that when ausearch is used to > look for events it doesn't display async io_uring events (URINGOP > records/events without an associated SYSCALL record/event). Take the > following snippet from /var/log/audit/audit.log ... Hi Steve, Before I start working around this in the audit-testsuite I just wanted to check and see if you already had a fix for ausearch? -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v9 07/16] uapi|audit|ipe: add ipe auditing support
On Tue, Jan 31, 2023 at 12:11 PM Steve Grubb wrote: > > Hello, > > On Monday, January 30, 2023 5:57:22 PM EST Fan Wu wrote: > > From: Deven Bowers > > > > Users of IPE require a way to identify when and why an operation fails, > > allowing them to both respond to violations of policy and be notified > > of potentially malicious actions on their systens with respect to IPE > > itself. > > > > The new 1420 audit, AUDIT_IPE_ACCESS indicates the result of a policy > > evaulation of a resource. The other two events, AUDIT_MAC_POLICY_LOAD, > > and AUDIT_MAC_CONFIG_CHANGE represent a new policy was loaded into the > > kernel and the currently active policy changed, respectively. > > Typically when you reuse an existing record type, it is expected to maintain > the same fields in the same order. Also, it is expect that fields that are > common across diferent records have the same meaning. To aid in this, we have > a field dictionary here: > > https://github.com/linux-audit/audit-documentation/blob/main/specs/fields/ > field-dictionary.csv > > For example, dev is expected to be 2 hex numbers separated by a colon which > are the device major and minor numbers. But down a couple lines from here, we > find dev="tmpfs". But isn't that a filesystem type? What Steve said. I'll also add an administrative note, we just moved upstream Linux audit development to a new mailing list, au...@vger.kernel.org, please use that in future patch submissions. As a positive, it's a fully open list so you won't run into moderation delays/notifications/etc. > > This patch also adds support for success auditing, allowing users to > > identify how a resource passed policy. It is recommended to use this > > option with caution, as it is quite noisy. > > > > This patch adds the following audit records: > > > > audit: AUDIT1420 path="/tmp/tmpwxmam366/deny/bin/hello" dev="tmpfs" > > ino=72 rule="DEFAULT op=EXECUTE action=DENY" > > Do we really need to log the whole rule? Fan, would it be reasonable to list the properties which caused the access denial? That seems like it might be more helpful than the specific rule, or am I missing something? > > The above audit record shows IPE blocked a file > > /tmp/tmpwxmam366/deny/bin/hello in the temp file system. > > > > audit: AUDIT1420 path="/tmp/tmpxkvb3d9x/deny/bin/hello" dev="tmpfs" > > ino=157 rule="DEFAULT action=DENY" > > > > The above audit record shows IPE blocked a file > > /tmp/tmpxkvb3d9x/deny/bin/hello in the temp file system via another > > rule. > > > > audit: MAC_POLICY_LOAD policy_name="dmverity_roothash" > > policy_version=0.0.0 sha256=DC67AC19E05894EFB3170A8E55DE529794E248C2 > > auid=4294967295 ses=4294967295 lsm=ipe res=1 > > The MAC_POLICY_LOAD record type simply states the lsm that had it's policy > loaded. There isn't name, version, and hash information. I'd prefer to see > all users of this record type decide if it should be extended because they > also have that information available to record. Not all LSMs which load policy have that information; as an example, SELinux doesn't have the concept of a policy name or version. The SELinux policy version you might see in the kernel sources refers to the policy format version and has no bearing on the actual policy content beyond that dictated by the format. If additional information is required by IPE, perhaps an auxiliary IPE policy load record could be created with those additional fields. > > The above audit record shows IPE loaded a new policy named > > "dmverity_roothash" with the sha256 hash of the policy. > > > > audit: MAC_CONFIG_CHANGE old_active_pol_name="Allow_All" > > old_active_pol_version=0.0.0 > > old_sha256=DA39A3EE5E6B4B0D3255BFEF95601890AFD80709 > > new_active_pol_name="dmverity_roothash" new_active_pol_version=0.0.0 > > new_sha256=DC67AC19E05894EFB3170A8E55DE529794E248C2 > > auid=4294967295 ses=4294967295 lsm=ipe res=1 > > > > The above audit record shows IPE's active policy switched from > > "Allow_All" to "dmverity_roothash". > > Shouldn't this just be another MAC_POLICY_LOAD? That would match other LSM's. > The MAC_CONFIG_CHANGE is to denote that a changeable option was modified from > one value to another. But it is still operating under the same policy. If it is just switching from one previously loaded policy to another, it seems like MAC_CONFIG_CHANGE might be the best choice. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v9 02/16] ipe: add policy parser
On Mon, Jan 30, 2023 at 5:58 PM Fan Wu wrote:
>
> From: Deven Bowers
>
> IPE's interpretation of the what the user trusts is accomplished through
> its policy. IPE's design is to not provide support for a single trust
> provider, but to support multiple providers to enable the end-user to
> choose the best one to seek their needs.
>
> This requires the policy to be rather flexible and modular so that
> integrity providers, like fs-verity, dm-verity, dm-integrity, or
> some other system, can plug into the policy with minimal code changes.
>
> Signed-off-by: Deven Bowers
> Signed-off-by: Fan Wu
...
> ---
> security/ipe/Makefile| 2 +
> security/ipe/policy.c| 99 +++
> security/ipe/policy.h| 77 ++
> security/ipe/policy_parser.c | 515 +++
> security/ipe/policy_parser.h | 11 +
> 5 files changed, 704 insertions(+)
> create mode 100644 security/ipe/policy.c
> create mode 100644 security/ipe/policy.h
> create mode 100644 security/ipe/policy_parser.c
> create mode 100644 security/ipe/policy_parser.h
>
> diff --git a/security/ipe/Makefile b/security/ipe/Makefile
> index 571648579991..16bbe80991f1 100644
> --- a/security/ipe/Makefile
> +++ b/security/ipe/Makefile
> @@ -8,3 +8,5 @@
> obj-$(CONFIG_SECURITY_IPE) += \
> hooks.o \
> ipe.o \
> + policy.o \
> + policy_parser.o \
> diff --git a/security/ipe/policy.c b/security/ipe/policy.c
> new file mode 100644
> index ..e446f4b84152
> --- /dev/null
> +++ b/security/ipe/policy.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) Microsoft Corporation. All rights reserved.
> + */
> +
> +#include "ipe.h"
> +#include "policy.h"
> +#include "policy_parser.h"
> +#include "digest.h"
> +
> +#include
Generally speaking the system/kernel-wide header files, e.g. headers
using '<...>', tend to come before the local header files, e.g.
headers using '"..."'. I wouldn't consider this a hard rule, but
unless you have a reason to put the local header files first I would
stick with convention here.
> +/**
> + * ipe_free_policy - Deallocate a given IPE policy.
> + * @p: Supplies the policy to free.
> + *
> + * Safe to call on IS_ERR/NULL.
> + */
> +void ipe_free_policy(struct ipe_policy *p)
> +{
> + if (IS_ERR_OR_NULL(p))
> + return;
> +
> + free_parsed_policy(p->parsed);
> + if (!p->pkcs7)
> + kfree(p->text);
> + kfree(p->pkcs7);
> + kfree(p);
> +}
> +
> +static int set_pkcs7_data(void *ctx, const void *data, size_t len,
> + size_t asn1hdrlen)
> +{
> + struct ipe_policy *p = ctx;
> +
> + p->text = (const char *)data;
> + p->textlen = len;
> +
> + return 0;
> +}
> +
> +/**
> + * ipe_new_policy - Allocate and parse an ipe_policy structure.
> + *
> + * @text: Supplies a pointer to the plain-text policy to parse.
> + * @textlen: Supplies the length of @text.
> + * @pkcs7: Supplies a pointer to a pkcs7-signed IPE policy.
> + * @pkcs7len: Supplies the length of @pkcs7.
> + *
> + * @text/@textlen Should be NULL/0 if @pkcs7/@pkcs7len is set.
> + *
> + * The result will still need to be associated with a context via
> + * ipe_add_policy.
> + *
> + * Return:
> + * * !IS_ERR - Success
> + * * -EBADMSG - Policy is invalid
> + * * -ENOMEM - Out of memory
> + */
> +struct ipe_policy *ipe_new_policy(const char *text, size_t textlen,
> + const char *pkcs7, size_t pkcs7len)
> +{
> + int rc = 0;
> + struct ipe_policy *new = NULL;
> +
> + new = kzalloc(sizeof(*new), GFP_KERNEL);
> + if (!new)
> + return ERR_PTR(-ENOMEM);
> +
> + if (!text) {
> + new->pkcs7len = pkcs7len;
> + new->pkcs7 = kmemdup(pkcs7, pkcs7len, GFP_KERNEL);
> + if (!new->pkcs7) {
> + rc = -ENOMEM;
> + goto err;
As Roberto already pointed out, and you acknowledged, this leaks @new.
However, as a FYI for future work, if you have a label with only one
return instruction after the jump, e.g. the 'err' label here, you
should replace the 'goto' with the single return instruction. Jumping
just to immediately return is a bit silly, but if you also need to
cleanup, e.g. free some memory, that's okay to use the goto/jump.
> + }
> +
> + rc = verify_pkcs7_signature(NULL, 0, new->pkcs7, pkcs7len,
> NULL,
> + VERIFYING_UNSPECIFIED_SIGNATURE,
> + set_pkcs7_data, new);
> + if (rc)
> + goto err;
> + } else {
> + new->textlen = textlen;
> + new->text = kstrdup(text, GFP_KERNEL);
> + if (!new->text) {
> + rc = -ENOMEM;
> + goto err;
> + }
> + }
> +
> + rc =
Re: [RFC PATCH v9 08/16] ipe: add permissive toggle
On Mon, Jan 30, 2023 at 5:58 PM Fan Wu wrote:
>
> From: Deven Bowers
>
> IPE, like SELinux, supports a permissive mode. This mode allows policy
> authors to test and evaluate IPE policy without it effecting their
> programs. When the mode is changed, a 1404 AUDIT_MAC_STATUS
> be reported.
>
> This patch adds the following audit records:
>
> audit: MAC_STATUS permissive=1 auid=4294967295 ses=4294967295 lsm=ipe
> res=1
> audit: MAC_STATUS permissive=0 auid=4294967295 ses=4294967295 lsm=ipe
> res=1
>
> These records are emitted within the following events:
>
> audit: MAC_STATUS permissive=1 auid=4294967295 ses=4294967295 lsm=ipe
> res=1
> audit[185]: SYSCALL arch=c03e syscall=1 success=yes exit=2 a0=1
> a1=56308bb3ecc0 a2=2 a3=7f290fdc53e0 items=0 ppid=183 pid=185
> auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
> tty=pts0 ses=4294967295 comm="bash" exe="/usr/bin/bash" key=(null)
> audit: PROCTITLE proctitle="-bash"
> audit: MAC_STATUS permissive=0 auid=4294967295 ses=4294967295 lsm=ipe
> res=1
> audit[185]: SYSCALL arch=c03e syscall=1 success=yes exit=2 a0=1
> a1=56308bb3ecc0 a2=2 a3=7f290fdc53e0 items=0 ppid=183 pid=185
> auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
> tty=pts0 ses=4294967295 comm="bash" exe="/usr/bin/bash" key=(null)
> audit: PROCTITLE proctitle="-bash"
>
> Implying user used bash to toggle the switch.
>
> Signed-off-by: Deven Bowers
> Signed-off-by: Fan Wu
...
> ---
> security/ipe/audit.c | 36 +++
> security/ipe/audit.h | 1 +
> security/ipe/eval.c | 9 ++
> security/ipe/eval.h | 1 +
> security/ipe/fs.c| 69 ++--
> 5 files changed, 114 insertions(+), 2 deletions(-)
>
> diff --git a/security/ipe/audit.c b/security/ipe/audit.c
> index 295e9f9f5146..ff74026a595f 100644
> --- a/security/ipe/audit.c
> +++ b/security/ipe/audit.c
> @@ -194,3 +194,39 @@ void ipe_audit_policy_load(const struct ipe_policy
> *const p)
>
> audit_log_end(ab);
> }
> +
> +/**
> + * ipe_audit_enforce - Audit a change in IPE's enforcement state.
> + */
> +void ipe_audit_enforce(void)
> +{
> + struct audit_buffer *ab;
> +
> + ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_MAC_STATUS);
> + if (!ab)
> + return;
> +
> + audit_log_format(ab, "permissive=%d", !READ_ONCE(enforce));
> + audit_log_format(ab, " auid=%u ses=%u lsm=ipe res=1",
> +from_kuid(_user_ns,
> audit_get_loginuid(current)),
> +audit_get_sessionid(current));
> +
> + audit_log_end(ab);
> +}
See the earlier comments in the patchset about consistent formatting
of a given record type. To the best of my knowledge only SELinux
currently uses the AUDIT_MAC_STATUS record and an example can be found
in `sel_write_enforce()`. The good news is that it looks like that
format could be made to work here without too much fuss.
> +/**
> + * emit_enforcement - Emit the enforcement state of IPE started with.
> + *
> + * Return:
> + * 0 - Always
> + */
> +static int emit_enforcement(void)
> +{
> + if (!ipe_enabled)
> + return -EOPNOTSUPP;
> +
> + ipe_audit_enforce();
> + return 0;
> +}
> +
> +late_initcall(emit_enforcement);
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v9 09/16] block|security: add LSM blob to block_device
On Mon, Jan 30, 2023 at 5:58 PM Fan Wu wrote:
>
> From: Deven Bowers
>
> block_device structures can have valuable security properties,
> based on how they are created, and what subsystem manages them.
>
> By adding LSM storage to this structure, this data can be accessed
> at the LSM layer.
>
> Signed-off-by: Deven Bowers
> Signed-off-by: Fan Wu
> Reviewed-by: Casey Schaufler
...
> ---
> block/bdev.c | 7
> include/linux/blk_types.h | 3 ++
> include/linux/lsm_hook_defs.h | 5 +++
> include/linux/lsm_hooks.h | 12 ++
> include/linux/security.h | 22 +++
> security/security.c | 70 +++
> 6 files changed, 119 insertions(+)
>
> diff --git a/block/bdev.c b/block/bdev.c
> index edc110d90df4..f8db53b47c00 100644
> --- a/block/bdev.c
> +++ b/block/bdev.c
> @@ -24,6 +24,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -396,6 +397,11 @@ static struct inode *bdev_alloc_inode(struct super_block
> *sb)
> if (!ei)
> return NULL;
> memset(>bdev, 0, sizeof(ei->bdev));
> +
> + if (security_bdev_alloc(>bdev)) {
> + kmem_cache_free(bdev_cachep, ei);
> + return NULL;
> + }
> return >vfs_inode;
> }
>
> @@ -405,6 +411,7 @@ static void bdev_free_inode(struct inode *inode)
>
> free_percpu(bdev->bd_stats);
> kfree(bdev->bd_meta_info);
> + security_bdev_free(bdev);
>
> if (!bdev_is_partition(bdev)) {
> if (bdev->bd_disk && bdev->bd_disk->bdi)
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index 99be590f952f..137a04f45c17 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -68,6 +68,9 @@ struct block_device {
> #ifdef CONFIG_FAIL_MAKE_REQUEST
> boolbd_make_it_fail;
> #endif
> +#ifdef CONFIG_SECURITY
> + void*security;
> +#endif
> } __randomize_layout;
>
> #define bdev_whole(_bdev) \
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index ed6cb2ac55fa..1f79029c9e28 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -417,3 +417,8 @@ LSM_HOOK(int, 0, uring_override_creds, const struct cred
> *new)
> LSM_HOOK(int, 0, uring_sqpoll, void)
> LSM_HOOK(int, 0, uring_cmd, struct io_uring_cmd *ioucmd)
> #endif /* CONFIG_IO_URING */
> +
> +LSM_HOOK(int, 0, bdev_alloc_security, struct block_device *bdev)
> +LSM_HOOK(void, LSM_RET_VOID, bdev_free_security, struct block_device *bdev)
> +LSM_HOOK(int, 0, bdev_setsecurity, struct block_device *bdev, const char
> *name,
> +const void *value, size_t size)
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 0a5ba81f7367..b622ceb57d83 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1618,6 +1618,17 @@
> * @what: kernel feature being accessed.
> * Return 0 if permission is granted.
> *
> + * @bdev_alloc_security:
> + * Initialize the security field inside a block_device structure.
> + *
> + * @bdev_free_security:
> + * Cleanup the security information stored inside a block_device
> structure.
> + *
> + * @bdev_setsecurity:
> + * Set a security property associated with @name for @bdev with
> + * value @value. @size indicates the size of @value in bytes.
> + * If a @name is not implemented, return -EOPNOTSUPP.
> + *
Just a heads-up that the LSM hook comment blocks are moving to
security/security.c very soon now (if they are not already there by
the time you read this).
https://lore.kernel.org/linux-security-module/20230217032625.678457-1-p...@paul-moore.com
> diff --git a/security/security.c b/security/security.c
> index d1571900a8c7..5c81dd3b1350 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2705,6 +2730,51 @@ int security_locked_down(enum lockdown_reason what)
> }
> EXPORT_SYMBOL(security_locked_down);
>
> +int security_bdev_alloc(struct block_device *bdev)
> +{
> + int rc = 0;
> +
> + rc = lsm_bdev_alloc(bdev);
> + if (unlikely(rc))
> + return rc;
> +
> + rc = call_int_hook(bdev_alloc_security, 0, bdev);
> + if (unlikely(rc))
> + security_bdev_free(bdev);
> +
> + return LSM_RET_DEFAULT(bdev_alloc_security);
> +}
> +EXPORT_SYMBOL(security_bdev_alloc);
> +
> +void security_bdev_free(struct block_device *bdev)
> +{
> + if (!bdev->security)
> + return;
> +
> + call_void_hook(bdev_free_security, bdev);
> +
> + kfree(bdev->security);
> + bdev->security = NULL;
> +}
> +EXPORT_SYMBOL(security_bdev_free);
> +
> +int security_bdev_setsecurity(struct block_device *bdev,
> + const char *name, const void *value,
> + size_t size)
> +{
> + int rc = 0;
> + struct
Re: [RFC PATCH v9 05/16] ipe: add userspace interface
On Mon, Jan 30, 2023 at 5:58 PM Fan Wu wrote:
>
> From: Deven Bowers
>
> As is typical with LSMs, IPE uses securityfs as its interface with
> userspace. for a complete list of the interfaces and the respective
> inputs/outputs, please see the documentation under
> admin-guide/LSM/ipe.rst
>
> Signed-off-by: Deven Bowers
> Signed-off-by: Fan Wu
...
> ---
> security/ipe/Makefile| 2 +
> security/ipe/fs.c| 101 +
> security/ipe/fs.h| 17 ++
> security/ipe/ipe.c | 3 +
> security/ipe/ipe.h | 2 +
> security/ipe/policy.c| 135
> security/ipe/policy.h| 7 +
> security/ipe/policy_fs.c | 459 +++
> 8 files changed, 726 insertions(+)
> create mode 100644 security/ipe/fs.c
> create mode 100644 security/ipe/fs.h
> create mode 100644 security/ipe/policy_fs.c
...
> diff --git a/security/ipe/policy.c b/security/ipe/policy.c
> index 772d876b1087..a5e9c6e5691b 100644
> --- a/security/ipe/policy.c
> +++ b/security/ipe/policy.c
> @@ -4,12 +4,39 @@
> */
>
> #include "ipe.h"
> +#include "eval.h"
> +#include "fs.h"
> #include "policy.h"
> #include "policy_parser.h"
> #include "digest.h"
>
> #include
>
> +/* lock for synchronizing writers across ipe policy */
> +DEFINE_SPINLOCK(ipe_policy_lock);
> +
> +/**
> + * ver_to_u64 - Convert an internal ipe_policy_version to a u64.
> + * @p: Policy to extract the version from.
> + *
> + * Bits (LSB is index 0):
> + * [48,32] -> Major
> + * [32,16] -> Minor
> + * [16, 0] -> Revision
> + *
> + * Return: u64 version of the embedded version structure.
> + */
> +static inline u64 ver_to_u64(const struct ipe_policy *const p)
> +{
> + u64 r = 0;
No need to set @r to 0 since you set it to the version immediately below.
> + r = (((u64)p->parsed->version.major) << 32)
> + | (((u64)p->parsed->version.minor) << 16)
> + | ((u64)(p->parsed->version.rev));
> +
> + return r;
> +}
> +
> /**
> * ipe_free_policy - Deallocate a given IPE policy.
> * @p: Supplies the policy to free.
> @@ -21,6 +48,7 @@ void ipe_free_policy(struct ipe_policy *p)
> if (IS_ERR_OR_NULL(p))
> return;
>
> + ipe_del_policyfs_node(p);
> free_parsed_policy(p->parsed);
> if (!p->pkcs7)
> kfree(p->text);
> @@ -39,6 +67,70 @@ static int set_pkcs7_data(void *ctx, const void *data,
> size_t len,
> return 0;
> }
>
> +/**
> + * ipe_update_policy - parse a new policy and replace @old with it.
> + * @addr: Supplies a pointer to the i_private for saving policy.
> + * @text: Supplies a pointer to the plain text policy.
> + * @textlen: Supplies the length of @text.
> + * @pkcs7: Supplies a pointer to a buffer containing a pkcs7 message.
> + * @pkcs7len: Supplies the length of @pkcs7len.
> + *
> + * @text/@textlen is mutually exclusive with @pkcs7/@pkcs7len - see
> + * ipe_new_policy.
> + *
> + * Return:
> + * * !IS_ERR - OK
> + * * -ENOENT - Policy doesn't exist
> + * * -EINVAL - New policy is invalid
> + */
> +struct ipe_policy *ipe_update_policy(struct ipe_policy __rcu **addr,
> +const char *text, size_t textlen,
> +const char *pkcs7, size_t pkcs7len)
> +{
> + int rc = 0;
> + struct ipe_policy *old, *new;
> +
> + old = ipe_get_policy_rcu(*addr);
> + if (!old) {
> + rc = -ENOENT;
> + goto err;
> + }
> +
> + new = ipe_new_policy(text, textlen, pkcs7, pkcs7len);
> + if (IS_ERR(new)) {
> + rc = PTR_ERR(new);
> + goto err;
> + }
> +
> + if (strcmp(new->parsed->name, old->parsed->name)) {
> + rc = -EINVAL;
> + goto err;
> + }
> +
> + if (ver_to_u64(old) > ver_to_u64(new)) {
> + rc = -EINVAL;
> + goto err;
> + }
> +
> + if (ipe_is_policy_active(old)) {
I don't understand the is-active check, you want to make @new the new
active policy regardless, right? Could this is-active check ever be
false?
> + spin_lock(_policy_lock);
> + rcu_assign_pointer(ipe_active_policy, new);
> + spin_unlock(_policy_lock);
> + synchronize_rcu();
> + }
> +
> + rcu_assign_pointer(*addr, new);
> +
> + swap(new->policyfs, old->policyfs);
> + ipe_free_policy(old);
> +
> + goto out;
> +err:
> + ipe_free_policy(new);
> +out:
> + return (rc < 0) ? ERR_PTR(rc) : new;
> +}
> +
> /**
> * ipe_new_policy - Allocate and parse an ipe_policy structure.
> *
> @@ -117,3 +209,46 @@ struct ipe_policy *ipe_get_policy_rcu(struct ipe_policy
> __rcu *p)
>
> return rv;
> }
> +
> +/**
> + * ipe_set_active_pol - Make @p the active policy.
> + * @p: Supplies a pointer to the policy to make active.
> + */
> +int ipe_set_active_pol(const struct ipe_policy *p)
> +{
> + int
Re: [RFC PATCH v9 03/16] ipe: add evaluation loop and introduce 'boot_verified' as a trust provider
On Mon, Jan 30, 2023 at 5:58 PM Fan Wu wrote:
>
> From: Deven Bowers
>
> IPE must have a centralized function to evaluate incoming callers
> against IPE's policy. This iteration of the policy against the rules
> for that specific caller is known as the evaluation loop.
>
> In addition, IPE is designed to provide system level trust guarantees,
> this usually implies that trust starts from bootup with a hardware root
> of trust, which validates the bootloader. After this, the bootloader
> verifies the kernel and the initramfs.
>
> As there's no currently supported integrity method for initramfs, and
> it's typically already verified by the bootloader, introduce a property
> that causes the first superblock to have an execution to be "pinned",
> which is typically initramfs.
>
> Signed-off-by: Deven Bowers
> Signed-off-by: Fan Wu
...
> ---
> security/ipe/Makefile| 1 +
> security/ipe/eval.c | 180 +++
> security/ipe/eval.h | 28 ++
> security/ipe/hooks.c | 25 +
> security/ipe/hooks.h | 14 +++
> security/ipe/ipe.c | 1 +
> security/ipe/policy.c| 20
> security/ipe/policy.h| 3 +
> security/ipe/policy_parser.c | 8 +-
> 9 files changed, 279 insertions(+), 1 deletion(-)
> create mode 100644 security/ipe/eval.c
> create mode 100644 security/ipe/eval.h
> create mode 100644 security/ipe/hooks.c
> create mode 100644 security/ipe/hooks.h
>
> diff --git a/security/ipe/Makefile b/security/ipe/Makefile
> index 16bbe80991f1..d7f2870d7c09 100644
> --- a/security/ipe/Makefile
> +++ b/security/ipe/Makefile
> @@ -6,6 +6,7 @@
> #
>
> obj-$(CONFIG_SECURITY_IPE) += \
> + eval.o \
> hooks.o \
> ipe.o \
> policy.o \
> diff --git a/security/ipe/eval.c b/security/ipe/eval.c
> new file mode 100644
> index ..48b5104a3463
> --- /dev/null
> +++ b/security/ipe/eval.c
> @@ -0,0 +1,180 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) Microsoft Corporation. All rights reserved.
> + */
> +
> +#include "ipe.h"
> +#include "eval.h"
> +#include "hooks.h"
> +#include "policy.h"
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +struct ipe_policy __rcu *ipe_active_policy;
> +
> +static struct super_block *pinned_sb;
> +static DEFINE_SPINLOCK(pin_lock);
> +#define FILE_SUPERBLOCK(f) ((f)->f_path.mnt->mnt_sb)
> +
> +/**
> + * pin_sb - Pin the underlying superblock of @f, marking it as trusted.
> + * @f: Supplies a file structure to source the super_block from.
> + */
> +static void pin_sb(const struct file *f)
> +{
> + if (!f)
> + return;
> + spin_lock(_lock);
> + if (pinned_sb)
> + goto out;
> + pinned_sb = FILE_SUPERBLOCK(f);
> +out:
> + spin_unlock(_lock);
> +}
Since you don't actually use @f, just the super_block, you might
consider passing the super_block as the parameter and not the
associated file.
I'd probably also flip the if-then to avoid the 'goto', for example:
static void pin_sb(const struct super_block *sb)
{
if (!sb)
return;
spin_lock(_lock);
if (!pinned_sb)
pinned_sb = sb;
spin_unlock(_lock);
}
Also, do we need to worry about the initramfs' being unmounted and the
super_block going away?
> +/**
> + * from_pinned - Determine whether @f is source from the pinned super_block.
> + * @f: Supplies a file structure to check against the pinned super_block.
> + *
> + * Return:
> + * * true - @f is sourced from the pinned super_block
> + * * false - @f is not sourced from the pinned super_block
> + */
> +static bool from_pinned(const struct file *f)
> +{
> + bool rv;
> +
> + if (!f)
> + return false;
> + spin_lock(_lock);
> + rv = !IS_ERR_OR_NULL(pinned_sb) && pinned_sb == FILE_SUPERBLOCK(f);
> + spin_unlock(_lock);
> + return rv;
> +}
> +
> +/**
> + * build_eval_ctx - Build an evaluation context.
> + * @ctx: Supplies a pointer to the context to be populdated.
> + * @file: Supplies a pointer to the file to associated with the evaluation.
> + * @op: Supplies the IPE policy operation associated with the evaluation.
> + */
> +void build_eval_ctx(struct ipe_eval_ctx *ctx,
> + const struct file *file,
> + enum ipe_op_type op)
> +{
> + ctx->file = file;
> + ctx->op = op;
> + ctx->from_init_sb = from_pinned(file);
> +}
I was a little concerned about the spinlock around the pinned
superblock being a potential issue so I was checking the callers of
`build_eval_ctx()` and realized there are no callers in this patch ...
? Maybe it makes sense for `build_eval_ctx()` to be in this patch but
it seems a little odd.
> +/**
> + * evaluate_property - Analyze @ctx against a property.
> + * @ctx: Supplies a pointer to the context to be evaluated.
> + * @p: Supplies a pointer to the property to be
Re: [RFC PATCH v9 03/16] ipe: add evaluation loop and introduce 'boot_verified' as a trust provider
On Fri, Feb 10, 2023 at 6:21 PM Fan Wu wrote: > On Tue, Jan 31, 2023 at 04:49:44PM +0100, Roberto Sassu wrote: > > On Mon, 2023-01-30 at 14:57 -0800, Fan Wu wrote: > > > From: Deven Bowers > > > > > > IPE must have a centralized function to evaluate incoming callers > > > against IPE's policy. This iteration of the policy against the rules > > > for that specific caller is known as the evaluation loop. > > > > Not sure if you check the properties at every access. > > > > >From my previous comments (also for previous versions of the patches) > > you could evaluate the property once, by calling the respective > > functions in the other subsystems. > > > > Then, you reserve space in the security blob for inodes and superblocks > > to cache the decision. The format could be a policy sequence number, to > > ensure that the cache is valid only for the current policy, and a bit > > for every hook you enforce. > > Thanks for raising this idea. I agree that if the property evaluation > leads to a performance issue, it will be better to cache the evaluation > result. But for this version, all the property evaluations are simple, > so it is just as fast as accessing a cache. Also, for the initial > version we prefer to keep the patch as minimal as possible. FWIW, I think that is the right decision. Keeping the initial submission relatively small and focused has a lot of advantages when it comes both to review and prematurely optimizing things that might not need optimization. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v9 01/16] security: add ipe lsm
On Mon, Jan 30, 2023 at 5:58 PM Fan Wu wrote: > > From: Deven Bowers > > Integrity Policy Enforcement (IPE) is an LSM that provides an > complimentary approach to Mandatory Access Control than existing LSMs > today. > > Existing LSMs have centered around the concept of access to a resource > should be controlled by the current user's credentials. IPE's approach, > is that access to a resource should be controlled by the system's trust > of a current resource. > > The basis of this approach is defining a global policy to specify which > resource can be trusted. > > Signed-off-by: Deven Bowers > Signed-off-by: Fan Wu ... > --- > MAINTAINERS | 5 + > security/Kconfig | 11 ++- > security/Makefile | 1 + > security/ipe/Kconfig | 17 + > security/ipe/Makefile | 10 ++ > security/ipe/ipe.c| 40 > security/ipe/ipe.h| 13 + > 7 files changed, 92 insertions(+), 5 deletions(-) > create mode 100644 security/ipe/Kconfig > create mode 100644 security/ipe/Makefile > create mode 100644 security/ipe/ipe.c > create mode 100644 security/ipe/ipe.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 8a5c25c20d00..5e27e84763cc 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -10273,6 +10273,11 @@ T: git > git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git > F: security/integrity/ima/ > F: security/integrity/ > > +INTEGRITY POLICY ENFORCEMENT (IPE) > +M: Fan Wu > +S: Supported > +F: security/ipe/ You should probably add a mailing list (L:) and source tree URL (T:) to the IPE entry. You can use the LSM mailing list to start if you like, there are several LSMs that do that today, e.g. Smack, Landlock, etc. As far as the source tree is concerned, probably the easiest option is a simple GitHub repo, but there are plenty of other choices too. Both the mailing list and the source URLs can always be updated in the future so don't worry too much about being stuck with either long term. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: Key based rate limiter (audit_set_rate_limit)
On Thu, Mar 2, 2023 at 12:24 PM Lenny Bruzenak wrote: > On 3/1/23 22:13, Anurag Aggarwal wrote: >> >> Or if selinux is in force, create policy for the events you definitely want, >> then look for those types (either subject or object) in your rule. This is >> something I've seen before, where renames that are desired to be audited use >> the provided system tools, but for locally developed application code, they >> are made to run inside a certain type of a custom executable and then that >> type is excluded from the rename syscall rule. Ideally, the code which is >> written would self-audit a 1-liner like "I am going to rename every file >> under dir /opt/special/stuff/" using audit_log_user_message so you still >> have some idea what is happening (if you care). >> >> Then your "my-rename" program subject type of my_rename_t can be used as an >> exclude on the rule. Of course, the caller must then know to use this rather >> than the standard utilities. > > > This sounds useful and might solve our problem, will it be possible to share > some examples on how this can be achieved? > > Replying off-list as it is not specifically audit-focused. See Paul, I CAN > learn. ;) -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
audit userspace problems with io_uring async ops
Hi all,
We just recently started picking up audit-testsuite failures with the
latest upstream kernels and I tracked it down to a change in how the
IORING_OP_OPENAT operation is handled, and how Steve's audit userspace
displays async io_uring ops. It appears that when ausearch is used to
look for events it doesn't display async io_uring events (URINGOP
records/events without an associated SYSCALL record/event). Take the
following snippet from /var/log/audit/audit.log:
--- 287 ---
type=SYSCALL msg=audit(1677618568.199:287): arch=c03e syscall=426
success=yes exit=1 a0=4 a1=1 a2=0 a3=0 items=1 ppid=1498 pid=1499
auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
tty=pts1 ses=3 comm="iouring" exe="/.../iouring"
subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
key="testsuite-1677618568-WJBbDxKg"ARCH=x86_64 SYSCALL=io_uring_enter ...
type=CWD msg=audit(1677618568.199:287):
cwd="/root/sources/audit-testsuite/tests/io_uring"
type=PATH msg=audit(1677618568.199:287): item=0
name="/tmp/iouring.4.txt" nametype=UNKNOWN ...
type=PROCTITLE msg=audit(1677618568.199:287):
proctitle=2E2F696F7572696E670074315F6368696C64
--- 288 ---
type=URINGOP msg=audit(1677618568.199:288): uring_op=18 success=yes exit=0
items=1 ppid=1498 pid=1499
uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
key="testsuite-1677618568-WJBbDxKg"URING_OP=openat ...
type=CWD msg=audit(1677618568.199:288):
cwd="/root/sources/audit-testsuite/tests/io_uring"
type=PATH msg=audit(1677618568.199:288): item=0 name="/tmp/iouring.4.txt"
inode=33 dev=00:22 mode=0100644 ouid=0 ogid=0 rdev=00:00
obj=unconfined_u:object_r:user_tmp_t:s0 nametype=NORMAL ...
--- 289 ---
type=SYSCALL msg=audit(1677618568.199:289): arch=c03e syscall=426
success=yes exit=0 a0=4 a1=0 a2=1 a3=1 items=0 ppid=1498 pid=1499
auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
tty=pts1 ses=3 comm="iouring" exe="/.../iouring"
subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
key="testsuite-1677618568-WJBbDxKg"ARCH=x86_64 SYSCALL=io_uring_enter ...
type=PROCTITLE msg=audit(1677618568.199:289):
proctitle=2E2F696F7572696E670074315F6368696C64
... and the matching ausearch output:
% ausearch -i -k "testsuite-1677618568-WJBbDxKg"
type=PROCTITLE msg=audit(02/28/2023 16:09:28.199:287) :
proctitle=./iouring t1_child
type=PATH msg=audit(02/28/2023 16:09:28.199:287) : item=0
name=/tmp/iouring.4.txt nametype=UNKNOWN ...
type=CWD msg=audit(02/28/2023 16:09:28.199:287) :
cwd=/root/sources/audit-testsuite/tests/io_uring
type=SYSCALL msg=audit(02/28/2023 16:09:28.199:287) : arch=x86_64
syscall=io_uring_enter success=yes exit=1 a0=0x4 a1=0x1 a2=0x0 a3=0x0 items=1
ppid=1498 pid=1499 auid=root uid=root gid=root euid=root suid=root fsuid=root
egid=root sgid=root fsgid=root tty=pts1 ses=3 comm=iouring exe=/.../iouring
subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
key=testsuite-1677618568-WJBbDxKg
type=PROCTITLE msg=audit(02/28/2023 16:09:28.199:289) :
proctitle=./iouring t1_child
type=SYSCALL msg=audit(02/28/2023 16:09:28.199:289) : arch=x86_64
syscall=io_uring_enter success=yes exit=0 a0=0x4 a1=0x0 a2=0x1 a3=0x1 items=0
ppid=1498 pid=1499 auid=root uid=root gid=root euid=root suid=root fsuid=root
egid=root sgid=root fsgid=root tty=pts1 ses=3 comm=iouring exe=/.../iouring
subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
key=testsuite-1677618568-WJBbDxKg
... if you look closely you'll notice that the #289 event (the async
URINGOP) is missing from the ausearch output.
The good news is that this is easily reproducible on current upstream
kernels and Steve's v3.1 audit userspace release using the
audit-testsuite io_uring tests. This can also be seen in Rawhide with
current packages:
% uname -r
6.3.0-0.rc0.20230228gitae3419fb.9.1.secnext.fc39.x86_64
% rpm -q audit
audit-3.1-2.fc39.x86_64
% pwd
/root/sources/audit-testsuite/tests/io_uring
% git log --oneline | head -n 1
44c933e tests/filter_exclude: euid filtering now possible in exclude filter
Once ausearch is fixed we will also need to update the audit-testsuite
to add an explicit io_uring filter for the IORING_OP_OPENAT op. The
patch below is untested (blocked on ausearch), but I expect it to
resolve the issue in the test suite:
>>>
diff --git a/tests/io_uring/test b/tests/io_uring/test
index 9eb427a..df13af0 100755
--- a/tests/io_uring/test
+++ b/tests/io_uring/test
@@ -49,6 +49,7 @@ system("auditctl -D >& /dev/null");
# set our io_uring filters
system("auditctl -a exit,always -F arch=b$abi_bits -S io_uring_setup -k $key");
system("auditctl -a exit,always -F arch=b$abi_bits -S io_uring_enter -k $key");
+system("auditctl -a io_uring,always -S openat -k $key");
# run the "t1" test
system("$basedir/iouring t1");
>>>
For the kernel folks, the relevant commit is likely 0ffae640ad83
("io_uring: always go async for unsupported open flags"). I believe
Re: Key based rate limiter (audit_set_rate_limit)
On Tue, Feb 28, 2023 at 10:35 AM Anurag Aggarwal wrote: > > Hello Paul, > > Thank you for your information. > >> If you have a particular audit >> rule which is too verbose *and* you are willing to lose audit records >> from that filter rule (which is what would happen if they were rate >> limited), you might want to consider making that audit filter rule >> more targeted to the event you are interested in logging. Generating >> more audit records than you want to see can be a sign of an overly >> general audit rule. > > I agree that having rules which are too verbose is not a very good idea. > > Beside this, is there any other mechanism which we can use to get a similar > effect? Nothing comes quickly to mind, perhaps others on the mailing list might have some ideas ... ? -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: Key based rate limiter (audit_set_rate_limit)
On Tue, Feb 28, 2023 at 5:53 AM Anurag Aggarwal wrote: > Hello All, > > The current rate limiter, audit_set_rate_limit limits all types of events. In > our case, we want to limit auditd events with a specific key, as they are > very noisy and consume very high CPU. > > From my understanding, this support is currently missing in AuditD. > > Is my understanding correct? Hello. Limiting of audit records is actually done in the kernel, and currently the rate limit applies equally[1] to all records, there is no ability to enforce limits per-key. If you have a particular audit rule which is too verbose *and* you are willing to lose audit records from that filter rule (which is what would happen if they were rate limited), you might want to consider making that audit filter rule more targeted to the event you are interested in logging. Generating more audit records than you want to see can be a sign of an overly general audit rule. Good luck! [1] Audit records generated by auditd/auditctl are exempt from rate limiting to help prevent lockups/contention. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
[GIT PULL] Audit patches for v6.3
Hi Linus, Just a single audit patch for the v6.3 merge window, and even that patch is pretty trivial as it only updates the mailing list entry in the MAINTAINERS file. Please merge for Linux v6.3. Thanks, -Paul -- The following changes since commit 88603b6dc419445847923fcb7fe5080067a30f98: Linux 6.2-rc2 (2023-01-01 13:53:16 -0800) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git tags/audit-pr-20230220 for you to fetch changes up to 6c6cd913accd77008f74a1a9d57b816db3651daa: audit: update the mailing list in MAINTAINERS (2023-02-07 10:22:29 -0500) audit/stable-6.3 PR 20230220 Paul Moore (1): audit: update the mailing list in MAINTAINERS MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] tests/filter_exclude: euid filtering now possible in exclude filter
On Tue, Feb 14, 2023 at 2:23 PM Paul Moore wrote:
>
> Starting with Steve's audit userspace v3.1, an euid exclude filter no
> longer results in an error. Adjust the test accordingly.
>
> Signed-off-by: Paul Moore
> ---
> tests/filter_exclude/test | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
As we are nearing the next upstream kernel release I'm going to merge
this right now so the tests continue to run clear (making it easier to
catch other regressions quickly), but if anyone has any concerns over
this patch please still voice them and I'll be sure to fix them up.
> diff --git a/tests/filter_exclude/test b/tests/filter_exclude/test
> index 7a8e3fb..248fc54 100755
> --- a/tests/filter_exclude/test
> +++ b/tests/filter_exclude/test
> @@ -103,7 +103,7 @@ $result = system("auditctl -a exclude,always -F
> ppid=$ppid >/dev/null 2>&1");
> ok( $result ne 0 );
> system("auditctl -d exclude,always -F ppid=$ppid >/dev/null 2>&1");
> $result = system("auditctl -a exclude,always -F euid=$euid >/dev/null 2>&1");
> -ok( $result ne 0 );
> +ok( $result, 0 );
> system("auditctl -d exclude,always -F euid=$euid >/dev/null 2>&1");
> $result =
>system("auditctl -a exclude,always -F obj_user=$obj_user >/dev/null 2>&1");
> --
> 2.39.1
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
[PATCH] tests/filter_exclude: euid filtering now possible in exclude filter
Starting with Steve's audit userspace v3.1, an euid exclude filter no
longer results in an error. Adjust the test accordingly.
Signed-off-by: Paul Moore
---
tests/filter_exclude/test | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/filter_exclude/test b/tests/filter_exclude/test
index 7a8e3fb..248fc54 100755
--- a/tests/filter_exclude/test
+++ b/tests/filter_exclude/test
@@ -103,7 +103,7 @@ $result = system("auditctl -a exclude,always -F ppid=$ppid
>/dev/null 2>&1");
ok( $result ne 0 );
system("auditctl -d exclude,always -F ppid=$ppid >/dev/null 2>&1");
$result = system("auditctl -a exclude,always -F euid=$euid >/dev/null 2>&1");
-ok( $result ne 0 );
+ok( $result, 0 );
system("auditctl -d exclude,always -F euid=$euid >/dev/null 2>&1");
$result =
system("auditctl -a exclude,always -F obj_user=$obj_user >/dev/null 2>&1");
--
2.39.1
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2] io_uring,audit: don't log IORING_OP_MADVISE
On Fri, Feb 10, 2023 at 5:00 PM Richard Guy Briggs wrote: > On 2023-02-10 11:52, Paul Moore wrote: > > On Fri, Feb 10, 2023 at 11:00 AM Jens Axboe wrote: > > > On 2/10/23 8:39?AM, Paul Moore wrote: > > > > On Thu, Feb 9, 2023 at 7:15 PM Jens Axboe wrote: > > > >> On 2/9/23 3:54?PM, Steve Grubb wrote: > > > >>> On Thursday, February 9, 2023 5:37:22 PM EST Paul Moore wrote: > > > >>>> On Thu, Feb 9, 2023 at 4:53 PM Richard Guy Briggs > > > >>>> wrote: > > > >>>>> On 2023-02-01 16:18, Paul Moore wrote: > > > >>>>>> On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs > > > >>> wrote: > > > >>>>>>> fadvise and madvise both provide hints for caching or access > > > >>>>>>> pattern > > > >>>>>>> for file and memory respectively. Skip them. > > > >>>>>> > > > >>>>>> You forgot to update the first sentence in the commit description > > > >>>>>> :/ > > > >>>>> > > > >>>>> I didn't forget. I updated that sentence to reflect the fact that > > > >>>>> the > > > >>>>> two should be treated similarly rather than differently. > > > >>>> > > > >>>> Ooookay. Can we at least agree that the commit description should be > > > >>>> rephrased to make it clear that the patch only adjusts madvise? > > > >>>> Right > > > >>>> now I read the commit description and it sounds like you are > > > >>>> adjusting > > > >>>> the behavior for both fadvise and madvise in this patch, which is not > > > >>>> true. > > > >>>> > > > >>>>>> I'm still looking for some type of statement that you've done some > > > >>>>>> homework on the IORING_OP_MADVISE case to ensure that it doesn't > > > >>>>>> end > > > >>>>>> up calling into the LSM, see my previous emails on this. I need > > > >>>>>> more > > > >>>>>> than "Steve told me to do this". > > > >>>>>> > > > >>>>>> I basically just want to see that some care and thought has gone > > > >>>>>> into > > > >>>>>> this patch to verify it is correct and good. > > > >>>>> > > > >>>>> Steve suggested I look into a number of iouring ops. I looked at > > > >>>>> the > > > >>>>> description code and agreed that it wasn't necessary to audit > > > >>>>> madvise. > > > >>>>> The rationale for fadvise was detemined to have been conflated with > > > >>>>> fallocate and subsequently dropped. Steve also suggested a number > > > >>>>> of > > > >>>>> others and after investigation I decided that their current state > > > >>>>> was > > > >>>>> correct. *getxattr you've advised against, so it was dropped. It > > > >>>>> appears fewer modifications were necessary than originally > > > >>>>> suspected. > > > >>>> > > > >>>> My concern is that three of the four changes you initially proposed > > > >>>> were rejected, which gives me pause about the fourth. You mention > > > >>>> that based on your reading of madvise's description you feel auditing > > > >>>> isn't necessary - and you may be right - but based on our experience > > > >>>> so far with this patchset I would like to hear that you have properly > > > >>>> investigated all of the madvise code paths, and I would like that in > > > >>>> the commit description. > > > >>> > > > >>> I think you're being unnecessarily hard on this. Yes, the commit > > > >>> message > > > >>> might be touched up. But madvise is advisory in nature. It is not > > > >>> security > > > >>> relevant. And a grep through the security directory doesn't turn up > > > >>> any > > > >>> hooks. > > > >> > > > >> Agree, it's getting a
Re: [PATCH v2] io_uring,audit: don't log IORING_OP_MADVISE
On Fri, Feb 10, 2023 at 11:00 AM Jens Axboe wrote: > On 2/10/23 8:39?AM, Paul Moore wrote: > > On Thu, Feb 9, 2023 at 7:15 PM Jens Axboe wrote: > >> On 2/9/23 3:54?PM, Steve Grubb wrote: > >>> On Thursday, February 9, 2023 5:37:22 PM EST Paul Moore wrote: > >>>> On Thu, Feb 9, 2023 at 4:53 PM Richard Guy Briggs > >>>> wrote: > >>>>> On 2023-02-01 16:18, Paul Moore wrote: > >>>>>> On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs > >>> wrote: > >>>>>>> fadvise and madvise both provide hints for caching or access pattern > >>>>>>> for file and memory respectively. Skip them. > >>>>>> > >>>>>> You forgot to update the first sentence in the commit description :/ > >>>>> > >>>>> I didn't forget. I updated that sentence to reflect the fact that the > >>>>> two should be treated similarly rather than differently. > >>>> > >>>> Ooookay. Can we at least agree that the commit description should be > >>>> rephrased to make it clear that the patch only adjusts madvise? Right > >>>> now I read the commit description and it sounds like you are adjusting > >>>> the behavior for both fadvise and madvise in this patch, which is not > >>>> true. > >>>> > >>>>>> I'm still looking for some type of statement that you've done some > >>>>>> homework on the IORING_OP_MADVISE case to ensure that it doesn't end > >>>>>> up calling into the LSM, see my previous emails on this. I need more > >>>>>> than "Steve told me to do this". > >>>>>> > >>>>>> I basically just want to see that some care and thought has gone into > >>>>>> this patch to verify it is correct and good. > >>>>> > >>>>> Steve suggested I look into a number of iouring ops. I looked at the > >>>>> description code and agreed that it wasn't necessary to audit madvise. > >>>>> The rationale for fadvise was detemined to have been conflated with > >>>>> fallocate and subsequently dropped. Steve also suggested a number of > >>>>> others and after investigation I decided that their current state was > >>>>> correct. *getxattr you've advised against, so it was dropped. It > >>>>> appears fewer modifications were necessary than originally suspected. > >>>> > >>>> My concern is that three of the four changes you initially proposed > >>>> were rejected, which gives me pause about the fourth. You mention > >>>> that based on your reading of madvise's description you feel auditing > >>>> isn't necessary - and you may be right - but based on our experience > >>>> so far with this patchset I would like to hear that you have properly > >>>> investigated all of the madvise code paths, and I would like that in > >>>> the commit description. > >>> > >>> I think you're being unnecessarily hard on this. Yes, the commit message > >>> might be touched up. But madvise is advisory in nature. It is not security > >>> relevant. And a grep through the security directory doesn't turn up any > >>> hooks. > >> > >> Agree, it's getting a bit anal... FWIW, patch looks fine to me. > > > > Call it whatever you want, but the details are often important at this > > level of code, and when I see a patch author pushing back on verifying > > that their patch is correct it makes me very skeptical. > > Maybe it isn't intended, but the replies have generally had a pretty > condescending tone to them. That's not the best way to engage folks, and > may very well be why people just kind of give up on it. Nobody likes > debating one-liners forever, particularly not if it isn't inviting. I appreciate that you are coming from a different space, but I stand by my comments. Of course you are welcome to your own opinion, but I would encourage you to spend some time reading the audit mail archives going back a few years before you make comments like the above ... or not, that's your call; I recognize it is usually easier to criticize. On a quasi related note to the list/archives: unfortunately there was continued resistance to opening up the linux-audit list so I've setup audit@vger for upstream audit kernel work moving forward. The list address in MAINTAINERS will get updated during the next merge window so hopefull
Re: [PATCH v2] io_uring,audit: don't log IORING_OP_MADVISE
On Thu, Feb 9, 2023 at 7:15 PM Jens Axboe wrote: > On 2/9/23 3:54 PM, Steve Grubb wrote: > > On Thursday, February 9, 2023 5:37:22 PM EST Paul Moore wrote: > >> On Thu, Feb 9, 2023 at 4:53 PM Richard Guy Briggs wrote: > >>> On 2023-02-01 16:18, Paul Moore wrote: > >>>> On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs > > wrote: > >>>>> fadvise and madvise both provide hints for caching or access pattern > >>>>> for file and memory respectively. Skip them. > >>>> > >>>> You forgot to update the first sentence in the commit description :/ > >>> > >>> I didn't forget. I updated that sentence to reflect the fact that the > >>> two should be treated similarly rather than differently. > >> > >> Ooookay. Can we at least agree that the commit description should be > >> rephrased to make it clear that the patch only adjusts madvise? Right > >> now I read the commit description and it sounds like you are adjusting > >> the behavior for both fadvise and madvise in this patch, which is not > >> true. > >> > >>>> I'm still looking for some type of statement that you've done some > >>>> homework on the IORING_OP_MADVISE case to ensure that it doesn't end > >>>> up calling into the LSM, see my previous emails on this. I need more > >>>> than "Steve told me to do this". > >>>> > >>>> I basically just want to see that some care and thought has gone into > >>>> this patch to verify it is correct and good. > >>> > >>> Steve suggested I look into a number of iouring ops. I looked at the > >>> description code and agreed that it wasn't necessary to audit madvise. > >>> The rationale for fadvise was detemined to have been conflated with > >>> fallocate and subsequently dropped. Steve also suggested a number of > >>> others and after investigation I decided that their current state was > >>> correct. *getxattr you've advised against, so it was dropped. It > >>> appears fewer modifications were necessary than originally suspected. > >> > >> My concern is that three of the four changes you initially proposed > >> were rejected, which gives me pause about the fourth. You mention > >> that based on your reading of madvise's description you feel auditing > >> isn't necessary - and you may be right - but based on our experience > >> so far with this patchset I would like to hear that you have properly > >> investigated all of the madvise code paths, and I would like that in > >> the commit description. > > > > I think you're being unnecessarily hard on this. Yes, the commit message > > might be touched up. But madvise is advisory in nature. It is not security > > relevant. And a grep through the security directory doesn't turn up any > > hooks. > > Agree, it's getting a bit anal... FWIW, patch looks fine to me. Call it whatever you want, but the details are often important at this level of code, and when I see a patch author pushing back on verifying that their patch is correct it makes me very skeptical. I really would have preferred that you held off from merging this until this was resolved and ACK'd ... oh well. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2] io_uring,audit: don't log IORING_OP_MADVISE
On Thu, Feb 9, 2023 at 5:54 PM Steve Grubb wrote: > On Thursday, February 9, 2023 5:37:22 PM EST Paul Moore wrote: > > On Thu, Feb 9, 2023 at 4:53 PM Richard Guy Briggs wrote: > > > On 2023-02-01 16:18, Paul Moore wrote: > > > > On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs > wrote: > > > > > fadvise and madvise both provide hints for caching or access pattern > > > > > for file and memory respectively. Skip them. > > > > > > > > You forgot to update the first sentence in the commit description :/ > > > > > > I didn't forget. I updated that sentence to reflect the fact that the > > > two should be treated similarly rather than differently. > > > > Ooookay. Can we at least agree that the commit description should be > > rephrased to make it clear that the patch only adjusts madvise? Right > > now I read the commit description and it sounds like you are adjusting > > the behavior for both fadvise and madvise in this patch, which is not > > true. > > > > > > I'm still looking for some type of statement that you've done some > > > > homework on the IORING_OP_MADVISE case to ensure that it doesn't end > > > > up calling into the LSM, see my previous emails on this. I need more > > > > than "Steve told me to do this". > > > > > > > > I basically just want to see that some care and thought has gone into > > > > this patch to verify it is correct and good. > > > > > > Steve suggested I look into a number of iouring ops. I looked at the > > > description code and agreed that it wasn't necessary to audit madvise. > > > The rationale for fadvise was detemined to have been conflated with > > > fallocate and subsequently dropped. Steve also suggested a number of > > > others and after investigation I decided that their current state was > > > correct. *getxattr you've advised against, so it was dropped. It > > > appears fewer modifications were necessary than originally suspected. > > > > My concern is that three of the four changes you initially proposed > > were rejected, which gives me pause about the fourth. You mention > > that based on your reading of madvise's description you feel auditing > > isn't necessary - and you may be right - but based on our experience > > so far with this patchset I would like to hear that you have properly > > investigated all of the madvise code paths, and I would like that in > > the commit description. > > I think you're being unnecessarily hard on this. Asking that a patch author does the proper level of due diligence to ensure that the patch they are submitting is correct isn't being "unnecessarily hard", it's part of being a good code reviewer and maintainer. I'm a bit amazed that you and Richard would rather spend your time arguing about this rather than spending the hour (?) it would take to verify the change and make a proper statement about the correctness of the patch. > Yes, the commit message > might be touched up. But madvise is advisory in nature. It is not security > relevant. And a grep through the security directory doesn't turn up any > hooks. You can't rely on grep, you need to chase the code paths to see what code might be exercised. For example, look at the truncate syscalls (truncate, truncate64, ftruncate64, etc.), if you grep through the SELinux hook code looking for some form of "truncate" you will not find anything relevant; SELinux doesn't provide implementations for either the security_file_truncate() or security_path_truncate() hooks. However, if you follow the syscall code paths you will see that the truncate syscalls end up calling security_inode_permission() which *is* implemented in SELinux. You need to do your homework; relying only on a gut feeling, a few lines from a manpage, or a code comment is a good way to introduce bugs. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2] io_uring,audit: don't log IORING_OP_MADVISE
On Thu, Feb 9, 2023 at 4:53 PM Richard Guy Briggs wrote: > On 2023-02-01 16:18, Paul Moore wrote: > > On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs wrote: > > > fadvise and madvise both provide hints for caching or access pattern for > > > file and memory respectively. Skip them. > > > > You forgot to update the first sentence in the commit description :/ > > I didn't forget. I updated that sentence to reflect the fact that the > two should be treated similarly rather than differently. Ooookay. Can we at least agree that the commit description should be rephrased to make it clear that the patch only adjusts madvise? Right now I read the commit description and it sounds like you are adjusting the behavior for both fadvise and madvise in this patch, which is not true. > > I'm still looking for some type of statement that you've done some > > homework on the IORING_OP_MADVISE case to ensure that it doesn't end > > up calling into the LSM, see my previous emails on this. I need more > > than "Steve told me to do this". > > > > I basically just want to see that some care and thought has gone into > > this patch to verify it is correct and good. > > Steve suggested I look into a number of iouring ops. I looked at the > description code and agreed that it wasn't necessary to audit madvise. > The rationale for fadvise was detemined to have been conflated with > fallocate and subsequently dropped. Steve also suggested a number of > others and after investigation I decided that their current state was > correct. *getxattr you've advised against, so it was dropped. It > appears fewer modifications were necessary than originally suspected. My concern is that three of the four changes you initially proposed were rejected, which gives me pause about the fourth. You mention that based on your reading of madvise's description you feel auditing isn't necessary - and you may be right - but based on our experience so far with this patchset I would like to hear that you have properly investigated all of the madvise code paths, and I would like that in the commit description. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v7 0/3] fanotify: Allow user space to pass back additional audit info
On Wed, Feb 8, 2023 at 12:37 PM Richard Guy Briggs wrote: > On 2023-02-08 11:24, Paul Moore wrote: > > On Wed, Feb 8, 2023 at 10:27 AM Steve Grubb wrote: > > > On Wednesday, February 8, 2023 10:03:24 AM EST Paul Moore wrote: > > > > On Wed, Feb 8, 2023 at 7:08 AM Jan Kara wrote: > > > > > On Tue 07-02-23 09:54:11, Paul Moore wrote: > > > > > > On Tue, Feb 7, 2023 at 7:09 AM Jan Kara wrote: > > > > > > > On Fri 03-02-23 16:35:13, Richard Guy Briggs wrote: > > > > > > > > The Fanotify API can be used for access control by requesting > > > > > > > > permission > > > > > > > > event notification. The user space tooling that uses it may > > > > > > > > have a > > > > > > > > complicated policy that inherently contains additional context > > > > > > > > for > > > > > > > > the > > > > > > > > decision. If this information were available in the audit trail, > > > > > > > > policy > > > > > > > > writers can close the loop on debugging policy. Also, if this > > > > > > > > additional > > > > > > > > information were available, it would enable the creation of > > > > > > > > tools > > > > > > > > that > > > > > > > > can suggest changes to the policy similar to how audit2allow can > > > > > > > > help > > > > > > > > refine labeled security. > > > > > > > > > > > > > > > > This patchset defines a new flag (FAN_INFO) and new extensions > > > > > > > > that > > > > > > > > define additional information which are appended after the > > > > > > > > response > > > > > > > > structure returned from user space on a permission event. The > > > > > > > > appended > > > > > > > > information is organized with headers containing a type and size > > > > > > > > that > > > > > > > > can be delegated to interested subsystems. One new information > > > > > > > > type is > > > > > > > > defined to audit the triggering rule number. > > > > > > > > > > > > > > > > A newer kernel will work with an older userspace and an older > > > > > > > > kernel > > > > > > > > will behave as expected and reject a newer userspace, leaving > > > > > > > > it up > > > > > > > > to > > > > > > > > the newer userspace to test appropriately and adapt as > > > > > > > > necessary. > > > > > > > > This > > > > > > > > is done by providing a a fully-formed FAN_INFO extension but > > > > > > > > setting the > > > > > > > > fd to FAN_NOFD. On a capable kernel, it will succeed but issue > > > > > > > > no > > > > > > > > audit > > > > > > > > record, whereas on an older kernel it will fail. > > > > > > > > > > > > > > > > The audit function was updated to log the additional > > > > > > > > information in > > > > > > > > the > > > > > > > > AUDIT_FANOTIFY record. The following are examples of the new > > > > > > > > record > > > > > > > > > > > > > > > > format: > > > > > > > > type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 > > > > > > > > fan_info=3137 subj_trust=3 obj_trust=5 type=FANOTIFY > > > > > > > > msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=0 > > > > > > > > subj_trust=2 obj_trust=2> > > > > > > > > > Thanks! I've applied this series to my tree. > > > > > > > > > > > > While I think this version of the patchset is fine, for future > > > > > > reference it would have been nice if you had waited for my ACK on > > > > > > patch 3/3; while Steve maintains his userspace tools, I'm the one > > > > > > responsible for maintaining the Linux Kernel's audit subsystem. > >
Re: [PATCH v7 0/3] fanotify: Allow user space to pass back additional audit info
On Wed, Feb 8, 2023 at 10:27 AM Steve Grubb wrote: > On Wednesday, February 8, 2023 10:03:24 AM EST Paul Moore wrote: > > On Wed, Feb 8, 2023 at 7:08 AM Jan Kara wrote: > > > On Tue 07-02-23 09:54:11, Paul Moore wrote: > > > > On Tue, Feb 7, 2023 at 7:09 AM Jan Kara wrote: > > > > > On Fri 03-02-23 16:35:13, Richard Guy Briggs wrote: > > > > > > The Fanotify API can be used for access control by requesting > > > > > > permission > > > > > > event notification. The user space tooling that uses it may have a > > > > > > complicated policy that inherently contains additional context for > > > > > > the > > > > > > decision. If this information were available in the audit trail, > > > > > > policy > > > > > > writers can close the loop on debugging policy. Also, if this > > > > > > additional > > > > > > information were available, it would enable the creation of tools > > > > > > that > > > > > > can suggest changes to the policy similar to how audit2allow can > > > > > > help > > > > > > refine labeled security. > > > > > > > > > > > > This patchset defines a new flag (FAN_INFO) and new extensions that > > > > > > define additional information which are appended after the response > > > > > > structure returned from user space on a permission event. The > > > > > > appended > > > > > > information is organized with headers containing a type and size > > > > > > that > > > > > > can be delegated to interested subsystems. One new information > > > > > > type is > > > > > > defined to audit the triggering rule number. > > > > > > > > > > > > A newer kernel will work with an older userspace and an older > > > > > > kernel > > > > > > will behave as expected and reject a newer userspace, leaving it up > > > > > > to > > > > > > the newer userspace to test appropriately and adapt as necessary. > > > > > > This > > > > > > is done by providing a a fully-formed FAN_INFO extension but > > > > > > setting the > > > > > > fd to FAN_NOFD. On a capable kernel, it will succeed but issue no > > > > > > audit > > > > > > record, whereas on an older kernel it will fail. > > > > > > > > > > > > The audit function was updated to log the additional information in > > > > > > the > > > > > > AUDIT_FANOTIFY record. The following are examples of the new record > > > > > > > > > > > > format: > > > > > > type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 > > > > > > fan_info=3137 subj_trust=3 obj_trust=5 type=FANOTIFY > > > > > > msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=0 > > > > > > subj_trust=2 obj_trust=2> > > > > > > > Thanks! I've applied this series to my tree. > > > > > > > > While I think this version of the patchset is fine, for future > > > > reference it would have been nice if you had waited for my ACK on > > > > patch 3/3; while Steve maintains his userspace tools, I'm the one > > > > responsible for maintaining the Linux Kernel's audit subsystem. > > > > > > Aha, I'm sorry for that. I had the impression that on the last version of > > > the series you've said you don't see anything for which the series should > > > be respun so once Steve's objections where addressed and you were silent > > > for a few days, I thought you consider the thing settled... My bad. > > > > That's understandable, especially given inconsistencies across > > subsystems. If it helps, if I'm going to ACK something I make it > > explicit with a proper 'Acked-by: ...' line in my reply; if I say > > something looks good but there is no explicit ACK, there is usually > > something outstanding that needs to be resolved, e.g. questions, > > additional testing, etc. > > > > In this particular case I posed some questions in that thread and > > never saw a reply with any answers, hence the lack of an ACK. While I > > think the patches were reasonable, I withheld my ACK until the > > questions were answered ... which they never were from what I can &
Re: [PATCH v7 0/3] fanotify: Allow user space to pass back additional audit info
On Wed, Feb 8, 2023 at 7:08 AM Jan Kara wrote: > On Tue 07-02-23 09:54:11, Paul Moore wrote: > > On Tue, Feb 7, 2023 at 7:09 AM Jan Kara wrote: > > > On Fri 03-02-23 16:35:13, Richard Guy Briggs wrote: > > > > The Fanotify API can be used for access control by requesting permission > > > > event notification. The user space tooling that uses it may have a > > > > complicated policy that inherently contains additional context for the > > > > decision. If this information were available in the audit trail, policy > > > > writers can close the loop on debugging policy. Also, if this additional > > > > information were available, it would enable the creation of tools that > > > > can suggest changes to the policy similar to how audit2allow can help > > > > refine labeled security. > > > > > > > > This patchset defines a new flag (FAN_INFO) and new extensions that > > > > define additional information which are appended after the response > > > > structure returned from user space on a permission event. The appended > > > > information is organized with headers containing a type and size that > > > > can be delegated to interested subsystems. One new information type is > > > > defined to audit the triggering rule number. > > > > > > > > A newer kernel will work with an older userspace and an older kernel > > > > will behave as expected and reject a newer userspace, leaving it up to > > > > the newer userspace to test appropriately and adapt as necessary. This > > > > is done by providing a a fully-formed FAN_INFO extension but setting the > > > > fd to FAN_NOFD. On a capable kernel, it will succeed but issue no audit > > > > record, whereas on an older kernel it will fail. > > > > > > > > The audit function was updated to log the additional information in the > > > > AUDIT_FANOTIFY record. The following are examples of the new record > > > > format: > > > > type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 > > > > fan_info=3137 subj_trust=3 obj_trust=5 > > > > type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 > > > > fan_info=0 subj_trust=2 obj_trust=2 > > > > > > Thanks! I've applied this series to my tree. > > > > While I think this version of the patchset is fine, for future > > reference it would have been nice if you had waited for my ACK on > > patch 3/3; while Steve maintains his userspace tools, I'm the one > > responsible for maintaining the Linux Kernel's audit subsystem. > > Aha, I'm sorry for that. I had the impression that on the last version of > the series you've said you don't see anything for which the series should > be respun so once Steve's objections where addressed and you were silent > for a few days, I thought you consider the thing settled... My bad. That's understandable, especially given inconsistencies across subsystems. If it helps, if I'm going to ACK something I make it explicit with a proper 'Acked-by: ...' line in my reply; if I say something looks good but there is no explicit ACK, there is usually something outstanding that needs to be resolved, e.g. questions, additional testing, etc. In this particular case I posed some questions in that thread and never saw a reply with any answers, hence the lack of an ACK. While I think the patches were reasonable, I withheld my ACK until the questions were answered ... which they never were from what I can tell, we just saw a new patchset with changes. /me shrugs -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] audit: update the mailing list in MAINTAINERS
On Tue, Feb 7, 2023 at 1:44 PM Paul Moore wrote: > > We've moved the upstream Linux Kernel audit subsystem discussions to > a new mailing list, this patch updates the MAINTAINERS info with the > new list address. > > Marking this for stable inclusion to help speed uptake of the new > list across all of the supported kernel releases. This is a doc only > patch so the risk should be close to nil. > > Cc: sta...@vger.kernel.org > Signed-off-by: Paul Moore > --- > MAINTAINERS | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) I've just merged this into audit/next. > diff --git a/MAINTAINERS b/MAINTAINERS > index 7f86d02cb427..b686c3af313f 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3511,7 +3511,7 @@ F:drivers/net/ieee802154/atusb.h > AUDIT SUBSYSTEM > M: Paul Moore > M: Eric Paris > -L: linux-audit@redhat.com (moderated for non-subscribers) > +L: au...@vger.kernel.org > S: Supported > W: https://github.com/linux-audit > T: git git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git > -- > 2.39.1 -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
[PATCH] audit: update the mailing list in MAINTAINERS
We've moved the upstream Linux Kernel audit subsystem discussions to a new mailing list, this patch updates the MAINTAINERS info with the new list address. Marking this for stable inclusion to help speed uptake of the new list across all of the supported kernel releases. This is a doc only patch so the risk should be close to nil. Cc: sta...@vger.kernel.org Signed-off-by: Paul Moore --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 7f86d02cb427..b686c3af313f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3511,7 +3511,7 @@ F:drivers/net/ieee802154/atusb.h AUDIT SUBSYSTEM M: Paul Moore M: Eric Paris -L: linux-audit@redhat.com (moderated for non-subscribers) +L: au...@vger.kernel.org S: Supported W: https://github.com/linux-audit T: git git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git -- 2.39.1 -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: Upstream kernel development and the linux-audit mailing list
On Fri, Feb 3, 2023 at 11:39 AM Paul Moore wrote: > On Tue, Jan 31, 2023 at 2:44 PM Paul Moore wrote: > > > > Hello all, > > ... > > > I'll hold off on list creation for a couple of days in case anyone has > > a well reasoned argument against moving upstream kernel development to > > a new list. However, I want to underscore that any argument to keep > > upstream discussions on a moderated list will need to be strong enough > > to counter potentially excluding other subsystems from the discussion. > > Seeing no comments, I just sent a request off to the vger > postmaster(s) to create a new list; I'll send another update when it > is up and running. The new list, au...@vger.kernel.org, is up and running and is CC'd on this email. While I'll continue to monitor linux-audit@redhat for upstream Linux Kernel patch submissions for a period of time to ease the transition, I do ask that everyone start submitting their upstream kernel patches and bug reports to audit@vger; at some point in the future I plan to stop monitoring linux-audit@redhat for patch submissions, bug reports, etc. Information on subscribing to the audit@vger list can be found below: * http://vger.kernel.org/vger-lists.html#audit * http://vger.kernel.org/majordomo-info.html I'll submit a patch to update the MAINTAINERS file with the new list address later today. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v7 0/3] fanotify: Allow user space to pass back additional audit info
On Tue, Feb 7, 2023 at 7:09 AM Jan Kara wrote: > On Fri 03-02-23 16:35:13, Richard Guy Briggs wrote: > > The Fanotify API can be used for access control by requesting permission > > event notification. The user space tooling that uses it may have a > > complicated policy that inherently contains additional context for the > > decision. If this information were available in the audit trail, policy > > writers can close the loop on debugging policy. Also, if this additional > > information were available, it would enable the creation of tools that > > can suggest changes to the policy similar to how audit2allow can help > > refine labeled security. > > > > This patchset defines a new flag (FAN_INFO) and new extensions that > > define additional information which are appended after the response > > structure returned from user space on a permission event. The appended > > information is organized with headers containing a type and size that > > can be delegated to interested subsystems. One new information type is > > defined to audit the triggering rule number. > > > > A newer kernel will work with an older userspace and an older kernel > > will behave as expected and reject a newer userspace, leaving it up to > > the newer userspace to test appropriately and adapt as necessary. This > > is done by providing a a fully-formed FAN_INFO extension but setting the > > fd to FAN_NOFD. On a capable kernel, it will succeed but issue no audit > > record, whereas on an older kernel it will fail. > > > > The audit function was updated to log the additional information in the > > AUDIT_FANOTIFY record. The following are examples of the new record > > format: > > type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 > > fan_info=3137 subj_trust=3 obj_trust=5 > > type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=0 > > subj_trust=2 obj_trust=2 > > Thanks! I've applied this series to my tree. While I think this version of the patchset is fine, for future reference it would have been nice if you had waited for my ACK on patch 3/3; while Steve maintains his userspace tools, I'm the one responsible for maintaining the Linux Kernel's audit subsystem. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: Upstream kernel development and the linux-audit mailing list
On Tue, Jan 31, 2023 at 2:44 PM Paul Moore wrote: > > Hello all, ... > I'll hold off on list creation for a couple of days in case anyone has > a well reasoned argument against moving upstream kernel development to > a new list. However, I want to underscore that any argument to keep > upstream discussions on a moderated list will need to be strong enough > to counter potentially excluding other subsystems from the discussion. Seeing no comments, I just sent a request off to the vger postmaster(s) to create a new list; I'll send another update when it is up and running. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v2] io_uring,audit: don't log IORING_OP_MADVISE
On Wed, Feb 1, 2023 at 3:34 PM Richard Guy Briggs wrote:
>
> fadvise and madvise both provide hints for caching or access pattern for
> file and memory respectively. Skip them.
You forgot to update the first sentence in the commit description :/
I'm still looking for some type of statement that you've done some
homework on the IORING_OP_MADVISE case to ensure that it doesn't end
up calling into the LSM, see my previous emails on this. I need more
than "Steve told me to do this".
I basically just want to see that some care and thought has gone into
this patch to verify it is correct and good.
> Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to
> io_uring")
> Signed-off-by: Richard Guy Briggs
> ---
> changelog
> v2:
> - drop *GETXATTR patch
> - drop FADVISE hunk
>
> io_uring/opdef.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/io_uring/opdef.c b/io_uring/opdef.c
> index 3aa0d65c50e3..d3f36c633ceb 100644
> --- a/io_uring/opdef.c
> +++ b/io_uring/opdef.c
> @@ -312,6 +312,7 @@ const struct io_op_def io_op_defs[] = {
> .issue = io_fadvise,
> },
> [IORING_OP_MADVISE] = {
> + .audit_skip = 1,
> .name = "MADVISE",
> .prep = io_madvise_prep,
> .issue = io_madvise,
> --
> 2.27.0
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Upstream kernel development and the linux-audit mailing list
Hello all, Those of you who have been following the linux-audit mailing list closely over the past several years have likely seen some issues relating to upstream Linux Kernel development and the mailing list: disagreements on the focus of the list (upstream vs upstream+distro), and a moderated vs open posting policy. In general the disagreements around the focus of the list, while annoying at times, tended not to be significant issues. Unfortunately, the issue of list moderation has been a problem with other subsystem maintainers and developers dropping the audit list from their discussions due to the moderation settings. While I had hoped that we might change the list to an open list, just like most of the upstream kernel mailing lists, Steve has mentioned that he does not want to make the change due to concerns over SPAM. With that in mind, I'm going to suggest moving the development of audit in the upstream Linux Kernel to a new mailing list hosted on vger.kernel.org. Many of you who participate, or at least monitor, kernel development are no doubt already subscribed to at least one mailing list hosted on vger as it is one of the most common (*the* most common?) list host for Linux Kernel related development. I don't want to make any statements with respect to Steve's audit userspace, that is up to him, but I don't have any problem if he wants to move upstream discussion of his audit userspace tools to the same vger-based list. I'll hold off on list creation for a couple of days in case anyone has a well reasoned argument against moving upstream kernel development to a new list. However, I want to underscore that any argument to keep upstream discussions on a moderated list will need to be strong enough to counter potentially excluding other subsystems from the discussion. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR
On Sat, Jan 28, 2023 at 12:26 PM Steve Grubb wrote:
> On Friday, January 27, 2023 5:43:02 PM EST Paul Moore wrote:
> > On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs wrote:
> > > Getting XATTRs is not particularly interesting security-wise.
> > >
> > > Suggested-by: Steve Grubb
> > > Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > > io_uring/opdef.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> >
> > Depending on your security policy, fetching file data, including
> > xattrs, can be interesting from a security perspective. As an
> > example, look at the SELinux file/getattr permission.
> >
> > https://github.com/SELinuxProject/selinux-notebook/blob/main/src/object_cla
> > sses_permissions.md#common-file-permissions
>
> We're mostly interested in setting attributes because that changes policy.
> Reading them is not interesting unless the access fails with EPERM.
See my earlier comments, SELinux does have provisions for caring about
reading xattrs, and now that I look at the rest of the LSMs it looks
like Smack cares about reading xattrs too. Regardless of whether a
given security policy cares about xattr access, the LSMs support
enforcing access on reading xattrs so we need to ensure the audit is
setup properly in these cases.
> I was updating the user space piece recently and saw there was a bunch of
> "new" operations. I was commenting that we need to audit 5 or 6 of the "new"
> operations such as IORING_OP_MKDIRATor IORING_OP_SETXATTR. But now that I see
> the patch, it looks like they are auditable and we can just let a couple be
> skipped. IORING_OP_MADVISE is not interesting as it just gives hiints about
> the expected access patterns of memory. If there were an equivalent of
> mprotect, that would be of interest, but not madvise.
Once again, as discussed previously, it is likely that skipping
auditing for IORING_OP_MADVISE is okay, but given that several of the
changes in this patchset were incorrect, I'd like a little more
thorough investigation before we skip auditing on madvise.
> There are some I'm not sure about such as IORING_OP_MSG_RING and
> IORING_OP_URING_CMD. What do they do?
Look at 4f57f06ce218 ("io_uring: add support for IORING_OP_MSG_RING
command") for the patch which added IORING_OP_MSG_RING as it has a
decent commit description. As for IORING_OP_URING_CMD, there were
lengthy discussions about it on the mailing lists (including audit)
back in March 2022 and then later in August on the LSM, SELinux, etc.
mailing lists when we landed some patches for it (there were no audit
changes). I also covered the IORING_OP_URING_CMD, albeit briefly, in
a presentation at LSS-EU last year:
https://www.youtube.com/watch?v=AaaH6skUEI8
https://www.paul-moore.com/docs/2022-lss_eu-iouring_lsm-pcmoore-r3.pdf
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 0/2] two suggested iouring op audit updates
On Sat, Jan 28, 2023 at 11:48 AM Steve Grubb wrote: > On Friday, January 27, 2023 5:53:24 PM EST Paul Moore wrote: > > On Fri, Jan 27, 2023 at 5:46 PM Jens Axboe wrote: > > > On 1/27/23 3:38 PM, Paul Moore wrote: > > > > On Fri, Jan 27, 2023 at 2:43 PM Jens Axboe wrote: > > > >> On 1/27/23 12:42 PM, Paul Moore wrote: > > > >>> On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe wrote: > > > >>>> On 1/27/23 10:23 AM, Richard Guy Briggs wrote: > > > >>>>> A couple of updates to the iouring ops audit bypass selections > > > >>>>> suggested in consultation with Steve Grubb. > > > >>>>> > > > >>>>> Richard Guy Briggs (2): > > > >>>>> io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE > > > >>>>> io_uring,audit: do not log IORING_OP_*GETXATTR > > > >>>>> > > > >>>>> io_uring/opdef.c | 4 +++- > > > >>>>> 1 file changed, 3 insertions(+), 1 deletion(-) > > > >>>> > > > >>>> Look fine to me - we should probably add stable to both of them, > > > >>>> just to keep things consistent across releases. I can queue them up > > > >>>> for 6.3. > > > >>> > > > >>> Please hold off until I've had a chance to look them over ... > > > >> > > > >> I haven't taken anything yet, for things like this I always let it > > > >> simmer until people have had a chance to do so. > > > > > > > > Thanks. FWIW, that sounds very reasonable to me, but I've seen lots > > > > of different behaviors across subsystems and wanted to make sure we > > > > were on the same page. > > > > > > Sounds fair. BTW, can we stop CC'ing closed lists on patch > > > submissions? Getting these: > > > > > > Your message to Linux-audit awaits moderator approval > > > > > > on every reply is really annoying. > > > > We kinda need audit related stuff on the linux-audit list, that's our > > mailing list for audit stuff. > > > > However, I agree that it is crap that the linux-audit list is > > moderated, but unfortunately that isn't something I control (I haven't > > worked for RH in years, and even then the list owner was really weird > > about managing the list). Occasionally I grumble about moving the > > kernel audit development to a linux-audit list on vger but haven't > > bothered yet, perhaps this is as good a reason as any. > > > > Richard, Steve - any chance of opening the linux-audit list? > > Unfortunately, it really has to be this way. I deleted 10 spam emails > yesterday. It seems like some people subscribed to this list are compromised. > Because everytime there is a legit email, it's followed in a few seconds by a > spam email. > > Anyways, all legit email will be approved without needing to be subscribed. The problem is that other subsystem developers who aren't subscribed to the linux-audit list end up getting held mail notices (see the comments from Jens). The moderation of linux-audit, as permissive as it may be for proper emails, is a problem for upstream linux audit development, I would say much more so than 10/day mails. If you are unable/unwilling to switch linux-audit over to an open mailing list we should revisit moving over to a vger list; at least for upstream kernel development, you are welcome to stick with the existing redhat.com list for discussion of your userspace tools. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR
On Fri, Jan 27, 2023 at 6:01 PM Richard Guy Briggs wrote:
> On 2023-01-27 17:43, Paul Moore wrote:
> > On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs wrote:
> > > Getting XATTRs is not particularly interesting security-wise.
> > >
> > > Suggested-by: Steve Grubb
> > > Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > > io_uring/opdef.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> >
> > Depending on your security policy, fetching file data, including
> > xattrs, can be interesting from a security perspective. As an
> > example, look at the SELinux file/getattr permission.
> >
> > https://github.com/SELinuxProject/selinux-notebook/blob/main/src/object_classes_permissions.md#common-file-permissions
>
> The intent here is to lessen the impact of audit operations. Read and
> Write were explicitly removed from io_uring auditing due to performance
> concerns coupled with the denial of service implications from sheer
> volume of records making other messages harder to locate. Those
> operations are still possible for syscall auditing but they are strongly
> discouraged for normal use.
We need to balance security needs and performance needs. You are
correct that general read() and write() operations are not audited,
and generally not checked from a LSM perspective as the auditing and
access control happens at open() time instead (access to fds is
revalidated when they are passed). However, in the case of getxattr
and fgetxattr, these are not normal file read operations, and do not
go through the same code path in the kernel; there is a reason why we
have xattr_permission() and security_inode_getxattr().
We need to continue to audit IORING_OP_FGETXATTR and IORING_OP_GETXATTR.
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 0/2] two suggested iouring op audit updates
On Fri, Jan 27, 2023 at 6:02 PM Jens Axboe wrote: > On 1/27/23 3:53 PM, Paul Moore wrote: > > On Fri, Jan 27, 2023 at 5:46 PM Jens Axboe wrote: > >> On 1/27/23 3:38 PM, Paul Moore wrote: > >>> On Fri, Jan 27, 2023 at 2:43 PM Jens Axboe wrote: > >>>> On 1/27/23 12:42 PM, Paul Moore wrote: > >>>>> On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe wrote: > >>>>>> On 1/27/23 10:23 AM, Richard Guy Briggs wrote: > >>>>>>> A couple of updates to the iouring ops audit bypass selections > >>>>>>> suggested in > >>>>>>> consultation with Steve Grubb. > >>>>>>> > >>>>>>> Richard Guy Briggs (2): > >>>>>>> io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE > >>>>>>> io_uring,audit: do not log IORING_OP_*GETXATTR > >>>>>>> > >>>>>>> io_uring/opdef.c | 4 +++- > >>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-) > >>>>>> > >>>>>> Look fine to me - we should probably add stable to both of them, just > >>>>>> to keep things consistent across releases. I can queue them up for 6.3. > >>>>> > >>>>> Please hold off until I've had a chance to look them over ... > >>>> > >>>> I haven't taken anything yet, for things like this I always let it > >>>> simmer until people have had a chance to do so. > >>> > >>> Thanks. FWIW, that sounds very reasonable to me, but I've seen lots > >>> of different behaviors across subsystems and wanted to make sure we > >>> were on the same page. > >> > >> Sounds fair. BTW, can we stop CC'ing closed lists on patch > >> submissions? Getting these: > >> > >> Your message to Linux-audit awaits moderator approval > >> > >> on every reply is really annoying. > > > > We kinda need audit related stuff on the linux-audit list, that's our > > mailing list for audit stuff. > > Sure, but then it should be open. Or do separate postings or something. > CC'ing a closed list with open lists and sending email to people that > are not on that closed list is bad form. Agree, that's why I said in my reply that it was crap that the linux-audit list is moderated and asked Richard/Steve to open it up. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 1/2] io_uring, audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
On Fri, Jan 27, 2023 at 5:55 PM Richard Guy Briggs wrote:
> On 2023-01-27 17:35, Paul Moore wrote:
> > On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs wrote:
> > >
> > > Since FADVISE can truncate files and MADVISE operates on memory, reverse
> > > the audit_skip tags.
> > >
> > > Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support
> > > to io_uring")
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > > io_uring/opdef.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/io_uring/opdef.c b/io_uring/opdef.c
> > > index 3aa0d65c50e3..a2bf53b4a38a 100644
> > > --- a/io_uring/opdef.c
> > > +++ b/io_uring/opdef.c
> > > @@ -306,12 +306,12 @@ const struct io_op_def io_op_defs[] = {
> > > },
> > > [IORING_OP_FADVISE] = {
> > > .needs_file = 1,
> > > - .audit_skip = 1,
> > > .name = "FADVISE",
> > > .prep = io_fadvise_prep,
> > > .issue = io_fadvise,
> > > },
> >
> > I've never used posix_fadvise() or the associated fadvise64*()
> > syscalls, but from quickly reading the manpages and the
> > generic_fadvise() function in the kernel I'm missing where the fadvise
> > family of functions could be used to truncate a file, can you show me
> > where this happens? The closest I can see is the manipulation of the
> > page cache, but that shouldn't actually modify the file ... right?
>
> I don't know. I was going on the advice of Steve Grubb. I'm looking
> for feedback, validation, correction, here.
Keep in mind it's your name on the patch, not Steve's, and I would
hope that you should be able to stand up and vouch for your own patch.
Something to keep in mind for the future.
As it stands, I think the audit_skip line should stay for
IORING_OP_FADVISE, if you feel otherwise please provide more
explanation as to why auditing is necessary for this operation.
> > > [IORING_OP_MADVISE] = {
> > > + .audit_skip = 1,
> > > .name = "MADVISE",
> > > .prep = io_madvise_prep,
> > > .issue = io_madvise,
> >
> > I *think* this should be okay, what testing/verification have you done
> > on this? One of the things I like to check is to see if any LSMs
> > might perform an access check and/or generate an audit record on an
> > operation, if there is a case where that could happen we should setup
> > audit properly. I did a very quick check of do_madvise() and nothing
> > jumped out at me, but I would be interested in knowing what testing or
> > verification you did here.
>
> No testing other than build/boot/audit-testsuite. You had a test you
> had developed that went through several iterations?
There is an io_uring test in the audit-testsuite that verifies basic
audit record generation, it is not exhaustive.
Patch 2/2 is a no-go from a security perspective (we want to see those
records), and I think skipping on IORING_OP_FADVISE is the correct
thing to do. It may be that skipping on IORING_OP_MADVISE is the
correct thing, but given that it doesn't appear a lot of in-depth
investigation has gone into these patches I would really like to see
some more reasoning on this before we change the current behavior.
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 1/2] io_uring, audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
On Fri, Jan 27, 2023 at 5:45 PM Jens Axboe wrote:
> On 1/27/23 3:35?PM, Paul Moore wrote:
> > On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs wrote:
> >>
> >> Since FADVISE can truncate files and MADVISE operates on memory, reverse
> >> the audit_skip tags.
> >>
> >> Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support
> >> to io_uring")
> >> Signed-off-by: Richard Guy Briggs
> >> ---
> >> io_uring/opdef.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/io_uring/opdef.c b/io_uring/opdef.c
> >> index 3aa0d65c50e3..a2bf53b4a38a 100644
> >> --- a/io_uring/opdef.c
> >> +++ b/io_uring/opdef.c
> >> @@ -306,12 +306,12 @@ const struct io_op_def io_op_defs[] = {
> >> },
> >> [IORING_OP_FADVISE] = {
> >> .needs_file = 1,
> >> - .audit_skip = 1,
> >> .name = "FADVISE",
> >> .prep = io_fadvise_prep,
> >> .issue = io_fadvise,
> >> },
> >
> > I've never used posix_fadvise() or the associated fadvise64*()
> > syscalls, but from quickly reading the manpages and the
> > generic_fadvise() function in the kernel I'm missing where the fadvise
> > family of functions could be used to truncate a file, can you show me
> > where this happens? The closest I can see is the manipulation of the
> > page cache, but that shouldn't actually modify the file ... right?
>
> Yeah, honestly not sure where that came from. Maybe it's being mixed up
> with fallocate?
That was my thought too when I was looking at it.
> All fadvise (or madvise, for that matter) does is
> provide hints on the caching or access pattern. On second thought, both
> of these should be able to set audit_skip as far as I can tell.
Agreed on the fadvise side, and probably the madvise side too,
although the latter has more options/code to sift through so I'm
curious to hear what analysis Richard has done on that one.
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 0/2] two suggested iouring op audit updates
On Fri, Jan 27, 2023 at 5:46 PM Jens Axboe wrote: > On 1/27/23 3:38 PM, Paul Moore wrote: > > On Fri, Jan 27, 2023 at 2:43 PM Jens Axboe wrote: > >> On 1/27/23 12:42 PM, Paul Moore wrote: > >>> On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe wrote: > >>>> On 1/27/23 10:23 AM, Richard Guy Briggs wrote: > >>>>> A couple of updates to the iouring ops audit bypass selections > >>>>> suggested in > >>>>> consultation with Steve Grubb. > >>>>> > >>>>> Richard Guy Briggs (2): > >>>>> io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE > >>>>> io_uring,audit: do not log IORING_OP_*GETXATTR > >>>>> > >>>>> io_uring/opdef.c | 4 +++- > >>>>> 1 file changed, 3 insertions(+), 1 deletion(-) > >>>> > >>>> Look fine to me - we should probably add stable to both of them, just > >>>> to keep things consistent across releases. I can queue them up for 6.3. > >>> > >>> Please hold off until I've had a chance to look them over ... > >> > >> I haven't taken anything yet, for things like this I always let it > >> simmer until people have had a chance to do so. > > > > Thanks. FWIW, that sounds very reasonable to me, but I've seen lots > > of different behaviors across subsystems and wanted to make sure we > > were on the same page. > > Sounds fair. BTW, can we stop CC'ing closed lists on patch > submissions? Getting these: > > Your message to Linux-audit awaits moderator approval > > on every reply is really annoying. We kinda need audit related stuff on the linux-audit list, that's our mailing list for audit stuff. However, I agree that it is crap that the linux-audit list is moderated, but unfortunately that isn't something I control (I haven't worked for RH in years, and even then the list owner was really weird about managing the list). Occasionally I grumble about moving the kernel audit development to a linux-audit list on vger but haven't bothered yet, perhaps this is as good a reason as any. Richard, Steve - any chance of opening the linux-audit list? -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 2/2] io_uring,audit: do not log IORING_OP_*GETXATTR
On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs wrote:
>
> Getting XATTRs is not particularly interesting security-wise.
>
> Suggested-by: Steve Grubb
> Fixes: a56834e0fafe ("io_uring: add fgetxattr and getxattr support")
> Signed-off-by: Richard Guy Briggs
> ---
> io_uring/opdef.c | 2 ++
> 1 file changed, 2 insertions(+)
Depending on your security policy, fetching file data, including
xattrs, can be interesting from a security perspective. As an
example, look at the SELinux file/getattr permission.
https://github.com/SELinuxProject/selinux-notebook/blob/main/src/object_classes_permissions.md#common-file-permissions
> diff --git a/io_uring/opdef.c b/io_uring/opdef.c
> index a2bf53b4a38a..f6bfe2cf078c 100644
> --- a/io_uring/opdef.c
> +++ b/io_uring/opdef.c
> @@ -462,12 +462,14 @@ const struct io_op_def io_op_defs[] = {
> },
> [IORING_OP_FGETXATTR] = {
> .needs_file = 1,
> + .audit_skip = 1,
> .name = "FGETXATTR",
> .prep = io_fgetxattr_prep,
> .issue = io_fgetxattr,
> .cleanup= io_xattr_cleanup,
> },
> [IORING_OP_GETXATTR] = {
> + .audit_skip = 1,
> .name = "GETXATTR",
> .prep = io_getxattr_prep,
> .issue = io_getxattr,
> --
> 2.27.0
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 0/2] two suggested iouring op audit updates
On Fri, Jan 27, 2023 at 2:43 PM Jens Axboe wrote: > On 1/27/23 12:42 PM, Paul Moore wrote: > > On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe wrote: > >> On 1/27/23 10:23 AM, Richard Guy Briggs wrote: > >>> A couple of updates to the iouring ops audit bypass selections suggested > >>> in > >>> consultation with Steve Grubb. > >>> > >>> Richard Guy Briggs (2): > >>> io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE > >>> io_uring,audit: do not log IORING_OP_*GETXATTR > >>> > >>> io_uring/opdef.c | 4 +++- > >>> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> Look fine to me - we should probably add stable to both of them, just > >> to keep things consistent across releases. I can queue them up for 6.3. > > > > Please hold off until I've had a chance to look them over ... > > I haven't taken anything yet, for things like this I always let it > simmer until people have had a chance to do so. Thanks. FWIW, that sounds very reasonable to me, but I've seen lots of different behaviors across subsystems and wanted to make sure we were on the same page. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 1/2] io_uring, audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE
On Fri, Jan 27, 2023 at 12:24 PM Richard Guy Briggs wrote:
>
> Since FADVISE can truncate files and MADVISE operates on memory, reverse
> the audit_skip tags.
>
> Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to
> io_uring")
> Signed-off-by: Richard Guy Briggs
> ---
> io_uring/opdef.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/io_uring/opdef.c b/io_uring/opdef.c
> index 3aa0d65c50e3..a2bf53b4a38a 100644
> --- a/io_uring/opdef.c
> +++ b/io_uring/opdef.c
> @@ -306,12 +306,12 @@ const struct io_op_def io_op_defs[] = {
> },
> [IORING_OP_FADVISE] = {
> .needs_file = 1,
> - .audit_skip = 1,
> .name = "FADVISE",
> .prep = io_fadvise_prep,
> .issue = io_fadvise,
> },
I've never used posix_fadvise() or the associated fadvise64*()
syscalls, but from quickly reading the manpages and the
generic_fadvise() function in the kernel I'm missing where the fadvise
family of functions could be used to truncate a file, can you show me
where this happens? The closest I can see is the manipulation of the
page cache, but that shouldn't actually modify the file ... right?
> [IORING_OP_MADVISE] = {
> + .audit_skip = 1,
> .name = "MADVISE",
> .prep = io_madvise_prep,
> .issue = io_madvise,
I *think* this should be okay, what testing/verification have you done
on this? One of the things I like to check is to see if any LSMs
might perform an access check and/or generate an audit record on an
operation, if there is a case where that could happen we should setup
audit properly. I did a very quick check of do_madvise() and nothing
jumped out at me, but I would be interested in knowing what testing or
verification you did here.
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v6 3/3] fanotify,audit: Allow audit to use the full permission event response
On Wed, Jan 25, 2023 at 5:11 PM Richard Guy Briggs wrote:
>
> On 2023-01-20 13:58, Paul Moore wrote:
> > On Tue, Jan 17, 2023 at 4:14 PM Richard Guy Briggs wrote:
> > >
> > > This patch passes the full response so that the audit function can use all
> > > of it. The audit function was updated to log the additional information in
> > > the AUDIT_FANOTIFY record.
> > >
> > > Currently the only type of fanotify info that is defined is an audit
> > > rule number, but convert it to hex encoding to future-proof the field.
> > > Hex encoding suggested by Paul Moore .
> > >
> > > The {subj,obj}_trust values are {0,1,2}, corresponding to no, yes,
> > > unknown.
> > >
> > > Sample records:
> > > type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1
> > > fan_info=3137 subj_trust=3 obj_trust=5
> > > type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0
> > > fan_info=3F subj_trust=2 obj_trust=2
> > >
> > > Suggested-by: Steve Grubb
> > > Link: https://lore.kernel.org/r/3075502.aeNJFYEL58@x2
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > > fs/notify/fanotify/fanotify.c | 3 ++-
> > > include/linux/audit.h | 9 +
> > > kernel/auditsc.c | 16 +---
> > > 3 files changed, 20 insertions(+), 8 deletions(-)
> >
> > ...
> >
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index d1fb821de104..3133c4175c15 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -2877,10 +2878,19 @@ void __audit_log_kern_module(char *name)
> > > context->type = AUDIT_KERN_MODULE;
> > > }
> > >
> > > -void __audit_fanotify(u32 response)
> > > +void __audit_fanotify(u32 response, struct
> > > fanotify_response_info_audit_rule *friar)
> > > {
> > > - audit_log(audit_context(), GFP_KERNEL,
> > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > + /* {subj,obj}_trust values are {0,1,2}: no,yes,unknown */
> > > + if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) {
> > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > > + "resp=%u fan_type=%u fan_info=3F subj_trust=2
> > > obj_trust=2",
> > > + response, FAN_RESPONSE_INFO_NONE);
> > > + return;
> > > + }
> > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > > + "resp=%u fan_type=%u fan_info=%X subj_trust=%u
> > > obj_trust=%u",
> > > + response, friar->hdr.type, friar->rule_number,
> > > + friar->subj_trust, friar->obj_trust);
> > > }
> >
> > The only thing that comes to mind might be to convert the if-return
> > into a switch statement to make it a bit cleaner and easier to patch
> > in the future, but that is s far removed from any real concern
> > that I debated even mentioning it. I only bring it up in case the
> > "3F" discussion results in a respin, and even then I'm not going to
> > hold my ACK over something as silly as a if-return vs switch.
> >
> > For clarity, this is what I was thinking:
> >
> > void __audit_fanontify(...)
> > {
> > switch (type) {
> > case FAN_RESPONSE_INFO_NONE:
> > audit_log(...);
> > break;
> > default:
> > audit_log(...);
> > }
> > }
>
> I agree that would be cleaner ...
As I said, the "3F" concern of Steve is really the only thing I would
bother respinning for, my other comments were just passing
observations.
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v6 3/3] fanotify,audit: Allow audit to use the full permission event response
On Wed, Jan 25, 2023 at 5:06 PM Richard Guy Briggs wrote:
> On 2023-01-20 13:52, Paul Moore wrote:
> > On Wed, Jan 18, 2023 at 1:34 PM Steve Grubb wrote:
> > > Hello Richard,
> > >
> > > I built a new kernel and tested this with old and new user space. It is
> > > working as advertised. The only thing I'm wondering about is why we have
> > > 3F
> > > as the default value when no additional info was sent? Would it be better
> > > to
> > > just make it 0?
> >
> > ...
> >
> > > On Tuesday, January 17, 2023 4:14:07 PM EST Richard Guy Briggs wrote:
> > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > index d1fb821de104..3133c4175c15 100644
> > > > --- a/kernel/auditsc.c
> > > > +++ b/kernel/auditsc.c
> > > > @@ -2877,10 +2878,19 @@ void __audit_log_kern_module(char *name)
> > > > context->type = AUDIT_KERN_MODULE;
> > > > }
> > > >
> > > > -void __audit_fanotify(u32 response)
> > > > +void __audit_fanotify(u32 response, struct
> > > > fanotify_response_info_audit_rule *friar) {
> > > > - audit_log(audit_context(), GFP_KERNEL,
> > > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > > + /* {subj,obj}_trust values are {0,1,2}: no,yes,unknown */
> > > > + if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) {
> > > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > > > + "resp=%u fan_type=%u fan_info=3F subj_trust=2
> > > obj_trust=2",
> > > > + response, FAN_RESPONSE_INFO_NONE);
> > > > + return;
> > > > + }
> >
> > (I'm working under the assumption that the "fan_info=3F" in the record
> > above is what Steve was referring to in his comment.)
> >
> > I vaguely recall Richard commenting on this in the past, although
> > maybe not ... my thought is that the "3F" is simply the hex encoded
> > "?" character in ASCII ('man 7 ascii' is your friend). I suppose the
> > question is what to do in the FAN_RESPONSE_INFO_NONE case.
> >
> > Historically when we had a missing field we would follow the "field=?"
> > pattern, but I don't recall doing that for a field which was
> > potentially hex encoded, is there an existing case where we use "?"
> > for a field that is hex encoded? If so, we can swap out the "3F" for
> > a more obvious "?".
>
> I was presuming encoding the zero: "30"
I'm sorry, but you've lost me here.
> > However, another option might be to simply output the current
> > AUDIT_FANOTIFY record format in the FAN_RESPONSE_INFO_NONE case, e.g.
> > only "resp=%u". This is a little against the usual guidance of
> > "fields should not disappear from a record", but considering that
> > userspace will always need to support the original resp-only format
> > for compatibility reasons this may be an option.
>
> I don't have a strong opinion.
I'm not sure I care too much either. I will admit that the "3F" seems
to be bordering on the "bit too clever" side of things, but it's easy
to argue it is in keeping with the general idea of using "?" to denote
absent/unknown fields.
As Steve was the one who raised the question in this latest round, and
he knows his userspace tools the best, it seems wise to get his input
on this.
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v1 0/2] two suggested iouring op audit updates
On Fri, Jan 27, 2023 at 12:40 PM Jens Axboe wrote: > On 1/27/23 10:23 AM, Richard Guy Briggs wrote: > > A couple of updates to the iouring ops audit bypass selections suggested in > > consultation with Steve Grubb. > > > > Richard Guy Briggs (2): > > io_uring,audit: audit IORING_OP_FADVISE but not IORING_OP_MADVISE > > io_uring,audit: do not log IORING_OP_*GETXATTR > > > > io_uring/opdef.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > Look fine to me - we should probably add stable to both of them, just > to keep things consistent across releases. I can queue them up for 6.3. Please hold off until I've had a chance to look them over ... -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v6 3/3] fanotify,audit: Allow audit to use the full permission event response
On Tue, Jan 17, 2023 at 4:14 PM Richard Guy Briggs wrote:
>
> This patch passes the full response so that the audit function can use all
> of it. The audit function was updated to log the additional information in
> the AUDIT_FANOTIFY record.
>
> Currently the only type of fanotify info that is defined is an audit
> rule number, but convert it to hex encoding to future-proof the field.
> Hex encoding suggested by Paul Moore .
>
> The {subj,obj}_trust values are {0,1,2}, corresponding to no, yes, unknown.
>
> Sample records:
> type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1
> fan_info=3137 subj_trust=3 obj_trust=5
> type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=3F
> subj_trust=2 obj_trust=2
>
> Suggested-by: Steve Grubb
> Link: https://lore.kernel.org/r/3075502.aeNJFYEL58@x2
> Signed-off-by: Richard Guy Briggs
> ---
> fs/notify/fanotify/fanotify.c | 3 ++-
> include/linux/audit.h | 9 +
> kernel/auditsc.c | 16 +---
> 3 files changed, 20 insertions(+), 8 deletions(-)
...
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index d1fb821de104..3133c4175c15 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2877,10 +2878,19 @@ void __audit_log_kern_module(char *name)
> context->type = AUDIT_KERN_MODULE;
> }
>
> -void __audit_fanotify(u32 response)
> +void __audit_fanotify(u32 response, struct fanotify_response_info_audit_rule
> *friar)
> {
> - audit_log(audit_context(), GFP_KERNEL,
> - AUDIT_FANOTIFY, "resp=%u", response);
> + /* {subj,obj}_trust values are {0,1,2}: no,yes,unknown */
> + if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) {
> + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> + "resp=%u fan_type=%u fan_info=3F subj_trust=2
> obj_trust=2",
> + response, FAN_RESPONSE_INFO_NONE);
> + return;
> + }
> + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> + "resp=%u fan_type=%u fan_info=%X subj_trust=%u
> obj_trust=%u",
> + response, friar->hdr.type, friar->rule_number,
> + friar->subj_trust, friar->obj_trust);
> }
The only thing that comes to mind might be to convert the if-return
into a switch statement to make it a bit cleaner and easier to patch
in the future, but that is s far removed from any real concern
that I debated even mentioning it. I only bring it up in case the
"3F" discussion results in a respin, and even then I'm not going to
hold my ACK over something as silly as a if-return vs switch.
For clarity, this is what I was thinking:
void __audit_fanontify(...)
{
switch (type) {
case FAN_RESPONSE_INFO_NONE:
audit_log(...);
break;
default:
audit_log(...);
}
}
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v6 3/3] fanotify,audit: Allow audit to use the full permission event response
On Wed, Jan 18, 2023 at 1:34 PM Steve Grubb wrote:
>
> Hello Richard,
>
> I built a new kernel and tested this with old and new user space. It is
> working as advertised. The only thing I'm wondering about is why we have 3F
> as the default value when no additional info was sent? Would it be better to
> just make it 0?
...
> On Tuesday, January 17, 2023 4:14:07 PM EST Richard Guy Briggs wrote:
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index d1fb821de104..3133c4175c15 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2877,10 +2878,19 @@ void __audit_log_kern_module(char *name)
> > context->type = AUDIT_KERN_MODULE;
> > }
> >
> > -void __audit_fanotify(u32 response)
> > +void __audit_fanotify(u32 response, struct
> > fanotify_response_info_audit_rule *friar) {
> > - audit_log(audit_context(), GFP_KERNEL,
> > - AUDIT_FANOTIFY, "resp=%u", response);
> > + /* {subj,obj}_trust values are {0,1,2}: no,yes,unknown */
> > + if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) {
> > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > + "resp=%u fan_type=%u fan_info=3F subj_trust=2
> obj_trust=2",
> > + response, FAN_RESPONSE_INFO_NONE);
> > + return;
> > + }
(I'm working under the assumption that the "fan_info=3F" in the record
above is what Steve was referring to in his comment.)
I vaguely recall Richard commenting on this in the past, although
maybe not ... my thought is that the "3F" is simply the hex encoded
"?" character in ASCII ('man 7 ascii' is your friend). I suppose the
question is what to do in the FAN_RESPONSE_INFO_NONE case.
Historically when we had a missing field we would follow the "field=?"
pattern, but I don't recall doing that for a field which was
potentially hex encoded, is there an existing case where we use "?"
for a field that is hex encoded? If so, we can swap out the "3F" for
a more obvious "?".
However, another option might be to simply output the current
AUDIT_FANOTIFY record format in the FAN_RESPONSE_INFO_NONE case, e.g.
only "resp=%u". This is a little against the usual guidance of
"fields should not disappear from a record", but considering that
userspace will always need to support the original resp-only format
for compatibility reasons this may be an option.
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3 1/2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD
On Tue, Jan 10, 2023 at 4:10 AM Jiri Olsa wrote:
> On Fri, Jan 06, 2023 at 10:43:59AM -0500, Paul Moore wrote:
> > When changing the ebpf program put() routines to support being called
> > from within IRQ context the program ID was reset to zero prior to
> > calling the perf event and audit UNLOAD record generators, which
> > resulted in problems as the ebpf program ID was bogus (always zero).
> > This patch addresses this problem by removing an unnecessary call to
> > bpf_prog_free_id() in __bpf_prog_offload_destroy() and adjusting
> > __bpf_prog_put() to only call bpf_prog_free_id() after audit and perf
> > have finished their bpf program unload tasks in
> > bpf_prog_put_deferred(). For the record, no one can determine, or
> > remember, why it was necessary to free the program ID, and remove it
> > from the IDR, prior to executing bpf_prog_put_deferred();
> > regardless, both Stanislav and Alexei agree that the approach in this
> > patch should be safe.
> >
> > It is worth noting that when moving the bpf_prog_free_id() call, the
> > do_idr_lock parameter was forced to true as the ebpf devs determined
> > this was the correct as the do_idr_lock should always be true. The
> > do_idr_lock parameter will be removed in a follow-up patch, but it
> > was kept here to keep the patch small in an effort to ease any stable
> > backports.
> >
> > I also modified the bpf_audit_prog() logic used to associate the
> > AUDIT_BPF record with other associated records, e.g. @ctx != NULL.
> > Instead of keying off the operation, it now keys off the execution
> > context, e.g. '!in_irg && !irqs_disabled()', which is much more
> > appropriate and should help better connect the UNLOAD operations with
> > the associated audit state (other audit records).
> >
> > Cc: sta...@vger.kernel.org
> > Fixes: d809e134be7a ("bpf: Prepare bpf_prog_put() to be called from irq
> > context.")
> > Reported-by: Burn Alting
> > Reported-by: Jiri Olsa
> > Suggested-by: Stanislav Fomichev
> > Suggested-by: Alexei Starovoitov
> > Signed-off-by: Paul Moore
> >
> > ---
> > * v3
> > - abandon most of the changes in v2
> > - move bpf_prog_free_id() after the audit/perf unload hooks
> > - remove bpf_prog_free_id() from __bpf_prog_offload_destroy()
> > - added stable tag
>
> fwiw I checked and the perf UNLOAD events have proper id now
> thanks for fixing this
No problem, thanks for verifying that this solves the perf problem too.
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: New bug in Audit
On Mon, Jan 9, 2023 at 10:59 AM Steve Grubb wrote: > On Friday, January 6, 2023 3:33:18 PM EST Paul Moore wrote: > > > This mailing list is *focused* on upstream work and support, and while > > > it does not preclude talking about distro specific bugs, I believe > > > there are better avenues for those discussions (e.g. see the RHBZ link > > > I provided in my response) as upstream isn't really going to be able > > > to provide adequate help for someone experiencing problems with a > > > distro kernel which has a number of patches and backports. > > > > > > If you have a problem with this approach, perhaps we should move > > > upstream development to an audit mailing list on vger.kernel.org and > > > leave this list for RH specific issues? > > > > Steve, I realize it's only been ~24hrs, but should I assume you are > > okay with that (the upstream focused approach)? > > For the 18 years I've spent on this mail list, it has alway been open to any > topic audit related. I've answered questions for many distributions. If I can > reproduce the issue, then it's a bug worth looking at. If I can't reproduce > it, I let them know. I've even answered questions for people writing their > own audit implementation. Since I was asked to maintain the upstream Linux Kernel audit subsystem I've generally asked people to try and reproduce their problems on a modern~ish upstream Linux Kernel as it simply isn't sustainable for me to replicate the environment of every problem report. Enterprise distributions which run old and/or heavily patched Linux Kernels should have their own support staff to provide assistance in these areas, the upstream developers can't support every distro kernel that ships. > A lot of the email is upstream kernel work - no doubt. But Many times, we > miss upstream kernel bugs because no one is running upstream code. We usually > hear about it when a distribution which stays close to upstream releases a > new update. In which case I would expect the distro support team to reproduce the problem and report it upstream and/or submit an upstream patch for review. This has been shown to work very well, and fits nicely within the "upstream first" motto adopted by some of the better Linux distributions. > The text where you sign up for this mail list does not limit the topc to > upstream work, Perhaps the term "limit" is a bit strong, but I think it would be good if the list welcome message indicates that the list is primarily for the development and support of the upstream Linux audit tools, distribution specific concerns should be sent to the distribution provider. > it allows for any discussion as long as it's audit related. I > do not think making a new mail list is in anyone's interest. Bugs will always > get misreported if there are 2 lists. I disagree, the upstream and Fedora SELinux mailing lists have been a good example of this working well. I also tend to think there is some value in having a vendor agnostic mailing list host, but that's more of a tie breaker in my mind, and not reason enough alone to force a switch. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3 2/2] bpf: remove the do_idr_lock parameter from bpf_prog_free_id()
On Mon, Jan 9, 2023 at 12:58 PM wrote: > On 01/09, Paul Moore wrote: > > On Fri, Jan 6, 2023 at 2:45 PM Stanislav Fomichev wrote: > > > > > > On Fri, Jan 6, 2023 at 7:44 AM Paul Moore wrote: > > > > > > > > It was determined that the do_idr_lock parameter to > > > > bpf_prog_free_id() was not necessary as it should always be true. > > > > > > > > Suggested-by: Stanislav Fomichev > > > > > > nit: I believe it's been suggested several times by different people > > > As much as I would like to follow all of the kernel relevant mailing > > lists, I'm short about 30hrs in a day to do that, and you were the > > first one I saw suggesting that change :) > > Sure, sure, I'm just stating it for the other people on the CC. So maybe > we can drop this line when applying. That's fine with me. To be honest, you folks can do whatever tweaks you want to these patches and that's okay with me; or even just dump them and rewrite them as you see fit, if that's easier. I'm only concerned with getting the regression fixed, who fixes it isn't something I'm worried about. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3 2/2] bpf: remove the do_idr_lock parameter from bpf_prog_free_id()
On Fri, Jan 6, 2023 at 2:45 PM Stanislav Fomichev wrote: > > On Fri, Jan 6, 2023 at 7:44 AM Paul Moore wrote: > > > > It was determined that the do_idr_lock parameter to > > bpf_prog_free_id() was not necessary as it should always be true. > > > > Suggested-by: Stanislav Fomichev > > nit: I believe it's been suggested several times by different people As much as I would like to follow all of the kernel relevant mailing lists, I'm short about 30hrs in a day to do that, and you were the first one I saw suggesting that change :) > Acked-by: Stanislav Fomichev -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3 1/2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD
On Fri, Jan 6, 2023 at 2:45 PM Stanislav Fomichev wrote:
> On Fri, Jan 6, 2023 at 7:44 AM Paul Moore wrote:
> >
> > When changing the ebpf program put() routines to support being called
> > from within IRQ context the program ID was reset to zero prior to
> > calling the perf event and audit UNLOAD record generators, which
> > resulted in problems as the ebpf program ID was bogus (always zero).
> > This patch addresses this problem by removing an unnecessary call to
> > bpf_prog_free_id() in __bpf_prog_offload_destroy() and adjusting
> > __bpf_prog_put() to only call bpf_prog_free_id() after audit and perf
> > have finished their bpf program unload tasks in
> > bpf_prog_put_deferred(). For the record, no one can determine, or
> > remember, why it was necessary to free the program ID, and remove it
> > from the IDR, prior to executing bpf_prog_put_deferred();
> > regardless, both Stanislav and Alexei agree that the approach in this
> > patch should be safe.
> >
> > It is worth noting that when moving the bpf_prog_free_id() call, the
> > do_idr_lock parameter was forced to true as the ebpf devs determined
> > this was the correct as the do_idr_lock should always be true. The
> > do_idr_lock parameter will be removed in a follow-up patch, but it
> > was kept here to keep the patch small in an effort to ease any stable
> > backports.
> >
> > I also modified the bpf_audit_prog() logic used to associate the
> > AUDIT_BPF record with other associated records, e.g. @ctx != NULL.
> > Instead of keying off the operation, it now keys off the execution
> > context, e.g. '!in_irg && !irqs_disabled()', which is much more
> > appropriate and should help better connect the UNLOAD operations with
> > the associated audit state (other audit records).
> >
> > Cc: sta...@vger.kernel.org
> > Fixes: d809e134be7a ("bpf: Prepare bpf_prog_put() to be called from irq
> > context.")
> > Reported-by: Burn Alting
> > Reported-by: Jiri Olsa
> > Suggested-by: Stanislav Fomichev
> > Suggested-by: Alexei Starovoitov
> > Signed-off-by: Paul Moore
>
> Acked-by: Stanislav Fomichev
>
> Thank you! There might be a chance it breaks test_offload.py (I don't
> remember whether it checks this prog-is-removed-from-id part or not),
> but I don't think it's fair to ask to address it :-)
> Since it doesn't trigger in CI, I'll take another look next week when
> doing a respin of my 'xdp-hints' series.
No problem, I'm glad we found a solution that works for everyone; and
thank you for chasing down any test changes that may be necessary.
I'd like to get this patch into Linus' tree sooner rather than later
as it fixes a kinda ugly problem, would you be okay if this went in
via the bpf tree? With the appropriate ACKs I could send it to Linus
via the audit tree, but I think it would be much better to send it via
the bpf/netdev tree.
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: A question on monitoring time or time management changes in the kernel and the adjtimex system call
On Mon, Jan 9, 2023 at 2:33 AM Burn Alting wrote: > > All, > > Would it be correct to say that when one sees an adjtimex system call audit > event, a change has occurred ONLY if either a AUDIT_TIME_ADJNTPVAL (algorithm > change) or AUDIT_TIME_INJOFFSET (time change) record is present in the event? Looking at audit_log_time() and audit_tk_injoffset() it appears that an AUDIT_TIME_INJOFFSET record would indicate a time shift given by the "sec"/"nsec" fields while an AUDIT_TIME_ADJNTPVAL *might* indicate a shift depending on what was adjusted, you probably want to check the adjtimex(2) manpage, specifically the struct timex definition for more information on the AUDIT_TIME_ADJNTPVAL "op" field and "new"/"old" values. * https://man7.org/linux/man-pages/man2/adjtimex.2.html Hopefully that helps a little bit. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: New bug in Audit
On Mon, Jan 9, 2023 at 3:30 AM Ariel Silver wrote:
>
> Hey guys and thank you for the quick reply, Much appreciated!
>
> As Richard and Steve mentioned the commit:
> https://github.com/linux-audit/audit-kernel/commit/1b2263a807ca651f94517b1b22dc5f13e494984d
> Fixed this issue.
A quick note for anyone looking to backport, the actual commit in the
upstream Linux Kernel is d4fefa4801a1 ("audit: move audit_return_fixup
before the filters"). Looking at the commit ID posted above and the
note in the commit about the manual merge, it looks like the 1b22
commit had a munged subject line that was fixed in d4fe.
> Any timeframe to when we can get a new version of auditd with that fix?
> Or should I count on redhat to release an update to the kernel ?
>
> Any update will be good.
This is one of those reasons why you really need to contact RH
directly via their bugzilla and/or a support representative; those of
us who work on the upstream Linux Kernel do not have visibility into
RH's kernel release process. I maintain the Linux Kernel's audit
subsystem and I don't currently work for RH, and haven't for over four
years ;)
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: New bug in Audit
On Thu, Jan 5, 2023 at 2:32 PM Paul Moore wrote: > On Thu, Jan 5, 2023 at 11:32 AM Steve Grubb wrote: > > On Thursday, January 5, 2023 10:41:49 AM EST Paul Moore wrote: > > > On Thu, Jan 5, 2023 at 8:38 AM Ariel Silver > > wrote: > > > > I found the following bug: > > > > > > > > OS version = Red Hat Enterprise Linux release 8.6 (Ootpa) > > > > Kernel version = 4.18.0-425.3.1.el8.x86_64 > > > > auditctl version = 3.0.7 > > > > > > This mailing list is focused on the development and support of > > > upstream Linux Kernels and Steve's audit userspace, we don't really > > > provide support for paid distributions. If you are seeing problems > > > with the upstream Linux Kernel or tools, please report them here, but > > > issues with distribution kernels and/or tools should be sent to the > > > distribution for support/assistance. > > > > Paul, we take bug reports and help requests from anyone. Often, > > distributions > > are how we first hear of problems. > > Steve, re-read what I wrote. > > This mailing list is *focused* on upstream work and support, and while > it does not preclude talking about distro specific bugs, I believe > there are better avenues for those discussions (e.g. see the RHBZ link > I provided in my response) as upstream isn't really going to be able > to provide adequate help for someone experiencing problems with a > distro kernel which has a number of patches and backports. > > If you have a problem with this approach, perhaps we should move > upstream development to an audit mailing list on vger.kernel.org and > leave this list for RH specific issues? Steve, I realize it's only been ~24hrs, but should I assume you are okay with that (the upstream focused approach)? -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
[PATCH v3 1/2] bpf: restore the ebpf program ID for BPF_AUDIT_UNLOAD and PERF_BPF_EVENT_PROG_UNLOAD
When changing the ebpf program put() routines to support being called
from within IRQ context the program ID was reset to zero prior to
calling the perf event and audit UNLOAD record generators, which
resulted in problems as the ebpf program ID was bogus (always zero).
This patch addresses this problem by removing an unnecessary call to
bpf_prog_free_id() in __bpf_prog_offload_destroy() and adjusting
__bpf_prog_put() to only call bpf_prog_free_id() after audit and perf
have finished their bpf program unload tasks in
bpf_prog_put_deferred(). For the record, no one can determine, or
remember, why it was necessary to free the program ID, and remove it
from the IDR, prior to executing bpf_prog_put_deferred();
regardless, both Stanislav and Alexei agree that the approach in this
patch should be safe.
It is worth noting that when moving the bpf_prog_free_id() call, the
do_idr_lock parameter was forced to true as the ebpf devs determined
this was the correct as the do_idr_lock should always be true. The
do_idr_lock parameter will be removed in a follow-up patch, but it
was kept here to keep the patch small in an effort to ease any stable
backports.
I also modified the bpf_audit_prog() logic used to associate the
AUDIT_BPF record with other associated records, e.g. @ctx != NULL.
Instead of keying off the operation, it now keys off the execution
context, e.g. '!in_irg && !irqs_disabled()', which is much more
appropriate and should help better connect the UNLOAD operations with
the associated audit state (other audit records).
Cc: sta...@vger.kernel.org
Fixes: d809e134be7a ("bpf: Prepare bpf_prog_put() to be called from irq
context.")
Reported-by: Burn Alting
Reported-by: Jiri Olsa
Suggested-by: Stanislav Fomichev
Suggested-by: Alexei Starovoitov
Signed-off-by: Paul Moore
---
* v3
- abandon most of the changes in v2
- move bpf_prog_free_id() after the audit/perf unload hooks
- remove bpf_prog_free_id() from __bpf_prog_offload_destroy()
- added stable tag
* v2
- change subj
- add mention of the perf regression
- drop the dedicated program audit ID
- add the bpf_prog::valid_id flag, bpf_prog_get_id() getter
- convert prog ID users to new ID getter
* v1
- subj was: "bpf: restore the ebpf audit UNLOAD id field"
- initial draft
---
kernel/bpf/offload.c | 3 ---
kernel/bpf/syscall.c | 6 ++
2 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
index 13e4efc971e6..190d9f9dc987 100644
--- a/kernel/bpf/offload.c
+++ b/kernel/bpf/offload.c
@@ -216,9 +216,6 @@ static void __bpf_prog_offload_destroy(struct bpf_prog
*prog)
if (offload->dev_state)
offload->offdev->ops->destroy(prog);
- /* Make sure BPF_PROG_GET_NEXT_ID can't find this dead program */
- bpf_prog_free_id(prog, true);
-
list_del_init(>offloads);
kfree(offload);
prog->aux->offload = NULL;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 64131f88c553..61bb19e81b9c 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1972,7 +1972,7 @@ static void bpf_audit_prog(const struct bpf_prog *prog,
unsigned int op)
return;
if (audit_enabled == AUDIT_OFF)
return;
- if (op == BPF_AUDIT_LOAD)
+ if (!in_irq() && !irqs_disabled())
ctx = audit_context();
ab = audit_log_start(ctx, GFP_ATOMIC, AUDIT_BPF);
if (unlikely(!ab))
@@ -2067,6 +2067,7 @@ static void bpf_prog_put_deferred(struct work_struct
*work)
prog = aux->prog;
perf_event_bpf_event(prog, PERF_BPF_EVENT_PROG_UNLOAD, 0);
bpf_audit_prog(prog, BPF_AUDIT_UNLOAD);
+ bpf_prog_free_id(prog, true);
__bpf_prog_put_noref(prog, true);
}
@@ -2075,9 +2076,6 @@ static void __bpf_prog_put(struct bpf_prog *prog, bool
do_idr_lock)
struct bpf_prog_aux *aux = prog->aux;
if (atomic64_dec_and_test(>refcnt)) {
- /* bpf_prog_free_id() must be called first */
- bpf_prog_free_id(prog, do_idr_lock);
-
if (in_irq() || irqs_disabled()) {
INIT_WORK(>work, bpf_prog_put_deferred);
schedule_work(>work);
--
2.39.0
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
[PATCH v3 2/2] bpf: remove the do_idr_lock parameter from bpf_prog_free_id()
It was determined that the do_idr_lock parameter to
bpf_prog_free_id() was not necessary as it should always be true.
Suggested-by: Stanislav Fomichev
Signed-off-by: Paul Moore
---
* v3
- initial draft
---
include/linux/bpf.h | 2 +-
kernel/bpf/syscall.c | 20 ++--
2 files changed, 7 insertions(+), 15 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 3de24cfb7a3d..634d37a599fa 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1832,7 +1832,7 @@ void bpf_prog_inc(struct bpf_prog *prog);
struct bpf_prog * __must_check bpf_prog_inc_not_zero(struct bpf_prog *prog);
void bpf_prog_put(struct bpf_prog *prog);
-void bpf_prog_free_id(struct bpf_prog *prog, bool do_idr_lock);
+void bpf_prog_free_id(struct bpf_prog *prog);
void bpf_map_free_id(struct bpf_map *map, bool do_idr_lock);
struct btf_field *btf_record_find(const struct btf_record *rec,
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 61bb19e81b9c..ecca9366c7a6 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2001,7 +2001,7 @@ static int bpf_prog_alloc_id(struct bpf_prog *prog)
return id > 0 ? 0 : id;
}
-void bpf_prog_free_id(struct bpf_prog *prog, bool do_idr_lock)
+void bpf_prog_free_id(struct bpf_prog *prog)
{
unsigned long flags;
@@ -2013,18 +2013,10 @@ void bpf_prog_free_id(struct bpf_prog *prog, bool
do_idr_lock)
if (!prog->aux->id)
return;
- if (do_idr_lock)
- spin_lock_irqsave(_idr_lock, flags);
- else
- __acquire(_idr_lock);
-
+ spin_lock_irqsave(_idr_lock, flags);
idr_remove(_idr, prog->aux->id);
prog->aux->id = 0;
-
- if (do_idr_lock)
- spin_unlock_irqrestore(_idr_lock, flags);
- else
- __release(_idr_lock);
+ spin_unlock_irqrestore(_idr_lock, flags);
}
static void __bpf_prog_put_rcu(struct rcu_head *rcu)
@@ -2067,11 +2059,11 @@ static void bpf_prog_put_deferred(struct work_struct
*work)
prog = aux->prog;
perf_event_bpf_event(prog, PERF_BPF_EVENT_PROG_UNLOAD, 0);
bpf_audit_prog(prog, BPF_AUDIT_UNLOAD);
- bpf_prog_free_id(prog, true);
+ bpf_prog_free_id(prog);
__bpf_prog_put_noref(prog, true);
}
-static void __bpf_prog_put(struct bpf_prog *prog, bool do_idr_lock)
+static void __bpf_prog_put(struct bpf_prog *prog)
{
struct bpf_prog_aux *aux = prog->aux;
@@ -2087,7 +2079,7 @@ static void __bpf_prog_put(struct bpf_prog *prog, bool
do_idr_lock)
void bpf_prog_put(struct bpf_prog *prog)
{
- __bpf_prog_put(prog, true);
+ __bpf_prog_put(prog);
}
EXPORT_SYMBOL_GPL(bpf_prog_put);
--
2.39.0
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: New bug in Audit
On Thu, Jan 5, 2023 at 11:32 AM Steve Grubb wrote: > On Thursday, January 5, 2023 10:41:49 AM EST Paul Moore wrote: > > On Thu, Jan 5, 2023 at 8:38 AM Ariel Silver > wrote: > > > I found the following bug: > > > > > > OS version = Red Hat Enterprise Linux release 8.6 (Ootpa) > > > Kernel version = 4.18.0-425.3.1.el8.x86_64 > > > auditctl version = 3.0.7 > > > > This mailing list is focused on the development and support of > > upstream Linux Kernels and Steve's audit userspace, we don't really > > provide support for paid distributions. If you are seeing problems > > with the upstream Linux Kernel or tools, please report them here, but > > issues with distribution kernels and/or tools should be sent to the > > distribution for support/assistance. > > Paul, we take bug reports and help requests from anyone. Often, distributions > are how we first hear of problems. Steve, re-read what I wrote. This mailing list is *focused* on upstream work and support, and while it does not preclude talking about distro specific bugs, I believe there are better avenues for those discussions (e.g. see the RHBZ link I provided in my response) as upstream isn't really going to be able to provide adequate help for someone experiencing problems with a distro kernel which has a number of patches and backports. If you have a problem with this approach, perhaps we should move upstream development to an audit mailing list on vger.kernel.org and leave this list for RH specific issues? -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
