Fix secondary index scope isolation and harden chain_plugin#284
Conversation
Secondary index iteration in get_table_rows leaked entries across scopes because sec_key lacked a scope discriminator. The CDT now encodes [scope:8B BE] as a prefix on secondary keys and stores pri_key as [pk:8B] (down from 16B). Chain-side, the unused by_code_table_idx_prikey index is dropped, reducing kv_index_object billing from 152 to 120 bytes per entry. Chain plugin hardening: - collect_next validates sec_key scope prefix and minimum size before pointer arithmetic (prevents size_t underflow and cross-scope next_key leak on deadline timeout) - pri_key size guard (kv_pri_key_size) in both forward and reverse iteration prevents OOB read from malformed kv_index_object - Reverse iteration uses break (not continue) on scope mismatch for consistency with forward path - scope_next_be computation moved inside non-overflow branch New constants: kv_scope_prefix_size, kv_pri_key_size Tests: 7 cross-scope tests (iteration, find, erase, upper_bound, double secondary, scope=0, reverse iteration) Recompiled contracts (sysio.system, sysio.roa) and regenerated snapshot reference data for updated billing.
Recompile all test contracts that use secondary indexes with the updated CDT that encodes [scope:8B][value] in secondary keys. Without this, chain_plugin get_table_rows queries return 0 rows because the scope-prefixed bounds don't match the stored keys. Bump kv_idx_payer_billing RAM allocation from 1.0 to 1.1 SYS to accommodate the test_kv_api WASM size with the 10x set_code multiplier.
brianjohnson5972
left a comment
brianjohnson5972
left a comment
There was a problem hiding this comment.
Some comments, otherwise good
|
|
||
| // If lower bound specified, scope-prefix it; otherwise use bare scope prefix | ||
| // to position at the first entry in this scope. | ||
| auto scoped_lb = has_lower ? prepend_scope(lb_bytes) |
There was a problem hiding this comment.
could just make it
auto scoped_lb = prepend_scope(lb_bytes);
| inline constexpr size_t kv_key_size = 24; ///< [table:8B][scope:8B][pk:8B] | ||
| inline constexpr size_t kv_prefix_size = 16; ///< [table:8B][scope:8B] | ||
| inline constexpr size_t kv_table_prefix_size = 8; ///< [table:8B] | ||
| inline constexpr size_t kv_key_size = 24; ///< [table:8B][scope:8B][pk:8B] |
There was a problem hiding this comment.
I know the comments handle this, but I think the code would document this if we changed the order and put the last 3 constants at the beginning and then set kv_prefix_size and kv_key_size by the sum of their parts.
Only do this if you agree.
| auto collect_next = [&](const chain::kv_index_object& obj) { | ||
| // Validate sec_key has scope prefix and matches our scope | ||
| if (obj.sec_key.size() < chain::kv_scope_prefix_size || | ||
| memcmp(obj.sec_key.data(), scope_be, chain::kv_scope_prefix_size) != 0) |
There was a problem hiding this comment.
I think we should either have a lambda to do this checking of sec_key==scope_be
or possibly make a struct for scope_be (/scope_next_be), with a method for this, that also returns sec_data, and possibly other scope_be stuff, like a constructor above, decode_sec_key being a method, the reverse upper_bound case. I'll let you decide between which makes sense, but I think there is a lot of room to make this code cleaner possibly, (particularly if this code isn't in its last state). Also used at lines 1873, and 1919.
There was a problem hiding this comment.
I'll take these in account for my current work. Going to merge this to get past master test failure.
2 participants
Summary
Bug: Secondary index iteration in
get_table_rowsleaked entries across scopes becausesec_keyhad no scope discriminator.Fix (CDT-side encoding, no intrinsic signature changes):
sec_key: prepend[scope:8B BE]prefixpri_key: shrink from[scope:8B][pk:8B]to[pk:8B]by_code_table_idx_prikeychainbase index (consensus change)kv_index_objectbilling: 152 → 120 bytes per entry (-21%)Chain plugin hardening:
collect_nextvalidates scope prefix and minimum size before pointer arithmetic (preventssize_tunderflow and cross-scopenext_keyleak on deadline timeout)pri_keysize guard (kv_pri_key_size) in both forward and reverse iteration prevents OOB readkv_scope_prefix_size,kv_pri_key_sizeTests: 7 cross-scope tests (iteration, find, erase, upper_bound, double secondary, scope=0, reverse)
Recompiled production contracts and regenerated snapshot reference data
Updated RAM billing docs with EOS mainnet estimates from actual snapshot data
Depends on: Wire-Network/wire-cdt#47 (CDT encoding changes)