Skip to content

fix: handle negative node read size in ubifs walk#123

Open
urknall wants to merge 1 commit intoonekey-sec:mainfrom
urknall:main
Open

fix: handle negative node read size in ubifs walk#123
urknall wants to merge 1 commit intoonekey-sec:mainfrom
urknall:main

Conversation

@urknall
Copy link

@urknall urknall commented Mar 10, 2026

In ubi_reader ≤0.8.13 (master as of 2026-03), _index() computes

read_size = chdr.len - UBIFS_COMMON_HDR_SZ
node_buf  = ubifs.file.read(read_size)

without first checking that read_size >= 0. A zero-filled erase block (factory bad block, or any block whose first bytes are all 0x00) causes the common-node header to be read as all-zeros, so chdr.len == 0 and read_size == -24 (== -UBIFS_COMMON_HDR_SZ). Python's io.BufferedReader raises

ValueError: read length must be non-negative or -1

which propagates through walk.index() and is caught by extract_files as:

extract_files Error: read length must be non-negative or -1

This aborts the entire UBIFS extraction even though only one LEB is affected and the rest of the filesystem is intact.

Fix: add a bounds check for read_size < 0 immediately after the subtraction, matching the style of the adjacent len(buf) guard that already handles the case where the common header itself is short:

  • With -w / --warn-only-block-read-errors the corrupted node is skipped with an 'Error'-level message and _index() returns.
  • Without -w the existing 'Fatal' path is taken, preserving the pre-fix behaviour for strict mode.

In ubi_reader ≤0.8.13 (master as of 2026-03), _index() computes

    read_size = chdr.len - UBIFS_COMMON_HDR_SZ
    node_buf  = ubifs.file.read(read_size)

without first checking that read_size >= 0.  A zero-filled erase block
(factory bad block, or any block whose first bytes are all 0x00) causes
the common-node header to be read as all-zeros, so chdr.len == 0 and
read_size == -24 (== -UBIFS_COMMON_HDR_SZ).  Python's io.BufferedReader
raises

    ValueError: read length must be non-negative or -1

which propagates through walk.index() and is caught by extract_files as:

    extract_files Error: read length must be non-negative or -1

This aborts the entire UBIFS extraction even though only one LEB is
affected and the rest of the filesystem is intact.

Fix: add a bounds check for read_size < 0 immediately after the
subtraction, matching the style of the adjacent len(buf) guard that
already handles the case where the common header itself is short:
- With -w / --warn-only-block-read-errors the corrupted node is skipped
  with an 'Error'-level message and _index() returns.
- Without -w the existing 'Fatal' path is taken, preserving the pre-fix
  behaviour for strict mode.
@qkaiser qkaiser self-assigned this Mar 10, 2026
@qkaiser qkaiser added bug enhancement python Pull requests that update python code labels Mar 10, 2026
@qkaiser qkaiser requested a review from e3krisztian March 10, 2026 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug enhancement python Pull requests that update python code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants