From 867b0632a3ecb8755156925b73133a0dd705918d Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Fri, 9 Jan 2026 23:36:59 +0000 Subject: [PATCH 1/2] fix!: multiple traces had ghost row injection vulnerabilities --- .../relations/emit_unencrypted_log.test.cpp | 185 +++++++++++++++ .../relations/get_contract_instance.test.cpp | 178 ++++++++++++++ .../constraining/relations/to_radix.test.cpp | 224 ++++++++++++++++++ 3 files changed, 587 insertions(+) diff --git a/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/emit_unencrypted_log.test.cpp b/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/emit_unencrypted_log.test.cpp index 8a90548fd41b..2d3b90e7ce99 100644 --- a/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/emit_unencrypted_log.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/emit_unencrypted_log.test.cpp @@ -10,6 +10,7 @@ #include "barretenberg/vm2/constraining/testing/check_relation.hpp" #include "barretenberg/vm2/generated/relations/emit_unencrypted_log.hpp" #include "barretenberg/vm2/generated/relations/lookups_emit_unencrypted_log.hpp" +#include "barretenberg/vm2/generated/relations/perms_emit_unencrypted_log.hpp" #include "barretenberg/vm2/simulation/events/emit_unencrypted_log_event.hpp" #include "barretenberg/vm2/simulation/events/event_emitter.hpp" #include "barretenberg/vm2/simulation/events/gt_event.hpp" @@ -18,6 +19,7 @@ #include "barretenberg/vm2/testing/macros.hpp" #include "barretenberg/vm2/testing/public_inputs_builder.hpp" #include "barretenberg/vm2/tracegen/gt_trace.hpp" +#include "barretenberg/vm2/tracegen/memory_trace.hpp" #include "barretenberg/vm2/tracegen/opcodes/emit_unencrypted_log_trace.hpp" #include "barretenberg/vm2/tracegen/precomputed_trace.hpp" #include "barretenberg/vm2/tracegen/public_inputs_trace.hpp" @@ -31,6 +33,8 @@ using simulation::EmitUnencryptedLogWriteEvent; using simulation::TrackedSideEffects; using testing::PublicInputsBuilder; using tracegen::EmitUnencryptedLogTraceBuilder; +using tracegen::MemoryTraceBuilder; +using tracegen::PrecomputedTraceBuilder; using tracegen::PublicInputsTraceBuilder; using tracegen::TestTraceContainer; using FF = AvmFlavorSettings::FF; @@ -639,6 +643,187 @@ TEST(EmitUnencryptedLogConstrainingTest, NegativeContractAddressConsistency) "CONTRACT_ADDRESS_CONSISTENCY"); } +// ===================================================================== +// Ghost Row Injection Vulnerability Tests +// ===================================================================== +// These tests verify that ghost rows (sel=0) cannot fire permutations. +// The vulnerability: is_write_memory_value is only boolean-constrained, +// not constrained to be 0 when sel=0. This allows ghost rows to fire +// the #[READ_MEM] permutation via sel_should_read_memory. +// +// NOTE: A simple relations-only test is complex for emit_unencrypted_log +// due to many derived constraints. A full attack test following the +// pattern from PR #19470 (NegativeFullAttackWithAllTraces) is needed. +// +// VULNERABILITY SUMMARY: +// - is_write_memory_value is only boolean-constrained +// - When sel=0, is_write_memory_value can still be set to 1 +// - This makes sel_should_read_memory = 1 (via derived constraint) +// - This fires the #[READ_MEM] permutation from a ghost row +// +// REQUIRED FIX: +// is_write_memory_value * (1 - sel) = 0; +// +// See pil/vm2/claude-audits/ghost-row-injection/opcodes-audit.md for details. + +// ===================================================================== +// Full Attack Test with All Traces +// ===================================================================== +// This test demonstrates a complete ghost row injection attack: +// 1. Create legitimate memory READ events (destination side) +// 2. Build memory trace from those events +// 3. Inject ghost emit_unencrypted_log row with sel=0 but is_write_memory_value=1 +// 4. This makes sel_should_read_memory=1 which fires the #[READ_MEM] permutation +// 5. Verify the permutation matches - attack succeeds! +// +// This follows the pattern from PR #19470 (NegativeFullAttackWithAllTraces). +TEST(EmitUnencryptedLogConstrainingTest, NegativeFullAttackWithAllTraces) +{ + TestTraceContainer trace; + MemoryTraceBuilder memory_trace_builder; + PrecomputedTraceBuilder precomputed_trace_builder; + + // ========== STEP 1: Attacker-controlled values ========== + uint32_t malicious_clk = 42; + uint16_t malicious_space_id = 1; + MemoryAddress malicious_log_addr = 0xDEAD; + FF malicious_value = 0x1337; + MemoryTag malicious_tag = MemoryTag::FF; // FF tag for log values + + // ========== STEP 2: Create legitimate memory events ========== + // These events will be processed by the MemoryTraceBuilder to create + // legitimate destination rows that the ghost source can match. + // The permutation is for READS (rw=0), so we create READ events. + std::vector mem_events = { + { + .execution_clk = malicious_clk, + .mode = simulation::MemoryMode::READ, // rw = 0 for memory reads + .addr = malicious_log_addr, + .value = MemoryValue::from(malicious_value), + .space_id = malicious_space_id, + }, + }; + + // ========== STEP 3: Build memory trace (destination side) ========== + precomputed_trace_builder.process_sel_range_8(trace); + precomputed_trace_builder.process_sel_range_16(trace); + precomputed_trace_builder.process_misc(trace, 1 << 16); + precomputed_trace_builder.process_tag_parameters(trace); + memory_trace_builder.process(mem_events, trace); + + // Find where the memory row was placed + uint32_t memory_row = 0; + for (uint32_t row = 0; row < trace.get_num_rows(); row++) { + if (trace.get(C::memory_sel, row) == 1) { + memory_row = row; + break; + } + } + + // ========== STEP 4: Inject ghost emit_unencrypted_log row ========== + // The vulnerability: When sel=0, is_write_memory_value can still be 1 + // which makes sel_should_read_memory = is_write_memory_value * (1 - error_out_of_bounds) = 1 + // This fires the #[READ_MEM] permutation from a ghost row! + uint32_t ghost_row = 0; + trace.set(ghost_row, + std::vector>{ + { C::precomputed_first_row, 1 }, + { C::precomputed_clk, ghost_row }, + { C::precomputed_zero, 0 }, + // Ghost row: emit_unencrypted_log_sel = 0, but is_write_memory_value = 1 + { C::emit_unencrypted_log_sel, 0 }, + { C::emit_unencrypted_log_is_write_memory_value, 1 }, // Vulnerability! + { C::emit_unencrypted_log_error_out_of_bounds, 0 }, // No error, so sel_should_read_memory = 1 + { C::emit_unencrypted_log_sel_should_read_memory, 1 }, // Fires #[READ_MEM] permutation! + // Permutation tuple values - must match memory destination + { C::emit_unencrypted_log_execution_clk, malicious_clk }, + { C::emit_unencrypted_log_space_id, malicious_space_id }, + { C::emit_unencrypted_log_log_address, malicious_log_addr }, + { C::emit_unencrypted_log_value, malicious_value }, + { C::emit_unencrypted_log_tag, static_cast(malicious_tag) }, + // Derived constraint: is_write_memory_value * (value - public_inputs_value) = 0 + { C::emit_unencrypted_log_public_inputs_value, malicious_value }, + }); + + // Set the destination selector on the memory row to mark it as matching emit_unencrypted_log + trace.set(C::memory_sel_unencrypted_log_read, memory_row, 1); + + // ========== STEP 5: Verify attack ========== + std::cout << "\n=== GHOST ROW INJECTION ATTACK TEST (emit_unencrypted_log) ===" << std::endl; + std::cout << "Ghost emit_unencrypted_log row at row " << ghost_row + << " with sel=0, is_write_memory_value=1, sel_should_read_memory=1" << std::endl; + std::cout << "Memory destination row at row " << memory_row << " with clk=" << malicious_clk << std::endl; + + // Debug: Print source tuple + std::cout << "\nSource tuple (emit_unencrypted_log row " << ghost_row << "):" << std::endl; + std::cout << " execution_clk = " << trace.get(C::emit_unencrypted_log_execution_clk, ghost_row) << std::endl; + std::cout << " space_id = " << trace.get(C::emit_unencrypted_log_space_id, ghost_row) << std::endl; + std::cout << " log_address = " << trace.get(C::emit_unencrypted_log_log_address, ghost_row) << std::endl; + std::cout << " value = " << trace.get(C::emit_unencrypted_log_value, ghost_row) << std::endl; + std::cout << " tag = " << trace.get(C::emit_unencrypted_log_tag, ghost_row) << std::endl; + std::cout << " sel_should_read_memory = " << trace.get(C::emit_unencrypted_log_sel_should_read_memory, ghost_row) + << std::endl; + std::cout << " precomputed_zero = " << trace.get(C::precomputed_zero, ghost_row) << std::endl; + + // Debug: Print destination tuple + std::cout << "\nDestination tuple (memory row " << memory_row << "):" << std::endl; + std::cout << " memory_clk = " << trace.get(C::memory_clk, memory_row) << std::endl; + std::cout << " memory_space_id = " << trace.get(C::memory_space_id, memory_row) << std::endl; + std::cout << " memory_address = " << trace.get(C::memory_address, memory_row) << std::endl; + std::cout << " memory_value = " << trace.get(C::memory_value, memory_row) << std::endl; + std::cout << " memory_tag = " << trace.get(C::memory_tag, memory_row) << std::endl; + std::cout << " memory_rw = " << trace.get(C::memory_rw, memory_row) << std::endl; + std::cout << " memory_sel = " << trace.get(C::memory_sel, memory_row) << std::endl; + std::cout << " memory_sel_unencrypted_log_read = " << trace.get(C::memory_sel_unencrypted_log_read, memory_row) + << std::endl; + + // emit_unencrypted_log relation should PASS (this is the vulnerability) + std::cout << "\nemit_unencrypted_log relation: "; + check_relation(trace); + std::cout << "PASSED (vulnerability confirmed)" << std::endl; + + // ========== STEP 6: Verify attack succeeded ========== + // The attack succeeds because: + // 1. emit_unencrypted_log relation PASSED (no constraint prevents ghost rows from setting is_write_memory_value=1) + // 2. The source tuple values match a legitimate memory destination tuple + // 3. The permutation would match if we ran the full permutation check + // + // Note: The permutation is part of a complex multi-permutation in MemoryTraceBuilder, + // so we verify matching tuples directly instead of using check_multipermutation_interaction. + + // Verify tuples match + // Source: execution_clk, space_id, log_address, value, tag, precomputed_zero + // Dest: memory_clk, memory_space_id, memory_address, memory_value, memory_tag, memory_rw + bool tuples_match = + (trace.get(C::emit_unencrypted_log_execution_clk, ghost_row) == trace.get(C::memory_clk, memory_row)) && + (trace.get(C::emit_unencrypted_log_space_id, ghost_row) == trace.get(C::memory_space_id, memory_row)) && + (trace.get(C::emit_unencrypted_log_log_address, ghost_row) == trace.get(C::memory_address, memory_row)) && + (trace.get(C::emit_unencrypted_log_value, ghost_row) == trace.get(C::memory_value, memory_row)) && + (trace.get(C::emit_unencrypted_log_tag, ghost_row) == trace.get(C::memory_tag, memory_row)) && + (trace.get(C::precomputed_zero, ghost_row) == trace.get(C::memory_rw, memory_row)); // rw=0 for reads + + std::cout << "\nTuples match: " << (tuples_match ? "YES" : "NO") << std::endl; + + // Attack succeeds if: + // 1. emit_unencrypted_log relation passed (no constraint prevents ghost row) + // 2. Tuples match (permutation would match) + bool attack_succeeded = tuples_match; + + std::cout << "\n=== ATTACK RESULT ===" << std::endl; + if (attack_succeeded) { + std::cout << "CRITICAL: Ghost row injection attack SUCCEEDED!" << std::endl; + std::cout << "Attacker can inject arbitrary memory reads via emit_unencrypted_log." << std::endl; + std::cout << "\nRequired fix in emit_unencrypted_log.pil:" << std::endl; + std::cout << " #[IS_WRITE_MEMORY_VALUE_REQUIRES_SEL]" << std::endl; + std::cout << " is_write_memory_value * (1 - sel) = 0;" << std::endl; + } else { + std::cout << "Attack blocked." << std::endl; + } + + // Test documents vulnerability - we expect the attack to succeed until fixed + EXPECT_TRUE(attack_succeeded) << "Ghost row injection attack should succeed (vulnerability exists)"; +} + } // namespace } // namespace bb::avm2::constraining diff --git a/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/get_contract_instance.test.cpp b/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/get_contract_instance.test.cpp index c107ef70307d..9f4221ebcb9c 100644 --- a/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/get_contract_instance.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/get_contract_instance.test.cpp @@ -10,10 +10,12 @@ #include "barretenberg/vm2/constraining/testing/check_relation.hpp" #include "barretenberg/vm2/generated/relations/get_contract_instance.hpp" #include "barretenberg/vm2/generated/relations/lookups_get_contract_instance.hpp" +#include "barretenberg/vm2/generated/relations/perms_get_contract_instance.hpp" #include "barretenberg/vm2/simulation/events/event_emitter.hpp" #include "barretenberg/vm2/simulation/events/get_contract_instance_event.hpp" #include "barretenberg/vm2/testing/fixtures.hpp" #include "barretenberg/vm2/testing/macros.hpp" +#include "barretenberg/vm2/tracegen/memory_trace.hpp" #include "barretenberg/vm2/tracegen/opcodes/get_contract_instance_trace.hpp" #include "barretenberg/vm2/tracegen/precomputed_trace.hpp" #include "barretenberg/vm2/tracegen/test_trace_container.hpp" @@ -24,6 +26,7 @@ namespace { using simulation::EventEmitter; using simulation::GetContractInstanceEvent; using tracegen::GetContractInstanceTraceBuilder; +using tracegen::MemoryTraceBuilder; using tracegen::PrecomputedTraceBuilder; using tracegen::TestTraceContainer; using FF = AvmFlavorSettings::FF; @@ -398,5 +401,180 @@ TEST(GetContractInstanceConstrainingTest, IntegrationTracegenOutOfBounds) check_relation(trace); } +// ===================================================================== +// Ghost Row Injection Vulnerability Tests +// ===================================================================== +// These tests verify that ghost rows (sel=0) cannot fire permutations. +// The vulnerability: is_valid_member_enum is only constrained via lookup +// and WRITES_OUT_OF_BOUNDS check, but NOT constrained to be 0 when sel=0. +// This allows ghost rows to fire the memory write permutations. +// +// See pil/vm2/claude-audits/ghost-row-injection/opcodes-audit.md for details. + +// ===================================================================== +// Full Attack Test with All Traces +// ===================================================================== +// This test demonstrates a complete ghost row injection attack: +// 1. Create legitimate memory WRITE events (destination side) +// 2. Build memory trace from those events +// 3. Inject ghost get_contract_instance row with sel=0 but is_valid_member_enum=1 +// 4. This fires the #[MEM_WRITE_CONTRACT_INSTANCE_EXISTS] permutation +// 5. Verify the permutation matches - attack succeeds! +// +// This follows the pattern from PR #19470 (NegativeFullAttackWithAllTraces). +TEST(GetContractInstanceConstrainingTest, NegativeFullAttackWithAllTraces) +{ + TestTraceContainer trace; + MemoryTraceBuilder memory_trace_builder; + PrecomputedTraceBuilder precomputed_trace_builder; + + // ========== STEP 1: Attacker-controlled values ========== + uint32_t malicious_clk = 42; + uint16_t malicious_space_id = 1; + MemoryAddress malicious_dst_offset = 0x100; // dst_offset for exists write + uint1_t malicious_instance_exists = 1; // Arbitrary exists value (bool) + MemoryTag exists_tag = MemoryTag::U1; // U1 tag for exists + + // ========== STEP 2: Create legitimate memory events ========== + // These events will be processed by the MemoryTraceBuilder to create + // legitimate destination rows that the ghost source can match. + // The permutation is for WRITES (rw=1), so we create WRITE events. + std::vector mem_events = { + { + .execution_clk = malicious_clk, + .mode = simulation::MemoryMode::WRITE, // rw = 1 for memory writes + .addr = malicious_dst_offset, + .value = MemoryValue::from(malicious_instance_exists), + .space_id = malicious_space_id, + }, + }; + + // ========== STEP 3: Build memory trace (destination side) ========== + precomputed_trace_builder.process_sel_range_8(trace); + precomputed_trace_builder.process_sel_range_16(trace); + precomputed_trace_builder.process_misc(trace, 1 << 16); + precomputed_trace_builder.process_tag_parameters(trace); + memory_trace_builder.process(mem_events, trace); + + // Find where the memory row was placed + uint32_t memory_row = 0; + for (uint32_t row = 0; row < trace.get_num_rows(); row++) { + if (trace.get(C::memory_sel, row) == 1) { + memory_row = row; + break; + } + } + + // ========== STEP 4: Inject ghost get_contract_instance row ========== + // The vulnerability: When sel=0, is_valid_member_enum can still be 1 + // which fires the #[MEM_WRITE_CONTRACT_INSTANCE_EXISTS] permutation. + // + // Key constraints to satisfy: + // - is_valid_writes_in_bounds must be 1 (so WRITES_OUT_OF_BOUNDS = 0) + // - WRITES_OUT_OF_BOUNDS * is_valid_member_enum = 0 is satisfied when WRITES_OUT_OF_BOUNDS = 0 + // - exists_tag = is_valid_writes_in_bounds * MEM_TAG_U1 = 1 * 1 = 1 + uint32_t ghost_row = 0; + trace.set(ghost_row, + std::vector>{ + { C::precomputed_first_row, 1 }, + { C::precomputed_clk, ghost_row }, + // Ghost row: sel = 0, but is_valid_member_enum = 1 + { C::get_contract_instance_sel, 0 }, + { C::get_contract_instance_is_valid_member_enum, 1 }, // Vulnerability! Fires permutation + { C::get_contract_instance_is_valid_writes_in_bounds, 1 }, // Needed for derived constraints + // Derived constraint: exists_tag = is_valid_writes_in_bounds * MEM_TAG_U1 + { C::get_contract_instance_exists_tag, static_cast(exists_tag) }, + // Permutation tuple values - must match memory destination + { C::get_contract_instance_clk, malicious_clk }, + { C::get_contract_instance_space_id, malicious_space_id }, + { C::get_contract_instance_dst_offset, malicious_dst_offset }, + { C::get_contract_instance_instance_exists, static_cast(malicious_instance_exists) }, + // Additional columns needed for derived constraints + { C::get_contract_instance_sel_error, + 0 }, // sel_error = sel * (1 - is_valid_writes_in_bounds * is_valid_member_enum) + { C::get_contract_instance_is_deployer, 0 }, + { C::get_contract_instance_is_class_id, 0 }, + { C::get_contract_instance_is_init_hash, 0 }, + { C::get_contract_instance_selected_member, 0 }, // 0 since no is_* flags are set + { C::get_contract_instance_member_write_offset, malicious_dst_offset + 1 }, + { C::get_contract_instance_member_tag, static_cast(MemoryTag::FF) }, + }); + + // Set the destination selector on the memory row to mark it as matching get_contract_instance + trace.set(C::memory_sel_get_contract_instance_exists_write, memory_row, 1); + + // ========== STEP 5: Verify attack ========== + std::cout << "\n=== GHOST ROW INJECTION ATTACK TEST (get_contract_instance) ===" << std::endl; + std::cout << "Ghost get_contract_instance row at row " << ghost_row << " with sel=0, is_valid_member_enum=1" + << std::endl; + std::cout << "Memory destination row at row " << memory_row << " with clk=" << malicious_clk << std::endl; + + // Debug: Print source tuple + std::cout << "\nSource tuple (get_contract_instance row " << ghost_row << "):" << std::endl; + std::cout << " clk = " << trace.get(C::get_contract_instance_clk, ghost_row) << std::endl; + std::cout << " space_id = " << trace.get(C::get_contract_instance_space_id, ghost_row) << std::endl; + std::cout << " dst_offset = " << trace.get(C::get_contract_instance_dst_offset, ghost_row) << std::endl; + std::cout << " instance_exists = " << trace.get(C::get_contract_instance_instance_exists, ghost_row) << std::endl; + std::cout << " exists_tag = " << trace.get(C::get_contract_instance_exists_tag, ghost_row) << std::endl; + std::cout << " is_valid_member_enum = " << trace.get(C::get_contract_instance_is_valid_member_enum, ghost_row) + << std::endl; + + // Debug: Print destination tuple + std::cout << "\nDestination tuple (memory row " << memory_row << "):" << std::endl; + std::cout << " memory_clk = " << trace.get(C::memory_clk, memory_row) << std::endl; + std::cout << " memory_space_id = " << trace.get(C::memory_space_id, memory_row) << std::endl; + std::cout << " memory_address = " << trace.get(C::memory_address, memory_row) << std::endl; + std::cout << " memory_value = " << trace.get(C::memory_value, memory_row) << std::endl; + std::cout << " memory_tag = " << trace.get(C::memory_tag, memory_row) << std::endl; + std::cout << " memory_rw = " << trace.get(C::memory_rw, memory_row) << std::endl; + std::cout << " memory_sel = " << trace.get(C::memory_sel, memory_row) << std::endl; + std::cout << " memory_sel_get_contract_instance_exists_write = " + << trace.get(C::memory_sel_get_contract_instance_exists_write, memory_row) << std::endl; + + // get_contract_instance relation should PASS (this is the vulnerability) + std::cout << "\nget_contract_instance relation: "; + check_relation(trace); + std::cout << "PASSED (vulnerability confirmed)" << std::endl; + + // ========== STEP 6: Verify attack succeeded ========== + // The attack succeeds because: + // 1. get_contract_instance relation PASSED (no constraint prevents ghost rows from setting is_valid_member_enum=1) + // 2. The source tuple values match a legitimate memory destination tuple + // 3. The permutation would match if we ran the full permutation check + + // Verify tuples match + // Source: clk, space_id, dst_offset, instance_exists, exists_tag, is_valid_member_enum + // Dest: memory_clk, memory_space_id, memory_address, memory_value, memory_tag, memory_rw + bool tuples_match = + (trace.get(C::get_contract_instance_clk, ghost_row) == trace.get(C::memory_clk, memory_row)) && + (trace.get(C::get_contract_instance_space_id, ghost_row) == trace.get(C::memory_space_id, memory_row)) && + (trace.get(C::get_contract_instance_dst_offset, ghost_row) == trace.get(C::memory_address, memory_row)) && + (trace.get(C::get_contract_instance_instance_exists, ghost_row) == trace.get(C::memory_value, memory_row)) && + (trace.get(C::get_contract_instance_exists_tag, ghost_row) == trace.get(C::memory_tag, memory_row)) && + (trace.get(C::get_contract_instance_is_valid_member_enum, ghost_row) == + trace.get(C::memory_rw, memory_row)); // rw=1 for writes + + std::cout << "\nTuples match: " << (tuples_match ? "YES" : "NO") << std::endl; + + // Attack succeeds if: + // 1. get_contract_instance relation passed (no constraint prevents ghost row) + // 2. Tuples match (permutation would match) + bool attack_succeeded = tuples_match; + + std::cout << "\n=== ATTACK RESULT ===" << std::endl; + if (attack_succeeded) { + std::cout << "CRITICAL: Ghost row injection attack SUCCEEDED!" << std::endl; + std::cout << "Attacker can inject arbitrary memory writes via get_contract_instance." << std::endl; + std::cout << "\nRequired fix in get_contract_instance.pil:" << std::endl; + std::cout << " #[IS_VALID_MEMBER_ENUM_REQUIRES_SEL]" << std::endl; + std::cout << " is_valid_member_enum * (1 - sel) = 0;" << std::endl; + } else { + std::cout << "Attack blocked." << std::endl; + } + + // Test documents vulnerability - we expect the attack to succeed until fixed + EXPECT_TRUE(attack_succeeded) << "Ghost row injection attack should succeed (vulnerability exists)"; +} + } // namespace } // namespace bb::avm2::constraining diff --git a/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/to_radix.test.cpp b/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/to_radix.test.cpp index 1093d70d7c85..a145a521397e 100644 --- a/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/to_radix.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/to_radix.test.cpp @@ -8,6 +8,7 @@ #include "barretenberg/vm2/constraining/testing/check_relation.hpp" #include "barretenberg/vm2/generated/relations/lookups_to_radix.hpp" #include "barretenberg/vm2/generated/relations/lookups_to_radix_mem.hpp" +#include "barretenberg/vm2/generated/relations/perms_to_radix_mem.hpp" #include "barretenberg/vm2/simulation/events/gt_event.hpp" #include "barretenberg/vm2/simulation/events/range_check_event.hpp" #include "barretenberg/vm2/simulation/gadgets/range_check.hpp" @@ -20,6 +21,7 @@ #include "barretenberg/vm2/testing/macros.hpp" #include "barretenberg/vm2/tracegen/execution_trace.hpp" #include "barretenberg/vm2/tracegen/gt_trace.hpp" +#include "barretenberg/vm2/tracegen/memory_trace.hpp" #include "barretenberg/vm2/tracegen/precomputed_trace.hpp" #include "barretenberg/vm2/tracegen/test_trace_container.hpp" #include "barretenberg/vm2/tracegen/to_radix_trace.hpp" @@ -32,6 +34,7 @@ using ::testing::StrictMock; using tracegen::ExecutionTraceBuilder; using tracegen::GreaterThanTraceBuilder; +using tracegen::MemoryTraceBuilder; using tracegen::PrecomputedTraceBuilder; using tracegen::TestTraceContainer; using tracegen::ToRadixTraceBuilder; @@ -1006,6 +1009,227 @@ TEST(ToRadixMemoryConstrainingTest, ComplexTest) lookup_to_radix_mem_input_output_to_radix_settings>(trace); } +// ===================================================================== +// Ghost Row Injection Vulnerability Tests +// ===================================================================== +// These tests verify that ghost rows (sel=0) cannot fire permutations. +// The vulnerability: sel_should_write_mem is only constrained via start +// row assignment and NOT_LAST propagation, but NOT_LAST = sel * (1 - LATCH_CONDITION). +// When sel=0, sel_should_write_mem is unconstrained, allowing ghost rows +// to fire the #[WRITE_MEM] permutation. + +// Test that ghost rows (sel=0) cannot set sel_should_write_mem=1 +// This verifies the fix: sel_should_write_mem * (1 - sel) = 0 +TEST(ToRadixMemoryConstrainingTest, NegativeGhostRowMemoryWrite_RelationsOnly) +{ + // Try to create a ghost row (sel=0) with sel_should_write_mem=1 + // which would fire the #[WRITE_MEM] permutation + TestTraceContainer trace({ + { + { C::precomputed_first_row, 1 }, + }, + { + { C::to_radix_mem_sel, 0 }, // Ghost row: gadget not active + { C::to_radix_mem_sel_should_write_mem, 1 }, // Try to fire memory write anyway + { C::to_radix_mem_execution_clk, 1 }, + { C::to_radix_mem_space_id, 1 }, + { C::to_radix_mem_dst_addr, 100 }, + { C::to_radix_mem_limb_value, 999 }, // Arbitrary limb value + { C::to_radix_mem_output_tag, 2 }, // U8 tag + }, + }); + + // The fix: sel_should_write_mem * (1 - sel) = 0 + // When sel=0 and sel_should_write_mem=1: 1 * (1-0) = 1 != 0 -> FAILS + // TODO: Uncomment once fix is applied to PIL + // EXPECT_THROW_WITH_MESSAGE(check_relation(trace), + // "SEL_SHOULD_WRITE_MEM_REQUIRES_SEL"); + + // For now, this test documents the vulnerability: + // The relation PASSES when it should FAIL (no constraint prevents this) + check_relation(trace); +} + +// Also test sel_should_decompose which gates the INPUT_OUTPUT_TO_RADIX lookup +TEST(ToRadixMemoryConstrainingTest, NegativeGhostRowDecomposeLookup_RelationsOnly) +{ + // Try to create a ghost row (sel=0) with sel_should_decompose=1 + // which would fire the #[INPUT_OUTPUT_TO_RADIX] lookup + // + // Note: Must satisfy derived constraint: + // limb_index_to_lookup = sel_should_decompose * (num_limbs - 1) + TestTraceContainer trace({ + { + { C::precomputed_first_row, 1 }, + }, + { + { C::to_radix_mem_sel, 0 }, // Ghost row: gadget not active + { C::to_radix_mem_sel_should_decompose, 1 }, // Try to fire lookup anyway + { C::to_radix_mem_value_to_decompose, 1337 }, + { C::to_radix_mem_num_limbs, 4 }, // Set num_limbs for derived constraint + { C::to_radix_mem_limb_index_to_lookup, 3 }, // Must be: sel_should_decompose * (num_limbs - 1) = 1 * 3 = 3 + { C::to_radix_mem_radix, 10 }, + { C::to_radix_mem_limb_value, 7 }, + { C::to_radix_mem_value_found, 1 }, + }, + }); + + // The fix: sel_should_decompose * (1 - sel) = 0 + // When sel=0 and sel_should_decompose=1: 1 * (1-0) = 1 != 0 -> FAILS + // TODO: Uncomment once fix is applied to PIL + // EXPECT_THROW_WITH_MESSAGE(check_relation(trace), + // "SEL_SHOULD_DECOMPOSE_REQUIRES_SEL"); + + // For now, this test documents the vulnerability: + // The relation PASSES when it should FAIL (no constraint prevents this) + check_relation(trace); +} + +// ===================================================================== +// Full Attack Test with All Traces +// ===================================================================== +// This test demonstrates a complete ghost row injection attack: +// 1. Create legitimate memory WRITE events (destination side) +// 2. Build memory trace from those events +// 3. Inject ghost to_radix_mem row at matching clk (source side) +// 4. Verify the permutation matches - attack succeeds! +// +// This follows the pattern from PR #19470 (NegativeFullAttackWithAllTraces). +TEST(ToRadixMemoryConstrainingTest, NegativeFullAttackWithAllTraces) +{ + TestTraceContainer trace; + MemoryTraceBuilder memory_trace_builder; + PrecomputedTraceBuilder precomputed_trace_builder; + + // ========== STEP 1: Attacker-controlled values ========== + uint32_t malicious_clk = 42; + uint16_t malicious_space_id = 1; + MemoryAddress malicious_addr = 0xDEAD; + uint8_t malicious_limb_value = 0x99; // Arbitrary byte value + MemoryTag malicious_tag = MemoryTag::U8; + + // ========== STEP 2: Create legitimate memory events ========== + // These events will be processed by the MemoryTraceBuilder to create + // legitimate destination rows that the ghost source can match. + std::vector mem_events = { + { + .execution_clk = malicious_clk, + .mode = simulation::MemoryMode::WRITE, // rw = 1 + .addr = malicious_addr, + .value = MemoryValue::from_tag(malicious_tag, malicious_limb_value), + .space_id = malicious_space_id, + }, + }; + + // ========== STEP 3: Build memory trace (destination side) ========== + precomputed_trace_builder.process_sel_range_8(trace); + precomputed_trace_builder.process_sel_range_16(trace); + precomputed_trace_builder.process_misc(trace, 1 << 16); + precomputed_trace_builder.process_tag_parameters(trace); + memory_trace_builder.process(mem_events, trace); + + // Find where the memory row was placed + uint32_t memory_row = 0; + for (uint32_t row = 0; row < trace.get_num_rows(); row++) { + if (trace.get(C::memory_sel, row) == 1) { + memory_row = row; + break; + } + } + + // ========== STEP 4: Inject ghost to_radix_mem row ========== + // Place ghost row at row 0 (precomputed_clk = 0 by default). + // For this attack, we need the source's execution_clk to match memory_clk. + // We set the ghost row values to match the memory destination. + uint32_t ghost_row = 0; + trace.set(ghost_row, + std::vector>{ + { C::precomputed_first_row, 1 }, + { C::precomputed_clk, ghost_row }, + // Ghost row: to_radix_mem_sel = 0, but sel_should_write_mem = 1 + { C::to_radix_mem_sel, 0 }, + { C::to_radix_mem_sel_should_write_mem, 1 }, // Fires #[WRITE_MEM] permutation! + // Permutation tuple values - must match memory destination + { C::to_radix_mem_execution_clk, malicious_clk }, + { C::to_radix_mem_space_id, malicious_space_id }, + { C::to_radix_mem_dst_addr, malicious_addr }, + { C::to_radix_mem_limb_value, malicious_limb_value }, + { C::to_radix_mem_output_tag, static_cast(malicious_tag) }, + }); + + // Set the destination selector on the memory row to mark it as matching to_radix + trace.set(C::memory_sel_to_radix_write, memory_row, 1); + + // ========== STEP 5: Verify attack ========== + std::cout << "\n=== GHOST ROW INJECTION ATTACK TEST (to_radix_mem) ===" << std::endl; + std::cout << "Ghost to_radix_mem row at row " << ghost_row << " with sel=0, sel_should_write_mem=1" << std::endl; + std::cout << "Memory destination row at row " << memory_row << " with clk=" << malicious_clk << std::endl; + + // Debug: Print source tuple + std::cout << "\nSource tuple (to_radix_mem row " << ghost_row << "):" << std::endl; + std::cout << " execution_clk = " << trace.get(C::to_radix_mem_execution_clk, ghost_row) << std::endl; + std::cout << " space_id = " << trace.get(C::to_radix_mem_space_id, ghost_row) << std::endl; + std::cout << " dst_addr = " << trace.get(C::to_radix_mem_dst_addr, ghost_row) << std::endl; + std::cout << " limb_value = " << trace.get(C::to_radix_mem_limb_value, ghost_row) << std::endl; + std::cout << " output_tag = " << trace.get(C::to_radix_mem_output_tag, ghost_row) << std::endl; + std::cout << " sel_should_write_mem = " << trace.get(C::to_radix_mem_sel_should_write_mem, ghost_row) << std::endl; + + // Debug: Print destination tuple + std::cout << "\nDestination tuple (memory row " << memory_row << "):" << std::endl; + std::cout << " memory_clk = " << trace.get(C::memory_clk, memory_row) << std::endl; + std::cout << " memory_space_id = " << trace.get(C::memory_space_id, memory_row) << std::endl; + std::cout << " memory_address = " << trace.get(C::memory_address, memory_row) << std::endl; + std::cout << " memory_value = " << trace.get(C::memory_value, memory_row) << std::endl; + std::cout << " memory_tag = " << trace.get(C::memory_tag, memory_row) << std::endl; + std::cout << " memory_rw = " << trace.get(C::memory_rw, memory_row) << std::endl; + std::cout << " memory_sel = " << trace.get(C::memory_sel, memory_row) << std::endl; + std::cout << " memory_sel_to_radix_write = " << trace.get(C::memory_sel_to_radix_write, memory_row) << std::endl; + + // to_radix_mem relation should PASS (this is the vulnerability) + std::cout << "to_radix_mem relation: "; + check_relation(trace); + std::cout << "PASSED (vulnerability confirmed)" << std::endl; + + // ========== STEP 6: Verify attack succeeded ========== + // The attack succeeds because: + // 1. to_radix_mem relation PASSED (no constraint prevents ghost rows from setting sel_should_write_mem=1) + // 2. The source tuple values match a legitimate memory destination tuple + // 3. The permutation would match if we ran the full permutation check + // + // Note: The permutation is part of a complex multi-permutation in MemoryTraceBuilder, + // so we verify matching tuples directly instead of using check_multipermutation_interaction. + + // Verify tuples match + bool tuples_match = + (trace.get(C::to_radix_mem_execution_clk, ghost_row) == trace.get(C::memory_clk, memory_row)) && + (trace.get(C::to_radix_mem_space_id, ghost_row) == trace.get(C::memory_space_id, memory_row)) && + (trace.get(C::to_radix_mem_dst_addr, ghost_row) == trace.get(C::memory_address, memory_row)) && + (trace.get(C::to_radix_mem_limb_value, ghost_row) == trace.get(C::memory_value, memory_row)) && + (trace.get(C::to_radix_mem_output_tag, ghost_row) == trace.get(C::memory_tag, memory_row)) && + (trace.get(C::to_radix_mem_sel_should_write_mem, ghost_row) == trace.get(C::memory_rw, memory_row)); + + std::cout << "\nTuples match: " << (tuples_match ? "YES" : "NO") << std::endl; + + // Attack succeeds if: + // 1. to_radix_mem relation passed (no constraint prevents ghost row) + // 2. Tuples match (permutation would match) + bool attack_succeeded = tuples_match; + + std::cout << "\n=== ATTACK RESULT ===" << std::endl; + if (attack_succeeded) { + std::cout << "CRITICAL: Ghost row injection attack SUCCEEDED!" << std::endl; + std::cout << "Attacker can inject arbitrary memory writes via to_radix_mem." << std::endl; + std::cout << "\nRequired fix in to_radix_mem.pil:" << std::endl; + std::cout << " #[SEL_SHOULD_WRITE_MEM_REQUIRES_SEL]" << std::endl; + std::cout << " sel_should_write_mem * (1 - sel) = 0;" << std::endl; + } else { + std::cout << "Attack blocked." << std::endl; + } + + // Test documents vulnerability - we expect the attack to succeed until fixed + EXPECT_TRUE(attack_succeeded) << "Ghost row injection attack should succeed (vulnerability exists)"; +} + } // namespace } // namespace bb::avm2::constraining From c5ce1370f7b772bb1c028898808c534e0cee4aea Mon Sep 17 00:00:00 2001 From: dbanks12 Date: Fri, 9 Jan 2026 23:49:16 +0000 Subject: [PATCH 2/2] fix them --- .../pil/vm2/opcodes/emit_unencrypted_log.pil | 5 +- .../pil/vm2/opcodes/get_contract_instance.pil | 4 + barretenberg/cpp/pil/vm2/to_radix_mem.pil | 3 + .../relations/emit_unencrypted_log.test.cpp | 132 +++------------ .../relations/get_contract_instance.test.cpp | 137 +++------------ .../constraining/relations/to_radix.test.cpp | 157 ++---------------- .../relations/emit_unencrypted_log.hpp | 5 +- .../relations/emit_unencrypted_log_impl.hpp | 5 +- .../relations/get_contract_instance.hpp | 9 +- .../relations/get_contract_instance_impl.hpp | 26 +-- .../vm2/generated/relations/to_radix_mem.hpp | 7 +- .../generated/relations/to_radix_mem_impl.hpp | 10 +- 12 files changed, 112 insertions(+), 388 deletions(-) diff --git a/barretenberg/cpp/pil/vm2/opcodes/emit_unencrypted_log.pil b/barretenberg/cpp/pil/vm2/opcodes/emit_unencrypted_log.pil index 8499f2acf179..c20a7573b2b5 100644 --- a/barretenberg/cpp/pil/vm2/opcodes/emit_unencrypted_log.pil +++ b/barretenberg/cpp/pil/vm2/opcodes/emit_unencrypted_log.pil @@ -221,7 +221,9 @@ namespace emit_unencrypted_log; // So what we landed with is to always read memory except we can't read memory due to the first case. // This should be fine since the user already paid for the memory reads pol commit sel_should_read_memory; - sel_should_read_memory = is_write_memory_value * (1 - error_out_of_bounds); + // Gate by sel to avoid ghost rows triggering memory reads. + #[SEL_SHOULD_READ_MEMORY_IS_SEL_AND_WRITE_MEM_AND_NO_ERR] + sel_should_read_memory = sel * is_write_memory_value * (1 - error_out_of_bounds); #[LOG_ADDRESS_INCREMENT] NOT_END * ((log_address + is_write_memory_value) - log_address') = 0; @@ -291,4 +293,3 @@ namespace emit_unencrypted_log; precomputed.clk, public_inputs.cols[0] }; - diff --git a/barretenberg/cpp/pil/vm2/opcodes/get_contract_instance.pil b/barretenberg/cpp/pil/vm2/opcodes/get_contract_instance.pil index bd0a2a1a33bb..7dc71a2ef958 100644 --- a/barretenberg/cpp/pil/vm2/opcodes/get_contract_instance.pil +++ b/barretenberg/cpp/pil/vm2/opcodes/get_contract_instance.pil @@ -140,6 +140,10 @@ namespace get_contract_instance; // Do not allow is_valid_member_enum to be 1 if the precomputed lookup is disabled. #[IS_VALID_MEMBER_ENUM_ONLY_SET_BY_PRECOMPUTED_LOOKUP] WRITES_OUT_OF_BOUNDS * is_valid_member_enum = 0; + // Force is_valid_writes_in_bounds to 0 when sel is 0, and therefore also force is_valid_member_enum to 0 + // when combined with the constraint above. This prevents ghost row injection attacks. + #[IS_VALID_WRITES_IN_BOUNDS_REQUIRES_SEL] + is_valid_writes_in_bounds * (1 - sel) = 0; // Error aggregation // "Some/any error happened" diff --git a/barretenberg/cpp/pil/vm2/to_radix_mem.pil b/barretenberg/cpp/pil/vm2/to_radix_mem.pil index 5fe017279eba..71d8cd390df0 100644 --- a/barretenberg/cpp/pil/vm2/to_radix_mem.pil +++ b/barretenberg/cpp/pil/vm2/to_radix_mem.pil @@ -267,6 +267,9 @@ namespace to_radix_mem; // On following rows, we propagate sel_should_write_mem. #[SEL_SHOULD_WRITE_MEM_CONTINUITY] NOT_LAST * (sel_should_write_mem' - sel_should_write_mem) = 0; + // Force sel_should_write_mem to 0 when sel is 0 (prevents ghost row injection attacks) + #[SEL_SHOULD_WRITE_MEM_REQUIRES_SEL] + sel_should_write_mem * (1 - sel) = 0; pol commit output_tag; // Conditional Assignment: is_output_bits ? U1 : U8 diff --git a/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/emit_unencrypted_log.test.cpp b/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/emit_unencrypted_log.test.cpp index 2d3b90e7ce99..4b15b0ded3d2 100644 --- a/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/emit_unencrypted_log.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/emit_unencrypted_log.test.cpp @@ -647,14 +647,12 @@ TEST(EmitUnencryptedLogConstrainingTest, NegativeContractAddressConsistency) // Ghost Row Injection Vulnerability Tests // ===================================================================== // These tests verify that ghost rows (sel=0) cannot fire permutations. +// This is a defensive/sanity check: even though the situation is hard to exploit, +// we still enforce the selector gating to prevent accidental ghost reads. // The vulnerability: is_write_memory_value is only boolean-constrained, // not constrained to be 0 when sel=0. This allows ghost rows to fire // the #[READ_MEM] permutation via sel_should_read_memory. // -// NOTE: A simple relations-only test is complex for emit_unencrypted_log -// due to many derived constraints. A full attack test following the -// pattern from PR #19470 (NegativeFullAttackWithAllTraces) is needed. -// // VULNERABILITY SUMMARY: // - is_write_memory_value is only boolean-constrained // - When sel=0, is_write_memory_value can still be set to 1 @@ -662,56 +660,40 @@ TEST(EmitUnencryptedLogConstrainingTest, NegativeContractAddressConsistency) // - This fires the #[READ_MEM] permutation from a ghost row // // REQUIRED FIX: -// is_write_memory_value * (1 - sel) = 0; -// -// See pil/vm2/claude-audits/ghost-row-injection/opcodes-audit.md for details. +// Gate by sel to avoid ghost rows triggering memory reads. +// sel_should_read_memory = sel * is_write_memory_value * (1 - error_out_of_bounds); -// ===================================================================== -// Full Attack Test with All Traces -// ===================================================================== -// This test demonstrates a complete ghost row injection attack: -// 1. Create legitimate memory READ events (destination side) -// 2. Build memory trace from those events -// 3. Inject ghost emit_unencrypted_log row with sel=0 but is_write_memory_value=1 -// 4. This makes sel_should_read_memory=1 which fires the #[READ_MEM] permutation -// 5. Verify the permutation matches - attack succeeds! -// -// This follows the pattern from PR #19470 (NegativeFullAttackWithAllTraces). -TEST(EmitUnencryptedLogConstrainingTest, NegativeFullAttackWithAllTraces) +// This test verifies that the fix for the ghost row injection vulnerability works. +// The constraint `is_write_memory_value * (1 - sel) = 0` should prevent ghost rows +// from setting is_write_memory_value=1 when sel=0. +TEST(EmitUnencryptedLogConstrainingTest, NegativeGhostRowInjectionBlocked) { TestTraceContainer trace; MemoryTraceBuilder memory_trace_builder; PrecomputedTraceBuilder precomputed_trace_builder; - // ========== STEP 1: Attacker-controlled values ========== uint32_t malicious_clk = 42; uint16_t malicious_space_id = 1; MemoryAddress malicious_log_addr = 0xDEAD; FF malicious_value = 0x1337; - MemoryTag malicious_tag = MemoryTag::FF; // FF tag for log values + MemoryTag malicious_tag = MemoryTag::FF; - // ========== STEP 2: Create legitimate memory events ========== - // These events will be processed by the MemoryTraceBuilder to create - // legitimate destination rows that the ghost source can match. - // The permutation is for READS (rw=0), so we create READ events. std::vector mem_events = { { .execution_clk = malicious_clk, - .mode = simulation::MemoryMode::READ, // rw = 0 for memory reads + .mode = simulation::MemoryMode::READ, .addr = malicious_log_addr, .value = MemoryValue::from(malicious_value), .space_id = malicious_space_id, }, }; - // ========== STEP 3: Build memory trace (destination side) ========== precomputed_trace_builder.process_sel_range_8(trace); precomputed_trace_builder.process_sel_range_16(trace); precomputed_trace_builder.process_misc(trace, 1 << 16); precomputed_trace_builder.process_tag_parameters(trace); memory_trace_builder.process(mem_events, trace); - // Find where the memory row was placed uint32_t memory_row = 0; for (uint32_t row = 0; row < trace.get_num_rows(); row++) { if (trace.get(C::memory_sel, row) == 1) { @@ -720,108 +702,32 @@ TEST(EmitUnencryptedLogConstrainingTest, NegativeFullAttackWithAllTraces) } } - // ========== STEP 4: Inject ghost emit_unencrypted_log row ========== - // The vulnerability: When sel=0, is_write_memory_value can still be 1 - // which makes sel_should_read_memory = is_write_memory_value * (1 - error_out_of_bounds) = 1 - // This fires the #[READ_MEM] permutation from a ghost row! + // Attempt ghost row injection: sel=0 but is_write_memory_value=1 uint32_t ghost_row = 0; trace.set(ghost_row, std::vector>{ { C::precomputed_first_row, 1 }, { C::precomputed_clk, ghost_row }, { C::precomputed_zero, 0 }, - // Ghost row: emit_unencrypted_log_sel = 0, but is_write_memory_value = 1 { C::emit_unencrypted_log_sel, 0 }, - { C::emit_unencrypted_log_is_write_memory_value, 1 }, // Vulnerability! - { C::emit_unencrypted_log_error_out_of_bounds, 0 }, // No error, so sel_should_read_memory = 1 - { C::emit_unencrypted_log_sel_should_read_memory, 1 }, // Fires #[READ_MEM] permutation! - // Permutation tuple values - must match memory destination + { C::emit_unencrypted_log_is_write_memory_value, 1 }, + { C::emit_unencrypted_log_error_out_of_bounds, 0 }, + { C::emit_unencrypted_log_sel_should_read_memory, 1 }, { C::emit_unencrypted_log_execution_clk, malicious_clk }, { C::emit_unencrypted_log_space_id, malicious_space_id }, { C::emit_unencrypted_log_log_address, malicious_log_addr }, { C::emit_unencrypted_log_value, malicious_value }, { C::emit_unencrypted_log_tag, static_cast(malicious_tag) }, - // Derived constraint: is_write_memory_value * (value - public_inputs_value) = 0 { C::emit_unencrypted_log_public_inputs_value, malicious_value }, }); - // Set the destination selector on the memory row to mark it as matching emit_unencrypted_log trace.set(C::memory_sel_unencrypted_log_read, memory_row, 1); - // ========== STEP 5: Verify attack ========== - std::cout << "\n=== GHOST ROW INJECTION ATTACK TEST (emit_unencrypted_log) ===" << std::endl; - std::cout << "Ghost emit_unencrypted_log row at row " << ghost_row - << " with sel=0, is_write_memory_value=1, sel_should_read_memory=1" << std::endl; - std::cout << "Memory destination row at row " << memory_row << " with clk=" << malicious_clk << std::endl; - - // Debug: Print source tuple - std::cout << "\nSource tuple (emit_unencrypted_log row " << ghost_row << "):" << std::endl; - std::cout << " execution_clk = " << trace.get(C::emit_unencrypted_log_execution_clk, ghost_row) << std::endl; - std::cout << " space_id = " << trace.get(C::emit_unencrypted_log_space_id, ghost_row) << std::endl; - std::cout << " log_address = " << trace.get(C::emit_unencrypted_log_log_address, ghost_row) << std::endl; - std::cout << " value = " << trace.get(C::emit_unencrypted_log_value, ghost_row) << std::endl; - std::cout << " tag = " << trace.get(C::emit_unencrypted_log_tag, ghost_row) << std::endl; - std::cout << " sel_should_read_memory = " << trace.get(C::emit_unencrypted_log_sel_should_read_memory, ghost_row) - << std::endl; - std::cout << " precomputed_zero = " << trace.get(C::precomputed_zero, ghost_row) << std::endl; - - // Debug: Print destination tuple - std::cout << "\nDestination tuple (memory row " << memory_row << "):" << std::endl; - std::cout << " memory_clk = " << trace.get(C::memory_clk, memory_row) << std::endl; - std::cout << " memory_space_id = " << trace.get(C::memory_space_id, memory_row) << std::endl; - std::cout << " memory_address = " << trace.get(C::memory_address, memory_row) << std::endl; - std::cout << " memory_value = " << trace.get(C::memory_value, memory_row) << std::endl; - std::cout << " memory_tag = " << trace.get(C::memory_tag, memory_row) << std::endl; - std::cout << " memory_rw = " << trace.get(C::memory_rw, memory_row) << std::endl; - std::cout << " memory_sel = " << trace.get(C::memory_sel, memory_row) << std::endl; - std::cout << " memory_sel_unencrypted_log_read = " << trace.get(C::memory_sel_unencrypted_log_read, memory_row) - << std::endl; - - // emit_unencrypted_log relation should PASS (this is the vulnerability) - std::cout << "\nemit_unencrypted_log relation: "; - check_relation(trace); - std::cout << "PASSED (vulnerability confirmed)" << std::endl; - - // ========== STEP 6: Verify attack succeeded ========== - // The attack succeeds because: - // 1. emit_unencrypted_log relation PASSED (no constraint prevents ghost rows from setting is_write_memory_value=1) - // 2. The source tuple values match a legitimate memory destination tuple - // 3. The permutation would match if we ran the full permutation check - // - // Note: The permutation is part of a complex multi-permutation in MemoryTraceBuilder, - // so we verify matching tuples directly instead of using check_multipermutation_interaction. - - // Verify tuples match - // Source: execution_clk, space_id, log_address, value, tag, precomputed_zero - // Dest: memory_clk, memory_space_id, memory_address, memory_value, memory_tag, memory_rw - bool tuples_match = - (trace.get(C::emit_unencrypted_log_execution_clk, ghost_row) == trace.get(C::memory_clk, memory_row)) && - (trace.get(C::emit_unencrypted_log_space_id, ghost_row) == trace.get(C::memory_space_id, memory_row)) && - (trace.get(C::emit_unencrypted_log_log_address, ghost_row) == trace.get(C::memory_address, memory_row)) && - (trace.get(C::emit_unencrypted_log_value, ghost_row) == trace.get(C::memory_value, memory_row)) && - (trace.get(C::emit_unencrypted_log_tag, ghost_row) == trace.get(C::memory_tag, memory_row)) && - (trace.get(C::precomputed_zero, ghost_row) == trace.get(C::memory_rw, memory_row)); // rw=0 for reads - - std::cout << "\nTuples match: " << (tuples_match ? "YES" : "NO") << std::endl; - - // Attack succeeds if: - // 1. emit_unencrypted_log relation passed (no constraint prevents ghost row) - // 2. Tuples match (permutation would match) - bool attack_succeeded = tuples_match; - - std::cout << "\n=== ATTACK RESULT ===" << std::endl; - if (attack_succeeded) { - std::cout << "CRITICAL: Ghost row injection attack SUCCEEDED!" << std::endl; - std::cout << "Attacker can inject arbitrary memory reads via emit_unencrypted_log." << std::endl; - std::cout << "\nRequired fix in emit_unencrypted_log.pil:" << std::endl; - std::cout << " #[IS_WRITE_MEMORY_VALUE_REQUIRES_SEL]" << std::endl; - std::cout << " is_write_memory_value * (1 - sel) = 0;" << std::endl; - } else { - std::cout << "Attack blocked." << std::endl; - } - - // Test documents vulnerability - we expect the attack to succeed until fixed - EXPECT_TRUE(attack_succeeded) << "Ghost row injection attack should succeed (vulnerability exists)"; + // The fix: sel_should_read_memory = sel * is_write_memory_value * (1 - error_out_of_bounds) + // Gating by sel should cause the relation check to fail + // because sel_should_read_memory=1 and sel=0 violates this constraint + EXPECT_THROW_WITH_MESSAGE(check_relation(trace), + "SEL_SHOULD_READ_MEMORY_IS_SEL_AND_WRITE_MEM_AND_NO_ERR"); } } // namespace diff --git a/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/get_contract_instance.test.cpp b/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/get_contract_instance.test.cpp index 9f4221ebcb9c..aafee5cc6adb 100644 --- a/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/get_contract_instance.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/get_contract_instance.test.cpp @@ -405,51 +405,40 @@ TEST(GetContractInstanceConstrainingTest, IntegrationTracegenOutOfBounds) // Ghost Row Injection Vulnerability Tests // ===================================================================== // These tests verify that ghost rows (sel=0) cannot fire permutations. -// The vulnerability: is_valid_member_enum is only constrained via lookup -// and WRITES_OUT_OF_BOUNDS check, but NOT constrained to be 0 when sel=0. -// This allows ghost rows to fire the memory write permutations. +// The fix: is_valid_member_enum * (1 - sel) = 0 ensures is_valid_member_enum +// is forced to 0 when sel=0, preventing ghost rows from firing permutations. // -// See pil/vm2/claude-audits/ghost-row-injection/opcodes-audit.md for details. - -// ===================================================================== -// Full Attack Test with All Traces -// ===================================================================== -// This test demonstrates a complete ghost row injection attack: +// Test that the fix blocks ghost row injection attacks. +// Attack pattern: // 1. Create legitimate memory WRITE events (destination side) // 2. Build memory trace from those events // 3. Inject ghost get_contract_instance row with sel=0 but is_valid_member_enum=1 -// 4. This fires the #[MEM_WRITE_CONTRACT_INSTANCE_EXISTS] permutation -// 5. Verify the permutation matches - attack succeeds! -// -// This follows the pattern from PR #19470 (NegativeFullAttackWithAllTraces). -TEST(GetContractInstanceConstrainingTest, NegativeFullAttackWithAllTraces) +// 4. The fix should cause the relation check to fail +TEST(GetContractInstanceConstrainingTest, NegativeGhostRowInjectionBlocked) { TestTraceContainer trace; MemoryTraceBuilder memory_trace_builder; PrecomputedTraceBuilder precomputed_trace_builder; - // ========== STEP 1: Attacker-controlled values ========== + // Attacker-controlled values uint32_t malicious_clk = 42; uint16_t malicious_space_id = 1; - MemoryAddress malicious_dst_offset = 0x100; // dst_offset for exists write - uint1_t malicious_instance_exists = 1; // Arbitrary exists value (bool) - MemoryTag exists_tag = MemoryTag::U1; // U1 tag for exists - - // ========== STEP 2: Create legitimate memory events ========== - // These events will be processed by the MemoryTraceBuilder to create - // legitimate destination rows that the ghost source can match. - // The permutation is for WRITES (rw=1), so we create WRITE events. + MemoryAddress malicious_dst_offset = 0x100; + uint1_t malicious_instance_exists = 1; + MemoryTag exists_tag = MemoryTag::U1; + + // Create legitimate memory events std::vector mem_events = { { .execution_clk = malicious_clk, - .mode = simulation::MemoryMode::WRITE, // rw = 1 for memory writes + .mode = simulation::MemoryMode::WRITE, .addr = malicious_dst_offset, .value = MemoryValue::from(malicious_instance_exists), .space_id = malicious_space_id, }, }; - // ========== STEP 3: Build memory trace (destination side) ========== + // Build memory trace (destination side) precomputed_trace_builder.process_sel_range_8(trace); precomputed_trace_builder.process_sel_range_16(trace); precomputed_trace_builder.process_misc(trace, 1 << 16); @@ -465,115 +454,35 @@ TEST(GetContractInstanceConstrainingTest, NegativeFullAttackWithAllTraces) } } - // ========== STEP 4: Inject ghost get_contract_instance row ========== - // The vulnerability: When sel=0, is_valid_member_enum can still be 1 - // which fires the #[MEM_WRITE_CONTRACT_INSTANCE_EXISTS] permutation. - // - // Key constraints to satisfy: - // - is_valid_writes_in_bounds must be 1 (so WRITES_OUT_OF_BOUNDS = 0) - // - WRITES_OUT_OF_BOUNDS * is_valid_member_enum = 0 is satisfied when WRITES_OUT_OF_BOUNDS = 0 - // - exists_tag = is_valid_writes_in_bounds * MEM_TAG_U1 = 1 * 1 = 1 + // Inject ghost get_contract_instance row + // Ghost row: sel = 0, but is_valid_member_enum = 1 (attack attempt) uint32_t ghost_row = 0; trace.set(ghost_row, std::vector>{ { C::precomputed_first_row, 1 }, { C::precomputed_clk, ghost_row }, - // Ghost row: sel = 0, but is_valid_member_enum = 1 { C::get_contract_instance_sel, 0 }, - { C::get_contract_instance_is_valid_member_enum, 1 }, // Vulnerability! Fires permutation - { C::get_contract_instance_is_valid_writes_in_bounds, 1 }, // Needed for derived constraints - // Derived constraint: exists_tag = is_valid_writes_in_bounds * MEM_TAG_U1 + { C::get_contract_instance_is_valid_member_enum, 1 }, + { C::get_contract_instance_is_valid_writes_in_bounds, 1 }, { C::get_contract_instance_exists_tag, static_cast(exists_tag) }, - // Permutation tuple values - must match memory destination { C::get_contract_instance_clk, malicious_clk }, { C::get_contract_instance_space_id, malicious_space_id }, { C::get_contract_instance_dst_offset, malicious_dst_offset }, { C::get_contract_instance_instance_exists, static_cast(malicious_instance_exists) }, - // Additional columns needed for derived constraints - { C::get_contract_instance_sel_error, - 0 }, // sel_error = sel * (1 - is_valid_writes_in_bounds * is_valid_member_enum) + { C::get_contract_instance_sel_error, 0 }, { C::get_contract_instance_is_deployer, 0 }, { C::get_contract_instance_is_class_id, 0 }, { C::get_contract_instance_is_init_hash, 0 }, - { C::get_contract_instance_selected_member, 0 }, // 0 since no is_* flags are set + { C::get_contract_instance_selected_member, 0 }, { C::get_contract_instance_member_write_offset, malicious_dst_offset + 1 }, { C::get_contract_instance_member_tag, static_cast(MemoryTag::FF) }, }); - // Set the destination selector on the memory row to mark it as matching get_contract_instance trace.set(C::memory_sel_get_contract_instance_exists_write, memory_row, 1); - // ========== STEP 5: Verify attack ========== - std::cout << "\n=== GHOST ROW INJECTION ATTACK TEST (get_contract_instance) ===" << std::endl; - std::cout << "Ghost get_contract_instance row at row " << ghost_row << " with sel=0, is_valid_member_enum=1" - << std::endl; - std::cout << "Memory destination row at row " << memory_row << " with clk=" << malicious_clk << std::endl; - - // Debug: Print source tuple - std::cout << "\nSource tuple (get_contract_instance row " << ghost_row << "):" << std::endl; - std::cout << " clk = " << trace.get(C::get_contract_instance_clk, ghost_row) << std::endl; - std::cout << " space_id = " << trace.get(C::get_contract_instance_space_id, ghost_row) << std::endl; - std::cout << " dst_offset = " << trace.get(C::get_contract_instance_dst_offset, ghost_row) << std::endl; - std::cout << " instance_exists = " << trace.get(C::get_contract_instance_instance_exists, ghost_row) << std::endl; - std::cout << " exists_tag = " << trace.get(C::get_contract_instance_exists_tag, ghost_row) << std::endl; - std::cout << " is_valid_member_enum = " << trace.get(C::get_contract_instance_is_valid_member_enum, ghost_row) - << std::endl; - - // Debug: Print destination tuple - std::cout << "\nDestination tuple (memory row " << memory_row << "):" << std::endl; - std::cout << " memory_clk = " << trace.get(C::memory_clk, memory_row) << std::endl; - std::cout << " memory_space_id = " << trace.get(C::memory_space_id, memory_row) << std::endl; - std::cout << " memory_address = " << trace.get(C::memory_address, memory_row) << std::endl; - std::cout << " memory_value = " << trace.get(C::memory_value, memory_row) << std::endl; - std::cout << " memory_tag = " << trace.get(C::memory_tag, memory_row) << std::endl; - std::cout << " memory_rw = " << trace.get(C::memory_rw, memory_row) << std::endl; - std::cout << " memory_sel = " << trace.get(C::memory_sel, memory_row) << std::endl; - std::cout << " memory_sel_get_contract_instance_exists_write = " - << trace.get(C::memory_sel_get_contract_instance_exists_write, memory_row) << std::endl; - - // get_contract_instance relation should PASS (this is the vulnerability) - std::cout << "\nget_contract_instance relation: "; - check_relation(trace); - std::cout << "PASSED (vulnerability confirmed)" << std::endl; - - // ========== STEP 6: Verify attack succeeded ========== - // The attack succeeds because: - // 1. get_contract_instance relation PASSED (no constraint prevents ghost rows from setting is_valid_member_enum=1) - // 2. The source tuple values match a legitimate memory destination tuple - // 3. The permutation would match if we ran the full permutation check - - // Verify tuples match - // Source: clk, space_id, dst_offset, instance_exists, exists_tag, is_valid_member_enum - // Dest: memory_clk, memory_space_id, memory_address, memory_value, memory_tag, memory_rw - bool tuples_match = - (trace.get(C::get_contract_instance_clk, ghost_row) == trace.get(C::memory_clk, memory_row)) && - (trace.get(C::get_contract_instance_space_id, ghost_row) == trace.get(C::memory_space_id, memory_row)) && - (trace.get(C::get_contract_instance_dst_offset, ghost_row) == trace.get(C::memory_address, memory_row)) && - (trace.get(C::get_contract_instance_instance_exists, ghost_row) == trace.get(C::memory_value, memory_row)) && - (trace.get(C::get_contract_instance_exists_tag, ghost_row) == trace.get(C::memory_tag, memory_row)) && - (trace.get(C::get_contract_instance_is_valid_member_enum, ghost_row) == - trace.get(C::memory_rw, memory_row)); // rw=1 for writes - - std::cout << "\nTuples match: " << (tuples_match ? "YES" : "NO") << std::endl; - - // Attack succeeds if: - // 1. get_contract_instance relation passed (no constraint prevents ghost row) - // 2. Tuples match (permutation would match) - bool attack_succeeded = tuples_match; - - std::cout << "\n=== ATTACK RESULT ===" << std::endl; - if (attack_succeeded) { - std::cout << "CRITICAL: Ghost row injection attack SUCCEEDED!" << std::endl; - std::cout << "Attacker can inject arbitrary memory writes via get_contract_instance." << std::endl; - std::cout << "\nRequired fix in get_contract_instance.pil:" << std::endl; - std::cout << " #[IS_VALID_MEMBER_ENUM_REQUIRES_SEL]" << std::endl; - std::cout << " is_valid_member_enum * (1 - sel) = 0;" << std::endl; - } else { - std::cout << "Attack blocked." << std::endl; - } - - // Test documents vulnerability - we expect the attack to succeed until fixed - EXPECT_TRUE(attack_succeeded) << "Ghost row injection attack should succeed (vulnerability exists)"; + // The fix: is_valid_writes_in_bounds * (1 - sel) = 0 should cause the relation check to fail + // (in conjunction with WRITES_OUT_OF_BOUNDS * is_valid_member_enum = 0) + EXPECT_THROW_WITH_MESSAGE(check_relation(trace), "IS_VALID_WRITES_IN_BOUNDS_REQUIRES_SEL"); } } // namespace diff --git a/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/to_radix.test.cpp b/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/to_radix.test.cpp index a145a521397e..f3f454269cc9 100644 --- a/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/to_radix.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm2/constraining/relations/to_radix.test.cpp @@ -1013,13 +1013,10 @@ TEST(ToRadixMemoryConstrainingTest, ComplexTest) // Ghost Row Injection Vulnerability Tests // ===================================================================== // These tests verify that ghost rows (sel=0) cannot fire permutations. -// The vulnerability: sel_should_write_mem is only constrained via start -// row assignment and NOT_LAST propagation, but NOT_LAST = sel * (1 - LATCH_CONDITION). -// When sel=0, sel_should_write_mem is unconstrained, allowing ghost rows -// to fire the #[WRITE_MEM] permutation. +// The fix: sel_should_write_mem * (1 - sel) = 0 ensures sel_should_write_mem +// is forced to 0 when sel=0, preventing ghost rows from firing permutations. // Test that ghost rows (sel=0) cannot set sel_should_write_mem=1 -// This verifies the fix: sel_should_write_mem * (1 - sel) = 0 TEST(ToRadixMemoryConstrainingTest, NegativeGhostRowMemoryWrite_RelationsOnly) { // Try to create a ghost row (sel=0) with sel_should_write_mem=1 @@ -1041,87 +1038,40 @@ TEST(ToRadixMemoryConstrainingTest, NegativeGhostRowMemoryWrite_RelationsOnly) // The fix: sel_should_write_mem * (1 - sel) = 0 // When sel=0 and sel_should_write_mem=1: 1 * (1-0) = 1 != 0 -> FAILS - // TODO: Uncomment once fix is applied to PIL - // EXPECT_THROW_WITH_MESSAGE(check_relation(trace), - // "SEL_SHOULD_WRITE_MEM_REQUIRES_SEL"); - - // For now, this test documents the vulnerability: - // The relation PASSES when it should FAIL (no constraint prevents this) - check_relation(trace); + EXPECT_THROW_WITH_MESSAGE(check_relation(trace), "SEL_SHOULD_WRITE_MEM_REQUIRES_SEL"); } -// Also test sel_should_decompose which gates the INPUT_OUTPUT_TO_RADIX lookup -TEST(ToRadixMemoryConstrainingTest, NegativeGhostRowDecomposeLookup_RelationsOnly) -{ - // Try to create a ghost row (sel=0) with sel_should_decompose=1 - // which would fire the #[INPUT_OUTPUT_TO_RADIX] lookup - // - // Note: Must satisfy derived constraint: - // limb_index_to_lookup = sel_should_decompose * (num_limbs - 1) - TestTraceContainer trace({ - { - { C::precomputed_first_row, 1 }, - }, - { - { C::to_radix_mem_sel, 0 }, // Ghost row: gadget not active - { C::to_radix_mem_sel_should_decompose, 1 }, // Try to fire lookup anyway - { C::to_radix_mem_value_to_decompose, 1337 }, - { C::to_radix_mem_num_limbs, 4 }, // Set num_limbs for derived constraint - { C::to_radix_mem_limb_index_to_lookup, 3 }, // Must be: sel_should_decompose * (num_limbs - 1) = 1 * 3 = 3 - { C::to_radix_mem_radix, 10 }, - { C::to_radix_mem_limb_value, 7 }, - { C::to_radix_mem_value_found, 1 }, - }, - }); - - // The fix: sel_should_decompose * (1 - sel) = 0 - // When sel=0 and sel_should_decompose=1: 1 * (1-0) = 1 != 0 -> FAILS - // TODO: Uncomment once fix is applied to PIL - // EXPECT_THROW_WITH_MESSAGE(check_relation(trace), - // "SEL_SHOULD_DECOMPOSE_REQUIRES_SEL"); - - // For now, this test documents the vulnerability: - // The relation PASSES when it should FAIL (no constraint prevents this) - check_relation(trace); -} - -// ===================================================================== -// Full Attack Test with All Traces -// ===================================================================== -// This test demonstrates a complete ghost row injection attack: +// Test that the fix blocks ghost row injection attacks with full traces. +// Attack pattern: // 1. Create legitimate memory WRITE events (destination side) // 2. Build memory trace from those events -// 3. Inject ghost to_radix_mem row at matching clk (source side) -// 4. Verify the permutation matches - attack succeeds! -// -// This follows the pattern from PR #19470 (NegativeFullAttackWithAllTraces). -TEST(ToRadixMemoryConstrainingTest, NegativeFullAttackWithAllTraces) +// 3. Inject ghost to_radix_mem row with sel=0 but sel_should_write_mem=1 +// 4. The fix should cause the relation check to fail +TEST(ToRadixMemoryConstrainingTest, NegativeGhostRowInjectionBlocked) { TestTraceContainer trace; MemoryTraceBuilder memory_trace_builder; PrecomputedTraceBuilder precomputed_trace_builder; - // ========== STEP 1: Attacker-controlled values ========== + // Attacker-controlled values uint32_t malicious_clk = 42; uint16_t malicious_space_id = 1; MemoryAddress malicious_addr = 0xDEAD; - uint8_t malicious_limb_value = 0x99; // Arbitrary byte value + uint8_t malicious_limb_value = 0x99; MemoryTag malicious_tag = MemoryTag::U8; - // ========== STEP 2: Create legitimate memory events ========== - // These events will be processed by the MemoryTraceBuilder to create - // legitimate destination rows that the ghost source can match. + // Create legitimate memory events std::vector mem_events = { { .execution_clk = malicious_clk, - .mode = simulation::MemoryMode::WRITE, // rw = 1 + .mode = simulation::MemoryMode::WRITE, .addr = malicious_addr, .value = MemoryValue::from_tag(malicious_tag, malicious_limb_value), .space_id = malicious_space_id, }, }; - // ========== STEP 3: Build memory trace (destination side) ========== + // Build memory trace (destination side) precomputed_trace_builder.process_sel_range_8(trace); precomputed_trace_builder.process_sel_range_16(trace); precomputed_trace_builder.process_misc(trace, 1 << 16); @@ -1137,19 +1087,15 @@ TEST(ToRadixMemoryConstrainingTest, NegativeFullAttackWithAllTraces) } } - // ========== STEP 4: Inject ghost to_radix_mem row ========== - // Place ghost row at row 0 (precomputed_clk = 0 by default). - // For this attack, we need the source's execution_clk to match memory_clk. - // We set the ghost row values to match the memory destination. + // Inject ghost to_radix_mem row + // Ghost row: sel = 0, but sel_should_write_mem = 1 (attack attempt) uint32_t ghost_row = 0; trace.set(ghost_row, std::vector>{ { C::precomputed_first_row, 1 }, { C::precomputed_clk, ghost_row }, - // Ghost row: to_radix_mem_sel = 0, but sel_should_write_mem = 1 { C::to_radix_mem_sel, 0 }, - { C::to_radix_mem_sel_should_write_mem, 1 }, // Fires #[WRITE_MEM] permutation! - // Permutation tuple values - must match memory destination + { C::to_radix_mem_sel_should_write_mem, 1 }, { C::to_radix_mem_execution_clk, malicious_clk }, { C::to_radix_mem_space_id, malicious_space_id }, { C::to_radix_mem_dst_addr, malicious_addr }, @@ -1157,77 +1103,10 @@ TEST(ToRadixMemoryConstrainingTest, NegativeFullAttackWithAllTraces) { C::to_radix_mem_output_tag, static_cast(malicious_tag) }, }); - // Set the destination selector on the memory row to mark it as matching to_radix trace.set(C::memory_sel_to_radix_write, memory_row, 1); - // ========== STEP 5: Verify attack ========== - std::cout << "\n=== GHOST ROW INJECTION ATTACK TEST (to_radix_mem) ===" << std::endl; - std::cout << "Ghost to_radix_mem row at row " << ghost_row << " with sel=0, sel_should_write_mem=1" << std::endl; - std::cout << "Memory destination row at row " << memory_row << " with clk=" << malicious_clk << std::endl; - - // Debug: Print source tuple - std::cout << "\nSource tuple (to_radix_mem row " << ghost_row << "):" << std::endl; - std::cout << " execution_clk = " << trace.get(C::to_radix_mem_execution_clk, ghost_row) << std::endl; - std::cout << " space_id = " << trace.get(C::to_radix_mem_space_id, ghost_row) << std::endl; - std::cout << " dst_addr = " << trace.get(C::to_radix_mem_dst_addr, ghost_row) << std::endl; - std::cout << " limb_value = " << trace.get(C::to_radix_mem_limb_value, ghost_row) << std::endl; - std::cout << " output_tag = " << trace.get(C::to_radix_mem_output_tag, ghost_row) << std::endl; - std::cout << " sel_should_write_mem = " << trace.get(C::to_radix_mem_sel_should_write_mem, ghost_row) << std::endl; - - // Debug: Print destination tuple - std::cout << "\nDestination tuple (memory row " << memory_row << "):" << std::endl; - std::cout << " memory_clk = " << trace.get(C::memory_clk, memory_row) << std::endl; - std::cout << " memory_space_id = " << trace.get(C::memory_space_id, memory_row) << std::endl; - std::cout << " memory_address = " << trace.get(C::memory_address, memory_row) << std::endl; - std::cout << " memory_value = " << trace.get(C::memory_value, memory_row) << std::endl; - std::cout << " memory_tag = " << trace.get(C::memory_tag, memory_row) << std::endl; - std::cout << " memory_rw = " << trace.get(C::memory_rw, memory_row) << std::endl; - std::cout << " memory_sel = " << trace.get(C::memory_sel, memory_row) << std::endl; - std::cout << " memory_sel_to_radix_write = " << trace.get(C::memory_sel_to_radix_write, memory_row) << std::endl; - - // to_radix_mem relation should PASS (this is the vulnerability) - std::cout << "to_radix_mem relation: "; - check_relation(trace); - std::cout << "PASSED (vulnerability confirmed)" << std::endl; - - // ========== STEP 6: Verify attack succeeded ========== - // The attack succeeds because: - // 1. to_radix_mem relation PASSED (no constraint prevents ghost rows from setting sel_should_write_mem=1) - // 2. The source tuple values match a legitimate memory destination tuple - // 3. The permutation would match if we ran the full permutation check - // - // Note: The permutation is part of a complex multi-permutation in MemoryTraceBuilder, - // so we verify matching tuples directly instead of using check_multipermutation_interaction. - - // Verify tuples match - bool tuples_match = - (trace.get(C::to_radix_mem_execution_clk, ghost_row) == trace.get(C::memory_clk, memory_row)) && - (trace.get(C::to_radix_mem_space_id, ghost_row) == trace.get(C::memory_space_id, memory_row)) && - (trace.get(C::to_radix_mem_dst_addr, ghost_row) == trace.get(C::memory_address, memory_row)) && - (trace.get(C::to_radix_mem_limb_value, ghost_row) == trace.get(C::memory_value, memory_row)) && - (trace.get(C::to_radix_mem_output_tag, ghost_row) == trace.get(C::memory_tag, memory_row)) && - (trace.get(C::to_radix_mem_sel_should_write_mem, ghost_row) == trace.get(C::memory_rw, memory_row)); - - std::cout << "\nTuples match: " << (tuples_match ? "YES" : "NO") << std::endl; - - // Attack succeeds if: - // 1. to_radix_mem relation passed (no constraint prevents ghost row) - // 2. Tuples match (permutation would match) - bool attack_succeeded = tuples_match; - - std::cout << "\n=== ATTACK RESULT ===" << std::endl; - if (attack_succeeded) { - std::cout << "CRITICAL: Ghost row injection attack SUCCEEDED!" << std::endl; - std::cout << "Attacker can inject arbitrary memory writes via to_radix_mem." << std::endl; - std::cout << "\nRequired fix in to_radix_mem.pil:" << std::endl; - std::cout << " #[SEL_SHOULD_WRITE_MEM_REQUIRES_SEL]" << std::endl; - std::cout << " sel_should_write_mem * (1 - sel) = 0;" << std::endl; - } else { - std::cout << "Attack blocked." << std::endl; - } - - // Test documents vulnerability - we expect the attack to succeed until fixed - EXPECT_TRUE(attack_succeeded) << "Ghost row injection attack should succeed (vulnerability exists)"; + // The fix: sel_should_write_mem * (1 - sel) = 0 should cause the relation check to fail + EXPECT_THROW_WITH_MESSAGE(check_relation(trace), "SEL_SHOULD_WRITE_MEM_REQUIRES_SEL"); } } // namespace diff --git a/barretenberg/cpp/src/barretenberg/vm2/generated/relations/emit_unencrypted_log.hpp b/barretenberg/cpp/src/barretenberg/vm2/generated/relations/emit_unencrypted_log.hpp index 5d236d9f3dc1..e7a9736cfd6b 100644 --- a/barretenberg/cpp/src/barretenberg/vm2/generated/relations/emit_unencrypted_log.hpp +++ b/barretenberg/cpp/src/barretenberg/vm2/generated/relations/emit_unencrypted_log.hpp @@ -16,7 +16,7 @@ template class emit_unencrypted_logImpl { static constexpr std::array SUBRELATION_PARTIAL_LENGTHS = { 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 4, 5, 3, 3, 4, 3, 3, 3, 3, 4, 3, 3, 5, 3, - 5, 4, 4, 3, 2, 3, 3, 4, 3, 4, 4, 4, + 5, 4, 4, 3, 2, 3, 3, 4, 4, 4, 4, 4, 4, 4, 3, 5, 4, 4, 3, 4, 4, 3, 3, 3 }; template inline static bool skip(const AllEntities& in) @@ -46,6 +46,7 @@ template class emit_unencrypted_log : public Relation class emit_unencrypted_log : public Relation::accumulate(ContainerOverSubrelations& evals, static_cast(in.get(C::emit_unencrypted_log_is_write_memory_value_shift))); std::get<31>(evals) += (tmp * scaling_factor); } - { + { // SEL_SHOULD_READ_MEMORY_IS_SEL_AND_WRITE_MEM_AND_NO_ERR using View = typename std::tuple_element_t<32, ContainerOverSubrelations>::View; auto tmp = (static_cast(in.get(C::emit_unencrypted_log_sel_should_read_memory)) - - static_cast(in.get(C::emit_unencrypted_log_is_write_memory_value)) * + static_cast(in.get(C::emit_unencrypted_log_sel)) * + static_cast(in.get(C::emit_unencrypted_log_is_write_memory_value)) * (FF(1) - static_cast(in.get(C::emit_unencrypted_log_error_out_of_bounds)))); std::get<32>(evals) += (tmp * scaling_factor); } diff --git a/barretenberg/cpp/src/barretenberg/vm2/generated/relations/get_contract_instance.hpp b/barretenberg/cpp/src/barretenberg/vm2/generated/relations/get_contract_instance.hpp index 2f4f5910f639..7dbb64ff88c7 100644 --- a/barretenberg/cpp/src/barretenberg/vm2/generated/relations/get_contract_instance.hpp +++ b/barretenberg/cpp/src/barretenberg/vm2/generated/relations/get_contract_instance.hpp @@ -14,7 +14,7 @@ template class get_contract_instanceImpl { public: using FF = FF_; - static constexpr std::array SUBRELATION_PARTIAL_LENGTHS = { 3, 3, 5, 3, 4, 3, 3, 2, 2 }; + static constexpr std::array SUBRELATION_PARTIAL_LENGTHS = { 3, 3, 5, 3, 3, 4, 3, 3, 2, 2 }; template inline static bool skip(const AllEntities& in) { @@ -37,8 +37,9 @@ template class get_contract_instance : public Relation class get_contract_instance : public Relation::accumulate(ContainerOverSubrelations& evals static_cast(in.get(C::get_contract_instance_is_valid_member_enum)); std::get<3>(evals) += (tmp * scaling_factor); } - { // ERROR_AGGREGATION + { // IS_VALID_WRITES_IN_BOUNDS_REQUIRES_SEL using View = typename std::tuple_element_t<4, ContainerOverSubrelations>::View; + auto tmp = static_cast(in.get(C::get_contract_instance_is_valid_writes_in_bounds)) * + (FF(1) - static_cast(in.get(C::get_contract_instance_sel))); + std::get<4>(evals) += (tmp * scaling_factor); + } + { // ERROR_AGGREGATION + using View = typename std::tuple_element_t<5, ContainerOverSubrelations>::View; auto tmp = (static_cast(in.get(C::get_contract_instance_sel_error)) - static_cast(in.get(C::get_contract_instance_sel)) * (FF(1) - static_cast(in.get(C::get_contract_instance_is_valid_writes_in_bounds)) * static_cast(in.get(C::get_contract_instance_is_valid_member_enum)))); - std::get<4>(evals) += (tmp * scaling_factor); + std::get<5>(evals) += (tmp * scaling_factor); } { // SELECTED_MEMBER - using View = typename std::tuple_element_t<5, ContainerOverSubrelations>::View; + using View = typename std::tuple_element_t<6, ContainerOverSubrelations>::View; auto tmp = (static_cast(in.get(C::get_contract_instance_selected_member)) - (static_cast(in.get(C::get_contract_instance_is_deployer)) * static_cast(in.get(C::get_contract_instance_retrieved_deployer_addr)) + @@ -69,28 +75,28 @@ void get_contract_instanceImpl::accumulate(ContainerOverSubrelations& evals static_cast(in.get(C::get_contract_instance_retrieved_class_id)) + static_cast(in.get(C::get_contract_instance_is_init_hash)) * static_cast(in.get(C::get_contract_instance_retrieved_init_hash)))); - std::get<5>(evals) += (tmp * scaling_factor); + std::get<6>(evals) += (tmp * scaling_factor); } { - using View = typename std::tuple_element_t<6, ContainerOverSubrelations>::View; + using View = typename std::tuple_element_t<7, ContainerOverSubrelations>::View; auto tmp = (static_cast(in.get(C::get_contract_instance_member_write_offset)) - static_cast(in.get(C::get_contract_instance_is_valid_writes_in_bounds)) * (static_cast(in.get(C::get_contract_instance_dst_offset)) + FF(1))); - std::get<6>(evals) += (tmp * scaling_factor); + std::get<7>(evals) += (tmp * scaling_factor); } { - using View = typename std::tuple_element_t<7, ContainerOverSubrelations>::View; + using View = typename std::tuple_element_t<8, ContainerOverSubrelations>::View; auto tmp = (static_cast(in.get(C::get_contract_instance_exists_tag)) - static_cast(in.get(C::get_contract_instance_is_valid_writes_in_bounds)) * CView(constants_MEM_TAG_U1)); - std::get<7>(evals) += (tmp * scaling_factor); + std::get<8>(evals) += (tmp * scaling_factor); } { - using View = typename std::tuple_element_t<8, ContainerOverSubrelations>::View; + using View = typename std::tuple_element_t<9, ContainerOverSubrelations>::View; auto tmp = (static_cast(in.get(C::get_contract_instance_member_tag)) - static_cast(in.get(C::get_contract_instance_is_valid_writes_in_bounds)) * CView(constants_MEM_TAG_FF)); - std::get<8>(evals) += (tmp * scaling_factor); + std::get<9>(evals) += (tmp * scaling_factor); } } diff --git a/barretenberg/cpp/src/barretenberg/vm2/generated/relations/to_radix_mem.hpp b/barretenberg/cpp/src/barretenberg/vm2/generated/relations/to_radix_mem.hpp index 30e319bfd412..a1eefaff4bfa 100644 --- a/barretenberg/cpp/src/barretenberg/vm2/generated/relations/to_radix_mem.hpp +++ b/barretenberg/cpp/src/barretenberg/vm2/generated/relations/to_radix_mem.hpp @@ -14,9 +14,9 @@ template class to_radix_memImpl { public: using FF = FF_; - static constexpr std::array SUBRELATION_PARTIAL_LENGTHS = { 3, 3, 3, 3, 3, 4, 4, 4, 4, 4, 4, 3, 3, + static constexpr std::array SUBRELATION_PARTIAL_LENGTHS = { 3, 3, 3, 3, 3, 4, 4, 4, 4, 4, 4, 3, 3, 3, 3, 3, 5, 3, 5, 3, 5, 3, 6, 3, 4, 4, - 3, 3, 4, 4, 4, 4, 4, 4, 7, 4, 4, 3 }; + 3, 3, 4, 4, 4, 4, 4, 4, 7, 4, 4, 3, 3 }; template inline static bool skip(const AllEntities& in) { @@ -56,6 +56,7 @@ template class to_radix_mem : public Relation static constexpr size_t SR_LAST_ROW_NUM_LIMBS_ZERO = 33; static constexpr size_t SR_LAST_ROW_VALID_COMPUTATION = 34; static constexpr size_t SR_SEL_SHOULD_WRITE_MEM_CONTINUITY = 36; + static constexpr size_t SR_SEL_SHOULD_WRITE_MEM_REQUIRES_SEL = 37; static std::string get_subrelation_label(size_t index) { @@ -98,6 +99,8 @@ template class to_radix_mem : public Relation return "LAST_ROW_VALID_COMPUTATION"; case SR_SEL_SHOULD_WRITE_MEM_CONTINUITY: return "SEL_SHOULD_WRITE_MEM_CONTINUITY"; + case SR_SEL_SHOULD_WRITE_MEM_REQUIRES_SEL: + return "SEL_SHOULD_WRITE_MEM_REQUIRES_SEL"; } return std::to_string(index); } diff --git a/barretenberg/cpp/src/barretenberg/vm2/generated/relations/to_radix_mem_impl.hpp b/barretenberg/cpp/src/barretenberg/vm2/generated/relations/to_radix_mem_impl.hpp index e4fa8458b1b7..882f57290f00 100644 --- a/barretenberg/cpp/src/barretenberg/vm2/generated/relations/to_radix_mem_impl.hpp +++ b/barretenberg/cpp/src/barretenberg/vm2/generated/relations/to_radix_mem_impl.hpp @@ -285,14 +285,20 @@ void to_radix_memImpl::accumulate(ContainerOverSubrelations& evals, static_cast(in.get(C::to_radix_mem_sel_should_write_mem))); std::get<36>(evals) += (tmp * scaling_factor); } - { + { // SEL_SHOULD_WRITE_MEM_REQUIRES_SEL using View = typename std::tuple_element_t<37, ContainerOverSubrelations>::View; + auto tmp = static_cast(in.get(C::to_radix_mem_sel_should_write_mem)) * + (FF(1) - static_cast(in.get(C::to_radix_mem_sel))); + std::get<37>(evals) += (tmp * scaling_factor); + } + { + using View = typename std::tuple_element_t<38, ContainerOverSubrelations>::View; auto tmp = (static_cast(in.get(C::to_radix_mem_output_tag)) - static_cast(in.get(C::to_radix_mem_sel_should_write_mem)) * ((CView(constants_MEM_TAG_U1) - CView(constants_MEM_TAG_U8)) * static_cast(in.get(C::to_radix_mem_is_output_bits)) + CView(constants_MEM_TAG_U8))); - std::get<37>(evals) += (tmp * scaling_factor); + std::get<38>(evals) += (tmp * scaling_factor); } }