Skip to content
Snippets Groups Projects
  1. Jul 06, 2022
  2. Jul 04, 2022
  3. Jun 29, 2022
  4. Jun 28, 2022
  5. Jun 07, 2022
    • Darren Kenny's avatar
      fs/btrfs: Fix more fuzz issues related to chunks · 2f4430cc
      Darren Kenny authored
      
      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: default avatarDarren Kenny <darren.kenny@oracle.com>
      Reviewed-by: default avatarDaniel Kiper <daniel.kiper@oracle.com>
      2f4430cc
    • Darren Kenny's avatar
      fs/btrfs: Fix more ASAN and SEGV issues found with fuzzing · 13dce204
      Darren Kenny authored
      
      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: default avatarDarren Kenny <darren.kenny@oracle.com>
      Reviewed-by: default avatarDaniel Kiper <daniel.kiper@oracle.com>
      13dce204
    • Darren Kenny's avatar
      fs/btrfs: Fix several fuzz issues with invalid dir item sizing · 11e1cffb
      Darren Kenny authored
      
      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: default avatarDarren Kenny <darren.kenny@oracle.com>
      Reviewed-by: default avatarDaniel Kiper <daniel.kiper@oracle.com>
      11e1cffb
    • Sudhakar Kuppusamy's avatar
      fs/f2fs: Do not copy file names that are too long · e40b8333
      Sudhakar Kuppusamy authored
      
      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: default avatarSudhakar Kuppusamy <sudhakar@linux.ibm.com>
      Signed-off-by: default avatarDaniel Axtens <dja@axtens.net>
      Reviewed-by: default avatarDaniel Kiper <daniel.kiper@oracle.com>
      e40b8333
    • Sudhakar Kuppusamy's avatar
      fs/f2fs: Do not read past the end of nat bitmap · deae293f
      Sudhakar Kuppusamy authored
      
      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: default avatarSudhakar Kuppusamy <sudhakar@linux.ibm.com>
      Signed-off-by: default avatarDaniel Axtens <dja@axtens.net>
      Reviewed-by: default avatarDaniel Kiper <daniel.kiper@oracle.com>
      deae293f
    • Sudhakar Kuppusamy's avatar
      fs/f2fs: Do not read past the end of nat journal entries · 4bd9877f
      Sudhakar Kuppusamy authored
      
      A corrupt f2fs file system could specify a nat journal entry count
      that is beyond the maximum NAT_JOURNAL_ENTRIES.
      
      Check if the specified nat journal entry count before accessing the
      array, and throw an error if it is too large.
      
      Signed-off-by: default avatarSudhakar Kuppusamy <sudhakar@linux.ibm.com>
      Signed-off-by: default avatarDaniel Axtens <dja@axtens.net>
      Reviewed-by: default avatarDaniel Kiper <daniel.kiper@oracle.com>
      4bd9877f
    • Daniel Axtens's avatar
      net/http: Error out on headers with LF without CR · b26b4c08
      Daniel Axtens authored
      
      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: default avatarDaniel Axtens <dja@axtens.net>
      Reviewed-by: default avatarDaniel Kiper <daniel.kiper@oracle.com>
      b26b4c08
    • Daniel Axtens's avatar
      net/http: Fix OOB write for split http headers · ec6bfd32
      Daniel Axtens authored
      
      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: default avatarDaniel Axtens <dja@axtens.net>
      Reviewed-by: default avatarDaniel Kiper <daniel.kiper@oracle.com>
      ec6bfd32
    • Daniel Axtens's avatar
      net/http: Do not tear down socket if it's already been torn down · dad94fff
      Daniel Axtens authored
      
      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: default avatarDaniel Axtens <dja@axtens.net>
      Reviewed-by: default avatarDaniel Kiper <daniel.kiper@oracle.com>
      dad94fff
    • Daniel Axtens's avatar
      net/tftp: Avoid a trivial UAF · 8f287c3e
      Daniel Axtens authored
      
      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: default avatarDaniel Axtens <dja@axtens.net>
      Reviewed-by: default avatarDaniel Kiper <daniel.kiper@oracle.com>
      8f287c3e
    • Daniel Axtens's avatar
      net/tftp: Prevent a UAF and double-free from a failed seek · ee965203
      Daniel Axtens authored
      
      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: default avatarDaniel Axtens <dja@axtens.net>
      Reviewed-by: default avatarDaniel Kiper <daniel.kiper@oracle.com>
      ee965203
    • Daniel Axtens's avatar
      net/dns: Don't read past the end of the string we're checking against · 96abf4fb
      Daniel Axtens authored
      
      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: default avatarDaniel Axtens <dja@axtens.net>
      Reviewed-by: default avatarDaniel Kiper <daniel.kiper@oracle.com>
      96abf4fb
    • Daniel Axtens's avatar
      net/dns: Fix double-free addresses on corrupt DNS response · c1b7eef9
      Daniel Axtens authored
      
      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: default avatarDaniel Axtens <dja@axtens.net>
      Reviewed-by: default avatarDaniel Kiper <daniel.kiper@oracle.com>
      c1b7eef9
    • Daniel Axtens's avatar
      net/netbuff: Block overly large netbuff allocs · f407e34f
      Daniel Axtens authored
      
      A netbuff shouldn't be too huge. It's bounded by MTU and TCP segment
      reassembly. If we are asked to create one that is unreasonably big, refuse.
      
      This is a hardening measure: if we hit this code, there's a bug somewhere
      else that we should catch and fix.
      
      This commit:
        - stops the bug propagating any further.
        - provides a spot to instrument in e.g. fuzzing to try to catch these bugs.
      
      I have put instrumentation (e.g. __builtin_trap() to force a crash) here and
      have not been able to find any more crashes.
      
      Signed-off-by: default avatarDaniel Axtens <dja@axtens.net>
      Reviewed-by: default avatarDaniel Kiper <daniel.kiper@oracle.com>
      f407e34f
    • Daniel Axtens's avatar
      net/ip: Do IP fragment maths safely · 3e481753
      Daniel Axtens authored
      
      We can receive packets with invalid IP fragmentation information. This
      can lead to rsm->total_len underflowing and becoming very large.
      
      Then, in grub_netbuff_alloc(), we add to this very large number, which can
      cause it to overflow and wrap back around to a small positive number.
      The allocation then succeeds, but the resulting buffer is too small and
      subsequent operations can write past the end of the buffer.
      
      Catch the underflow here.
      
      Fixes: CVE-2022-28733
      
      Signed-off-by: default avatarDaniel Axtens <dja@axtens.net>
      Reviewed-by: default avatarDaniel Kiper <daniel.kiper@oracle.com>
      3e481753
    • Daniel Axtens's avatar
      normal/charset: Fix array out-of-bounds formatting unicode for display · 830a9628
      Daniel Axtens authored
      
      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: default avatarDaniel Axtens <dja@axtens.net>
      Reviewed-by: default avatarDaniel Kiper <daniel.kiper@oracle.com>
      830a9628
    • Daniel Axtens's avatar
      video/readers/jpeg: Block int underflow -> wild pointer write · 22a3f97d
      Daniel Axtens authored
      
      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:
      
      for (; data->r1 < nr1 && (!data->dri || rst);
           data->r1++, data->bitmap_ptr += (vb * data->image_width - hb * nc1) * 3)
      
      We did not check if vb * width >= hb * nc1.
      
      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: default avatarDaniel Axtens <dja@axtens.net>
      Reviewed-by: default avatarDaniel Kiper <daniel.kiper@oracle.com>
      22a3f97d
    • Daniel Axtens's avatar
      video/readers/jpeg: Refuse to handle multiple start of streams · 166a4d61
      Daniel Axtens authored
      
      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: default avatarDaniel Axtens <dja@axtens.net>
      Reviewed-by: default avatarDaniel Kiper <daniel.kiper@oracle.com>
      166a4d61
    • Daniel Axtens's avatar
      video/readers/jpeg: Do not reallocate a given huff table · 768ef219
      Daniel Axtens authored
      
      Fix a memory leak where an invalid file could cause us to reallocate
      memory for a huffman table we had already allocated memory for.
      
      Signed-off-by: default avatarDaniel Axtens <dja@axtens.net>
      Reviewed-by: default avatarDaniel Kiper <daniel.kiper@oracle.com>
      768ef219
Loading