diff --git a/include/xrpl/ledger/CredentialHelpers.h b/include/xrpl/ledger/CredentialHelpers.h index cb6b3c98ee..2d83b75f39 100644 --- a/include/xrpl/ledger/CredentialHelpers.h +++ b/include/xrpl/ledger/CredentialHelpers.h @@ -39,16 +39,10 @@ namespace credentials { // Check if credential sfExpiration field has passed ledger's parentCloseTime bool -checkExpired( - std::shared_ptr const& sleCredential, - NetClock::time_point const& closed); - -// Return true if any expired credential was found in arr (and deleted) -bool -removeExpired(ApplyView& view, STVector256 const& arr, beast::Journal const j); +checkExpired(SLE const& sleCredential, NetClock::time_point const& closed); // Actually remove a credentials object from the ledger -TER +[[nodiscard]] TER deleteSLE( ApplyView& view, std::shared_ptr const& sleCredential, diff --git a/src/libxrpl/ledger/CredentialHelpers.cpp b/src/libxrpl/ledger/CredentialHelpers.cpp index 4c625f5eee..dcc102cfee 100644 --- a/src/libxrpl/ledger/CredentialHelpers.cpp +++ b/src/libxrpl/ledger/CredentialHelpers.cpp @@ -28,17 +28,16 @@ namespace ripple { namespace credentials { bool -checkExpired( - std::shared_ptr const& sleCredential, - NetClock::time_point const& closed) +checkExpired(SLE const& sleCredential, NetClock::time_point const& closed) { - std::uint32_t const exp = (*sleCredential)[~sfExpiration].value_or( + std::uint32_t const exp = sleCredential[~sfExpiration].value_or( std::numeric_limits::max()); std::uint32_t const now = closed.time_since_epoch().count(); return now > exp; } -bool +[[nodiscard]] +static Expected removeExpired(ApplyView& view, STVector256 const& arr, beast::Journal const j) { auto const closeTime = view.info().parentCloseTime; @@ -50,12 +49,14 @@ removeExpired(ApplyView& view, STVector256 const& arr, beast::Journal const j) auto const k = keylet::credential(h); auto const sleCred = view.peek(k); - if (sleCred && checkExpired(sleCred, closeTime)) + if (sleCred && checkExpired(*sleCred, closeTime)) { JLOG(j.trace()) << "Credentials are expired. Cred: " << sleCred->getText(); // delete expired credentials even if the transaction failed - deleteSLE(view, sleCred, j); + auto const err = deleteSLE(view, sleCred, j); + if (view.rules().enabled(fixSecurity3_1_3) && !isTesSuccess(err)) + return Unexpected(err); foundExpired = true; } } @@ -217,7 +218,7 @@ validDomain(ReadView const& view, uint256 domainID, AccountID const& subject) // allows expired credentials to be deleted by any transaction. if (sleCredential) { - if (checkExpired(sleCredential, closeTime)) + if (checkExpired(*sleCredential, closeTime)) { foundExpired = true; continue; @@ -343,7 +344,10 @@ verifyValidDomain( credentials.push_back(keyletCredential.key); } - bool const foundExpired = credentials::removeExpired(view, credentials, j); + auto const foundExpired = credentials::removeExpired(view, credentials, j); + if (!foundExpired.has_value()) + return foundExpired.error(); + for (auto const& h : credentials) { auto sleCredential = view.read(keylet::credential(h)); @@ -354,7 +358,7 @@ verifyValidDomain( return tesSUCCESS; } - return foundExpired ? tecEXPIRED : tecNO_PERMISSION; + return *foundExpired ? tecEXPIRED : tecNO_PERMISSION; } TER @@ -374,9 +378,15 @@ verifyDepositPreauth( bool const credentialsPresent = tx.isFieldPresent(sfCredentialIDs); - if (credentialsPresent && - credentials::removeExpired(view, tx.getFieldV256(sfCredentialIDs), j)) - return tecEXPIRED; + if (credentialsPresent) + { + auto const foundExpired = credentials::removeExpired( + view, tx.getFieldV256(sfCredentialIDs), j); + if (!foundExpired.has_value()) + return foundExpired.error(); + if (*foundExpired) + return tecEXPIRED; + } if (sleDst && (sleDst->getFlags() & lsfDepositAuth)) { diff --git a/src/test/app/Credentials_test.cpp b/src/test/app/Credentials_test.cpp index a1c936f35b..95bee5fdd3 100644 --- a/src/test/app/Credentials_test.cpp +++ b/src/test/app/Credentials_test.cpp @@ -1109,6 +1109,125 @@ struct Credentials_test : public beast::unit_test::suite } } + void + testRemoveExpiredCorruption(FeatureBitset features) + { + bool const fixEnabled = features[fixSecurity3_1_3]; + testcase( + "removeExpired ignores deleteSLE failure " + + (fixEnabled ? std::string(" after fix") + : std::string(" before fix"))); + + using namespace test::jtx; + + char const credType[] = "abcde"; + Account const issuer{"issuer"}; + Account const subject{"subject"}; + Account const becky{"becky"}; + + Env env{*this, features}; + env.fund(XRP(10000), issuer, subject, becky); + env.close(); + + // Create credential with short expiration + auto jv = credentials::create(subject, issuer, credType); + uint32_t const expiration = + env.current()->info().parentCloseTime.time_since_epoch().count() + + 40; + jv[sfExpiration.jsonName] = expiration; + env(jv); + env.close(); + + auto const credLE = + credentials::ledgerEntry(env, subject, issuer, credType); + std::string const credIdx = credLE[jss::result][jss::index].asString(); + + // Subject accepts the credential + env(credentials::accept(subject, issuer, credType)); + env.close(); + + // Build the credential keylet + auto const credKeylet = keylet::credential( + subject.id(), issuer.id(), Slice(credType, std::strlen(credType))); + + // Verify credential exists and is accepted + { + auto const sleCred = env.current()->read(credKeylet); + BEAST_EXPECT(sleCred && sleCred->getFlags() & lsfAccepted); + } + + // Create DepositPreauth + env(deposit::authCredentials(becky, {{subject, credType}})); + env.close(); + // env(); + auto jtx = + env.jt(pay(subject, becky, XRP(100)), credentials::ids({credIdx})); + if (!BEAST_EXPECT(jtx.stx)) + return; + auto const stx = std::make_shared(*jtx.stx); + + // Create PermissionedDomain + env(pdomain::setTx(becky, {{issuer, credType}})); + env.close(); + auto const domain = pdomain::getObjects(becky, env).begin()->first; + + using namespace std::chrono_literals; + env.close(50s); + + // Verify time has advanced past expiration + { + auto const sleCred = env.current()->read(credKeylet); + BEAST_EXPECT( + sleCred && + ripple::credentials::checkExpired( + *sleCred, env.current()->info().parentCloseTime)); + } + + // Create an ApplyViewImpl on top of the current closed ledger + // and corrupt it by erasing the issuer's account SLE + auto const open = env.current(); + ApplyViewImpl av(&*open, tapNONE); + + // Erase the issuer's account to simulate ledger corruption + auto sleIssuer = av.peek(keylet::account(issuer.id())); + if (!BEAST_EXPECT(sleIssuer)) + return; + av.erase(sleIssuer); + BEAST_EXPECT(!av.exists(keylet::account(issuer.id()))); + + // Credential still exists before removeExpired + BEAST_EXPECT(av.exists(credKeylet)); + + // Call removeExpired on the corrupted view + STVector256 credHashes; + credHashes.push_back(credKeylet.key); + beast::Journal const j{beast::Journal::getNullSink()}; + + auto const dpTer = + ripple::verifyDepositPreauth(*stx, av, subject, becky, {}, j); + auto sleCredAfter = av.read(credKeylet); + BEAST_EXPECT(sleCredAfter && (sleCredAfter->getFlags() & lsfAccepted)); + + auto const domTer = + ripple::verifyValidDomain(av, subject.id(), domain, j); + sleCredAfter = av.read(credKeylet); + BEAST_EXPECT(sleCredAfter && (sleCredAfter->getFlags() & lsfAccepted)); + + if (fixEnabled) + { + // removeExpired returns error, cred wasn't deleted + BEAST_EXPECT(dpTer == tecINTERNAL); + BEAST_EXPECT(domTer == tecINTERNAL); + } + else + { + // removeExpired returns true (claims it found & deleted expired + // creds) + BEAST_EXPECT(dpTer == tecEXPIRED); + BEAST_EXPECT(isTesSuccess(domTer)); + } + } + void run() override { @@ -1124,6 +1243,9 @@ struct Credentials_test : public beast::unit_test::suite testFlags(all - fixInvalidTxFlags); testFlags(all); testRPC(); + + testRemoveExpiredCorruption(all - fixSecurity3_1_3); + testRemoveExpiredCorruption(all | fixSecurity3_1_3); } }; diff --git a/src/xrpld/app/misc/PermissionedDEXHelpers.cpp b/src/xrpld/app/misc/PermissionedDEXHelpers.cpp index 279ef8d7e9..5996b664cc 100644 --- a/src/xrpld/app/misc/PermissionedDEXHelpers.cpp +++ b/src/xrpld/app/misc/PermissionedDEXHelpers.cpp @@ -48,7 +48,7 @@ accountInDomain( return false; return !credentials::checkExpired( - sleCred, view.info().parentCloseTime); + *sleCred, view.info().parentCloseTime); }); return inDomain; diff --git a/src/xrpld/app/tx/detail/Credentials.cpp b/src/xrpld/app/tx/detail/Credentials.cpp index 2277b4ace6..786aae7a0a 100644 --- a/src/xrpld/app/tx/detail/Credentials.cpp +++ b/src/xrpld/app/tx/detail/Credentials.cpp @@ -261,7 +261,7 @@ CredentialDelete::doApply() return tefINTERNAL; // LCOV_EXCL_LINE if ((subject != account_) && (issuer != account_) && - !checkExpired(sleCred, ctx_.view().info().parentCloseTime)) + !checkExpired(*sleCred, ctx_.view().info().parentCloseTime)) { JLOG(j_.trace()) << "Can't delete non-expired credential."; return tecNO_PERMISSION; @@ -354,8 +354,10 @@ CredentialAccept::doApply() auto const credType(ctx_.tx[sfCredentialType]); Keylet const credentialKey = keylet::credential(account_, issuer, credType); auto const sleCred = view().peek(credentialKey); // Checked in preclaim() + if (!sleCred) + return tefINTERNAL; // LCOV_EXCL_LINE - if (checkExpired(sleCred, view().info().parentCloseTime)) + if (checkExpired(*sleCred, view().info().parentCloseTime)) { JLOG(j_.trace()) << "Credential is expired: " << sleCred->getText(); // delete expired credentials even if the transaction failed diff --git a/src/xrpld/app/tx/detail/Transactor.cpp b/src/xrpld/app/tx/detail/Transactor.cpp index a866de8b2d..98f2ab1a7e 100644 --- a/src/xrpld/app/tx/detail/Transactor.cpp +++ b/src/xrpld/app/tx/detail/Transactor.cpp @@ -1091,7 +1091,15 @@ removeExpiredCredentials( for (auto const& index : creds) { if (auto const sle = view.peek(keylet::credential(index))) - credentials::deleteSLE(view, sle, viewJ); + { + if (auto const ter = credentials::deleteSLE(view, sle, viewJ); + !isTesSuccess(ter)) + { + JLOG(viewJ.error()) << "removeExpiredCredentials: failed to " + "delete expired credential. Err: " + << transToken(ter); + } + } } } diff --git a/src/xrpld/rpc/handlers/DepositAuthorized.cpp b/src/xrpld/rpc/handlers/DepositAuthorized.cpp index c184a7f845..c3854f765f 100644 --- a/src/xrpld/rpc/handlers/DepositAuthorized.cpp +++ b/src/xrpld/rpc/handlers/DepositAuthorized.cpp @@ -154,7 +154,7 @@ doDepositAuthorized(RPC::JsonContext& context) } if (credentials::checkExpired( - sleCred, ledger->info().parentCloseTime)) + *sleCred, ledger->info().parentCloseTime)) { RPC::inject_error( rpcBAD_CREDENTIALS, "credentials are expired", result);