Security Review 1.19.0

[bboilot-ledger]

Description

Please provide a detailed description of what was done in this PR.
(And mentioned if linked to an issue docs)

Changes include

• Bugfix (non-breaking change that solves an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (change that is not backwards-compatible and/or changes current functionality)
• Tests
• Documentation
• Other (for changes that might not fit in any category)

Breaking changes

Please complete this section if any breaking changes have been made, otherwise delete it.

Additional comments

Please post additional comments in this section if you have them, otherwise delete it.

To address the complex boolean expression, we can break down the multi-part condition into intermediate boolean variables, each responsible for a specific check. These variables should use descriptive names reflecting the intent behind each check (e.g. chain_id_ok, selector_ok, etc.). The main condition then becomes a composition of these booleans. This improves readability, and makes the function easier to understand and safer to alter in the future.

• The complex if statement at lines 219–226 should be replaced with local boolean variables, each validated separately just before the loop.
• The calls to value_get (that assign to ret) in the compound condition should be replaced by up-front calls, with their return values stored.
• The main conditional (if) is then a simple conjunction (&&) of the per-field variables, making the logic clear and straightforward.

All modifications are restricted to the format_param_calldata function, i.e. lines 206–265 of the file.

---

To fix this issue, you should ensure that the bounds check correctly prevents reading beyond the end of the buffer (payload). The code intends to check whether there is enough data remaining before reading the next field. The main problem seems that it's checking offset + sizeof(chain_id_flag) instead of checking if offset has already reached or is about to exceed length. Since chain_id_flag is likely a single byte, and the code reads it at payload[offset++], the correct check should be if (offset >= length) { ... } before reading. Review and update the bounds check to compare offset with length, preventing any out-of-bounds read.

Where to change:

• In file src_features/signMessageEIP712/filtering.c, at or immediately before line 740, replace the faulty if ((offset + sizeof(chain_id_flag)) > length) with if (offset >= length) (assuming a single-byte read).
• Do a similar review for adjacent flag checks: the pattern should be to check for at least 1 more byte before every single-byte read (flag = payload[offset++]).

No new methods, imports, or definitions are needed; this is a code logic correction.

---

To fix the problem, we should review the logic of checking available buffer space before extracting the next payload field. Instead of adding sizeof(selector_flag), which represents the size of the flag variable in memory (typically always 1 for uint8_t), we should be determining if there is at least one more byte available in the buffer at the current offset. The correct check should be offset >= length or offset < length. Thus, replace the conditional on line 745 with a proper boundary check, typically if (offset >= length) return false;. Make the same transformation for similar checks for other flags in the surrounding lines if they use an inappropriate + sizeof(...) pattern.

To fix the problem, the redundant comparison should be removed or replaced with a correct check that genuinely prevents overruns of the buffer. Since all the other similar checks in this block use sizeof with a local flag variable, and the operation is always payload[offset++] (indicating a read of a single byte), it's likely that amount_flag is a uint8_t, so sizeof(amount_flag) is 1. If all the remaining fields are single bytes, simply checking offset >= length before any dereference is sufficient; alternatively, these can be omitted if already guaranteed. The fix is to replace (offset + sizeof(amount_flag)) > length with offset >= length (if needed), or to remove the check if it has been made redundant by previous logic.

You should edit only the relevant lines (around line 750) in src_features/signMessageEIP712/filtering.c.

The correct fix is to remove the redundant and unreachable bounds check:

if ((offset + sizeof(sig_len)) > length) {
    return false;
}

from the code. This code check is always false and serves no functional purpose; having such dead code may conceal real bugs and reduce code clarity.

File/region to change:

• File: src_features/signMessageEIP712/filtering.c
• Lines: The block starting at if ((offset + sizeof(sig_len)) > length) { return false; }

No new imports, method, or variable definitions are needed—simply delete these lines.

---

[bboilot-ledger]

👋 Looks overall good to me. A few recommendations.

I can’t put it in the review because it’s not recent code:

• In path_exists_in_backup -> field_ptr = get_nth_field_from(path_backup, NULL, path_backup->depth_count); -> we need to check that field_ptr is not NULL.
• In hash_filtering_path -> path = ui_712_get_discarded_path(); -> we need to check that path is not NULL. (Ideally, hash_filtering_path should return an error to be clean.)
• In check_typename -> typename = get_struct_field_typename(path_get_field()); -> we need to check that typename is not NULL.

[bboilot-ledger]

FYI @apaillier-ledger