From 04a825298ee3d485abf8890e7026d4e7225fb69a Mon Sep 17 00:00:00 2001 From: Arthur Britto Date: Tue, 12 Mar 2013 12:53:06 -0700 Subject: [PATCH] Clean up and secure subscribe books. --- src/cpp/ripple/Ledger.cpp | 11 +++--- src/cpp/ripple/RPCHandler.cpp | 64 ++++++++++++++++++++++------------- 2 files changed, 44 insertions(+), 31 deletions(-) diff --git a/src/cpp/ripple/Ledger.cpp b/src/cpp/ripple/Ledger.cpp index c73a077ff5..2fbf26eb18 100644 --- a/src/cpp/ripple/Ledger.cpp +++ b/src/cpp/ripple/Ledger.cpp @@ -1307,6 +1307,9 @@ std::vector< std::pair > Ledger::getLedgerHashes() return ret; } +// XRP to XRP not allowed. +// Currencies must have appropriate issuer. +// Currencies or accounts must differ. bool Ledger::isValidBook(const uint160& uTakerPaysCurrency, const uint160& uTakerPaysIssuerID, const uint160& uTakerGetsCurrency, const uint160& uTakerGetsIssuerID) { @@ -1346,9 +1349,6 @@ bool Ledger::isValidBook(const uint160& uTakerPaysCurrency, const uint160& uTake uint256 Ledger::getBookBase(const uint160& uTakerPaysCurrency, const uint160& uTakerPaysIssuerID, const uint160& uTakerGetsCurrency, const uint160& uTakerGetsIssuerID) { - bool bInNative = uTakerPaysCurrency.isZero(); - bool bOutNative = uTakerGetsCurrency.isZero(); - Serializer s(82); s.add16(spaceBookDir); // 2 @@ -1366,10 +1366,7 @@ uint256 Ledger::getBookBase(const uint160& uTakerPaysCurrency, const uint160& uT % RippleAddress::createHumanAccountID(uTakerGetsIssuerID) % uBaseIndex.ToString()); - assert(!bInNative || !bOutNative); // XRP to XRP not allowed. - assert(bInNative == uTakerPaysIssuerID.isZero()); // Make sure issuer is specified as needed. - assert(bOutNative == uTakerGetsIssuerID.isZero()); // Make sure issuer is specified as needed. - assert(uTakerPaysCurrency != uTakerGetsCurrency || uTakerPaysIssuerID != uTakerGetsIssuerID); // Currencies or accounts must differ. + assert(isValidBook(uTakerPaysCurrency, uTakerPaysIssuerID, uTakerGetsCurrency, uTakerGetsIssuerID)); return uBaseIndex; } diff --git a/src/cpp/ripple/RPCHandler.cpp b/src/cpp/ripple/RPCHandler.cpp index 39b5cdc11f..1640285c3f 100644 --- a/src/cpp/ripple/RPCHandler.cpp +++ b/src/cpp/ripple/RPCHandler.cpp @@ -2642,11 +2642,11 @@ Json::Value RPCHandler::doSubscribe(Json::Value jvRequest, int& cost) std::string strUsername = jvRequest.isMember("url_username") ? jvRequest["url_username"].asString() : ""; std::string strPassword = jvRequest.isMember("url_password") ? jvRequest["url_password"].asString() : ""; - // DEPRICATED + // DEPRECATED if (jvRequest.isMember("username")) strUsername = jvRequest["username"].asString(); - // DEPRICATED + // DEPRECATED if (jvRequest.isMember("password")) strPassword = jvRequest["password"].asString(); @@ -2753,12 +2753,25 @@ Json::Value RPCHandler::doSubscribe(Json::Value jvRequest, int& cost) } if (jvRequest.isMember("books")) - { // FIXME: This can crash the server if the parameters to things like getBookPage are invalid + { for (Json::Value::iterator it = jvRequest["books"].begin(); it != jvRequest["books"].end(); it++) { - uint160 uTakerPaysCurrencyID; - uint160 uTakerPaysIssuerID; - Json::Value jvTakerPays = (*it)["taker_pays"]; + Json::Value& jvSubRequest = *it; + uint160 uTakerPaysCurrencyID; + uint160 uTakerPaysIssuerID; + uint160 uTakerGetsCurrencyID; + uint160 uTakerGetsIssuerID; + bool bBoth = (jvSubRequest.isMember("both") && jvSubRequest["both"].asBool()) + || (jvSubRequest.isMember("both_sides") && jvSubRequest["both_sides"].asBool()); // DEPRECATED + bool bSnapshot = (jvSubRequest.isMember("snapshot") && jvSubRequest["snapshot"].asBool()) + || (jvSubRequest.isMember("start_now") && jvSubRequest["start_now"].asBool()); // DEPRECATED + + + if (!jvSubRequest.isMember("taker_pays") || !jvSubRequest.isMember("taker_gets")) + return rpcError(rpcINVALID_PARAMS); + + Json::Value jvTakerPays = jvSubRequest["taker_pays"]; + Json::Value jvTakerGets = jvSubRequest["taker_gets"]; // Parse mandatory currency. if (!jvTakerPays.isMember("currency") @@ -2781,10 +2794,6 @@ Json::Value RPCHandler::doSubscribe(Json::Value jvRequest, int& cost) return rpcError(rpcSRC_ISR_MALFORMED); } - uint160 uTakerGetsCurrencyID; - uint160 uTakerGetsIssuerID; - Json::Value jvTakerGets = (*it)["taker_gets"]; - // Parse mandatory currency. if (!jvTakerGets.isMember("currency") || !STAmount::currencyFromString(uTakerGetsCurrencyID, jvTakerGets["currency"].asString())) @@ -2816,38 +2825,45 @@ Json::Value RPCHandler::doSubscribe(Json::Value jvRequest, int& cost) RippleAddress raTakerID; - if (!(*it).isMember("taker")) + if (!jvSubRequest.isMember("taker")) { raTakerID.setAccountID(ACCOUNT_ONE); } - else if (!raTakerID.setAccountID((*it)["taker"].asString())) + else if (!raTakerID.setAccountID(jvSubRequest["taker"].asString())) { return rpcError(rpcBAD_ISSUER); } - bool bothSides = (*it)["both_sides"].asBool(); - if (!Ledger::isValidBook(uTakerPaysCurrencyID, uTakerPaysIssuerID, uTakerGetsCurrencyID, uTakerGetsIssuerID)) { cLog(lsWARNING) << "Bad market: " << - uTakerPaysCurrencyID << ":" << uTakerPaysIssuerID << " -> " << + uTakerPaysCurrencyID << ":" << uTakerPaysIssuerID << " -> " << uTakerGetsCurrencyID << ":" << uTakerGetsIssuerID; return rpcError(rpcBAD_MARKET); } mNetOps->subBook(ispSub, uTakerPaysCurrencyID, uTakerGetsCurrencyID, uTakerPaysIssuerID, uTakerGetsIssuerID); - if (bothSides) mNetOps->subBook(ispSub, uTakerGetsCurrencyID, uTakerPaysCurrencyID, uTakerGetsIssuerID, uTakerPaysIssuerID); - if ((*it)["state_now"].asBool()) + if (bBoth) mNetOps->subBook(ispSub, uTakerGetsCurrencyID, uTakerPaysCurrencyID, uTakerGetsIssuerID, uTakerPaysIssuerID); + + if (bSnapshot) { - Ledger::pointer ledger= theApp->getLedgerMaster().getClosedLedger(); + Ledger::pointer lpLedger= theApp->getLedgerMaster().getClosedLedger(); const Json::Value jvMarker = Json::Value(Json::nullValue); - mNetOps->getBookPage(ledger, uTakerPaysCurrencyID, uTakerPaysIssuerID, uTakerGetsCurrencyID, uTakerGetsIssuerID, raTakerID.getAccountID(), false, 0, jvMarker, jvResult); - if (bothSides) + + if (bBoth) { - Json::Value tempJson(Json::objectValue); - if (jvResult.isMember("offers")) jvResult["bids"]=jvResult["offers"]; - mNetOps->getBookPage(ledger, uTakerGetsCurrencyID, uTakerGetsIssuerID, uTakerPaysCurrencyID, uTakerPaysIssuerID, raTakerID.getAccountID(), false, 0, jvMarker, tempJson); - if (tempJson.isMember("offers")) jvResult["asks"]=tempJson["offers"]; + Json::Value jvBids(Json::objectValue); + Json::Value jvAsks(Json::objectValue); + + mNetOps->getBookPage(lpLedger, uTakerPaysCurrencyID, uTakerPaysIssuerID, uTakerGetsCurrencyID, uTakerGetsIssuerID, raTakerID.getAccountID(), false, 0, jvMarker, jvBids); + if (jvBids.isMember("offers")) jvResult["bids"]=jvBids["offers"]; + + mNetOps->getBookPage(lpLedger, uTakerGetsCurrencyID, uTakerGetsIssuerID, uTakerPaysCurrencyID, uTakerPaysIssuerID, raTakerID.getAccountID(), false, 0, jvMarker, jvAsks); + if (jvAsks.isMember("offers")) jvResult["asks"]=jvAsks["offers"]; + } + else + { + mNetOps->getBookPage(lpLedger, uTakerPaysCurrencyID, uTakerPaysIssuerID, uTakerGetsCurrencyID, uTakerGetsIssuerID, raTakerID.getAccountID(), false, 0, jvMarker, jvResult); } } }