Thomas Lamprecht [Fri, 19 Apr 2024 07:25:11 +0000 (09:25 +0200)]
mkrescue: add opt-in quirk for secure-boot
When building the ISO we use grub-mkrescue to setup the outer GRUB on
the ISO that's used to boot the actual installer, but mkrescue sadly
has no native support to copy over the signed shim, so add that but
only enable it through an environment variable so that we do not have
to vet this overly closely as it won't affect any normal grub use
anyway, even less so as mkrescue is used rather rarely on running
systems.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
bump grub upstream SBAT for the pulled in CVE fixes
add grub.debian entry since we mostly re-use Debian's implementation, any
Debian-specific issue almost certainly would affect ours too
keep grub.proxmox at 1 - no signatures have been created yet using the
production keys, so there is no binary in existence that would need to be
revoked.
Steve McIntyre [Mon, 17 Apr 2023 20:59:41 +0000 (21:59 +0100)]
Optionally re-enable os-prober
Add debconf logic for GRUB_DISABLE_OS_PROBER to make it easier to
control things here. Particularly useful for the installer. Closes: #1031594, #1012865.
- disk/cryptodisk: When cheatmounting, use the sector info of the cheat
device
- osdep/devmapper/getroot: Have devmapper recognize LUKS2
- osdep/devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM
parameters
Colin Watson [Wed, 8 Feb 2023 23:15:47 +0000 (23:15 +0000)]
Remove myself from Uploaders
I've mostly retired from GRUB maintenance since early 2022, so I think
it would be better if I weren't listed as an uploader in bookworm.
Thanks to Steve and Julian for picking up the torch!
Steve McIntyre [Wed, 16 Nov 2022 00:11:16 +0000 (00:11 +0000)]
Add fonts to the EFI images
The previous security updates disallowed loading unsigned fonts when
in SB mode. To make things work again:
* Embed the "unicode" font into the embedded memdisk image so it can
be loaded.
* Add the memdisk to our normal grubx64.efi loader too
* Add a patch from Chris Coulson to make the font loader look for
fonts in the memdisk whenever they're loaded.
Zhang Boyang [Fri, 28 Oct 2022 13:31:39 +0000 (21:31 +0800)]
normal/charset: Fix an integer overflow in grub_unicode_aglomerate_comb()
The out->ncomb is a bit-field of 8 bits. So, the max possible value is 255.
However, code in grub_unicode_aglomerate_comb() doesn't check for an
overflow when incrementing out->ncomb. If out->ncomb is already 255,
after incrementing it will get 0 instead of 256, and cause illegal
memory access in subsequent processing.
This patch introduces GRUB_UNICODE_NCOMB_MAX to represent the max
acceptable value of ncomb. The code now checks for this limit and
ignores additional combining characters when limit is reached.
Reported-by: Daniel Axtens <dja@axtens.net> Signed-off-by: Zhang Boyang <zhangboyang.id@gmail.com> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Zhang Boyang [Fri, 28 Oct 2022 09:29:16 +0000 (17:29 +0800)]
font: Assign null_font to glyphs in ascii_font_glyph[]
The calculations in blit_comb() need information from glyph's font, e.g.
grub_font_get_xheight(main_glyph->font). However, main_glyph->font is
NULL if main_glyph comes from ascii_font_glyph[]. Therefore
grub_font_get_*() crashes because of NULL pointer.
There is already a solution, the null_font. So, assign it to those glyphs
in ascii_font_glyph[].
Reported-by: Daniel Axtens <dja@axtens.net> Signed-off-by: Zhang Boyang <zhangboyang.id@gmail.com> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Zhang Boyang [Sun, 23 Oct 2022 23:15:41 +0000 (07:15 +0800)]
font: Harden grub_font_blit_glyph() and grub_font_blit_glyph_mirror()
As a mitigation and hardening measure add sanity checks to
grub_font_blit_glyph() and grub_font_blit_glyph_mirror(). This patch
makes these two functions do nothing if target blitting area isn't fully
contained in target bitmap. Therefore, if complex calculations in caller
overflows and malicious coordinates are given, we are still safe because
any coordinates which result in out-of-bound-write are rejected. However,
this patch only checks for invalid coordinates, and doesn't provide any
protection against invalid source glyph or destination glyph, e.g.
mismatch between glyph size and buffer size.
This hardening measure is designed to mitigate possible overflows in
blit_comb(). If overflow occurs, it may return invalid bounding box
during dry run and call grub_font_blit_glyph() with malicious
coordinates during actual blitting. However, we are still safe because
the scratch glyph itself is valid, although its size makes no sense, and
any invalid coordinates are rejected.
It would be better to call grub_fatal() if illegal parameter is detected.
However, doing this may end up in a dangerous recursion because grub_fatal()
would print messages to the screen and we are in the progress of drawing
characters on the screen.
Reported-by: Daniel Axtens <dja@axtens.net> Signed-off-by: Zhang Boyang <zhangboyang.id@gmail.com> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Zhang Boyang [Mon, 24 Oct 2022 00:05:35 +0000 (08:05 +0800)]
font: Fix an integer underflow in blit_comb()
The expression (ctx.bounds.height - combining_glyphs[i]->height) / 2 may
evaluate to a very big invalid value even if both ctx.bounds.height and
combining_glyphs[i]->height are small integers. For example, if
ctx.bounds.height is 10 and combining_glyphs[i]->height is 12, this
expression evaluates to 2147483647 (expected -1). This is because
coordinates are allowed to be negative but ctx.bounds.height is an
unsigned int. So, the subtraction operates on unsigned ints and
underflows to a very big value. The division makes things even worse.
The quotient is still an invalid value even if converted back to int.
This patch fixes the problem by casting ctx.bounds.height to int. As
a result the subtraction will operate on int and grub_uint16_t which
will be promoted to an int. So, the underflow will no longer happen. Other
uses of ctx.bounds.height (and ctx.bounds.width) are also casted to int,
to ensure coordinates are always calculated on signed integers.
Fixes: CVE-2022-3775 Reported-by: Daniel Axtens <dja@axtens.net> Signed-off-by: Zhang Boyang <zhangboyang.id@gmail.com> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Expressions like u64 = u32 * u32 are unsafe because their products are
truncated to u32 even if left hand side is u64. This patch fixes all
problems like that one in fbutil.
To get right result not only left hand side have to be u64 but it's also
necessary to cast at least one of the operands of all leaf operators of
right hand side to u64, e.g. u64 = u32 * u32 + u32 * u32 should be
u64 = (u64)u32 * u32 + (u64)u32 * u32.
For 1-bit bitmaps grub_uint64_t have to be used. It's safe because any
combination of values in (grub_uint64_t)u32 * u32 + u32 expression will
not overflow grub_uint64_t.
Other expressions like ptr + u32 * u32 + u32 * u32 are also vulnerable.
They should be ptr + (grub_addr_t)u32 * u32 + (grub_addr_t)u32 * u32.
This patch also adds a comment to grub_video_fb_get_video_ptr() which
says it's arguments must be valid and no sanity check is performed
(like its siblings in grub-core/video/fb/fbutil.c).
Signed-off-by: Zhang Boyang <zhangboyang.id@gmail.com> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Zhang Boyang [Sun, 14 Aug 2022 07:51:54 +0000 (15:51 +0800)]
kern/efi/sb: Enforce verification of font files
As a mitigation and hardening measure enforce verification of font
files. Then only trusted font files can be load. This will reduce the
attack surface at cost of losing the ability of end-users to customize
fonts if e.g. UEFI Secure Boot is enabled. Vendors can always customize
fonts because they have ability to pack fonts into their GRUB bundles.
This goal is achieved by:
* Removing GRUB_FILE_TYPE_FONT from shim lock verifier's
skip-verification list.
* Adding GRUB_FILE_TYPE_FONT to lockdown verifier's defer-auth list,
so font files must be verified by a verifier before they can be loaded.
Suggested-by: Daniel Kiper <daniel.kiper@oracle.com> Signed-off-by: Zhang Boyang <zhangboyang.id@gmail.com> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Zhang Boyang [Sun, 14 Aug 2022 18:04:58 +0000 (02:04 +0800)]
font: Fix integer overflow in BMP index
The BMP index (font->bmp_idx) is designed as a reverse lookup table of
char entries (font->char_index), in order to speed up lookups for BMP
chars (i.e. code < 0x10000). The values in BMP index are the subscripts
of the corresponding char entries, stored in grub_uint16_t, while 0xffff
means not found.
This patch fixes the problem of large subscript truncated to grub_uint16_t,
leading BMP index to return wrong char entry or report false miss. The
code now checks for bounds and uses BMP index as a hint, and fallbacks
to binary-search if necessary.
On the occasion add a comment about BMP index is initialized to 0xffff.
Signed-off-by: Zhang Boyang <zhangboyang.id@gmail.com> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Zhang Boyang [Thu, 4 Aug 2022 18:27:05 +0000 (02:27 +0800)]
font: Fix integer overflow in ensure_comb_space()
In fact it can't overflow at all because glyph_id->ncomb is only 8-bit
wide. But let's keep safe if somebody changes the width of glyph_id->ncomb
in the future. This patch also fixes the inconsistency between
render_max_comb_glyphs and render_combining_glyphs when grub_malloc()
returns NULL.
Signed-off-by: Zhang Boyang <zhangboyang.id@gmail.com> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Zhang Boyang [Thu, 4 Aug 2022 17:58:27 +0000 (01:58 +0800)]
font: Fix several integer overflows in grub_font_construct_glyph()
This patch fixes several integer overflows in grub_font_construct_glyph().
Glyphs of invalid size, zero or leading to an overflow, are rejected.
The inconsistency between "glyph" and "max_glyph_size" when grub_malloc()
returns NULL is fixed too.
Zhang Boyang [Thu, 4 Aug 2022 16:51:20 +0000 (00:51 +0800)]
font: Fix size overflow in grub_font_get_glyph_internal()
The length of memory allocation and file read may overflow. This patch
fixes the problem by using safemath macros.
There is a lot of code repetition like "(x * y + 7) / 8". It is unsafe
if overflow happens. This patch introduces grub_video_bitmap_calc_1bpp_bufsz().
It is safe replacement for such code. It has safemath-like prototype.
This patch also introduces grub_cast(value, pointer), it casts value to
typeof(*pointer) then store the value to *pointer. It returns true when
overflow occurs or false if there is no overflow. The semantics of arguments
and return value are designed to be consistent with other safemath macros.
Signed-off-by: Zhang Boyang <zhangboyang.id@gmail.com> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Alec Brown [Thu, 27 Oct 2022 00:16:44 +0000 (20:16 -0400)]
video/readers: Add artificial limit to image dimensions
In grub-core/video/readers/jpeg.c, the height and width of a JPEG image don't
have an upper limit for how big the JPEG image can be. In Coverity, this is
getting flagged as an untrusted loop bound. This issue can also seen in PNG and
TGA format images as well but Coverity isn't flagging it. To prevent this, the
constant IMAGE_HW_MAX_PX is being added to include/grub/bitmap.h, which has
a value of 16384, to act as an artificial limit and restrict the height and
width of images. This value was picked as it is double the current max
resolution size, which is 8K.
Fixes: CID 292450 Signed-off-by: Alec Brown <alec.r.brown@oracle.com> Reviewed-by: Darren Kenny <darren.kenny@oracle.com> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Steve McIntyre [Sun, 18 Sep 2022 12:50:26 +0000 (12:50 +0000)]
Explicitly unset SOURCE_DATE_EPOCH before running fs tests.
In some filesystem utils like mksquashfs, it will silently change
behaviour and cause timestamps to unexpectedly change. Reproducible
builds are good and useful for shipped artifacts, but this causes
build-time tests to fail.
Darren Kenny [Thu, 7 Apr 2022 15:18:12 +0000 (15:18 +0000)]
fs/btrfs: Fix more fuzz issues related to chunks
The corpus was generating issues in grub_btrfs_read_logical() when
attempting to iterate over stripe entries in the superblock's
bootmapping.
In most cases the reason for the failure was that the number of stripes
in chunk->nstripes exceeded the possible space statically allocated in
superblock bootmapping space. Each stripe entry in the bootmapping block
consists of a grub_btrfs_key followed by a grub_btrfs_chunk_stripe.
Another issue that came up was that while calculating the chunk size,
in an earlier piece of code in that function, depending on the data
provided in the btrfs file system, it would end up calculating a size
that was too small to contain even 1 grub_btrfs_chunk_item, which is
obviously invalid too.
Signed-off-by: Darren Kenny <darren.kenny@oracle.com> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Darren Kenny [Tue, 29 Mar 2022 15:52:46 +0000 (15:52 +0000)]
fs/btrfs: Fix more ASAN and SEGV issues found with fuzzing
The fuzzer is generating btrfs file systems that have chunks with
invalid combinations of stripes and substripes for the given RAID
configurations.
After examining the Linux kernel fs/btrfs/tree-checker.c code, it
appears that sub-stripes should only be applied to RAID10, and in that
case there should only ever be 2 of them.
Similarly, RAID single should only have 1 stripe, and RAID1/1C3/1C4
should have 2. 3 or 4 stripes respectively, which is what redundancy
corresponds.
Some of the chunks ended up with a size of 0, which grub_malloc() still
returned memory for and in turn generated ASAN errors later when
accessed.
While it would be possible to specifically limit the number of stripes,
a more correct test was on the combination of the chunk item, and the
number of stripes by the size of the chunk stripe structure in
comparison to the size of the chunk itself.
Signed-off-by: Darren Kenny <darren.kenny@oracle.com> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Darren Kenny [Tue, 29 Mar 2022 10:49:56 +0000 (10:49 +0000)]
fs/btrfs: Fix several fuzz issues with invalid dir item sizing
According to the btrfs code in Linux, the structure of a directory item
leaf should be of the form:
|struct btrfs_dir_item|name|data|
in GRUB the name len and data len are in the grub_btrfs_dir_item
structure's n and m fields respectively.
The combined size of the structure, name and data should be less than
the allocated memory, a difference to the Linux kernel's struct
btrfs_dir_item is that the grub_btrfs_dir_item has an extra field for
where the name is stored, so we adjust for that too.
Signed-off-by: Darren Kenny <darren.kenny@oracle.com> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
A corrupt f2fs file system might specify a name length which is greater
than the maximum name length supported by the GRUB f2fs driver.
We will allocate enough memory to store the overly long name, but there
are only F2FS_NAME_LEN bytes in the source, so we would read past the end
of the source.
While checking directory entries, do not copy a file name with an invalid
length.
Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com> Signed-off-by: Daniel Axtens <dja@axtens.net> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
A corrupt f2fs filesystem could have a block offset or a bitmap
offset that would cause us to read beyond the bounds of the nat
bitmap.
Introduce the nat_bitmap_size member in grub_f2fs_data which holds
the size of nat bitmap.
Set the size when loading the nat bitmap in nat_bitmap_ptr(), and
catch when an invalid offset would create a pointer past the end of
the allocated space.
Check against the bitmap size in grub_f2fs_test_bit() test bit to avoid
reading past the end of the nat bitmap.
Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com> Signed-off-by: Daniel Axtens <dja@axtens.net> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel Axtens [Tue, 8 Mar 2022 08:04:40 +0000 (19:04 +1100)]
net/http: Error out on headers with LF without CR
In a similar vein to the previous patch, parse_line() would write
a NUL byte past the end of the buffer if there was an HTTP header
with a LF rather than a CRLF.
RFC-2616 says:
Many HTTP/1.1 header field values consist of words separated by LWS
or special characters. These special characters MUST be in a quoted
string to be used within a parameter value (as defined in section 3.6).
We don't support quoted sections or continuation lines, etc.
If we see an LF that's not part of a CRLF, bail out.
Fixes: CVE-2022-28734 Signed-off-by: Daniel Axtens <dja@axtens.net> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel Axtens [Tue, 8 Mar 2022 07:17:03 +0000 (18:17 +1100)]
net/http: Fix OOB write for split http headers
GRUB has special code for handling an http header that is split
across two packets.
The code tracks the end of line by looking for a "\n" byte. The
code for split headers has always advanced the pointer just past the
end of the line, whereas the code that handles unsplit headers does
not advance the pointer. This extra advance causes the length to be
one greater, which breaks an assumption in parse_line(), leading to
it writing a NUL byte one byte past the end of the buffer where we
reconstruct the line from the two packets.
It's conceivable that an attacker controlled set of packets could
cause this to zero out the first byte of the "next" pointer of the
grub_mm_region structure following the current_line buffer.
Do not advance the pointer in the split header case.
Fixes: CVE-2022-28734 Signed-off-by: Daniel Axtens <dja@axtens.net> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel Axtens [Tue, 1 Mar 2022 12:14:15 +0000 (23:14 +1100)]
net/http: Do not tear down socket if it's already been torn down
It's possible for data->sock to get torn down in tcp error handling.
If we unconditionally tear it down again we will end up doing writes
to an offset of the NULL pointer when we go to tear it down again.
Detect if it has been torn down and don't do it again.
Signed-off-by: Daniel Axtens <dja@axtens.net> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel Axtens [Tue, 18 Jan 2022 03:29:20 +0000 (14:29 +1100)]
net/tftp: Avoid a trivial UAF
Under tftp errors, we print a tftp error message from the tftp header.
However, the tftph pointer is a pointer inside nb, the netbuff. Previously,
we were freeing the nb and then dereferencing it. Don't do that, use it
and then free it later.
This isn't really _bad_ per se, especially as we're single-threaded, but
it trips up fuzzers.
Signed-off-by: Daniel Axtens <dja@axtens.net> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel Axtens [Sun, 19 Sep 2021 15:12:24 +0000 (01:12 +1000)]
net/tftp: Prevent a UAF and double-free from a failed seek
A malicious tftp server can cause UAFs and a double free.
An attempt to read from a network file is handled by grub_net_fs_read(). If
the read is at an offset other than the current offset, grub_net_seek_real()
is invoked.
In grub_net_seek_real(), if a backwards seek cannot be satisfied from the
currently received packets, and the underlying transport does not provide
a seek method, then grub_net_seek_real() will close and reopen the network
protocol layer.
For tftp, the ->close() call goes to tftp_close() and frees the tftp_data_t
file->data. The file->data pointer is not nulled out after the free.
If the ->open() call fails, the file->data will not be reallocated and will
continue point to a freed memory block. This could happen from a server
refusing to send the requisite ack to the new tftp request, for example.
The seek and the read will then fail, but the grub_file continues to exist:
the failed seek does not necessarily cause the entire file to be thrown
away (e.g. where the file is checked to see if it is gzipped/lzio/xz/etc.,
a read failure is interpreted as a decompressor passing on the file, not as
an invalidation of the entire grub_file_t structure).
This means subsequent attempts to read or seek the file will use the old
file->data after free. Eventually, the file will be close()d again and
file->data will be freed again.
Mark a net_fs file that doesn't reopen as broken. Do not permit read() or
close() on a broken file (seek is not exposed directly to the file API -
it is only called as part of read, so this blocks seeks as well).
As an additional defence, null out the ->data pointer if tftp_open() fails.
That would have lead to a simple null pointer dereference rather than
a mess of UAFs.
This may affect other protocols, I haven't checked.
Signed-off-by: Daniel Axtens <dja@axtens.net> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel Axtens [Mon, 20 Dec 2021 10:55:43 +0000 (21:55 +1100)]
net/dns: Don't read past the end of the string we're checking against
I don't really understand what's going on here but fuzzing found
a bug where we read past the end of check_with. That's a C string,
so use grub_strlen() to make sure we don't overread it.
Signed-off-by: Daniel Axtens <dja@axtens.net> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel Axtens [Wed, 15 Sep 2021 15:29:54 +0000 (01:29 +1000)]
net/dns: Fix double-free addresses on corrupt DNS response
grub_net_dns_lookup() takes as inputs a pointer to an array of addresses
("addresses") for the given name, and pointer to a number of addresses
("naddresses"). grub_net_dns_lookup() is responsible for allocating
"addresses", and the caller is responsible for freeing it if
"naddresses" > 0.
The DNS recv_hook will sometimes set and free the addresses array,
for example if the packet is too short:
if (ptr + 10 >= nb->tail)
{
if (!*data->naddresses)
grub_free (*data->addresses);
grub_netbuff_free (nb);
return GRUB_ERR_NONE;
}
Later on the nslookup command code unconditionally frees the "addresses"
array. Normally this is fine: the array is either populated with valid
data or is NULL. But in these sorts of error cases it is neither NULL
nor valid and we get a double-free.
Only free "addresses" if "naddresses" > 0.
It looks like the other use of grub_net_dns_lookup() is not affected.
Signed-off-by: Daniel Axtens <dja@axtens.net> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel Axtens [Tue, 13 Jul 2021 03:24:38 +0000 (13:24 +1000)]
normal/charset: Fix array out-of-bounds formatting unicode for display
In some cases attempting to display arbitrary binary strings leads
to ASAN splats reading the widthspec array out of bounds.
Check the index. If it would be out of bounds, return a width of 1.
I don't know if that's strictly correct, but we're not really expecting
great display of arbitrary binary data, and it's certainly not worse than
an OOB read.
Signed-off-by: Daniel Axtens <dja@axtens.net> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel Axtens [Wed, 7 Jul 2021 05:38:19 +0000 (15:38 +1000)]
video/readers/jpeg: Block int underflow -> wild pointer write
Certain 1 px wide images caused a wild pointer write in
grub_jpeg_ycrcb_to_rgb(). This was caused because in grub_jpeg_decode_data(),
we have the following loop:
On a 64-bit platform, if that turns out to be negative, it will underflow,
be interpreted as unsigned 64-bit, then be added to the 64-bit pointer, so
we see data->bitmap_ptr jump, e.g.:
0x6180_0000_0480 to
0x6181_0000_0498
^
~--- carry has occurred and this pointer is now far away from
any object.
On a 32-bit platform, it will decrement the pointer, creating a pointer
that won't crash but will overwrite random data.
Catch the underflow and error out.
Fixes: CVE-2021-3697 Signed-off-by: Daniel Axtens <dja@axtens.net> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel Axtens [Mon, 28 Jun 2021 04:25:17 +0000 (14:25 +1000)]
video/readers/jpeg: Refuse to handle multiple start of streams
An invalid file could contain multiple start of stream blocks, which
would cause us to reallocate and leak our bitmap. Refuse to handle
multiple start of streams.
Additionally, fix a grub_error() call formatting.
Signed-off-by: Daniel Axtens <dja@axtens.net> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
In fuzzing we observed crashes where a code would attempt to be inserted
into a huffman table before the start, leading to a set of heap OOB reads
and writes as table entries with negative indices were shifted around and
the new code written in.
Catch the case where we would underflow the array and bail.
Fixes: CVE-2021-3696 Signed-off-by: Daniel Axtens <dja@axtens.net> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel Axtens [Tue, 6 Jul 2021 08:51:35 +0000 (18:51 +1000)]
video/readers/png: Drop greyscale support to fix heap out-of-bounds write
A 16-bit greyscale PNG without alpha is processed in the following loop:
for (i = 0; i < (data->image_width * data->image_height);
i++, d1 += 4, d2 += 2)
{
d1[R3] = d2[1];
d1[G3] = d2[1];
d1[B3] = d2[1];
}
The increment of d1 is wrong. d1 is incremented by 4 bytes per iteration,
but there are only 3 bytes allocated for storage. This means that image
data will overwrite somewhat-attacker-controlled parts of memory - 3 bytes
out of every 4 following the end of the image.
This has existed since greyscale support was added in 2013 in commit 3ccf16dff98f (grub-core/video/readers/png.c: Support grayscale).
Saving starfield.png as a 16-bit greyscale image without alpha in the gimp
and attempting to load it causes grub-emu to crash - I don't think this code
has ever worked.
Delete all PNG greyscale support.
Fixes: CVE-2021-3695 Signed-off-by: Daniel Axtens <dja@axtens.net> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
projects / grub2.git / log