Fix Integer overflow in binary CPIO handler#5089
Conversation
github-actions
Bot
added
needs triage
zyv
added
area: vfs
| // case? | ||
|
|
||
| inode->linkname = g_malloc (st->st_size + 1); | ||
| if (st->st_size < 0 || (gsize) st->st_size >= G_MAXSIZE) |
There was a problem hiding this comment.
Why do you reduce off_t to gsize? I think, sizeof (off_t) >= sizeof (gsize), so the correct way is
st->st_size >= (off_t) G_MAXSIZE
| #endif | ||
| st.st_size = ((off_t) u.buf.c_filesizes[0] << 16) | u.buf.c_filesizes[1]; | ||
|
|
||
| if (st.st_size < 0 || st.st_size > MC_MAXPATHLEN) |
There was a problem hiding this comment.
Why do you compare file size with path length? It is correct for link not for regular file,
| u.st.st_size = hd.c_filesize; | ||
|
|
||
| if (u.st.st_size < 0 || u.st.st_size > MC_MAXPATHLEN) | ||
| { |
There was a problem hiding this comment.
You are completely right. I have update the PR w newer fix.
|
|
||
| inode->linkname = g_malloc (st->st_size + 1); | ||
| if (st->st_size < 0 || st->st_size >= (off_t) G_MAXSIZE | ||
| || st->st_size > MC_MAXPATHLEN) |
There was a problem hiding this comment.
MC_MAXPATHLEN is definitely less than G_MAXSIZE. Thus 2nd test isn't needed.
There was a problem hiding this comment.
@sectroyer I thought this comment was still open. Could you please comment and resolve all remarks which you think are fixed and re-request an approval?
|
@sectroyer, are you still planning to do the last fix? I'd like to merge this as soon as master gets unblocked. Thanks! |
|
@zyv i would definitely would like this fix to be pushed. Can it be applied directly or are there any issues and you would prefer me to first update pr to git head ? |
…ndler Three changes addressing reviewer feedback: - cpio_create_entry: fix cast direction in the bounds check — compare against (off_t) G_MAXSIZE rather than casting st_size to gsize. Also add the MC_MAXPATHLEN upper bound here, co-located with the g_malloc call and applied only in the symlink code path where st_size is used as a path length. - cpio_read_bin_head: check only st_size < 0 (catches the signed integer overflow root cause). Remove the > MC_MAXPATHLEN bound which was too restrictive for non-symlink entries processed by this general-purpose function. - cpio_read_crc_head: same as cpio_read_bin_head. Signed-off-by: Michał Majchrowicz <sectroyer@gmail.com> Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
|
Here is a list of changes in push request and replies to comments: Fixed in the updated commit — the cast direction is now correct: st->st_size >= (off_t) G_MAXSIZE. Thread 2 (outdated — st.st_size > MC_MAXPATHLEN in cpio_read_bin_head): Fixed. The MC_MAXPATHLEN upper bound has been removed from cpio_read_bin_head and cpio_read_crc_head. Those functions handle all entry types, so only the < 0 guard (which catches the Thread 3 (Likewise / cpio_read_crc_head — already replied): Same fix applied — > MC_MAXPATHLEN removed from cpio_read_crc_head. Thread 4 (new — G_MAXSIZE redundant given MC_MAXPATHLEN): You are correct — MC_MAXPATHLEN (4096) is far smaller than G_MAXSIZE, making the >= (off_t) G_MAXSIZE check completely redundant. Removed in the latest commit. The guard is now @zyv I think this resolves all issues ? |
3 participants
Add a bounds check on st_size immediately after it is computed, before it is used in any arithmetic or allocation. A reasonable upper bound for a symlink target is MC_MAXPATHLEN (4096).