Re: [RFC PATCH 1/5] security : hardening : prevent write to proces's read-only pages from another process

["Lev R. Oshvang ." <levonshe@gmail.com>]

Hi Kees,

The patch you referred to is scoped in /proc fs.

There is a chance that hackers may use other attack methods except procfs.
There is process_vm_writev syscall , /dev/mem.
An attacker can also hijack one of the process threads and write to
read-only pages.
I admit that I am the newbie and lack knowledge, but I think my
solution is more generic and protects from more possible attacks.
Second, you suggested to control it in run-time with a knob.
I think that configuration option I propose better fit embedded system needs.
There is no need in an embedded system to turn it on/off since there is no gdb.
(the same argument for a production system),  These systems are locked
down, and perhaps the proper place to put this option is in lockdown
LSM.
Thank you for your response.
Lev


On Mon, Apr 6, 2020 at 10:16 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Apr 06, 2020 at 05:20:41PM +0300, Lev Olshvang wrote:
> > The purpose of this patch is produce hardened kernel for Embedded
> > or Production systems.
> >
> > Typically debuggers, such as gdb, write to read-only code [text]
> > sections of target process.(ptrace)
> > This kind of page protectiion violation raises minor page fault, but
> > kernel's fault handler allows it by default.
> > This is clearly attack surface for adversary.
> >
> > The proposed kernel hardening configuration option checks the type of
> > protection of the foreign vma and blocks writes to read only vma.
> >
> > When enabled, it will stop attacks modifying code or jump tables, etc.
> >
> > Code of arch_vma_access_permitted() function was extended to
> > check foreign vma flags.
> >
> > Tested on x86_64 and ARM(QEMU) with dd command which writes to
> > /proc/PID/mem in r--p or r--xp of vma area addresses range
> >
> > dd reports IO failure when tries to write to adress taken from
> > from /proc/PID/maps (PLT or code section)
>
> So, just to give some background here: the reason for this behavior is
> so debuggers can insert software breakpoints in the .text section (0xcc)
> etc. This is implemented with the "FOLL_FORCE" flag, and an attempt to
> remove it was made here:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8ee74a91ac30
> but it was later reverted (see below).
>
> There have been many prior discussions about this behavior, and a
> good thread (which I link from https://github.com/KSPP/linux/issues/37
> "Block process from writing to its own /proc/$pid/mem") is this one:
> https://lore.kernel.org/lkml/CAGXu5j+PHzDwnJxJwMJ=WuhacDn_vJWe9xZx+Kbsh28vxOGRiA@mail.gmail.com/
>
> For details on the revert see:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f511c0b17b08
>
> All this said, I think this feature would still be nice to have,
> available with some kind of knob to control it. Do you get the
> results you were expecting from just re-applying 8ee74a91ac30? If
> so, that's a much smaller change, and a single place to apply
> a knob. It would likely be best implemented with a sysctl and a
> static_branch(). A possible example for this can be seen here:
> https://lore.kernel.org/lkml/20200324203231.64324-4-keescook@chromium.org/
> Though it doesn't use a sysctl. (And perhaps this feature needs to be a
> per-process setting like "dumpable", but let's start simple with a
> system-wide control.)
>
> Can you test the FOLL_FORCE removal and refactor things to use a
> static_branch() instead?
>
> -Kees
>
> > Signed-off-by: Lev Olshvang <levonshe@gmail.com>
> > ---
> >  include/asm-generic/mm_hooks.h |  5 +++++
> >  security/Kconfig               | 10 ++++++++++
> >  2 files changed, 15 insertions(+)
> >
> > diff --git a/include/asm-generic/mm_hooks.h b/include/asm-generic/mm_hooks.h
> > index 4dbb177d1150..6e1fcce44cc2 100644
> > --- a/include/asm-generic/mm_hooks.h
> > +++ b/include/asm-generic/mm_hooks.h
> > @@ -25,6 +25,11 @@ static inline void arch_unmap(struct mm_struct *mm,
> >  static inline bool arch_vma_access_permitted(struct vm_area_struct *vma,
> >               bool write, bool execute, bool foreign)
> >  {
> > +#ifdef CONFIG_PROTECT_READONLY_USER_MEMORY
> > +     /* Forbid write to PROT_READ pages of foreign process */
> > +     if (write && foreign && (!(vma->vm_flags & VM_WRITE)))
> > +             return false;
> > +#endif
> >       /* by default, allow everything */
> >       return true;
> >  }
> > diff --git a/security/Kconfig b/security/Kconfig
> > index cd3cc7da3a55..d92e79c90d67 100644
> > --- a/security/Kconfig
> > +++ b/security/Kconfig
> > @@ -143,6 +143,16 @@ config LSM_MMAP_MIN_ADDR
> >         this low address space will need the permission specific to the
> >         systems running LSM.
> >
> > +config PROTECT_READONLY_USER_MEMORY
> > +     bool "Protect read only process memory"
> > +     help
> > +       Protects read only memory of process code and PLT table
> > +       from possible attack through /proc/PID/mem or through /dev/mem.
> > +       Refuses to insert and stop at debuggers breakpoints (prtace,gdb)
> > +       Mostly advised for embedded and production system.
> > +       Stops attempts of the malicious process to modify read only memory of another process
> > +
> > +
> >  config HAVE_HARDENED_USERCOPY_ALLOCATOR
> >       bool
> >       help
> > --
> > 2.17.1
> >
>
> --
> Kees Cook