Message ID | 1562410493-8661-1-git-send-email-s.mesoraca16@gmail.com (mailing list archive) |
---|---|
Headers | show |
Series | S.A.R.A. a new stacked LSM | expand |
On Saturday, July 6, 2019 10:54 AM, Salvatore Mesoraca <s.mesoraca16@gmail.com> wrote: > S.A.R.A. is meant to be stacked but it needs cred blobs and the procattr > interface, so I temporarily implemented those parts in a way that won't > be acceptable for upstream, but it works for now. I know that there > is some ongoing work to make cred blobs and procattr stackable, as soon > as the new interfaces will be available I'll reimplement the involved > parts. I thought all stacking pieces for minor LSM were merged in Linux 5.1. Is there still something missing or is this comment out-fo-date? Jordan
You are right. I just forgot to remove that paragraph from the cover letter. My bad. Thank you for noticing that :) Il giorno sab 6 lug 2019 alle ore 16:33 Jordan Glover <Golden_Miller83@protonmail.ch> ha scritto: > > On Saturday, July 6, 2019 10:54 AM, Salvatore Mesoraca <s.mesoraca16@gmail.com> wrote: > > > S.A.R.A. is meant to be stacked but it needs cred blobs and the procattr > > interface, so I temporarily implemented those parts in a way that won't > > be acceptable for upstream, but it works for now. I know that there > > is some ongoing work to make cred blobs and procattr stackable, as soon > > as the new interfaces will be available I'll reimplement the involved > > parts. > > I thought all stacking pieces for minor LSM were merged in Linux 5.1. > Is there still something missing or is this comment out-fo-date? > > Jordan
On 7/6/19 3:54 AM, Salvatore Mesoraca wrote: > diff --git a/security/sara/Kconfig b/security/sara/Kconfig > index b98cf27..54a96e0 100644 > --- a/security/sara/Kconfig > +++ b/security/sara/Kconfig > @@ -60,3 +60,77 @@ config SECURITY_SARA_NO_RUNTIME_ENABLE > > If unsure, answer Y. > > +config SECURITY_SARA_WXPROT > + bool "WX Protection: W^X and W!->X protections" > + depends on SECURITY_SARA > + default y > + help > + WX Protection aims to improve user-space programs security by applying: > + - W^X memory restriction > + - W!->X (once writable never executable) mprotect restriction > + - Executable MMAP prevention > + See Documentation/admin-guide/LSM/SARA.rst. for further information. .rst for further information. > + > + If unsure, answer Y. > + > +choice > + prompt "Default action for W^X and W!->X protections" > + depends on SECURITY_SARA > + depends on SECURITY_SARA_WXPROT > + default SECURITY_SARA_WXPROT_DEFAULT_FLAGS_ALL_COMPLAIN_VERBOSE > + > + help Use tab instead of spaces for indentation above. > + Choose the default behaviour of WX Protection when no config > + rule matches or no rule is loaded. > + For further information on available flags and their meaning > + see Documentation/admin-guide/LSM/SARA.rst. > + > + config SECURITY_SARA_WXPROT_DEFAULT_FLAGS_ALL_COMPLAIN_VERBOSE > + bool "Protections enabled but not enforced." > + help > + All features enabled except "Executable MMAP prevention", > + verbose reporting, but no actual enforce: it just complains. > + Its numeric value is 0x3f, for more information see > + Documentation/admin-guide/LSM/SARA.rst. > + > + config SECURITY_SARA_WXPROT_DEFAULT_FLAGS_ALL_ENFORCE_VERBOSE > + bool "Full protection, verbose." > + help > + All features enabled except "Executable MMAP prevention". > + The enabled features will be enforced with verbose reporting. > + Its numeric value is 0x2f, for more information see > + Documentation/admin-guide/LSM/SARA.rst. > + > + config SECURITY_SARA_WXPROT_DEFAULT_FLAGS_ALL_ENFORCE > + bool "Full protection, quiet." > + help > + All features enabled except "Executable MMAP prevention". > + The enabled features will be enforced quietly. > + Its numeric value is 0xf, for more information see > + Documentation/admin-guide/LSM/SARA.rst. > + > + config SECURITY_SARA_WXPROT_DEFAULT_FLAGS_NONE > + bool "No protection at all." > + help > + All features disabled. > + Its numeric value is 0, for more information see > + Documentation/admin-guide/LSM/SARA.rst. > +endchoice > + > +config SECURITY_SARA_WXPROT_DISABLED > + bool "WX protection will be disabled at boot." > + depends on SECURITY_SARA_WXPROT > + default n Omit "default n" please. > + help > + If you say Y here WX protection won't be enabled at startup. You can > + override this option via user-space utilities or at boot time via > + "sara.wxprot_enabled=[0|1]" kernel parameter. > + > + If unsure, answer N. > + > +config SECURITY_SARA_WXPROT_DEFAULT_FLAGS > + hex > + default "0x3f" if SECURITY_SARA_WXPROT_DEFAULT_FLAGS_ALL_COMPLAIN_VERBOSE > + default "0x2f" if SECURITY_SARA_WXPROT_DEFAULT_FLAGS_ALL_ENFORCE_VERBOSE > + default "0xf" if SECURITY_SARA_WXPROT_DEFAULT_FLAGS_ALL_ENFORCE > + default "0" if SECURITY_SARA_WXPROT_DEFAULT_FLAGS_NONE
On Sat, 6 Jul 2019, Salvatore Mesoraca wrote:
> S.A.R.A. (S.A.R.A. is Another Recursive Acronym) is a stacked Linux
Please make this just SARA. Nobody wants to read or type S.A.R.A.
James Morris <jmorris@namei.org> wrote: > > On Sat, 6 Jul 2019, Salvatore Mesoraca wrote: > > > S.A.R.A. (S.A.R.A. is Another Recursive Acronym) is a stacked Linux > > Please make this just SARA. Nobody wants to read or type S.A.R.A. Agreed. Thank you for your suggestion.
Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Sat, Jul 06, 2019 at 12:54:47PM +0200, Salvatore Mesoraca wrote: > > > +#define sara_warn_or_return(err, msg) do { \ > > + if ((sara_wxp_flags & SARA_WXP_VERBOSE)) \ > > + pr_wxp(msg); \ > > + if (!(sara_wxp_flags & SARA_WXP_COMPLAIN)) \ > > + return -err; \ > > +} while (0) > > + > > +#define sara_warn_or_goto(label, msg) do { \ > > + if ((sara_wxp_flags & SARA_WXP_VERBOSE)) \ > > + pr_wxp(msg); \ > > + if (!(sara_wxp_flags & SARA_WXP_COMPLAIN)) \ > > + goto label; \ > > +} while (0) > > No. This kind of "style" has no place in the kernel. > > Don't hide control flow. It's nasty enough to reviewers, > but it's pure hell on anyone who strays into your code while > chasing a bug or doing general code audit. In effect, you > are creating your oh-so-private C dialect and assuming that > everyone who ever looks at your code will start with learning > that *AND* incorporating it into their mental C parser. > I'm sorry, but you are not that important. > > If it looks like a function call, a casual reader will assume > that this is exactly what it is. And when one is scanning > through a function (e.g. to tell if handling of some kind > of refcounts is correct, with twentieth grep through the > tree having brought something in your code into the view), > the last thing one wants is to switch between the area-specific > C dialects. Simply because looking at yours is sandwiched > between digging through some crap in drivers/target/ and that > weird thing in kernel/tracing/, hopefully staying limited > to 20 seconds of glancing through several functions in your > code. > > Don't Do That. Really. I understand your concerns. The first version of SARA didn't use these macros, they were added because I was asked[1] to do so. I have absolutely no problems in reverting this change. I just want to make sure that there is agreement on this matter. Maybe Kees can clarify his stance. Thank you for your suggestions. [1] https://lkml.kernel.org/r/CAGXu5jJuQx2qOt_aDqDQDcqGOZ5kmr5rQ9Zjv=MRRCJ65ERfGw@mail.gmail.com
From: Salvatore Mesoraca > Sent: 06 July 2019 11:55 ... > Executable MMAP prevention works by preventing any new executable > allocation after the dynamic libraries have been loaded. It works under the > assumption that, when the dynamic libraries have been finished loading, the > RELRO section will be marked read only. What about writing to the file of a dynamic library after it is loaded but before it is faulted it (or after evicting it from the I$). ... > +#define find_relro_section(ELFH, ELFP, FILE, RELRO, FOUND) do { \ > + unsigned long i; \ > + int _tmp; \ > + loff_t _pos = 0; \ > + if (ELFH.e_type == ET_DYN || ELFH.e_type == ET_EXEC) { \ > + for (i = 0; i < ELFH.e_phnum; ++i) { \ > + _pos = ELFH.e_phoff + i*sizeof(ELFP); \ > + _tmp = kernel_read(FILE, &ELFP, sizeof(ELFP), \ > + &_pos); \ > + if (_tmp != sizeof(ELFP)) \ > + break; \ > + if (ELFP.p_type == PT_GNU_RELRO) { \ > + RELRO = ELFP.p_offset >> PAGE_SHIFT; \ > + FOUND = true; \ > + break; \ > + } \ > + } \ > + } \ > +} while (0) This is big for a #define. Since it contains kernel_read() it can't really matter if it is a real function. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)