[RFC] security hardening: block write to read_only pages of a target process.
diff mbox series

Message ID 1566563895-2081-1-git-send-email-levonshe@yandex.com
State New
Headers show
Series
  • [RFC] security hardening: block write to read_only pages of a target process.
Related show

Commit Message

Lev Olshvang Aug. 23, 2019, 12:38 p.m. UTC
Target process is not a current process.
It is a foreign process in the terminogy of page fault handler.

Typically debuggers, such as gdb, write to read-only code [text]
sections of target process.
This patch introduce kernel hardening configuration option.
When enabled, it will stop attacks modifying code or jump tables.

Onky Code of arch_vma_access_permitted() function was extended to
check foreign vma vm_flags.

New logic denies to accept page fault caused by page protection violation.

Separatly applied for x86,powerpc and unicore32
arch_vma_access_permitted() function is not referenced in unicore32 and um
architectures and seems to be obsolete,IMHO.

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)

Signed-off-by: Lev Olshvang <levonshe@yandex.com>
---
 arch/powerpc/include/asm/mmu_context.h   |  7 +++++++
 arch/powerpc/mm/book3s64/pkeys.c         |  6 ++++++
 arch/um/include/asm/mmu_context.h        |  6 ++++++
 arch/unicore32/include/asm/mmu_context.h |  8 +++++++-
 arch/x86/include/asm/mmu_context.h       | 10 +++++++++-
 include/asm-generic/mm_hooks.h           |  6 ++++++
 security/Kconfig                         | 11 +++++++++++
 7 files changed, 52 insertions(+), 2 deletions(-)

Comments

Jann Horn Aug. 23, 2019, 2:09 p.m. UTC | #1
On Fri, Aug 23, 2019 at 2:38 PM Lev Olshvang <levonshe@yandex.com> wrote:
> Target process is not a current process.
> It is a foreign process in the terminogy of page fault handler.
>
> Typically debuggers, such as gdb, write to read-only code [text]
> sections of target process.
> This patch introduce kernel hardening configuration option.
> When enabled, it will stop attacks modifying code or jump tables.

This patch is missing context. What, at a high level, is your goal
with this patch? My guess is that you're trying to close gaps in the
protection that SELinux provides with "execmod" rules, and that you
want to ensure that an attacker who is exploiting a memory corruption
bug still can't modify machine code in the process being exploited.

You should CC appropriate kernel developers and lists for the code
you're modifying; see Documentation/process/submitting-patches.rst,
section "5) Select the recipients for your patch".

> Onky Code of arch_vma_access_permitted() function was extended to
> check foreign vma vm_flags.
>
> New logic denies to accept page fault caused by page protection violation.
>
> Separatly applied for x86,powerpc and unicore32
> arch_vma_access_permitted() function is not referenced in unicore32 and um
> architectures and seems to be obsolete,IMHO.

I think you're putting your checks in the wrong place.
If you put security checks into architecture-specific parts of the
codebase, then someone who wants to modify those checks might only
change some of the copies; and then the security checks become
inconsistent across architectures.
But also, there is already a function parameter at a higher level that
controls this behavior: In fs/proc/base.c, the function mem_rw() calls
access_remote_vm() with the flag FOLL_FORCE, which controls whether
overwriting non-writable memory is allowed. If you want to prevent the
use of /proc/*/mem to overwrite non-writable memory, then you can just
make it configurable whether that flag is used in that function.


> diff --git a/security/Kconfig b/security/Kconfig
> index 0d65594..03ff948 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> +config PROTECT_READONLY_USER_MEMORY
> +       bool "protect read only process memory"
> +       depends on !(CONFIG_CROSS_MEMORY_ATTACH)

Why this "depends on" rule? CONFIG_CROSS_MEMORY_ATTACH only controls
the process_vm_readv() and process_vm_writev() syscalls, and those
can't modify read-only memory because process_vm_rw_single_vec()
doesn't set FOLL_FORCE.

Also, I think the CONFIG_ prefix shouldn't be used inside Kconfig.

> +       help
> +         Protects read only memory of process code and PLT table from possible attack
> +         through /proc/PID/mem.
> +         Forbid writes to READ ONLY user pages of foreign process
> +         Mostly advised for embedded and production system.
> +         Disables process_vm_writev() syscall used in MP computing.

You are introducing a kernel config flag, but no corresponding sysctl,
which means that changing the flag requires recompiling the kernel.
If your intent really is to strengthen SELinux execmod rules as I
guessed, then you could also perform a check against the SELinux
context of the tracer in mem_open(), or something like that - then you
might not need to make this configurable via kconfig anymore.

Patch
diff mbox series

diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
index 58efca9..db37c61 100644
--- a/arch/powerpc/include/asm/mmu_context.h
+++ b/arch/powerpc/include/asm/mmu_context.h
@@ -251,6 +251,13 @@  void arch_dup_pkeys(struct mm_struct *oldmm, 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
+	if (write && foreign && (!(vma->vm_flags & VM_WRITE))) {
+		/* Forbid write to PROT_READ pages of foreign process */
+		return false;
+	}
+#endif
+
 	/* by default, allow everything */
 	return true;
 }
diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index ae7fca4..b70fdfd 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c
@@ -406,6 +406,12 @@  static inline bool vma_is_foreign(struct vm_area_struct *vma)
 bool arch_vma_access_permitted(struct vm_area_struct *vma, bool write,
 			       bool execute, bool foreign)
 {
+#ifdef CONFIG_PROTECT_READONLY_USER_MEMORY
+	if (write && foreign && (!(vma->vm_flags & VM_WRITE))) {
+		/* Forbid write to PROT_READ pages of foreign process */
+		return false;
+	}
+#endif
 	if (static_branch_likely(&pkey_disabled))
 		return true;
 	/*
diff --git a/arch/um/include/asm/mmu_context.h b/arch/um/include/asm/mmu_context.h
index 00cefd3..2c56ce9 100644
--- a/arch/um/include/asm/mmu_context.h
+++ b/arch/um/include/asm/mmu_context.h
@@ -33,6 +33,12 @@  static inline void arch_bprm_mm_init(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
+	if (write && foreign && (!(vma->vm_flags & VM_WRITE))) {
+		/* Forbid write to PROT_READ pages of foreign process */
+		return false;
+	}
+#endif
 	/* by default, allow everything */
 	return true;
 }
diff --git a/arch/unicore32/include/asm/mmu_context.h b/arch/unicore32/include/asm/mmu_context.h
index 247a07a..730997c 100644
--- a/arch/unicore32/include/asm/mmu_context.h
+++ b/arch/unicore32/include/asm/mmu_context.h
@@ -97,7 +97,13 @@  static inline void arch_bprm_mm_init(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
+	if (write && foreign && (!(vma->vm_flags & VM_WRITE))) {
+		/* Forbid write to PROT_READ pages of foreign process */
+		return false;
+	}
+#endif
 	/* by default, allow everything */
 	return true;
 }
-#endif
+#endif /*__UNICORE_MMU_CONTEXT_H__*/
diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
index 9024236..77b2801 100644
--- a/arch/x86/include/asm/mmu_context.h
+++ b/arch/x86/include/asm/mmu_context.h
@@ -329,12 +329,20 @@  static inline bool vma_is_foreign(struct vm_area_struct *vma)
 static inline bool arch_vma_access_permitted(struct vm_area_struct *vma,
 		bool write, bool execute, bool foreign)
 {
-	/* pkeys never affect instruction fetches */
+
+#ifdef CONFIG_PROTECT_READONLY_USER_MEMORY
+	if (write && foreign && (!(vma->vm_flags & VM_WRITE))) {
+		/* Forbid write to PROT_READ pages of foreign process */
+		return false;
+	}
+#endif
+	/* Don't check PKRU since pkeys never affect instruction fetches */
 	if (execute)
 		return true;
 	/* allow access if the VMA is not one from this process */
 	if (foreign || vma_is_foreign(vma))
 		return true;
+
 	return __pkru_allows_pkey(vma_pkey(vma), write);
 }
 
diff --git a/include/asm-generic/mm_hooks.h b/include/asm-generic/mm_hooks.h
index 6736ed2..31dae5a 100644
--- a/include/asm-generic/mm_hooks.h
+++ b/include/asm-generic/mm_hooks.h
@@ -30,6 +30,12 @@  static inline void arch_bprm_mm_init(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
+	if (write && foreign && (!(vma->vm_flags & VM_WRITE))) {
+		/* Forbid write to PROT_READ pages of foreign process */
+		return false;
+	}
+#endif
 	/* by default, allow everything */
 	return true;
 }
diff --git a/security/Kconfig b/security/Kconfig
index 0d65594..03ff948 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -143,6 +143,17 @@  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"
+	depends on !(CONFIG_CROSS_MEMORY_ATTACH)
+	help
+	  Protects read only memory of process code and PLT table from possible attack
+	  through /proc/PID/mem.
+	  Forbid writes to READ ONLY user pages of foreign process
+	  Mostly advised for embedded and production system.
+	  Disables process_vm_writev() syscall used in MP computing.
+
+
 config HAVE_HARDENED_USERCOPY_ALLOCATOR
 	bool
 	help