[devel] [PATCH] AltHa: avoid kernel stack overflow

[devel] [PATCH] AltHa: avoid kernel stack overflow

[Alexey Sheplyakov]

From: Alexey Sheplyakov <asheplyakov@altlinux.org>

* The kernel stack on most architectures is 4096 bytes.
* PATH_MAX is also 4096 (bytes).

Therefore

* Allocating PATH_MAX bytes on the stack definitely overflows
  the (kernel) stack (perhaps this can be exploited by creating
  a long enough path). To avoid the problem allocate `path_buffer`
  on the heap (and free it on all code paths)

* CONFIG_FRAME_WARN=6144 (which is 1.5x the kernel stack size)
  is wrong and dangerous (it papers over actual overflows and
  creates a false sense of safety), use the default value instead
  (2048 bytes, which is a half of the kernel stack)

Signed-off-by: Alexey Sheplyakov <asheplyakov@altlinux.org>
---
 config                     |  2 +-
 security/altha/altha_lsm.c | 21 ++++++++++++++++++---
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/config b/config
index da093b3f8e64..5ef328887321 100644
--- a/config
+++ b/config
@@ -10347,7 +10347,7 @@ CONFIG_DEBUG_INFO_BTF=y
 CONFIG_PAHOLE_HAS_SPLIT_BTF=y
 CONFIG_DEBUG_INFO_BTF_MODULES=y
 # CONFIG_GDB_SCRIPTS is not set
-CONFIG_FRAME_WARN=6144
+CONFIG_FRAME_WARN=2048
 # CONFIG_STRIP_ASM_SYMS is not set
 # CONFIG_READABLE_ASM is not set
 # CONFIG_HEADERS_INSTALL is not set
diff --git a/security/altha/altha_lsm.c b/security/altha/altha_lsm.c
index ccde83ebb26c..c670ad7ed458 100644
--- a/security/altha/altha_lsm.c
+++ b/security/altha/altha_lsm.c
@@ -243,8 +243,13 @@ static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * f
 	struct altha_list_struct *node;
 	/* when it's not a shebang issued script interpreter */
 	if (rstrscript_enabled && bprm->executable == bprm->interpreter) {
-		char path_buffer[PATH_MAX];
 		char *path_p;
+		char *path_buffer;
+
+		path_buffer = kmalloc(PATH_MAX, GFP_KERNEL);
+		if (!path_buffer)
+			return -ENOMEM;
+
 		path_p = d_path(&bprm->file->f_path,path_buffer,PATH_MAX);
 		down_read(&interpreters_sem);
 		list_for_each_entry(node, &interpreters_list, list) {
@@ -255,16 +260,24 @@ static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * f
 				    ("AltHa/RestrScript: %s is blocked to run directly by %d\n",
 				     bprm->filename, cur_uid);
 				up_read(&interpreters_sem);
+				kfree(path_buffer);
 				return -EPERM;
 			}
 		}
 		up_read(&interpreters_sem);
+		kfree(path_buffer);
 	}
 	if (unlikely(nosuid_enabled &&
 		     !uid_eq(bprm->cred->uid, bprm->cred->euid))) {
-		char path_buffer[PATH_MAX];
 		char *path_p;
-		uid_t cur_uid = from_kuid(bprm->cred->user_ns, bprm->cred->uid);
+		char *path_buffer;
+		uid_t cur_uid;
+
+		path_buffer = kmalloc(PATH_MAX, GFP_KERNEL);
+		if (!path_buffer)
+			return -ENOMEM;
+
+		cur_uid = from_kuid(bprm->cred->user_ns, bprm->cred->uid);
 		path_p = d_path(&bprm->file->f_path,path_buffer,PATH_MAX);
 		down_read(&nosuid_exceptions_sem);
 		list_for_each_entry(node, &nosuid_exceptions_list, list) {
@@ -273,6 +286,7 @@ static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * f
 				    ("AltHa/NoSUID: %s permitted to setuid from %d\n",
 				     bprm->filename, cur_uid);
 				up_read(&nosuid_exceptions_sem);
+				kfree(path_buffer);
 				return 0;
 			}
 		}
@@ -281,6 +295,7 @@ static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * f
 		    ("AltHa/NoSUID: %s prevented to setuid from %d\n",
 		     bprm->filename, cur_uid);
 		bprm->cred->euid = bprm->cred->uid;
+		kfree(path_buffer);
 	}
 	return 0;
 }
-- 
2.32.0

Re: [devel] [PATCH] AltHa: avoid kernel stack overflow

[Vladimir D. Seleznev]

On Fri, Jan 14, 2022 at 07:18:23PM +0400, Alexey Sheplyakov wrote:
> From: Alexey Sheplyakov <asheplyakov@altlinux.org>
> 
> * The kernel stack on most architectures is 4096 bytes.
> * PATH_MAX is also 4096 (bytes).
> 
> Therefore
> 
> * Allocating PATH_MAX bytes on the stack definitely overflows
>   the (kernel) stack (perhaps this can be exploited by creating
>   a long enough path).

To exploit this an attacker must have superuser privileges. But of
course the bug should be fixed.

Acked.

> To avoid the problem allocate `path_buffer`
>   on the heap (and free it on all code paths)
> 
> * CONFIG_FRAME_WARN=6144 (which is 1.5x the kernel stack size)
>   is wrong and dangerous (it papers over actual overflows and
>   creates a false sense of safety), use the default value instead
>   (2048 bytes, which is a half of the kernel stack)
> 
> Signed-off-by: Alexey Sheplyakov <asheplyakov@altlinux.org>
> ---
>  config                     |  2 +-
>  security/altha/altha_lsm.c | 21 ++++++++++++++++++---
>  2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/config b/config
> index da093b3f8e64..5ef328887321 100644
> --- a/config
> +++ b/config
> @@ -10347,7 +10347,7 @@ CONFIG_DEBUG_INFO_BTF=y
>  CONFIG_PAHOLE_HAS_SPLIT_BTF=y
>  CONFIG_DEBUG_INFO_BTF_MODULES=y
>  # CONFIG_GDB_SCRIPTS is not set
> -CONFIG_FRAME_WARN=6144
> +CONFIG_FRAME_WARN=2048
>  # CONFIG_STRIP_ASM_SYMS is not set
>  # CONFIG_READABLE_ASM is not set
>  # CONFIG_HEADERS_INSTALL is not set
> diff --git a/security/altha/altha_lsm.c b/security/altha/altha_lsm.c
> index ccde83ebb26c..c670ad7ed458 100644
> --- a/security/altha/altha_lsm.c
> +++ b/security/altha/altha_lsm.c
> @@ -243,8 +243,13 @@ static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * f
>  	struct altha_list_struct *node;
>  	/* when it's not a shebang issued script interpreter */
>  	if (rstrscript_enabled && bprm->executable == bprm->interpreter) {
> -		char path_buffer[PATH_MAX];
>  		char *path_p;
> +		char *path_buffer;
> +
> +		path_buffer = kmalloc(PATH_MAX, GFP_KERNEL);
> +		if (!path_buffer)
> +			return -ENOMEM;
> +
>  		path_p = d_path(&bprm->file->f_path,path_buffer,PATH_MAX);
>  		down_read(&interpreters_sem);
>  		list_for_each_entry(node, &interpreters_list, list) {
> @@ -255,16 +260,24 @@ static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * f
>  				    ("AltHa/RestrScript: %s is blocked to run directly by %d\n",
>  				     bprm->filename, cur_uid);
>  				up_read(&interpreters_sem);
> +				kfree(path_buffer);
>  				return -EPERM;
>  			}
>  		}
>  		up_read(&interpreters_sem);
> +		kfree(path_buffer);
>  	}
>  	if (unlikely(nosuid_enabled &&
>  		     !uid_eq(bprm->cred->uid, bprm->cred->euid))) {
> -		char path_buffer[PATH_MAX];
>  		char *path_p;
> -		uid_t cur_uid = from_kuid(bprm->cred->user_ns, bprm->cred->uid);
> +		char *path_buffer;
> +		uid_t cur_uid;
> +
> +		path_buffer = kmalloc(PATH_MAX, GFP_KERNEL);
> +		if (!path_buffer)
> +			return -ENOMEM;
> +
> +		cur_uid = from_kuid(bprm->cred->user_ns, bprm->cred->uid);
>  		path_p = d_path(&bprm->file->f_path,path_buffer,PATH_MAX);
>  		down_read(&nosuid_exceptions_sem);
>  		list_for_each_entry(node, &nosuid_exceptions_list, list) {
> @@ -273,6 +286,7 @@ static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * f
>  				    ("AltHa/NoSUID: %s permitted to setuid from %d\n",
>  				     bprm->filename, cur_uid);
>  				up_read(&nosuid_exceptions_sem);
> +				kfree(path_buffer);
>  				return 0;
>  			}
>  		}
> @@ -281,6 +295,7 @@ static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * f
>  		    ("AltHa/NoSUID: %s prevented to setuid from %d\n",
>  		     bprm->filename, cur_uid);
>  		bprm->cred->euid = bprm->cred->uid;
> +		kfree(path_buffer);
>  	}
>  	return 0;
>  }
> -- 
> 2.32.0

-- 
   WBR,
   Vladimir D. Seleznev

Re: [devel] [PATCH] AltHa: avoid kernel stack overflow

[Anton V. Boyarshinov]

В Fri, 14 Jan 2022 19:18:23 +0400
Alexey Sheplyakov <asheplyakov@basealt.ru> пишет:

> From: Alexey Sheplyakov <asheplyakov@altlinux.org>
 
Thank you! I've planned to do it, but too lazy.

 * The kernel stack on most architectures is 4096 bytes.
> * PATH_MAX is also 4096 (bytes).
> 
> Therefore
> 
> * Allocating PATH_MAX bytes on the stack definitely overflows
>   the (kernel) stack (perhaps this can be exploited by creating
>   a long enough path). To avoid the problem allocate `path_buffer`
>   on the heap (and free it on all code paths)
> 
> * CONFIG_FRAME_WARN=6144 (which is 1.5x the kernel stack size)
>   is wrong and dangerous (it papers over actual overflows and
>   creates a false sense of safety), use the default value instead
>   (2048 bytes, which is a half of the kernel stack)
> 
> Signed-off-by: Alexey Sheplyakov <asheplyakov@altlinux.org>
> ---
>  config                     |  2 +-
>  security/altha/altha_lsm.c | 21 ++++++++++++++++++---
>  2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/config b/config
> index da093b3f8e64..5ef328887321 100644
> --- a/config
> +++ b/config
> @@ -10347,7 +10347,7 @@ CONFIG_DEBUG_INFO_BTF=y
>  CONFIG_PAHOLE_HAS_SPLIT_BTF=y
>  CONFIG_DEBUG_INFO_BTF_MODULES=y
>  # CONFIG_GDB_SCRIPTS is not set
> -CONFIG_FRAME_WARN=6144
> +CONFIG_FRAME_WARN=2048
>  # CONFIG_STRIP_ASM_SYMS is not set
>  # CONFIG_READABLE_ASM is not set
>  # CONFIG_HEADERS_INSTALL is not set
> diff --git a/security/altha/altha_lsm.c b/security/altha/altha_lsm.c
> index ccde83ebb26c..c670ad7ed458 100644
> --- a/security/altha/altha_lsm.c
> +++ b/security/altha/altha_lsm.c
> @@ -243,8 +243,13 @@ static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * f
>  	struct altha_list_struct *node;
>  	/* when it's not a shebang issued script interpreter */
>  	if (rstrscript_enabled && bprm->executable == bprm->interpreter) {
> -		char path_buffer[PATH_MAX];
>  		char *path_p;
> +		char *path_buffer;
> +
> +		path_buffer = kmalloc(PATH_MAX, GFP_KERNEL);
> +		if (!path_buffer)
> +			return -ENOMEM;
> +
>  		path_p = d_path(&bprm->file->f_path,path_buffer,PATH_MAX);
>  		down_read(&interpreters_sem);
>  		list_for_each_entry(node, &interpreters_list, list) {
> @@ -255,16 +260,24 @@ static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * f
>  				    ("AltHa/RestrScript: %s is blocked to run directly by %d\n",
>  				     bprm->filename, cur_uid);
>  				up_read(&interpreters_sem);
> +				kfree(path_buffer);
>  				return -EPERM;
>  			}
>  		}
>  		up_read(&interpreters_sem);
> +		kfree(path_buffer);
>  	}
>  	if (unlikely(nosuid_enabled &&
>  		     !uid_eq(bprm->cred->uid, bprm->cred->euid))) {
> -		char path_buffer[PATH_MAX];
>  		char *path_p;
> -		uid_t cur_uid = from_kuid(bprm->cred->user_ns, bprm->cred->uid);
> +		char *path_buffer;
> +		uid_t cur_uid;
> +
> +		path_buffer = kmalloc(PATH_MAX, GFP_KERNEL);
> +		if (!path_buffer)
> +			return -ENOMEM;
> +
> +		cur_uid = from_kuid(bprm->cred->user_ns, bprm->cred->uid);
>  		path_p = d_path(&bprm->file->f_path,path_buffer,PATH_MAX);
>  		down_read(&nosuid_exceptions_sem);
>  		list_for_each_entry(node, &nosuid_exceptions_list, list) {
> @@ -273,6 +286,7 @@ static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * f
>  				    ("AltHa/NoSUID: %s permitted to setuid from %d\n",
>  				     bprm->filename, cur_uid);
>  				up_read(&nosuid_exceptions_sem);
> +				kfree(path_buffer);
>  				return 0;
>  			}
>  		}
> @@ -281,6 +295,7 @@ static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * f
>  		    ("AltHa/NoSUID: %s prevented to setuid from %d\n",
>  		     bprm->filename, cur_uid);
>  		bprm->cred->euid = bprm->cred->uid;
> +		kfree(path_buffer);
>  	}
>  	return 0;
>  }