diff --git a/bridge/ghidra/ReaGhidraBridge.java b/bridge/ghidra/ReaGhidraBridge.java index ef11ecec3..0fa3895a6 100644 --- a/bridge/ghidra/ReaGhidraBridge.java +++ b/bridge/ghidra/ReaGhidraBridge.java @@ -1007,6 +1007,9 @@ public final class ReaGhidraBridge extends HeadlessScript { Address entry = function.getEntryPoint(); if (function.isExternal() || !currentProgram.getMemory().contains(entry)) throw new RequestFailure("invalid_request", "Annotations require a local function entry: " + canonicalAddress(entry)); + for (String field : List.of("name", "comment", "inline_comment")) { + if (params.has(field)) validateAnnotationText(requireText(params, field), field); + } int transaction = currentProgram.startTransaction("REA function annotations"); boolean commit = false; try { @@ -1065,6 +1068,21 @@ public final class ReaGhidraBridge extends HeadlessScript { return value.getAsString(); } + private static void validateAnnotationText(String text, String field) { + for (int index = 0; index < text.length(); index++) { + char character = text.charAt(index); + if (character == 0) + throw new RequestFailure("invalid_request", "Annotation " + field + " contains NUL at UTF-16 index " + index + "; Ghidra's decompiler cannot marshal NUL text"); + if (Character.isHighSurrogate(character) && index + 1 < text.length() && + Character.isLowSurrogate(text.charAt(index + 1))) { + index++; + continue; + } + if (Character.isSurrogate(character)) + throw new RequestFailure("invalid_request", "Annotation " + field + " contains an unpaired Unicode surrogate at UTF-16 index " + index + "; Ghidra cannot preserve it as annotation text"); + } + } + private JsonObject analyzeFunction(JsonObject params) throws Exception { requireKeys(params, Set.of("procedure")); Function function = resolveProcedure(requireString(params, "procedure")); @@ -3228,7 +3246,16 @@ public final class ReaGhidraBridge extends HeadlessScript { private static void writeResponse(BufferedWriter writer, JsonObject response) throws IOException { String encoded = GSON.toJson(response); - writer.write(encoded); + // Gson leaves UTF-16 surrogates literal. Escape them in JSON so even + // diagnostics quoting ill-formed caller text remain lossless UTF-8. + int start = 0; + for (int index = 0; index < encoded.length(); index++) { + if (!Character.isSurrogate(encoded.charAt(index))) continue; + writer.write(encoded, start, index - start); + writer.write("\\u" + Integer.toHexString(encoded.charAt(index))); + start = index + 1; + } + writer.write(encoded, start, encoded.length() - start); writer.newLine(); writer.flush(); } diff --git a/docs/native-investigation.md b/docs/native-investigation.md index 533c8c787..4f61375bf 100644 --- a/docs/native-investigation.md +++ b/docs/native-investigation.md @@ -257,7 +257,10 @@ protected runtime DACLs, and handle-based path admission. See [Windows Ghidra P0 [issue #527](https://github.com/morluto/rea/issues/527). On Linux and macOS, `annotate_native_function` atomically edits a function name and entry comments in the ephemeral database, returning refreshed analysis -without changing executable bytes. Windows P0 remains read-only. Ghidra has no +without changing executable bytes. Annotation text must contain no NUL or +unpaired Unicode surrogate; a rejection identifies the field and UTF-16 index +and leaves every annotation unchanged. CRLF, supplementary Unicode characters, +and combining characters are preserved. Windows P0 remains read-only. Ghidra has no GUI authority, and REA never falls back automatically to Hopper. - `npm run verify:ghidra`: host-native debug/stripped targets, native type layout, diff --git a/docs/testing.md b/docs/testing.md index 6f3b53ff5..8221305c3 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -249,8 +249,12 @@ direct versus targetless calls, byte-read completeness, invalid input diagnostic CLI/MCP parity, atomic annotation rollback, refreshed inventories, unchanged executable bytes, and discarded edits after reopen. A deliberately long temporary path exercises private Unix socket allocation and cleanup, including cancellation -after a real headless process launches. Malformed annotation -readback remains a separate SDK/provider integration case; success from a real +after a real headless process launches. Native annotation probes reject NUL and +unpaired Unicode surrogates without partial edits or a broken bridge, preserve +supported Unicode and control text, and check lossless malformed-text diagnostics. +Memory-to-file mapping is checked against original artifact bytes. Malformed +annotation readback, memory completeness, and inventory data remain separate +SDK/provider integration cases; success from a real provider cannot establish rejection of a contradictory provider response. The Linux switch lane checks dense, sparse-with-holes, shared-body, nonzero, diff --git a/scripts/lib/real-ghidra-boundaries.mjs b/scripts/lib/real-ghidra-boundaries.mjs index 309eb1a24..eb8986c54 100644 --- a/scripts/lib/real-ghidra-boundaries.mjs +++ b/scripts/lib/real-ghidra-boundaries.mjs @@ -97,6 +97,16 @@ export async function verifyGhidraBoundaries( assert.equal(baseline.status, "decoded"); const bytes = await call("read_bytes", { address, length: baseline.length }); assert.equal(bytes.bytes_hex, baseline.bytes); + assert.equal((await call("read_bytes", { address })).requested_bytes, 256); + const mapped = await call("address_to_file_offset", { address }); + assert.equal(mapped.address, address); + assert.equal( + (await readFile(target.path)) + .subarray(mapped.file_offset, mapped.file_offset + baseline.length) + .toString("hex"), + bytes.bytes_hex, + "Memory-to-file mapping must identify the bytes in the caller's artifact", + ); const largeRead = await call("read_bytes", { address, length: Number.MAX_SAFE_INTEGER, @@ -182,6 +192,28 @@ export async function verifyGhidraBoundaries( } const strings = await call("list_strings"); assert.ok(strings.length > 0); + assert.deepEqual( + await call("list_strings", { address: strings[0].address }), + [strings[0]], + ); + const marker = strings.find( + (item) => item.value === "REA_GHIDRA_INVENTORY_ENTRY", + ); + assert.ok(marker); + assert.ok( + ( + await call("search_strings", { pattern: marker.value.toLowerCase() }) + ).some((item) => item.address === marker.address), + ); + assert.equal( + ( + await call("search_strings", { + pattern: marker.value.toLowerCase(), + case_sensitive: true, + }) + ).some((item) => item.address === marker.address), + false, + ); assert.equal( (await call("inspect_native_instruction", { address: strings[0].address })) .status, @@ -218,6 +250,26 @@ export async function verifyGhidraBoundaries( { document: "rea_missing_program" }, /rea_missing_program.*active program/u, ); + // JSON permits escaped lone surrogates. Error replies must preserve them + // without letting the Java UTF-8 writer terminate the provider session. + await invalid( + "procedure_address", + { procedure: "missing_\ud800" }, + /missing_\\ud800/u, + ); + await invalid( + "list_procedures", + { document: "missing_\udfff" }, + /missing_\\udfff/u, + ); + for (const [operation, args] of [ + ["read_bytes", { address, length: 0 }], + ["list_procedures", { limit: 100 }], + ["list_documents", { extra: true }], + ["xrefs", {}], + ["procedure_info", { procedure: address, extra: true }], + ]) + await invalid(operation, args); for (const changes of [ {}, { name: "" }, @@ -296,6 +348,46 @@ export async function verifyGhidraBoundaries( ).some((item) => item.address === address), ); + for (const field of ["name", "comment", "inline_comment"]) { + for (const text of ["bad\0text", "bad\ud800text", "bad\udffftext"]) { + await invalid( + "annotate_native_function", + { + procedure: address, + name: "MUST_ROLL_BACK", + comment: "MUST ROLL BACK", + inline_comment: "MUST ROLL BACK", + [field]: text, + }, + new RegExp( + `Annotation ${field}.*(?:NUL|unpaired Unicode surrogate).*index`, + "u", + ), + ); + assert.deepEqual( + ( + await call("annotate_native_function", { + procedure: address, + name: changes.name, + }) + ).annotations, + updated.annotations, + "Invalid native text must leave every annotation unchanged", + ); + } + } + const unicodeText = + "CRLF\r\nastral 🧪 decomposed e\u0301 control \u0001\b\v\f\u001f noncharacter \ufffe\uffff"; + const unicode = await call("annotate_native_function", { + procedure: address, + comment: unicodeText, + inline_comment: unicodeText, + }); + assert.equal(unicode.annotations.comment, unicodeText); + assert.equal(unicode.annotations.inline_comment, unicodeText); + assert.ok(unicode.dossier.comments.some((item) => item.text === unicodeText)); + await call("annotate_native_function", changes); + await invalid( "annotate_native_function", { @@ -339,15 +431,17 @@ export async function verifyGhidraBoundaries( ]); assert.deepEqual(cliUpdated.annotations, updated.annotations); assert.deepEqual(cliUpdated.effects, updated.effects); - await call("annotate_native_function", { - procedure: address, - name: "0xordinary", - }); - assert.equal( - await call("procedure_address", { procedure: "0xordinary" }), - address, - ); - assert.equal(await call("address_name", { address }), "0xordinary"); + for (const renamed of ["0xordinary", "probe::qualified", "🧪probe"]) { + await call("annotate_native_function", { + procedure: address, + name: renamed, + }); + assert.equal( + await call("procedure_address", { procedure: renamed }), + address, + ); + assert.equal(await call("address_name", { address }), renamed); + } await call("close_binary"); await assert.rejects(access(socketRoot), { code: "ENOENT" }); await assert.rejects(access(runtimeRoot), { code: "ENOENT" }); @@ -372,6 +466,8 @@ export async function verifyGhidraBoundaries( rejected_calls: rejectedCalls, cli_mcp_parity: true, mutation_rollback: true, + annotation_native_text_validation: true, + lossless_unicode_transport: true, source_immutable: true, reopen_discards_edits: true, long_tmpdir_private_socket_cleanup: true, diff --git a/src/ghidra/GhidraFunctionValues.test.ts b/src/ghidra/GhidraFunctionValues.test.ts index f50a3759a..d93f22f2b 100644 --- a/src/ghidra/GhidraFunctionValues.test.ts +++ b/src/ghidra/GhidraFunctionValues.test.ts @@ -3,7 +3,6 @@ import { describe, expect, it } from "vitest"; import type { JsonValue } from "../domain/jsonValue.js"; import { functionDossierSchema } from "../domain/hopperValues.js"; import { - parseGhidraFunctionInput, parseGhidraFunctionResult, type GhidraFunctionOperation, } from "./GhidraFunctionValues.js"; @@ -16,51 +15,6 @@ import { ghidraReferenceEdge, } from "../domain/ghidraValues.fixture.js"; -describe("Ghidra function-analysis boundary values", () => { - it("defaults inputs and rejects undeclared or implicit addresses", () => { - expect( - parseGhidraFunctionInput("procedure_info", { procedure: "main" }), - ).toEqual({ - ok: true, - value: { document: null, procedure: "main" }, - }); - expect( - parseGhidraFunctionInput("procedure_references", { procedure: "main" }), - ).toEqual({ - ok: true, - value: { - document: null, - procedure: "main", - direction: "outgoing", - }, - }); - expect( - parseGhidraFunctionInput("analyze_function", { procedure: "main" }), - ).toMatchObject({ - ok: true, - value: { procedure: "main" }, - }); - expect( - parseGhidraFunctionInput("read_function_instructions", { - procedure: "main", - }), - ).toEqual({ - ok: true, - value: { document: null, procedure: "main" }, - }); - expect(parseGhidraFunctionInput("xrefs", {})).toMatchObject({ - ok: false, - error: { _tag: "AnalysisInputError" }, - }); - expect( - parseGhidraFunctionInput("procedure_info", { - procedure: "main", - extra: true, - }), - ).toMatchObject({ ok: false, error: { _tag: "AnalysisInputError" } }); - }); -}); - describe("Ghidra function-analysis result values", () => { it("parses provider-classified function facts and reference kinds", () => { expect( diff --git a/src/ghidra/GhidraInventoryValues.test.ts b/src/ghidra/GhidraInventoryValues.test.ts index c0ebcfa37..8a882e27b 100644 --- a/src/ghidra/GhidraInventoryValues.test.ts +++ b/src/ghidra/GhidraInventoryValues.test.ts @@ -1,120 +1,6 @@ import { describe, expect, it } from "vitest"; -import { - isGhidraInventoryOperation, - parseGhidraInventoryInput, - parseGhidraInventoryResult, -} from "./GhidraInventoryValues.js"; - -describe("Ghidra inventory boundary values", () => { - it("admits explicit byte reads and source offset mapping without invented caps", () => { - expect( - parseGhidraInventoryInput("read_bytes", { address: "0x10000" }), - ).toEqual({ - ok: true, - value: { address: "0x10000", document: null, length: 256 }, - }); - expect( - parseGhidraInventoryInput("read_bytes", { - address: "0x10000", - length: 4097, - }).ok, - ).toBe(true); - expect( - parseGhidraInventoryInput("read_bytes", { address: "0x10000", length: 0 }) - .ok, - ).toBe(false); - expect( - parseGhidraInventoryInput("address_to_file_offset", { - address: "HEADER:0x1", - }).ok, - ).toBe(true); - }); - it("checks byte length and completeness before accepting memory Evidence", () => { - const result = { - address: "0x10000", - requested_bytes: 4, - returned_bytes: 2, - bytes_hex: "0410", - complete: false, - }; - expect(parseGhidraInventoryResult("read_bytes", result)).toEqual({ - ok: true, - value: result, - }); - for (const change of [ - { complete: true }, - { returned_bytes: 4 }, - { bytes_hex: "04" }, - { requested_bytes: 1 }, - ]) - expect( - parseGhidraInventoryResult("read_bytes", { ...result, ...change }).ok, - ).toBe(false); - expect( - parseGhidraInventoryResult("address_to_file_offset", { - address: "0x10000", - file_offset: -1, - }).ok, - ).toBe(false); - }); - it("accepts complete inventories and searches without page controls", () => { - expect(parseGhidraInventoryInput("list_procedures", {})).toEqual({ - ok: true, - value: { document: null }, - }); - expect( - parseGhidraInventoryInput("search_strings", { pattern: "needle" }), - ).toEqual({ - ok: true, - value: { - pattern: "needle", - mode: "literal", - case_sensitive: false, - document: null, - }, - }); - expect( - parseGhidraInventoryInput("list_procedures", { limit: 100 }), - ).toMatchObject({ ok: false, error: { _tag: "AnalysisInputError" } }); - expect( - parseGhidraInventoryInput("list_documents", { extra: true }), - ).toMatchObject({ ok: false, error: { _tag: "AnalysisInputError" } }); - }); - - it("keeps the admitted operation set closed", () => { - expect(isGhidraInventoryOperation("list_names")).toBe(true); - expect(isGhidraInventoryOperation("goto_address")).toBe(false); - expect(isGhidraInventoryOperation("set_comment")).toBe(false); - }); - - it("returns complete typed inventory arrays", () => { - const procedures = Array.from({ length: 700 }, (_, index) => ({ - address: `0x${(0x401000 + index).toString(16)}`, - value: `sub_${index}`, - procedure: { external: false, thunk: false, thunk_target: null }, - })); - expect(parseGhidraInventoryResult("list_procedures", procedures)).toEqual({ - ok: true, - value: procedures, - }); - }); - - it("rejects malformed inventory items", () => { - expect( - parseGhidraInventoryResult("list_procedures", [ - { - address: "00401000", - value: "main", - procedure: { external: false, thunk: false, thunk_target: null }, - }, - ]), - ).toMatchObject({ ok: false, error: { _tag: "AnalysisOutputError" } }); - expect( - parseGhidraInventoryResult("search_strings", [{ address: "0x1000" }]), - ).toMatchObject({ ok: false, error: { _tag: "AnalysisOutputError" } }); - }); -}); +import { parseGhidraInventoryResult } from "./GhidraInventoryValues.js"; // Wire-boundary fixture only; real-provider acceptance belongs to the DOS verifier. describe("containing-procedure complete body evidence", () => { diff --git a/src/ghidra/GhidraProviderCapabilities.ts b/src/ghidra/GhidraProviderCapabilities.ts index 402f6d1e8..2d3170083 100644 --- a/src/ghidra/GhidraProviderCapabilities.ts +++ b/src/ghidra/GhidraProviderCapabilities.ts @@ -46,6 +46,7 @@ export const limitationsFor = (operation: string): readonly string[] => { return [ ...common, "Names use Ghidra USER_DEFINED source. Name writes preserve the existing namespace; readback uses the fully qualified name. Regular comments map to PRE and inline comments to EOL at the exact function entry. Changes commit together after readback and refreshed analysis; failure rolls them all back.", + "Annotation text rejects NUL and unpaired Unicode surrogates before mutation, with the field and UTF-16 index in the error. Supported Unicode and line endings are preserved.", "Metadata edits invalidate immutable analysis snapshots and are discarded on close. CLI returns the updated dossier before session cleanup; this is not a saved Ghidra project.", ]; case "inspect_native_load_image": diff --git a/tests/boundary/mcp/ghidraInventoryMcp.test.ts b/tests/boundary/mcp/ghidraInventoryMcp.test.ts new file mode 100644 index 000000000..dc3d600c8 --- /dev/null +++ b/tests/boundary/mcp/ghidraInventoryMcp.test.ts @@ -0,0 +1,77 @@ +import { expect, it } from "vitest"; +import { jsonValueSchema } from "../../../src/domain/jsonValue.js"; +import { ok } from "../../../src/domain/result.js"; +import { connectGhidraMcp } from "./ghidraMcpHarness.js"; + +it("rejects contradictory provider memory and inventory data before emitting MCP Evidence", async () => { + const bytes = { + address: "0x401000", + requested_bytes: 4, + returned_bytes: 2, + bytes_hex: "0410", + complete: false, + }; + let output = jsonValueSchema.parse(bytes); + const harness = await connectGhidraMcp("ghidra-invalid-inventory", () => + Promise.resolve(ok(output)), + ); + try { + const accepted = await harness.mcp.callTool({ + name: "read_bytes", + arguments: { address: bytes.address, length: 4 }, + }); + expect(accepted.isError).not.toBe(true); + expect(accepted.structuredContent).toMatchObject({ result: bytes }); + const cases = [ + ...[ + { complete: true }, + { returned_bytes: 4 }, + { bytes_hex: "04" }, + { requested_bytes: 1 }, + ].map((change, index) => ({ + name: "read_bytes", + arguments: { + address: `0x${(0x401004 + index * 4).toString(16)}`, + length: 4, + }, + output: { ...bytes, ...change }, + })), + { + name: "address_to_file_offset", + arguments: { address: bytes.address }, + output: { address: bytes.address, file_offset: -1 }, + }, + { + name: "list_procedures", + arguments: {}, + output: [ + { + address: "00401000", + value: "main", + procedure: { external: false, thunk: false, thunk_target: null }, + }, + ], + }, + { + name: "search_strings", + arguments: { pattern: "needle" }, + output: [{ address: "0x401000" }], + }, + ]; + for (const probe of cases) { + output = jsonValueSchema.parse(probe.output); + const rejected = await harness.mcp.callTool({ + name: probe.name, + arguments: probe.arguments, + }); + expect(rejected.isError, probe.name).toBe(true); + expect(rejected.structuredContent).toMatchObject({ + error: { code: "unreadable_output" }, + }); + expect(rejected.structuredContent).not.toHaveProperty("evidence_id"); + expect(rejected.structuredContent).not.toHaveProperty("result"); + } + } finally { + await harness.close(); + } +});