[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f595e572-40bc-a052-f3f2-763433d6762f@gmail.com>
Date: Wed, 4 Nov 2020 17:19:19 +0200
From: Topi Miettinen <toiwoton@...il.com>
To: Catalin Marinas <catalin.marinas@....com>
Cc: Florian Weimer <fweimer@...hat.com>, Will Deacon <will@...nel.org>,
Mark Brown <broonie@...nel.org>, Szabolcs Nagy <szabolcs.nagy@....com>,
libc-alpha@...rceware.org, Jeremy Linton <jeremy.linton@....com>,
Mark Rutland <mark.rutland@....com>, Kees Cook <keescook@...omium.org>,
Salvatore Mesoraca <s.mesoraca16@...il.com>,
Lennart Poettering <mzxreary@...inter.de>, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, kernel-hardening@...ts.openwall.com,
linux-hardening@...r.kernel.org
Subject: Re: [PATCH 0/4] aarch64: avoid mprotect(PROT_BTI|PROT_EXEC) [BZ
#26831]
On 4.11.2020 16.35, Catalin Marinas wrote:
> On Wed, Nov 04, 2020 at 11:55:57AM +0200, Topi Miettinen wrote:
>> On 4.11.2020 11.29, Florian Weimer wrote:
>>> * Will Deacon:
>>>
>>>> Is there real value in this seccomp filter if it only looks at mprotect(),
>>>> or was it just implemented because it's easy to do and sounds like a good
>>>> idea?
>>>
>>> It seems bogus to me. Everyone will just create alias mappings instead,
>>> just like they did for the similar SELinux feature. See “Example code
>>> to avoid execmem violations” in:
>>>
>>> <https://www.akkadia.org/drepper/selinux-mem.html>
> [...]
>>> As you can see, this reference implementation creates a PROT_WRITE
>>> mapping aliased to a PROT_EXEC mapping, so it actually reduces security
>>> compared to something that generates the code in an anonymous mapping
>>> and calls mprotect to make it executable.
> [...]
>> If a service legitimately needs executable and writable mappings (due to
>> JIT, trampolines etc), it's easy to disable the filter whenever really
>> needed with "MemoryDenyWriteExecute=no" (which is the default) in case of
>> systemd or a TE rule like "allow type_t self:process { execmem };" for
>> SELinux. But this shouldn't be the default case, since there are many
>> services which don't need W&X.
>
> I think Drepper's point is that separate X and W mappings, with enough
> randomisation, would be more secure than allowing W&X at the same
> address (but, of course, less secure than not having W at all, though
> that's not always possible).
>
>> I'd also question what is the value of BTI if it can be easily circumvented
>> by removing PROT_BTI with mprotect()?
>
> Well, BTI is a protection against JOP attacks. The assumption here is
> that an attacker cannot invoke mprotect() to disable PROT_BTI. If it
> can, it's probably not worth bothering with a subsequent JOP attack, it
> can already call functions directly.
I suppose that the target for the attacker is to eventually perform
system calls rather than looping forever in JOP/ROP gadgets.
> I see MDWX not as a way of detecting attacks but rather plugging
> inadvertent security holes in certain programs. On arm64, such hardening
> currently gets in the way of another hardening feature, BTI.
I don't think it has to get in the way at all. Why wouldn't something
simple like this work:
diff --git a/elf/dl-load.c b/elf/dl-load.c
index 646c5dca40..12a74d15e8 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -1170,8 +1170,13 @@ _dl_map_object_from_fd (const char *name, const
char *origname, int fd,
c->prot |= PROT_READ;
if (ph->p_flags & PF_W)
c->prot |= PROT_WRITE;
- if (ph->p_flags & PF_X)
+ if (ph->p_flags & PF_X) {
c->prot |= PROT_EXEC;
+#ifdef PROT_BTI
+ if (GLRO(dl_bti) & 1)
+ c->prot |= PROT_BTI;
+#endif
+ }
#endif
break;
diff --git a/elf/dl-support.c b/elf/dl-support.c
index 7704c101c5..22c7cc7b81 100644
--- a/elf/dl-support.c
+++ b/elf/dl-support.c
@@ -222,7 +222,7 @@ __rtld_lock_define_initialized_recursive (,
_dl_load_write_lock)
#ifdef HAVE_AUX_VECTOR
-int _dl_clktck;
+int _dl_clktck, _dl_bti;
void
_dl_aux_init (ElfW(auxv_t) *av)
@@ -294,6 +294,11 @@ _dl_aux_init (ElfW(auxv_t) *av)
case AT_RANDOM:
_dl_random = (void *) av->a_un.a_val;
break;
+#ifdef PROT_BTI
+ case AT_BTI:
+ _dl_bti = (void *) av->a_un.a_val;
+ break;
+#endif
DL_PLATFORM_AUXV
}
if (seen == 0xf)
Kernel sets the aux vector to indicate that BTI should be enabled for
all segments and main exe is already protected.
-Topi
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
