* [PATCH v2] fs/squashfs: sqfs_read: Prevent arbitrary code execution
@ 2022-06-09 14:02 Miquel Raynal
2022-06-10 2:17 ` Jincheng Wang
2022-06-17 13:17 ` Tom Rini
0 siblings, 2 replies; 3+ messages in thread
From: Miquel Raynal @ 2022-06-09 14:02 UTC (permalink / raw)
To: u-boot; +Cc: Thomas Petazzoni, Joao Marcos Costa, Miquel Raynal, Jincheng Wang
Following Jincheng's report, an out-of-band write leading to arbitrary
code execution is possible because on one side the squashfs logic
accepts directory names up to 65535 bytes (u16), while U-Boot fs logic
accepts directory names up to 255 bytes long.
Prevent such an exploit from happening by capping directory name sizes
to 255. Use a define for this purpose so that developers can link the
limitation to its source and eventually kill it some day by dynamically
allocating this array (if ever desired).
Link: https://lore.kernel.org/all/CALO=DHFB+yBoXxVr5KcsK0iFdg+e7ywko4-e+72kjbcS8JBfPw@mail.gmail.com
Reported-by: Jincheng Wang <jc.w4ng@gmail.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
Changes in v2:
* Jincheng reported in private that there was a problem with small name
sizes, the last byte was lost. The reason is, dirs->entry->name_size
contains the length of the string minus one (and excluding the
trailing '\0'). The previous implementation had this handled correctly
but my initial fix did not kept the "+ 1" in place because it felt
wrong but is actually necessary. This information is actually
available in a comment a bit above in this file.
fs/squashfs/sqfs.c | 8 +++++---
include/fs.h | 4 +++-
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
index b4484fa17f5..3f1030057c4 100644
--- a/fs/squashfs/sqfs.c
+++ b/fs/squashfs/sqfs.c
@@ -976,6 +976,7 @@ int sqfs_readdir(struct fs_dir_stream *fs_dirs, struct fs_dirent **dentp)
int i_number, offset = 0, ret;
struct fs_dirent *dent;
unsigned char *ipos;
+ u16 name_size;
dirs = (struct squashfs_dir_stream *)fs_dirs;
if (!dirs->size) {
@@ -1058,9 +1059,10 @@ int sqfs_readdir(struct fs_dir_stream *fs_dirs, struct fs_dirent **dentp)
return -SQFS_STOP_READDIR;
}
- /* Set entry name */
- strncpy(dent->name, dirs->entry->name, dirs->entry->name_size + 1);
- dent->name[dirs->entry->name_size + 1] = '\0';
+ /* Set entry name (capped at FS_DIRENT_NAME_LEN which is a U-Boot limitation) */
+ name_size = min_t(u16, dirs->entry->name_size + 1, FS_DIRENT_NAME_LEN - 1);
+ strncpy(dent->name, dirs->entry->name, name_size);
+ dent->name[name_size] = '\0';
offset = dirs->entry->name_size + 1 + SQFS_ENTRY_BASE_LENGTH;
dirs->entry_count--;
diff --git a/include/fs.h b/include/fs.h
index b43f16a692f..2195dc172ec 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -174,6 +174,8 @@ int fs_write(const char *filename, ulong addr, loff_t offset, loff_t len,
#define FS_DT_REG 8 /* regular file */
#define FS_DT_LNK 10 /* symbolic link */
+#define FS_DIRENT_NAME_LEN 256
+
/**
* struct fs_dirent - directory entry
*
@@ -194,7 +196,7 @@ struct fs_dirent {
/** change_time: time of last modification */
struct rtc_time change_time;
/** name: file name */
- char name[256];
+ char name[FS_DIRENT_NAME_LEN];
};
/* Note: fs_dir_stream should be treated as opaque to the user of fs layer */
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] fs/squashfs: sqfs_read: Prevent arbitrary code execution
2022-06-09 14:02 [PATCH v2] fs/squashfs: sqfs_read: Prevent arbitrary code execution Miquel Raynal
@ 2022-06-10 2:17 ` Jincheng Wang
2022-06-17 13:17 ` Tom Rini
1 sibling, 0 replies; 3+ messages in thread
From: Jincheng Wang @ 2022-06-10 2:17 UTC (permalink / raw)
To: Miquel Raynal, u-boot, joaomarcos.costa, Tom Rini, thomas.petazzoni
It works well.
Tested-by: Jincheng Wang <jc.w4ng@gmail.com>
Miquel Raynal <miquel.raynal@bootlin.com> 于2022年6月9日周四 22:02写道:
> Following Jincheng's report, an out-of-band write leading to arbitrary
> code execution is possible because on one side the squashfs logic
> accepts directory names up to 65535 bytes (u16), while U-Boot fs logic
> accepts directory names up to 255 bytes long.
>
> Prevent such an exploit from happening by capping directory name sizes
> to 255. Use a define for this purpose so that developers can link the
> limitation to its source and eventually kill it some day by dynamically
> allocating this array (if ever desired).
>
> Link:
> https://lore.kernel.org/all/CALO=DHFB+yBoXxVr5KcsK0iFdg+e7ywko4-e+72kjbcS8JBfPw@mail.gmail.com
> Reported-by: Jincheng Wang <jc.w4ng@gmail.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>
> Changes in v2:
> * Jincheng reported in private that there was a problem with small name
> sizes, the last byte was lost. The reason is, dirs->entry->name_size
> contains the length of the string minus one (and excluding the
> trailing '\0'). The previous implementation had this handled correctly
> but my initial fix did not kept the "+ 1" in place because it felt
> wrong but is actually necessary. This information is actually
> available in a comment a bit above in this file.
>
>
> fs/squashfs/sqfs.c | 8 +++++---
> include/fs.h | 4 +++-
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
> index b4484fa17f5..3f1030057c4 100644
> --- a/fs/squashfs/sqfs.c
> +++ b/fs/squashfs/sqfs.c
> @@ -976,6 +976,7 @@ int sqfs_readdir(struct fs_dir_stream *fs_dirs, struct
> fs_dirent **dentp)
> int i_number, offset = 0, ret;
> struct fs_dirent *dent;
> unsigned char *ipos;
> + u16 name_size;
>
> dirs = (struct squashfs_dir_stream *)fs_dirs;
> if (!dirs->size) {
> @@ -1058,9 +1059,10 @@ int sqfs_readdir(struct fs_dir_stream *fs_dirs,
> struct fs_dirent **dentp)
> return -SQFS_STOP_READDIR;
> }
>
> - /* Set entry name */
> - strncpy(dent->name, dirs->entry->name, dirs->entry->name_size + 1);
> - dent->name[dirs->entry->name_size + 1] = '\0';
> + /* Set entry name (capped at FS_DIRENT_NAME_LEN which is a U-Boot
> limitation) */
> + name_size = min_t(u16, dirs->entry->name_size + 1,
> FS_DIRENT_NAME_LEN - 1);
> + strncpy(dent->name, dirs->entry->name, name_size);
> + dent->name[name_size] = '\0';
>
> offset = dirs->entry->name_size + 1 + SQFS_ENTRY_BASE_LENGTH;
> dirs->entry_count--;
> diff --git a/include/fs.h b/include/fs.h
> index b43f16a692f..2195dc172ec 100644
> --- a/include/fs.h
> +++ b/include/fs.h
> @@ -174,6 +174,8 @@ int fs_write(const char *filename, ulong addr, loff_t
> offset, loff_t len,
> #define FS_DT_REG 8 /* regular file */
> #define FS_DT_LNK 10 /* symbolic link */
>
> +#define FS_DIRENT_NAME_LEN 256
> +
> /**
> * struct fs_dirent - directory entry
> *
> @@ -194,7 +196,7 @@ struct fs_dirent {
> /** change_time: time of last modification */
> struct rtc_time change_time;
> /** name: file name */
> - char name[256];
> + char name[FS_DIRENT_NAME_LEN];
> };
>
> /* Note: fs_dir_stream should be treated as opaque to the user of fs
> layer */
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] fs/squashfs: sqfs_read: Prevent arbitrary code execution
2022-06-09 14:02 [PATCH v2] fs/squashfs: sqfs_read: Prevent arbitrary code execution Miquel Raynal
2022-06-10 2:17 ` Jincheng Wang
@ 2022-06-17 13:17 ` Tom Rini
1 sibling, 0 replies; 3+ messages in thread
From: Tom Rini @ 2022-06-17 13:17 UTC (permalink / raw)
To: Miquel Raynal; +Cc: u-boot, Thomas Petazzoni, Joao Marcos Costa, Jincheng Wang
[-- Attachment #1: Type: text/plain, Size: 917 bytes --]
On Thu, Jun 09, 2022 at 04:02:06PM +0200, Miquel Raynal wrote:
> Following Jincheng's report, an out-of-band write leading to arbitrary
> code execution is possible because on one side the squashfs logic
> accepts directory names up to 65535 bytes (u16), while U-Boot fs logic
> accepts directory names up to 255 bytes long.
>
> Prevent such an exploit from happening by capping directory name sizes
> to 255. Use a define for this purpose so that developers can link the
> limitation to its source and eventually kill it some day by dynamically
> allocating this array (if ever desired).
>
> Link: https://lore.kernel.org/all/CALO=DHFB+yBoXxVr5KcsK0iFdg+e7ywko4-e+72kjbcS8JBfPw@mail.gmail.com
> Reported-by: Jincheng Wang <jc.w4ng@gmail.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Tested-by: Jincheng Wang <jc.w4ng@gmail.com>
Applied to u-boot/master, thanks!
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-06-17 13:17 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-09 14:02 [PATCH v2] fs/squashfs: sqfs_read: Prevent arbitrary code execution Miquel Raynal
2022-06-10 2:17 ` Jincheng Wang
2022-06-17 13:17 ` Tom Rini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.