Re: [PATCH v5 06/12] S.A.R.A.: WX protection
On Sun, Jul 07, 2019 at 05:49:35PM +0200, Salvatore Mesoraca wrote:
> Al Viro 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=mrrcj65er...@mail.gmail.com
I just didn't like how difficult it was to review the repeated checking.
I thought then (and still think now) it's worth the unusual style to
improve the immediate readability. Obviously Al disagrees. I'm not
against dropping my suggestion; it's just a pain to review it and it
seems like an area that would be highly prone to subtle typos. Perhaps
some middle ground:
#define sara_warn(msg) ({ \
if ((sara_wxp_flags & SARA_WXP_VERBOSE))\
pr_wxp(msg);\
!(sara_wxp_flags & SARA_WXP_COMPLAIN); \
})
...
if (unlikely(sara_wxp_flags & SARA_WXP_WXORX &&
vm_flags & VM_WRITE &&
vm_flags & VM_EXEC &&
sara_warn("W^X")))
return -EPERM;
that way the copy/pasting isn't present but the control flow is visible?
--
Kees Cook
RE: [PATCH v5 06/12] S.A.R.A.: WX protection
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, , 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)
Re: [PATCH v5 06/12] S.A.R.A.: WX protection
Al Viro 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=mrrcj65er...@mail.gmail.com
Re: [PATCH v5 06/12] S.A.R.A.: WX protection
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.
Re: [PATCH v5 06/12] S.A.R.A.: WX protection
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 -- ~Randy
[PATCH v5 06/12] S.A.R.A.: WX protection
Introduction of S.A.R.A. WX Protection. It aims to improve user-space programs security by applying: - W^X enforcement - W!->X (once writable never executable) mprotect restriction - Executable MMAP prevention All of the above features can be enabled or disabled both system wide or on a per executable basis through the use of configuration. W^X enforcement works by blocking any memory allocation or mprotect invocation with both the WRITE and the EXEC flags enabled. W!->X restriction works by preventing any mprotect invocation that makes executable any page that is flagged VM_MAYWRITE. Additional restrictions are in place for System V shared memory segments: if a segment was attached as writable (executable) in the past it won't be allowed to be attached as executable (writable) in the future. This feature can be configured separately for stack, heap and other allocations. 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. Parts of WX Protection are inspired by some of the features available in PaX according to my understanding of the code. Changes or omissions from the original code are mine and don't reflect the original grsecurity/PaX code. Signed-off-by: Salvatore Mesoraca --- security/sara/Kconfig | 74 + security/sara/Makefile | 1 + security/sara/include/wxprot.h | 29 ++ security/sara/main.c | 6 + security/sara/wxprot.c | 679 + 5 files changed, 789 insertions(+) create mode 100644 security/sara/include/wxprot.h create mode 100644 security/sara/wxprot.c 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. + + 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 + 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 + 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
