chore: stdlib byte_array and bool external audit fixes

[iakovenkos]

Critical Changes

Issues 1 & 2: Prevent Unconstrained Value Propagation

Root Cause: Constructors allowed creation of byte_arrays with unconstrained field_t values, enabling malicious provers to inject invalid "bytes" (>255) into hash function lookup tables.

Fixes:

• 1 (6585762): Removed byte_array(Builder*, size_t) constructor entirely
• 2 (de5b87e): Made byte_array(Builder*, bytes_t const&) and byte_array(Builder*, bytes_t&&) constructors private

Impact: Establishes security boundary where:

• External code must use public constructors that add 8-bit range constraints
• Internal methods (slice, reverse) safely use private constructors only on already-constrained data
• Unconstrained values cannot propagate to hash functions

Cascading Benefits: Issues 5 and 8 automatically prevented at source.

---

📝 byte_array API Security & Correctness

Issue 3: Allow Valid 32-Byte ↔ field_t Round-Trips

• Commit: f899a23
• Fix: Removed overly restrictive BB_ASSERT(bytes < 32) from operator field_t()
• Result: Enables testing of critical modulus boundary cases

Issue 4: Proper Bounds Checking & Prevent Mutation

• Commits: bf58e85, e897223
• Fix: Changed const operator[] from assert(size > 0) to BB_ASSERT_LT(index, size); removed non-const operator[]
• Result: Proper bounds validation; no direct mutation possible

Issue 5: write_at() Cannot Accept Unconstrained Arrays

• Commit: de5b87e (architectural fix)
• Fix: With private bytes_t constructors, only constrained byte_arrays can exist
• Result: write_at() validated at construction time, not call time

Issue 6: Handle Empty Array Edge Case in slice()

• Commit: 5f916a6
• Fix: Changed BB_ASSERT_LT(offset, size) to BB_ASSERT_LTE(offset, size)
• Result: Allows valid slice(0) on empty arrays

Issue 7: Document Division Safety Invariant

• Commit: 8d9f4e1
• Fix: Added SAFETY comment explaining reconstructed_hi / 2^128 divisibility invariant
• Result: Critical assumption now documented for maintainers and auditors

Issue 8: slice() Cannot Propagate Unconstrained Fields

• Commit: de5b87e (architectural fix)
• Fix: slice() uses private constructor only on already-constrained internal data
• Result: No external bypass possible; constraints always preserved

Issue 9: No Checks/Documentation About field_t Conversion Not Adding Constraints

• Commits: de5b87e + 6585762 (architectural fixes)
• Fix: The operator field_t() conversion doesn't need to check constraints because the type system now guarantees all byte_arrays contain constrained values:
• Result: The conversion's "trust model" is valid - it's not trusting user input, it's relying on a type-level invariant enforced at construction

Issue 10: Remove Undefined Behavior in get_string()

• Commit: 2cf61e6
• Fix: Removed get_string() method entirely
• Result: No signed char UB with values 128-255

---

🔧 bool_t Security & Correctness

Issue 11: Validate Builder Context in Operators

• Commit: a9f6608
• Fix: Use validate_context<Builder>() instead of manual context ? context : other.context in &, |, ^, == operators
• Result: Ensures both operands belong to same builder

Issue 12: Use Canonical Zero Witness

• Commit: 1fb4b6e
• Fix: Use context->zero_idx() instead of same witness with q_r=0 in normalize()
• Result: Cleaner gate construction following best practices

---

🚀 Performance & Correctness

Issue 13: Honor Move Semantics in Rvalue Constructor

• Commit: eaa34b7
• Fix: Changed to values(std::move(input)) in rvalue constructor
• Result: Avoids unnecessary copies for large arrays

Issue 14: Prevent Underflow in reverse() on Empty Array

• Commit: 6cf505d
• Fix: Added early return: if (values.empty()) { return *this; }
• Result: Prevents underflow from size() - 1 computation

Issue 15: Fix Selector Order in Boolean Equality Gate

• Commit: cf791df
• Fix: Swapped q_l and q_r to match canonical (q_m, q_l, q_r, q_o, q_c) order
• Result: Conforms to documented layout; prevents future bugs if coefficients diverge

bool_t: Always normalize conditional_assign Output

• Commit: d0032b2
• Fix: Fixed concept leakage in bool_t::conditional_assign that could return un-normalized bool_t and cause in APIs at higher level
• Note: Minimal gate overhead - only 0-1 gates added when values are actually inverted.

Summary

The key insight: Making internal bytes_t constructors private creates a security boundary that prevents unconstrained field_t values from ever entering byte_arrays. Combined with removing unsafe constructors and the non-const operator[], this ensures that only properly range-constrained bytes can reach hash function lookup tables throughout the circuit stack.

[suyash67]

nit: no need for the comment explaining why we removed the non-const operator[].

[suyash67]

might be worth adding assertion to check if indeed input values are bytes?

[iakovenkos]

I think there's no way to propagate something which is not a uint8_t here. it is a private member that's called by constant_padding() externally. I guess one can try to call it with smth that can be truncated to uint8_t, but then it either has no effect or breaks some padding assumptions in hash functions

[suyash67]

nice

[iakovenkos]

terribly complicated one tbh :D

[suyash67]

Q: Since output limb is the result of the lookup table, is it range-constrained implicitly? In that case, do we need a way to "opt-out" of range constraining while performing write?

[iakovenkos]

You're right. There's no need to constrain it. Otoh, hash and all its helpers are un-used. So I'd just keep it like that and nuke in a follow-up. We only need keccak1600 from this module.

[iakovenkos]

@federicobarbacovi can you check that this is fine? I think the values need to be conditionally assigned first and then fed to byte_array constructors

[iakovenkos]

@federicobarbacovi just flagging that now it's the canonical way to create an empty array