Repository navigation
fix(api): harden shielded proof generation and note scans #7096
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2868,6 +2868,9 @@ public SpendResult isSpend(NoteParameters noteParameters) throws | |
| note.getRcm().toByteArray()); | ||
| byte[] nf = baseNote.nullifier(ak, nk, voucherContainer.position()); | ||
|
|
||
| if (nf == null) { | ||
| throw new ZksnarkException("compute nullifier error"); | ||
| } | ||
| if (chainBaseManager.getNullifierStore().has(nf)) { | ||
| result = SpendResult.newBuilder() | ||
| .setResult(true) | ||
|
|
@@ -2893,7 +2896,9 @@ public BytesMessage createSpendAuthSig(SpendAuthSigParameters spendAuthSigParame | |
| spendAuthSigParameters.getAlpha().toByteArray(), | ||
| spendAuthSigParameters.getTxHash().toByteArray(), | ||
| result); | ||
| if (!JLibrustzcash.librustzcashSaplingSpendSig(spendSigParams)) { | ||
| throw new ZksnarkException("Spend signature generation failed"); | ||
| } | ||
|
|
||
| return BytesMessage.newBuilder().setValue(ByteString.copyFrom(result)).build(); | ||
| } | ||
|
|
@@ -2925,7 +2930,7 @@ public BytesMessage createShieldNullifier(NfParameters nfParameters) throws Zksn | |
| incrementalMerkleVoucherContainer.position(), | ||
| result); | ||
| if (!JLibrustzcash.librustzcashComputeNf(computeNfParams)) { | ||
| throw new ZksnarkException("compute nullifier error"); | ||
|
Collaborator
There was a problem hiding this comment. [SHOULD] Native nullifier failures still cannot reach this check. JLibrustzcash.librustzcashComputeNf() calls the SDK and unconditionally returns true. The SDK Java method returns void, and the JNI bridge discards Rust’s boolean return value. Rust can return false before writing the output buffer. The Java-side ComputeNfParams.valid() checks lengths and non-negative values, but does not cover all native semantic checks. Consequently, createShieldNullifier() can still return the untouched output buffer as a successful result. The new null check in isSpend() has the same limitation: Note.nullifier() returns null only when that wrapper returns false. Please preserve an explicit success/failure status through Rust → JNI → SDK Java → JLibrustzcash, or provide an equivalent reliable error contract. Add regression coverage across that boundary: the current failure tests mock JLibrustzcash itself, so they exercise the Wallet branches without detecting the status loss. An all-zero output check should not substitute for the success status.
Collaborator
Author
There was a problem hiding this comment. @3for Agreed. The current checks do not complete the fix: JNI discards the native return value, the SDK method returns void, and As noted in the PR description, this remains incomplete until the SDK/JNI/native changes are published and the dependency is upgraded. I'll wire |
||
| } | ||
|
|
||
| return BytesMessage.newBuilder() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[NIT / SDK & Native Hardening] Handle invalid serialized ZIP32 keys without native panics.
The new
params.valid()checks validate buffer lengths but do not validate the serialized extended-key contents:Zip32XskDeriveParams.valid()checks the 169-byte buffer lengths, but native decoding ofxsk_parentstill uses ExtendedSpendingKey::read(...).expect(...).Zip32XfvkAddressParams.valid()checks buffer lengths, but native decoding ofxfvkstill uses ExtendedFullViewingKey::read(...).expect(...).Consequently, correctly sized buffers containing invalid serialized keys can still trigger a native panic during decoding.
I found no production callers of these wrappers in the current java-tron tree, so this can be tracked as a follow-up rather than blocking this PR.
Before accepting untrusted extended keys through these interfaces, please replace the native
expect()calls with controlled error handling and propagate failures safely through JNI and the SDK. The derive API currently returnsvoid, so its failure-reporting mechanism also needs to be addressed.Please add regression tests using correctly sized but invalid serialized XSK/XFVK values, verifying that decoding failures are handled without native panics.
The absence of production callers in java-tron does not establish whether other SDK consumers expose these functions to untrusted inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@3for Agreed. The length checks do not validate serialized key contents, so this remains incomplete, as noted in the PR description.
I'll address it in a separate SDK/native follow-up: replace both decoding
expect()calls with controlled error handling, propagate failures through JNI and Java—including an explicit failure contract for XSK derivation—and add regression tests using correctly sized but invalid XSK/XFVK inputs. Other SDK consumers also need to be considered, even though these wrappers currently have no production callers in java-tron.