Re: add CONFIG_SECURITY_SELINUX_LOAD_ONCE
On Fri, Apr 7, 2017 at 1:34 PM, Nick Kralevich <n...@google.com> wrote: > I wanted to draw people's attention to the following proposed change: > > https://android-review.googlesource.com/367695 > > In the case of Android, it's common for security policy to be loaded once, > and never reloaded again. In that case, the locking / unlocking surrounding > the in-kernel policy is unnecessary and can be avoided. The patch above > turns the locks into no-ops and ensures that the kernel cannot load a policy > more than once. End result is that locking and preemption overhead is > avoided and there's less attack surface / code compiled into the kernel. Without looking at any of the proposed code (see my comment at the end of this mail), my initial reaction is that isn't something we want to have to support in the mainline kernel. For the majority (all?) of the general purpose and enterprise Linux distributions, the ability to reload SELinux policy is critical (run time customization, policy module installs/updates, etc.), and I'm not convinced that this facility would ever be enabled outside of some embedded/appliance scenarios. Even then, I question the usefulness since the policy itself should be able to prevent policy reloads. If you are seeing measurable, and problematic, overhead with the policy locks I'm open to new locking approaches. From an attack surface reduction point of view, I have no objection to looking at removing some of the interfaces from the selinuxfs mount if the loaded SELinux policy completely removes that privilege from all domains ... although the code here might be more trouble than it is worth. On Fri, Apr 7, 2017 at 1:57 PM, Nick Kralevich <n...@google.com> wrote: > In the interests of avoiding duplicate conversation in multiple > places, let's continue this discussion at > https://android-review.googlesource.com/367695 Posting links to discussions about Android specific changes is fine as a FYI, and welcome as far as I'm concerned. However, SELinux patches which you wish to submit for review and possible inclusion into the mainline Linux Kernel need to be posted and discussed on the SELinux mailing list. -- paul moore www.paul-moore.com ___ Seandroid-list mailing list Seandroid-list@tycho.nsa.gov To unsubscribe, send email to seandroid-list-le...@tycho.nsa.gov. To get help, send an email containing "help" to seandroid-list-requ...@tycho.nsa.gov.
Re: [PATCH 3/3] selinux: fix overflow and 0 length allocations
On Tue, Aug 23, 2016 at 4:49 PM, <william.c.robe...@intel.com> wrote: > From: William Roberts <william.c.robe...@intel.com> > > Throughout the SE Linux LSM, values taken from sepolicy are > used in places where length == 0 or length == > matter, find and fix these. > > Signed-off-by: William Roberts <william.c.robe...@intel.com> > --- > security/selinux/ss/conditional.c | 3 +++ > security/selinux/ss/policydb.c| 4 > security/selinux/ss/private.h | 7 +++ > 3 files changed, 14 insertions(+) > create mode 100644 security/selinux/ss/private.h > > diff --git a/security/selinux/ss/conditional.c > b/security/selinux/ss/conditional.c > index 456e1a9..ecc0fb6 100644 > --- a/security/selinux/ss/conditional.c > +++ b/security/selinux/ss/conditional.c > @@ -16,6 +16,7 @@ > #include "security.h" > #include "conditional.h" > #include "services.h" > +#include "private.h" > > /* > * cond_evaluate_expr evaluates a conditional expr > @@ -242,6 +243,8 @@ int cond_read_bool(struct policydb *p, struct hashtab *h, > void *fp) > goto err; > > len = le32_to_cpu(buf[2]); > + if (zero_or_saturated(len)) > + goto err; > > rc = -ENOMEM; > key = kmalloc(len + 1, GFP_KERNEL); > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > index 4b24385..0e881f3 100644 > --- a/security/selinux/ss/policydb.c > +++ b/security/selinux/ss/policydb.c > @@ -38,6 +38,7 @@ > #include "conditional.h" > #include "mls.h" > #include "services.h" > +#include "private.h" > > #define _DEBUG_HASHES > > @@ -1094,6 +1095,9 @@ static int str_read(char **strp, gfp_t flags, void *fp, > u32 len) > int rc; > char *str; > > + if (zero_or_saturated(len)) > + return -EINVAL; > + > str = kmalloc(len + 1, flags); > if (!str) > return -ENOMEM; > diff --git a/security/selinux/ss/private.h b/security/selinux/ss/private.h > new file mode 100644 > index 000..0e81a78 > --- /dev/null > +++ b/security/selinux/ss/private.h > @@ -0,0 +1,7 @@ > +#ifndef PRIVATE_H_ > +#define PRIVATE_H_ > + > +#define is_saturated(x) (x == (typeof(x))-1) > +#define zero_or_saturated(x) ((x == 0) || is_saturated(x)) > + > +#endif While I'm not opposed to the idea of using a macro for this purpose, e.g. is_saturated() and zero_or_saturated(), I don't see much value in creating this macro buried in the SELinux directory. Especially if we end up creating a new header file just for these macros. Something as generic as this should be something we inherit from the generic kernel code so we can leverage existing conventions. If you can find a macro like these in the core kernel code, go ahead and use it, otherwise please respin this with the bound checks open coded. Oh, and use "SELinux" ;) -- paul moore www.paul-moore.com ___ Seandroid-list mailing list Seandroid-list@tycho.nsa.gov To unsubscribe, send email to seandroid-list-le...@tycho.nsa.gov. To get help, send an email containing "help" to seandroid-list-requ...@tycho.nsa.gov.
Re: [PATCH 1/3] selinux: detect invalid ebitmap
On Tue, Aug 23, 2016 at 4:49 PM, <william.c.robe...@intel.com> wrote:
> From: William Roberts <william.c.robe...@intel.com>
>
> When count is 0 and the highbit is not zero, the ebitmap is not
> valid and the internal node is not allocated. This causes issues
> when routines, like mls_context_isvalid() attempt to use the
> ebitmap_for_each_bit() and ebitmap_node_get_bit() as they assume
> a highbit > 0 will have a node allocated.
> ---
> security/selinux/ss/ebitmap.c | 3 +++
> 1 file changed, 3 insertions(+)
Hi William,
This patch looks good to me, but do I have your permission to add your sign-off?
> diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
> index 894b6cd..7d10e5d 100644
> --- a/security/selinux/ss/ebitmap.c
> +++ b/security/selinux/ss/ebitmap.c
> @@ -374,6 +374,9 @@ int ebitmap_read(struct ebitmap *e, void *fp)
> goto ok;
> }
>
> + if (e->highbit && !count)
> + goto bad;
> +
> for (i = 0; i < count; i++) {
> rc = next_entry(, fp, sizeof(u32));
> if (rc < 0) {
> --
> 1.9.1
>
> ___
> Selinux mailing list
> seli...@tycho.nsa.gov
> To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
> To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
--
paul moore
www.paul-moore.com
___
Seandroid-list mailing list
Seandroid-list@tycho.nsa.gov
To unsubscribe, send email to seandroid-list-le...@tycho.nsa.gov.
To get help, send an email containing "help" to
seandroid-list-requ...@tycho.nsa.gov.
Re: [PATCH 3/3] selinux: fix overflow and 0 length allocations
On Tue, Aug 23, 2016 at 4:49 PM, <william.c.robe...@intel.com> wrote: > From: William Roberts <william.c.robe...@intel.com> > > Throughout the SE Linux LSM, values taken from sepolicy are I'll take a closer look at this patchset after LinuxCon/LSS, but thanks for doing this ... however, one little bikeshed thing that drives me crazy is the use of "SE Linux" instead of "SELinux". Don't change anything you've submitted, but in the future please use SELinux ;) > used in places where length == 0 or length == > matter, find and fix these. > > Signed-off-by: William Roberts <william.c.robe...@intel.com> > --- > security/selinux/ss/conditional.c | 3 +++ > security/selinux/ss/policydb.c| 4 > security/selinux/ss/private.h | 7 +++ > 3 files changed, 14 insertions(+) > create mode 100644 security/selinux/ss/private.h > > diff --git a/security/selinux/ss/conditional.c > b/security/selinux/ss/conditional.c > index 456e1a9..ecc0fb6 100644 > --- a/security/selinux/ss/conditional.c > +++ b/security/selinux/ss/conditional.c > @@ -16,6 +16,7 @@ > #include "security.h" > #include "conditional.h" > #include "services.h" > +#include "private.h" > > /* > * cond_evaluate_expr evaluates a conditional expr > @@ -242,6 +243,8 @@ int cond_read_bool(struct policydb *p, struct hashtab *h, > void *fp) > goto err; > > len = le32_to_cpu(buf[2]); > + if (zero_or_saturated(len)) > + goto err; > > rc = -ENOMEM; > key = kmalloc(len + 1, GFP_KERNEL); > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > index 4b24385..0e881f3 100644 > --- a/security/selinux/ss/policydb.c > +++ b/security/selinux/ss/policydb.c > @@ -38,6 +38,7 @@ > #include "conditional.h" > #include "mls.h" > #include "services.h" > +#include "private.h" > > #define _DEBUG_HASHES > > @@ -1094,6 +1095,9 @@ static int str_read(char **strp, gfp_t flags, void *fp, > u32 len) > int rc; > char *str; > > + if (zero_or_saturated(len)) > + return -EINVAL; > + > str = kmalloc(len + 1, flags); > if (!str) > return -ENOMEM; > diff --git a/security/selinux/ss/private.h b/security/selinux/ss/private.h > new file mode 100644 > index 000..0e81a78 > --- /dev/null > +++ b/security/selinux/ss/private.h > @@ -0,0 +1,7 @@ > +#ifndef PRIVATE_H_ > +#define PRIVATE_H_ > + > +#define is_saturated(x) (x == (typeof(x))-1) > +#define zero_or_saturated(x) ((x == 0) || is_saturated(x)) > + > +#endif > -- > 1.9.1 > > ___ > Selinux mailing list > seli...@tycho.nsa.gov > To unsubscribe, send email to selinux-le...@tycho.nsa.gov. > To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov. -- paul moore www.paul-moore.com ___ Seandroid-list mailing list Seandroid-list@tycho.nsa.gov To unsubscribe, send email to seandroid-list-le...@tycho.nsa.gov. To get help, send an email containing "help" to seandroid-list-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: drop SECURITY_SELINUX_POLICYDB_VERSION_MAX
On Thu, Aug 18, 2016 at 8:27 PM, William Roberts <bill.c.robe...@gmail.com> wrote: > On Aug 18, 2016 17:07, "Paul Moore" <p...@paul-moore.com> wrote: >> >> On Mon, Aug 15, 2016 at 3:42 PM, <william.c.robe...@intel.com> wrote: >> > From: William Roberts <william.c.robe...@intel.com> >> > >> > Remove the SECURITY_SELINUX_POLICYDB_VERSION_MAX Kconfig option >> > >> > Per: https://github.com/SELinuxProject/selinux/wiki/Kernel-Todo >> > >> > This was only needed on Fedora 3 and 4 and just causes issues now, >> > so drop it. >> > >> > The MAX and MIN should just be whatever the kernel can support. >> > >> > Signed-off-by: William Roberts <william.c.robe...@intel.com> >> > --- >> > security/selinux/Kconfig| 38 >> > - >> > security/selinux/include/security.h | 4 >> > 2 files changed, 42 deletions(-) >> >> Merged, thanks for the help! > > I plan on tinkering with some of the things on that list, hopefully I can > help whittle it down. That would be great, thank you. -- paul moore www.paul-moore.com ___ Seandroid-list mailing list Seandroid-list@tycho.nsa.gov To unsubscribe, send email to seandroid-list-le...@tycho.nsa.gov. To get help, send an email containing "help" to seandroid-list-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: drop SECURITY_SELINUX_POLICYDB_VERSION_MAX
On Mon, Aug 15, 2016 at 3:42 PM, <william.c.robe...@intel.com> wrote: > From: William Roberts <william.c.robe...@intel.com> > > Remove the SECURITY_SELINUX_POLICYDB_VERSION_MAX Kconfig option > > Per: https://github.com/SELinuxProject/selinux/wiki/Kernel-Todo > > This was only needed on Fedora 3 and 4 and just causes issues now, > so drop it. > > The MAX and MIN should just be whatever the kernel can support. > > Signed-off-by: William Roberts <william.c.robe...@intel.com> > --- > security/selinux/Kconfig| 38 > - > security/selinux/include/security.h | 4 > 2 files changed, 42 deletions(-) Merged, thanks for the help! -- paul moore www.paul-moore.com ___ Seandroid-list mailing list Seandroid-list@tycho.nsa.gov To unsubscribe, send email to seandroid-list-le...@tycho.nsa.gov. To get help, send an email containing "help" to seandroid-list-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: print leading 0x on ioctlcmd audits
On Fri, Jul 15, 2016 at 3:31 PM, Roberts, William C <william.c.robe...@intel.com> wrote: > Does this mean then the patch will be applied? As I mentioned earlier, I added it to the SELinux next queue, as soon as the merge window closes (approx two weeks from this weekend) I will rotate the patch into the SELinux next branch. -- paul moore security @ redhat ___ Seandroid-list mailing list Seandroid-list@tycho.nsa.gov To unsubscribe, send email to seandroid-list-le...@tycho.nsa.gov. To get help, send an email containing "help" to seandroid-list-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: print leading 0x on ioctlcmd audits
On Fri, Jul 15, 2016 at 2:54 PM, Steve Grubb <sgr...@redhat.com> wrote: > On Thursday, July 14, 2016 6:17:32 PM EDT Paul Moore wrote: >> Re: [PATCH] selinux: print leading 0x on ioctlcmd audits >> From: Paul Moore <p...@paul-moore.com> >> To: william.c.robe...@intel.com >> CC: seli...@tycho.nsa.gov, seandroid-list@tycho.nsa.gov, Stephen Smalley >> <s...@tycho.nsa.gov>, Me, linux-au...@redhat.com Date: Yesterday 6:17 PM >> >> On Thu, Jul 14, 2016 at 3:29 PM, <william.c.robe...@intel.com> wrote: >> > From: William Roberts <william.c.robe...@intel.com> >> > >> > ioctlcmd is currently printing hex numbers, but their is no leading >> > 0x. Thus things like ioctlcmd=1234 are misleading, as the base is >> > not evident. >> > >> > Correct this by adding 0x as a prefix, so ioctlcmd=1234 becomes >> > ioctlcmd=0x1234. >> > >> > Signed-off-by: William Roberts <william.c.robe...@intel.com> >> > --- >> > security/lsm_audit.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> >> NOTE: adding Steve Grubb and the audit mailing list to the CC line >> >> Like it or not, I believe the general standard/convention when it >> comes to things like this is to leave off the "0x" prefix; the idea >> being that is saves precious space in the audit logs and the value is >> only ever going to be in hex anyway. > > We normally like the 0x prefix on anything that is hex so that stroul can > figure > it out itself. And since AVC's should in theory be rare or occassional, log > space is not a concern. > > That said, what is this ioctlcmd field name? Is this the ioctl number? As in > syscall arg a1? If so, it should be hooked up to the interpretation for that. > > Also, we have a field dictionary with some basic info about each field used in > audit events: > > http://people.redhat.com/sgrubb/audit/field-dictionary.txt Correction, that file now lives at the link below, the file on Steve's people page is deprecated. https://github.com/linux-audit/audit-documentation/blob/master/specs/fields/field-dictionary.csv > This is important so that people don't make up new ones that do the same > thing. The ioctlcmd field name should be recorded. Are there more that need > documenting? Steve/William, one of you want to send a patch/PR for the field dictionary? -- paul moore security @ redhat ___ Seandroid-list mailing list Seandroid-list@tycho.nsa.gov To unsubscribe, send email to seandroid-list-le...@tycho.nsa.gov. To get help, send an email containing "help" to seandroid-list-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: print leading 0x on ioctlcmd audits
On Thu, Jul 14, 2016 at 7:33 PM, William Roberts <bill.c.robe...@gmail.com> wrote: > On Thu, Jul 14, 2016 at 4:18 PM, William Roberts wrote: >> On Thu, Jul 14, 2016 at 3:17 PM, Paul Moore <p...@paul-moore.com> wrote: >>> On Thu, Jul 14, 2016 at 3:29 PM, <william.c.robe...@intel.com> wrote: >>> > From: William Roberts <william.c.robe...@intel.com> >>> > >>> > ioctlcmd is currently printing hex numbers, but their is no leading >>> > 0x. Thus things like ioctlcmd=1234 are misleading, as the base is >>> > not evident. >>> > >>> > Correct this by adding 0x as a prefix, so ioctlcmd=1234 becomes >>> > ioctlcmd=0x1234. >>> > >>> > Signed-off-by: William Roberts <william.c.robe...@intel.com> >>> > --- >>> > security/lsm_audit.c | 2 +- >>> > 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> NOTE: adding Steve Grubb and the audit mailing list to the CC line >>> >>> Like it or not, I believe the general standard/convention when it >>> comes to things like this is to leave off the "0x" prefix; the idea >>> being that is saves precious space in the audit logs and the value is >>> only ever going to be in hex anyway. >> >> Is it always in hex, what about pid? > > Outside of escaped untrusted input ... That's what I've been working on the past few days and it colored my view of things. I tracked down Steve just now and it looks like the preference *is* to have a "0x" prefix, my apologies for the confusion. I'll add this to the SELinux next queue. -- paul moore www.paul-moore.com ___ Seandroid-list mailing list Seandroid-list@tycho.nsa.gov To unsubscribe, send email to seandroid-list-le...@tycho.nsa.gov. To get help, send an email containing "help" to seandroid-list-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: print leading 0x on ioctlcmd audits
On Thu, Jul 14, 2016 at 3:29 PM, <william.c.robe...@intel.com> wrote:
> From: William Roberts <william.c.robe...@intel.com>
>
> ioctlcmd is currently printing hex numbers, but their is no leading
> 0x. Thus things like ioctlcmd=1234 are misleading, as the base is
> not evident.
>
> Correct this by adding 0x as a prefix, so ioctlcmd=1234 becomes
> ioctlcmd=0x1234.
>
> Signed-off-by: William Roberts <william.c.robe...@intel.com>
> ---
> security/lsm_audit.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
NOTE: adding Steve Grubb and the audit mailing list to the CC line
Like it or not, I believe the general standard/convention when it
comes to things like this is to leave off the "0x" prefix; the idea
being that is saves precious space in the audit logs and the value is
only ever going to be in hex anyway.
> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> index cccbf30..82e4dbb 100644
> --- a/security/lsm_audit.c
> +++ b/security/lsm_audit.c
> @@ -257,7 +257,7 @@ static void dump_common_audit_data(struct audit_buffer
> *ab,
> audit_log_format(ab, " ino=%lu", inode->i_ino);
> }
>
> - audit_log_format(ab, " ioctlcmd=%hx", a->u.op->cmd);
> + audit_log_format(ab, " ioctlcmd=0x%hx", a->u.op->cmd);
> break;
> }
> case LSM_AUDIT_DATA_DENTRY: {
> --
> 1.9.1
>
> ___
> Selinux mailing list
> seli...@tycho.nsa.gov
> To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
> To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
--
paul moore
www.paul-moore.com
___
Seandroid-list mailing list
Seandroid-list@tycho.nsa.gov
To unsubscribe, send email to seandroid-list-le...@tycho.nsa.gov.
To get help, send an email containing "help" to
seandroid-list-requ...@tycho.nsa.gov.
Re: Give out all the avc logs in ome time
On Fri, May 8, 2015 at 8:57 AM, Stephen Smalley s...@tycho.nsa.gov wrote: On 05/08/2015 04:46 AM, Zhi Xin wrote: Thanks for details information! For the switch question, I get your point. logd.auditd is the switch of whether logd can record selinux audit log. But I'm looking for the switch of ratelimit. I mean, removing ratelimit is really helpful for selinux debugging, especially in bringup stage. But meanwhile, removing it just opens the gate for potential DOS. So should we have a simple command that can disable ratelimit during bringup debugging and enable it for release. Just like we can switch to permissive mode by setenforce 0. For this, you'd need an audit boot parameter in order to fully disable the printk ratelimit even before logd starts. So it would require a kernel patch to define such a parameter. There are existing audit boot parameters for enabling/disabling audit (audit=0|1) and for setting the backlog limit (audit_backlog_limit=N). Those are defined in kernel/audit.c via __setup() calls. You could add an audit_ratelimit=N and/or an audit_printk_ratelimit=0|1 boot parameters. For the record, I don't have a problem with accepting a patch that added a boot parameter to control the audit ratelimit. -- paul moore www.paul-moore.com ___ Seandroid-list mailing list Seandroid-list@tycho.nsa.gov To unsubscribe, send email to seandroid-list-le...@tycho.nsa.gov. To get help, send an email containing help to seandroid-list-requ...@tycho.nsa.gov.
