Skip to content

[ALPHANET] P256 #29

Open
dangell7 wants to merge 8 commits into
developfrom
feature-p256
Open

[ALPHANET] P256 #29
dangell7 wants to merge 8 commits into
developfrom
feature-p256

Conversation

@dangell7

Copy link
Copy Markdown
Member

High Level Overview of Change

Context of Change

API Impact

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

@dangell7

dangell7 commented Apr 14, 2026

Copy link
Copy Markdown
Member Author

Love the direction here — the p256 plumbing (new KeyType::p256, 65-byte PublicKey buffer, sfPasskey / sfPasskeySignature inner objects, base64url_decode + sha256 helpers) is exactly the scaffolding session keys would also need, and I think it's worth considering a small generalization now so we don't end up maintaining two near-identical ledger objects later.

Why this matters for session keys. A session key can't meaningfully be issued server-side — checkSign runs inside the transactor during consensus, so anything a server "issues" on its own is just delegated custody of the master key, not a cryptographically enforced session. It has to be protocol-side to be real. Today the closest thing is Permission Delegation + a disposable account acting as the session key, but that costs reserve, has no native expiry, and scopes only by txn type.

The clean version, as a protocol feature, would mirror this PR exactly: a per-account ledger object listing {PublicKey, KeyType, NotAfter, AllowedTxTypes, optional SpendingLimit} entries, a set/rotate/revoke transaction, and an extension to checkSign that accepts any unexpired, in-scope entry. Raw ECDSA over the tx hash — no WebAuthn authenticatorData / clientDataJSON wrapping needed, so cheaper to verify and smaller on the wire than the passkey path.

Concrete suggestion for this PR. Rather than ship PasskeyList with a fixed {PasskeyID, PublicKey} shape and add a parallel SessionKeyList later, consider adding two optional fields to sfPasskey now:

  • sfExpiration (UINT32, optional) — NotAfter timestamp, absent = no expiry
  • sfAllowedTxTypes or a flags field (optional) — scope restriction, absent = unrestricted

With those, a "passkey" is just a session key with no expiry and no scope, and both use cases share one ledger object, one set-transaction, and one checkSign code path. The WebAuthn-specific verification (hashing authenticatorData || SHA256(clientDataJSON)) can stay gated on whether sfAuthenticatorData is present in the signature envelope, so raw-ECDSA session-key signatures just skip that branch.

Happy to open a follow-up issue tracking the session-key extension specifically if that's easier than expanding scope here — but locking the inner object layout before the amendment ships is the main thing, since adding fields to sfPasskey post-amendment is painful.

@dangell7 dangell7 changed the title Feature p256 [ALPHANET] P256 Apr 22, 2026
Comment thread include/xrpl/basics/Number.h Outdated

// logarithm with base 10
Number
log10(Number const& value, int iterations = 50);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The iterations parameter is int while every other count parameter in this header (power's n, root's d) is unsigned. When a caller passes 0 or a negative value, the inner Taylor-series loop for (int i = 1; i <= iterations; ++i) in the ln helper never executes — sum stays at zero and the function silently returns a wrong result (exponent * LN2 / LN10) with no exception and no error signal. This is a latent correctness hazard: any future caller that derives iterations from user-supplied data (e.g., a transaction field or RPC parameter) will get silently incorrect logarithm values. Change int iterations to unsigned iterations, add a precondition check that it is at least 1, and align with the unsigned convention used by all neighboring functions.

Suggested fix

Change the parameter to unsigned iterations = 50 and add a guard if (iterations == 0) throw std::domain_error("iterations must be >= 1"); (or XRPL_VERIFY) at the top of the implementation, matching the pattern used by power and root.

Comment thread include/xrpl/core/ServiceRegistry.h Outdated
virtual DatabaseCon&
getWalletDB() = 0;

virtual Fees

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getFees() sources all six fields — including extensionComputeLimit, extensionSizeLimit, and gasPrice — from local node config (config_->FEES), not from the on-ledger FeeSettings SLE, even though log messages inside EscrowCreate and EscrowFinish describe this as 'WASM runtime deactivated by fee voting.' The ServiceRegistry::registry field is a public member of ApplyContext, so any doApply code path — including WASM host functions in ContractContext — can call registry.get().getFees() and receive locally-configured values that differ between validators. If any host function gates WASM execution limits using getFees() during doApply, validators with different local configs will apply the same transaction with different execution envelopes, breaking determinism and causing a consensus fork.

Suggested fix

Expose WASM execution parameters through the on-ledger FeeSettings SLE so view.fees() returns them, giving all consensus-critical callers the same network-agreed values. Rename getFees() to something like getLocalNodeFeeConfig() to make the semantics unambiguous and prevent accidental misuse in preclaim/doApply. Add a comment or assertion preventing calls to getFees() outside preflight contexts.

Comment thread include/xrpl/core/ServiceRegistry.h Outdated
#include <xrpl/basics/Log.h>
#include <xrpl/basics/SHAMapHash.h>
#include <xrpl/basics/TaggedCache.h>
<<<<<<< HEAD

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file contains unresolved git conflict markers (<<<<<<< HEAD, =======, >>>>>>> origin/develop) at lines 7-12 in the include block. These are not valid C++ and will cause a compilation error in every translation unit that includes this header — which covers the entire Application, all tx transactors using ctx.registry, and the test harness. The correct resolution is to keep all three includes: <xrpl/basics/base_uint.h>, <xrpl/beast/utility/Journal.h> (both already transitively available, but harmless), and <xrpl/protocol/Fees.h> (genuinely required since Fees is returned by value from getFees() and is not reachable through any pre-existing transitive include chain).

Suggested fix

Resolve the conflict by replacing the conflict block with the union of both sides:
#include <xrpl/basics/base_uint.h>
#include <xrpl/beast/utility/Journal.h>
#include <xrpl/protocol/Fees.h>
Do NOT drop <xrpl/protocol/Fees.h> — it is the only declaration path for the Fees return type used by getFees() on line 251.

Comment thread cfg/xrpld-example.cfg Outdated
# If this parameter is unspecified, xrpld will use an internal
# default. Don't change this without understanding the consequences.
#
# Example:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gas_price config entry has two compounding problems: the type annotation says <bytes> but the runtime struct explicitly documents the unit as micro-drops, creating a plausible operator misreading that leads to silent misconfiguration. More critically, the implementation (confirmed in Config.cpp and both calculateBaseFee paths) accepts gas_price = 0 without any guard — when gasPrice is 0, WASM computation costs exactly zero extra drops, since the fee formula is allowance * gasPrice / MICRO_DROPS_PER_DROP. The EscrowFinish::preflight guard only checks extensionComputeLimit == 0 for the temTEMP_DISABLED path, not gasPrice == 0, so WASM execution proceeds for free whenever both size and compute limits are active but gas price is zero. A validator operator who reads <bytes> and sets gas_price = 0 thinking it means "unlimited" will vote for free WASM gas; if a majority of UNL validators do likewise, the ledger-wide gasPrice drops to 0, allowing any user to submit EscrowFinish or ContractCall transactions with maximum sfComputationAllowance at only the base transaction fee, enabling resource exhaustion of all validator nodes. Add if (fees.gasPrice == 0) return temTEMP_DISABLED; to both preflight paths alongside the existing extensionComputeLimit == 0 check, and fix the annotation to <micro-drops>.

Suggested fix
  1. Fix the type annotation in the config doc from <bytes> to <micro-drops> and add a note that 0 disables gas cost enforcement. 2) In EscrowFinish::preflight, add if (fees.gasPrice == 0) return temTEMP_DISABLED; alongside the existing extensionComputeLimit == 0 check at line 97. 3) In ContractCall::preflight, add guards for extensionComputeLimit == 0, extensionSizeLimit == 0, and gasPrice == 0 returning temTEMP_DISABLED when sfComputationAllowance is present — currently the ContractCall preflight has no such checks at all. 4) Consider adding a minimum floor check in setupFeeVote to reject gas_price = 0 at config parse time.

Comment thread conan.lock Outdated
"rocksdb/10.5.1#4a197eca381a3e5ae8adf8cffa5aacd0%1765850186.86",
"re2/20251105#8579cfd0bda4daf0683f9e3898f964b4%1774398111.888",
"protobuf/6.33.5#d96d52ba5baaaa532f47bda866ad87a5%1774467363.12",
"openssl/3.6.2#4789bbf131b77d0515d15e094c8f697f%1778071755.506",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The secp256k1 library is being pinned to an older Conan recipe revision (timestamp 1770910500 vs the previous 1782338841) while keeping the same upstream version 0.7.1. In Conan, the recipe revision controls the exact build configuration applied to the upstream source — compile flags, enabled modules, optimization settings, and any patches. Since secp256k1 is used in rippled to verify every XRPL transaction signature (secp256k1_ecdsa_verify in PublicKey.cpp) and to sign with RFC6979 nonces (secp256k1_ecdsa_sign in SecretKey.cpp), any difference in the recipe that weakens constant-time guarantees or changes the endomorphism/GLV optimizations could introduce a timing side-channel against validator signing keys. The recipe for secp256k1 in rippled lives in the XRPLF fork of conan-center-index and is not directly auditable from this diff.

Suggested fix

Diff the two recipe revisions (b1f450b7f78a36fff75bb6934a356f3a vs 481881709eb0bdd0185a12b912bbe8ad) in the XRPLF/conan-center-index fork. Confirm that constant-time compilation flags (--enable-endomorphism, USE_SCALAR_10X26 vs USE_SCALAR_4X64, etc.) are identical between revisions, and that no security-relevant patches are present in the newer recipe but absent in the older one. If the newer recipe corrected a build configuration issue, do not roll it back.

Comment thread conan.lock Outdated
"boost/1.91.0#ea540ca2133d831b560036aa24dece3c%1782392419.475605",
"abseil/20250127.0#bb0baf1f362bc4a725a24eddd419b8f7%1782307147.395833"
"gtest/1.17.0#5224b3b3ff3b4ce1133cbdd27d53ee7d%1768312129.152",
"grpc/1.78.1#b1a9e74b145cc471bed4dc64dc6eb2c1%1774467387.342",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gRPC is being downgraded from 1.81.1 to 1.78.1. Rippled uses gRPC with TLS-protected server credentials and optional mTLS client certificate verification (see GRPCServer.cpp) to serve the XRPLedgerAPIService API. A downgrade of three minor versions in gRPC is a meaningful regression given that gRPC's C++ core has had HTTP/2 flow-control and server-side stream handling security fixes in the 1.78–1.81 range. Additionally, gRPC pulls in abseil, protobuf, and c-ares as transitive dependencies — all of which also changed revision hashes in this lockfile — meaning the full security profile of the 1.78.1 build may differ from what was previously reviewed. The grpc::InsecureServerCredentials() fallback path in GRPCServer.cpp already represents a TLS downgrade risk if misconfigured; adding gRPC library vulnerabilities compounds that surface.

Suggested fix

Revert the gRPC pin to 1.81.1. Review the gRPC C++ changelog and GitHub security advisories between 1.78.1 and 1.81.1 to enumerate what CVEs or security-relevant fixes are being abandoned. If 1.81.1 is incompatible with protobuf 6.33.5 or the new WASM/P256 additions, resolve the API incompatibility rather than downgrading the security library.

Comment thread include/xrpl/basics/Number.h Outdated
power(Number const& f, unsigned n);

// logarithm with base 10
Number

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The iterations parameter is publicly exposed in the header declaration with no upper-bound validation in the implementation. The ln helper it calls runs a tight Number-arithmetic loop for (int i = 1; i <= iterations; ++i) with no termination guard — passing INT_MAX would spin for ~2 billion iterations of expensive Number multiply/add operations, hanging the validator process. Currently the only caller is the WASM floatLogImpl which hardcodes the default of 50, but the parameter exists in the public header API and any future caller that routes user-controlled input into iterations (e.g., a WASM API extension exposing precision control) creates a direct DoS. The parameter should be either removed from the public API or validated with an upper bound before delegating to ln.

Suggested fix

Change the parameter type from int iterations = 50 to unsigned iterations = 50, and add if (iterations == 0) throw std::invalid_argument("iterations must be > 0"); (or an XRPL_VERIFY) at the top of the implementation, consistent with how domain errors are already handled for non-positive input values.

Comment thread include/xrpl/ledger/ApplyViewImpl.h Outdated
deliver_ = amount;
}

void

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ContractCall::preflight does not validate sfComputationAllowance against fees.extensionComputeLimit before the value reaches setGasUsed, while EscrowFinish::preflight correctly rejects any allowance exceeding that network-governance cap with temBAD_LIMIT. Since sfComputationAllowance is SoeRequired for ContractCall (always present, never optional), every ContractCall transaction can bypass the validator-voted compute limit entirely — an attacker paying proportional fees can force all validators to execute up to UINT32_MAX fuel units of WASM per transaction, potentially 4000x the network-intended ceiling. Add the same extensionComputeLimit guard to ContractCall::preflight that EscrowFinish already enforces: reject when fees.extensionComputeLimit == 0 (temTEMP_DISABLED), when allowance == 0, or when allowance > fees.extensionComputeLimit (temBAD_LIMIT).

Suggested fix

In ContractCall::preflight, mirror the sfComputationAllowance validation from EscrowFinish::preflight (lines 93-110): read fees.extensionComputeLimit from the registry, return temTEMP_DISABLED if the limit is zero, return temBAD_LIMIT if sfComputationAllowance is zero or exceeds the limit. This prevents governance bypass regardless of fee payment.

Comment thread include/xrpl/ledger/OpenViewSandbox.h Outdated
}

OpenView const&
view() const

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

view() returns a raw OpenView& pointing directly into *sandbox_, but both commit() and discard() unconditionally destroy and replace sandbox_ via unique_ptr reassignment. Any caller that stores this reference — including ApplyContext::openView() which propagates it directly, and code in Transactor that constructs child views from ctx_.openView() and holds them across multiple operations — has a dangling reference the moment discard() or commit() is called. While no live UAF exists in the current code paths (each call site re-fetches the reference rather than caching it), the API provides zero indication that the returned reference is invalidated on every commit/discard, making a UAF one editorial step away every time this class is extended. Either remove the raw-reference exposure or add a strong lifetime contract enforced by documentation and debug-mode assertions.

Suggested fix

Either (a) remove view() and openView() raw-reference accessors entirely and route all access through a stable ApplyView& (which ApplyContext already refreshes via optional::emplace on discard/finalize), or (b) add XRPL_ASSERT or debug-mode poison tokens to detect use of stale view references after commit/discard. At minimum, add explicit documentation stating that any OpenView& obtained from view() is invalidated by the next commit() or discard() call.

Comment thread include/xrpl/ledger/OpenViewSandbox.h Outdated
}

void
commit()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The apply-then-reset ordering leaves the sandbox in a re-committable state after a successful apply. RawStateTable::apply is a const method — it iterates items and mutates the target but never touches its own items_ or dropsDestroyed_. If sandbox_->apply(parent_) completes successfully but the subsequent std::make_unique<OpenView>(kBatchView, parent_) throws (e.g., allocation failure from the monotonic buffer resource), sandbox_ is not replaced and still holds the full item set pointing at the just-committed changes. Any caller that catches the exception and retries commit() would call sandbox_->apply(parent_) a second time on an already-updated parent_, double-accounting rawDestroyXRP drops and triggering logic_error from RawStateTable's duplicate-action guards — or, for items where the intervening state changed, silently replacing live SLE data with stale content. The fix is to swap the sandbox out atomically before applying: auto old = std::exchange(sandbox_, std::make_unique<OpenView>(kBatchView, parent_)); old->apply(parent_); — this ensures the fresh sandbox is in place before any mutation of parent_ begins, so a partial or failed apply cannot be retried from the same sandbox.

Suggested fix

Replace the two-line sequence with: auto old = std::exchange(sandbox_, std::make_unique<OpenView>(kBatchView, parent_)); old->apply(parent_);. This guarantees that if make_unique throws, neither the apply nor the sandbox reset has occurred (clean state); if apply throws after the swap, sandbox_ is already the fresh view and the old sandbox is destroyed at end-of-scope, preventing any re-commit.

Comment thread include/xrpl/ledger/View.h Outdated
STAmount const& amount,
beast::Journal j);

enum class SendIssuerHandling { ihSENDER_NOT_ALLOWED, ihRECEIVER_NOT_ALLOWED, ihIGNORE };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ihSENDER_NOT_ALLOWED and ihRECEIVER_NOT_ALLOWED values accidentally couple issuer-role restrictions with the lsfDefaultRipple check in a way that inverts the expected behavior. The guard in canTransferIOU reads issuerHandling != ihSENDER_NOT_ALLOWED && issuerHandling != ihRECEIVER_NOT_ALLOWED && !lsfDefaultRipple, which means DefaultRipple is enforced only when ihIGNORE is passed — the two restriction modes paradoxically bypass it. Any future caller that passes ihSENDER_NOT_ALLOWED to prevent the issuer from being the sender will silently skip the DefaultRipple check, allowing IOU transfers through trust lines where rippling is explicitly disabled. Pull the DefaultRipple check out of the issuerHandling condition in canTransferIOU so it runs unconditionally after the issuer-identity guards, decoupling the two orthogonal concerns.

Suggested fix

In canTransferIOU (View.cpp), separate the lsfDefaultRipple check from the issuerHandling condition. Replace the compound guard with two independent checks: first if (issuer == sender && issuerHandling == ihSENDER_NOT_ALLOWED) return tecNO_PERMISSION;, then if (issuer == receiver && issuerHandling == ihRECEIVER_NOT_ALLOWED) return tecNO_PERMISSION;, and then if (!sleIssuer->isFlag(lsfDefaultRipple)) return terNO_RIPPLE; as a standalone unconditional check.

Comment thread include/xrpl/ledger/View.h Outdated
SendEscrowHandling escrowHandling,
SendAuthHandling authHandling,
SendFreezeHandling freezeHandling,
SendTransferHandling transferHandling,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SendTransferHandling transferHandling parameter implies that passing thCHECK enforces transfer authorization for all fungible token types, but canTransferIOU accepts the parameter and silently ignores it — the check is never read anywhere in the IOU path. Only canTransferMPT honors thCHECK by gating a canTransfer() call. A caller who passes thCHECK for an IOU-denominated amount receives zero enforcement with no error or diagnostic, producing a silent correctness gap that will be invisible until an IOU transfer that should have been rejected slips through. Either implement the equivalent IOU transfer-authorization check in canTransferIOU when thCHECK is set, or remove the parameter from the IOU dispatch path and document the asymmetry explicitly.

Suggested fix

In canTransferIOU (View.cpp), either add a transferHandling == thCHECK branch that enforces the IOU-equivalent transfer authorization (analogous to the MPT canTransfer() call at line 668), or remove transferHandling from the canTransferIOU static signature and assert/document that thCHECK is only meaningful for MPTs. If the parameter is intentionally unused for IOUs, add a [[maybe_unused]] annotation and a comment so future maintainers understand the asymmetry.

Comment thread include/xrpl/ledger/View.h Outdated
enum class SendEscrowHandling { ehIGNORE, ehCHECK };
enum class SendAuthHandling { ahCHECK_SENDER, ahCHECK_RECEIVER, ahBOTH, ahNEITHER };
enum class SendFreezeHandling { fhCHECK_SENDER, fhCHECK_RECEIVER, fhBOTH, fhNEITHER };
enum class SendTransferHandling { thIGNORE, thCHECK };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sole call site in ContractUtils.cpp passes SendTransferHandling::thIGNORE, which skips the canTransfer() call that enforces lsfMPTCanTransfer on the MPTokenIssuance SLE. When an issuer creates an MPT without setting lsfMPTCanTransfer, third-party transfers between non-issuer accounts must be rejected — this is the issuer's primary mechanism for restricting token transferability. By passing thIGNORE, neither the ContractCreate nor ContractCall preclaim checks this flag, and the doApply path (accountSendaccountSendMPTdirectSendNoFeeMPT) explicitly disclaims the check with a comment saying it must have been validated earlier, which it was not. Change the call site to SendTransferHandling::thCHECK, or document explicitly why contracts are exempt from issuer transferability restrictions and gate that exemption behind an amendment.

Suggested fix

In ContractUtils.cpp (preclaimFlagParameters, tfSendAmount case), change SendTransferHandling::thIGNORE to SendTransferHandling::thCHECK. The apply-time accountSendMPT explicitly relies on callers having pre-validated lsfMPTCanTransfer (see directSendNoFeeMPT line 1163 comment). If contracts are intentionally exempt from this restriction, the waiver must be explicit and amendment-gated, not an accidental omission.

Converts '-' to '+' and '_' to '/', adds padding, then decodes.
*/
std::string
base64urlDecode(std::string_view data);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function silently truncates output when it encounters invalid base64url characters — the underlying decoder breaks on the first unrecognized byte and returns a shorter string with no indication of failure. Because the return type is std::string, callers cannot distinguish a valid empty decode from a malformed input that produced zero bytes. The current WebAuthn call site in verifyPasskeySignature is safe because it compares both size and content against expectedSigningData, but any future caller that only checks for a non-empty result will silently accept malformed input. Return std::optional<std::string> (nullopt on error) or throw on invalid characters to make failure observable at the API boundary.

Suggested fix

Change the signature to std::optional<std::string> base64urlDecode(std::string_view data) and return std::nullopt if, after character substitution and padding, the decoded byte count is not consistent with the padded input length (i.e., if base64::decode consumed fewer characters than expected). Alternatively, validate that every character in the input is a member of the base64url alphabet (A-Z, a-z, 0-9, -, _) before decoding and return an error for any violation. Update the sole call site in verifyPasskeySignature to treat std::nullopt as a verification failure.

TER ter,
std::optional<STAmount> const& deliver,
std::optional<uint256 const> const& parentBatchId,
std::optional<std::uint32_t> const& gasUsed,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ContractCall.cpp passes gasUsed to this function via an unchecked static_cast<uint32_t>(re.value().cost), where cost is an int64_t returned from the WASM engine. EscrowFinish.cpp has an explicit guard (if (reCost < 0 || reCost > std::numeric_limits<uint32_t>::max()) return tecINTERNAL) before the same cast, but ContractCall.cpp has no equivalent check. The WASM VM produces cost as gas - moduleWrap_->getGas(), where gas can be INT64_MAX and getGas() can return negative values in error paths — making the resulting cost potentially negative or larger than UINT32_MAX, silently wrapping on the cast. Currently sfGasUsed is metadata-only, but the commented-out allowance-deduction block in ContractCall.cpp shows this value is slated for balance enforcement; once re-enabled, a corrupted gasUsed would produce incorrect balance mutations. Add the same range guard that EscrowFinish.cpp already uses before calling ctx_.setGasUsed().

Suggested fix

In ContractCall.cpp, before ctx_.setGasUsed(static_cast<uint32_t>(re.value().cost)), add: if (re.value().cost < 0 || re.value().cost > static_cast<int64_t>(std::numeric_limits<uint32_t>::max())) return tecINTERNAL; — mirroring the guard already present in EscrowFinish.cpp:435-437.

if (s == "ed25519")
return KeyType::Ed25519;

if (s == "p256")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keyTypeFromString now returns KeyType::P256 for the string "p256", but the entire downstream signing pipeline — publicKeyType(), verifySigObject(), checkSingleSign(), and checkMultiSign() in both STTx.cpp and Transactor.cpp — accepts P256 keys without ever consulting featurePasskey. The Passkey amendment only gates the PasskeyListSet transaction type, so any other transaction (Payment, TrustSet, OfferCreate) can be signed with a P256 key and accepted by nodes running this code regardless of whether the amendment has activated. Nodes that have not adopted this build will reject those transactions as invalid-signature, causing a ledger fork. Every P256 recognition point in publicKeyType(), verifySigObject(), and Transactor::checkSingleSign() must be gated behind rules.enabled(featurePasskey).

Suggested fix

Gate P256 key recognition inside publicKeyType() behind a Rules parameter checked against featurePasskey — returning nullopt when the amendment is not active causes the entire signing pipeline to reject P256 keys as malformed without needing changes throughout verifySigObject or checkSingleSign. Alternatively, add rules.enabled(featurePasskey) guards in verifySigObject and Transactor::checkSingleSign before the P256 routing branches.

if (s == "ed25519")
return KeyType::Ed25519;

if (s == "p256")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding P256 to keyTypeFromString without updating the allowlist in keypairForSignature (RPCHelpers.cpp) creates an unauthenticated remote crash. A USER-role channel_authorize request with "key_type": "p256" now gets a valid KeyType::P256 optional back from keyTypeFromString, which bypasses the !keyType early-exit guard at line 261 of RPCHelpers.cpp, then hits the hard allowlist if (keyType != KeyType::Secp256k1 && keyType != KeyType::Ed25519) logicError(...) at line 338-339 — which calls std::abort() and takes down the node. The fix is to add KeyType::P256 to that allowlist in keypairForSignature, or implement P256 key-pair generation and signing so the function can actually handle it; the same pattern was confirmed exploitable with KeyType::Dilithium on this codebase.

Suggested fix

In RPCHelpers.cpp keypairForSignature, update the allowlist check to include KeyType::P256. The current guard if (keyType != KeyType::Secp256k1 && keyType != KeyType::Ed25519) logicError(...) must be extended: if (keyType != KeyType::Secp256k1 && keyType != KeyType::Ed25519 && keyType != KeyType::P256) logicError(...). Longer term, replace the allowlist with a proper dispatch table so new KeyType additions cannot silently fall through to abort.

std::size_t constexpr maxContractFunctions = 32;

int64_t
contractCreateFee(uint64_t byteCount);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The feeCalculationFailed sentinel is returned by contractCreateFee to signal an unrepresentable overflow, but no call site ever explicitly compares the return value against this sentinel before using it. The caller in ContractCreate::calculateBaseFee wraps the raw return value directly into an XRPAmount and then attempts to catch the error through a secondary overflow guard; however, that guard is skipped when fees().increment equals zero, allowing the sentinel to propagate as the literal transaction base fee. The sentinel is declared in this header as the obvious guard value, yet it is never referenced in any comparison across the entire codebase. Both call sites must check if (result == contract::feeCalculationFailed) return <error> immediately after the call before performing any arithmetic with the result.

Suggested fix

In ContractCreate::calculateBaseFee and ContractModify::calculateBaseFee, after the call to contract::contractCreateFee(), add: if (createFee.drops() == contract::feeCalculationFailed) return XRPAmount{INVALID_FEE}; (or equivalent error signaling mechanism) before any further arithmetic. Remove the secondary overflow guard that depends on fees().increment being nonzero.

int64_t constexpr createByteMultiplier = 500ULL;

/** The value to return when the fee calculation failed. */
int64_t constexpr feeCalculationFailed = 0x7FFFFFFFFFFFFFFFLL;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The feeCalculationFailed sentinel defined in this header is never tested by any caller. Both ContractCreate::calculateBaseFee and ContractModify::calculateBaseFee pass the raw return value of contractCreateFee() directly into XRPAmount{} without checking whether it equals feeCalculationFailed. The overflow protection currently works by coincidence — INT64_MAX exceeds any valid XRP fee amount, so the downstream range guard catches it and falls back to INITIAL_XRP. If that range guard is ever relaxed, or if a future call site lacks a similar guard, the error sentinel will propagate silently as a legitimate (astronomical) fee value. Each call site should test fee == contract::feeCalculationFailed immediately after the call and short-circuit before constructing the XRPAmount.

Suggested fix

In ContractCreate.cpp and ContractModify.cpp, immediately after calling contractCreateFee(), add: if (fee == contract::feeCalculationFailed) return XRPAmount{INITIAL_XRP};. This makes the sentinel contract explicit and removes the latent dependency on the range guard as an accidental catch-all.

beast::Journal j);

TER
finalizeContractData(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The finalizeContractData interface accepts a ContractDataMap keyed by arbitrary AccountID values, and the implementation writes ledger entries and adjusts owner counts for every account in the map without verifying the transaction has authority over those accounts. The WASM host function that populates this map accepts any AccountID argument without checking it matches the contract account or transaction originator, meaning a malicious contract can target any account on the network. This allows a WASM contract execution to place data objects into any victim account's owner directory and charge XRP reserves against the victim's balance — no signature, delegation, or permission check exists on the target. Add an authorization guard in both the host function layer and inside finalizeContractData restricting target AccountID keys to the contract account and the transaction originator.

Suggested fix

In the WASM host function that inserts into the dataMap, reject any target AccountID that is not equal to contractAccount or sourceAccount before writing to the map. Add the same check at the top of the setContractData helper called by finalizeContractData: if (account != contractAccount && account != sourceAccount) return tecNO_PERMISSION.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant