Address issues identified by external review:

* RIPD-1617, RIPD-1619, RIPD-1621:
  Verify serialized public keys more strictly before
  using them.

* RIPD-1618:
    * Simplify the base58 decoder logic.
    * Reduce the complexity of the base58 encoder and
      eliminate a potential out-of-bounds memory access.
    * Improve type safety by using an `enum class` to
      enforce strict type checking for token types.

* RIPD-1616:
  Avoid calling `memcpy` with a null pointer even if the
  size is specified as zero, since it results in undefined
  behavior.

Acknowledgements:
Ripple thanks Guido Vranken for responsibly disclosing these
issues.

Bug Bounties and Responsible Disclosures:
We welcome reviews of the rippled code and urge researchers
to responsibly disclose any issues that they may find. For
more on Ripple's Bug Bounty program, please visit:
https://ripple.com/bug-bounty

src/ripple/protocol/impl/AccountID.cpp

@@ -29,18 +29,14 @@ namespace ripple {
 std::string
 toBase58 (AccountID const& v)
 {
-    return base58EncodeToken(
-        TOKEN_ACCOUNT_ID,
-            v.data(), v.size());
+    return base58EncodeToken(TokenType::AccountID, v.data(), v.size());
 }
 
 template<>
 boost::optional<AccountID>
 parseBase58 (std::string const& s)
 {
-    auto const result =
-        decodeBase58Token(
-            s, TOKEN_ACCOUNT_ID);
+    auto const result = decodeBase58Token(s, TokenType::AccountID);
     if (result.empty())
         return boost::none;
     AccountID id;
@@ -54,9 +50,7 @@ parseBase58 (std::string const& s)
 boost::optional<AccountID>
 deprecatedParseBitcoinAccountID (std::string const& s)
 {
-    auto const result =
-        decodeBase58TokenBitcoin(
-            s, TOKEN_ACCOUNT_ID);
+    auto const result = decodeBase58TokenBitcoin(s, TokenType::AccountID);
     if (result.empty())
         return boost::none;
     AccountID id;

src/ripple/protocol/impl/PublicKey.cpp

@@ -37,22 +37,15 @@ operator<<(std::ostream& os, PublicKey const& pk)
     return os;
 }
 
-using uint264 = boost::multiprecision::number<
-    boost::multiprecision::cpp_int_backend<
-        264, 264, boost::multiprecision::signed_magnitude,
-            boost::multiprecision::unchecked, void>>;
-
 template<>
 boost::optional<PublicKey>
 parseBase58 (TokenType type, std::string const& s)
 {
-    auto const result =
-        decodeBase58Token(s, type);
-    if (result.empty())
+    auto const result = decodeBase58Token(s, type);
+    auto const pks = makeSlice(result);
+    if (!publicKeyType(pks))
         return boost::none;
-    if (result.size() != 33)
-        return boost::none;
-    return PublicKey(makeSlice(result));
+    return PublicKey(pks);
 }
 
 //------------------------------------------------------------------------------
@@ -81,8 +74,7 @@ sigPart (Slice& buf)
         if ((buf[1] & 0x80) == 0)
             return boost::none;
     }
-    boost::optional<Slice> number =
-        Slice(buf.data(), len);
+    boost::optional<Slice> number = Slice(buf.data(), len);
     buf += len;
     return number;
 }
@@ -125,6 +117,11 @@ sliceToHex (Slice const& slice)
 boost::optional<ECDSACanonicality>
 ecdsaCanonicality (Slice const& sig)
 {
+    using uint264 = boost::multiprecision::number<
+        boost::multiprecision::cpp_int_backend<
+            264, 264, boost::multiprecision::signed_magnitude,
+            boost::multiprecision::unchecked, void>>;
+
     static uint264 const G(
         "0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141");
 
@@ -141,12 +138,13 @@ ecdsaCanonicality (Slice const& sig)
         return boost::none;
 
     uint264 R(sliceToHex(*r));
-    uint264 S(sliceToHex(*s));
-
     if (R >= G)
         return boost::none;
+
+    uint264 S(sliceToHex(*s));
     if (S >= G)
         return boost::none;
+
     // (R,S) and (R,G-S) are canonical,
     // but is fully canonical when S <= G-S
     auto const Sp = G - S;
@@ -186,7 +184,7 @@ PublicKey::PublicKey (Slice const& slice)
     if(! publicKeyType(slice))
         LogicError("PublicKey::PublicKey invalid type");
     size_ = slice.size();
-    std::memcpy(buf_, slice.data(), slice.size());
+    std::memcpy(buf_, slice.data(), size_);
 }
 
 PublicKey::PublicKey (PublicKey const& other)
@@ -196,8 +194,7 @@ PublicKey::PublicKey (PublicKey const& other)
 };
 
 PublicKey&
-PublicKey::operator=(
-    PublicKey const& other)
+PublicKey::operator=(PublicKey const& other)
 {
     size_ = other.size_;
     std::memcpy(buf_, other.buf_, size_);
@@ -209,13 +206,15 @@ PublicKey::operator=(
 boost::optional<KeyType>
 publicKeyType (Slice const& slice)
 {
-    if (slice.size() == 33 &&
-            slice[0] == 0xED)
-        return KeyType::ed25519;
-    if (slice.size() == 33 &&
-        (slice[0] == 0x02 ||
-            slice[0] == 0x03))
-        return KeyType::secp256k1;
+    if (slice.size() == 33)
+    {
+        if (slice[0] == 0xED)
+            return KeyType::ed25519;
+
+        if (slice[0] == 0x02 || slice[0] == 0x03)
+            return KeyType::secp256k1;
+    }
+
     return boost::none;
 }
 

src/ripple/protocol/impl/STValidation.cpp

@@ -29,15 +29,23 @@ namespace ripple {
 STValidation::STValidation (SerialIter& sit, bool checkSignature)
     : STObject (getFormat (), sit, sfValidation)
 {
-    mNodeID = calcNodeID(
-        PublicKey(makeSlice (getFieldVL (sfSigningPubKey))));
+    auto const spk = getFieldVL(sfSigningPubKey);
+
+    if (publicKeyType(makeSlice(spk)) != KeyType::secp256k1)
+    {
+        JLOG (debugLog().error())
+            << "Invalid public key in validation" << getJson (0);
+        Throw<std::runtime_error> ("Invalid public key in validation");
+    }
+
+    mNodeID = calcNodeID(PublicKey(makeSlice(spk)));
     assert (mNodeID.isNonZero ());
 
     if  (checkSignature && !isValid ())
     {
         JLOG (debugLog().error())
-            << "Invalid validation" << getJson (0);
-        Throw<std::runtime_error> ("Invalid validation");
+            << "Invalid signature in validation" << getJson (0);
+        Throw<std::runtime_error> ("Invalid signature in validation");
     }
 }
 
@@ -49,11 +57,15 @@ STValidation::STValidation (
     : STObject (getFormat (), sfValidation)
     , mSeen (signTime)
 {
+    // This is our own public key and it should always be valid.
+    if (!publicKeyType(publicKey))
+        LogicError ("Invalid validation public key");
+
     // Does not sign
     setFieldH256 (sfLedgerHash, ledgerHash);
     setFieldU32 (sfSigningTime, signTime.time_since_epoch().count());
-
     setFieldVL (sfSigningPubKey, publicKey.slice());
+
     mNodeID = calcNodeID(publicKey);
     assert (mNodeID.isNonZero ());
 
@@ -101,6 +113,9 @@ bool STValidation::isValid (uint256 const& signingHash) const
 {
     try
     {
+        if (publicKeyType(getSignerPublic()) != KeyType::secp256k1)
+            return false;
+
         return verifyDigest (getSignerPublic(),
             signingHash,
             makeSlice(getFieldVL (sfSignature)),

src/ripple/protocol/impl/SecretKey.cpp

@@ -87,10 +87,8 @@ public:
         std::memcpy(ui.data(), seed.data(), seed.size());
         auto gsk = generatePrivateDeterministicKey(gen_, ui, ordinal);
         auto gpk = generatePublicDeterministicKey(gen_, ordinal);
-        SecretKey const sk(Slice
-        { gsk.data(), gsk.size() });
-        PublicKey const pk(Slice
-        { gpk.data(), gpk.size() });
+        SecretKey const sk(Slice{ gsk.data(), gsk.size() });
+        PublicKey const pk(Slice{ gpk.data(), gpk.size() });
         beast::secure_erase(ui.data(), ui.size());
         beast::secure_erase(gsk.data(), gsk.size());
         return {pk, sk};
@@ -243,7 +241,7 @@ derivePublicKey (KeyType type, SecretKey const& sk)
             LogicError("derivePublicKey: secp256k1_ec_pubkey_create failed");
 
         unsigned char pubkey[33];
-        size_t len = sizeof(pubkey);
+        std::size_t len = sizeof(pubkey);
         if(secp256k1_ec_pubkey_serialize(
                 secp256k1Context(),
                 pubkey,
@@ -252,8 +250,7 @@ derivePublicKey (KeyType type, SecretKey const& sk)
                 SECP256K1_EC_COMPRESSED) != 1)
             LogicError("derivePublicKey: secp256k1_ec_pubkey_serialize failed");
 
-        return PublicKey{Slice{pubkey,
-            static_cast<std::size_t>(len)}};
+        return PublicKey{Slice{ pubkey, len }};
     }
     case KeyType::ed25519:
     {

src/ripple/protocol/impl/Seed.cpp

@@ -85,8 +85,7 @@ template <>
 boost::optional<Seed>
 parseBase58 (std::string const& s)
 {
-    auto const result = decodeBase58Token(
-         s, TokenType::TOKEN_FAMILY_SEED);
+    auto const result = decodeBase58Token(s, TokenType::FamilySeed);
     if (result.empty())
         return boost::none;
     if (result.size() != 16)
@@ -101,10 +100,10 @@ parseGenericSeed (std::string const& str)
         return boost::none;
 
     if (parseBase58<AccountID>(str) ||
-        parseBase58<PublicKey>(TokenType::TOKEN_NODE_PUBLIC, str) ||
-        parseBase58<PublicKey>(TokenType::TOKEN_ACCOUNT_PUBLIC, str) ||
-        parseBase58<SecretKey>(TokenType::TOKEN_NODE_PRIVATE, str) ||
-        parseBase58<SecretKey>(TokenType::TOKEN_ACCOUNT_SECRET, str))
+        parseBase58<PublicKey>(TokenType::NodePublic, str) ||
+        parseBase58<PublicKey>(TokenType::AccountPublic, str) ||
+        parseBase58<SecretKey>(TokenType::NodePrivate, str) ||
+        parseBase58<SecretKey>(TokenType::AccountSecret, str))
     {
         return boost::none;
     }

src/ripple/protocol/impl/Serializer.cpp

@@ -519,10 +519,16 @@ T SerialIter::getRawHelper (int size)
         Throw<std::runtime_error> (
             "invalid SerialIter getRaw");
     T result (size);
-    memcpy(result.data (), p_, size);
-    p_ += size;
-    used_ += size;
-    remain_ -= size;
+    if (size != 0)
+    {
+        // It's normally safe to call memcpy with size set to 0 (see the
+        // C99 standard 7.21.1/2). However, here this could mean that
+        // result.data would be null, which would trigger undefined behavior.
+        std::memcpy(result.data(), p_, size);
+        p_ += size;
+        used_ += size;
+        remain_ -= size;
+    }
     return result;
 }
 

src/ripple/protocol/impl/tokens.cpp

@@ -20,6 +20,7 @@
 #include <BeastConfig.h>
 #include <ripple/protocol/tokens.h>
 #include <ripple/protocol/digest.h>
+#include <boost/container/small_vector.hpp>
 #include <cassert>
 #include <cstring>
 #include <memory>
@@ -102,11 +103,12 @@ static
 std::string
 encodeBase58(
     void const* message, std::size_t size,
-        void *temp, char const* const alphabet)
+        void *temp, std::size_t temp_size,
+            char const* const alphabet)
 {
-    auto pbegin = reinterpret_cast<
-        unsigned char const*>(message);
+    auto pbegin = reinterpret_cast<unsigned char const*>(message);
     auto const pend = pbegin + size;
+
     // Skip & count leading zeroes.
     int zeroes = 0;
     while (pbegin != pend && *pbegin == 0)
@@ -114,12 +116,12 @@ encodeBase58(
         pbegin++;
         zeroes++;
     }
-    auto const b58begin = reinterpret_cast<
-        unsigned char*>(temp);
-    // log(256) / log(58), rounded up.
-    auto const b58end = b58begin +
-        size * (138 / 100 + 1);
+
+    auto const b58begin = reinterpret_cast<unsigned char*>(temp);
+    auto const b58end = b58begin + temp_size;
+
     std::fill(b58begin, b58end, 0);
+
     while (pbegin != pend)
     {
         int carry = *pbegin;
@@ -133,10 +135,12 @@ encodeBase58(
         assert(carry == 0);
         pbegin++;
     }
+
     // Skip leading zeroes in base58 result.
     auto iter = b58begin;
     while (iter != b58end && *iter == 0)
         ++iter;
+
     // Translate the result into a string.
     std::string str;
     str.reserve(zeroes + (b58end - iter));
@@ -148,48 +152,44 @@ encodeBase58(
 
 static
 std::string
-encodeToken (std::uint8_t type,
-    void const* token, std::size_t size, bool btc)
+encodeToken (TokenType type,
+    void const* token, std::size_t size, char const* const alphabet)
 {
-    char buf[1024];
-    // expanded token includes type + checksum
+    // expanded token includes type + 4 byte checksum
     auto const expanded = 1 + size + 4;
-    // add scratch, log(256) / log(58), rounded up.
-    auto const needed = expanded +
-        size * (138 / 100 + 1);
-    std::unique_ptr<
-        char[]> pbuf;
-    char* temp;
-    if (needed > sizeof(buf))
-    {
-        pbuf.reset(new char[needed]);
-        temp = pbuf.get();
-    }
-    else
-    {
-        temp = buf;
-    }
+
+    // We need expanded + expanded * (log(256) / log(58)) which is
+    // bounded by expanded + expanded * (138 / 100 + 1) which works
+    // out to expanded * 3:
+    auto const bufsize = expanded * 3;
+
+    boost::container::small_vector<std::uint8_t, 1024> buf (bufsize);
+
     // Lay the data out as
     //      <type><token><checksum>
-    temp[0] = type;
-    std::memcpy(temp + 1, token, size);
-    checksum(temp + 1 + size, temp, 1 + size);
-    return encodeBase58(temp, expanded,
-        temp + expanded, btc ? bitcoinAlphabet : rippleAlphabet);
+    buf[0] = static_cast<std::underlying_type_t <TokenType>>(type);
+    if (size)
+        std::memcpy(buf.data() + 1, token, size);
+    checksum(buf.data() + 1 + size, buf.data(), 1 + size);
+
+    return encodeBase58(
+        buf.data(), expanded,
+        buf.data() + expanded, bufsize - expanded,
+        alphabet);
 }
 
 std::string
-base58EncodeToken (std::uint8_t type,
+base58EncodeToken (TokenType type,
     void const* token, std::size_t size)
 {
-    return encodeToken(type, token, size, false);
+    return encodeToken(type, token, size, rippleAlphabet);
 }
 
 std::string
-base58EncodeTokenBitcoin (std::uint8_t type,
+base58EncodeTokenBitcoin (TokenType type,
     void const* token, std::size_t size)
 {
-    return encodeToken(type, token, size, true);
+    return encodeToken(type, token, size, bitcoinAlphabet);
 }
 
 //------------------------------------------------------------------------------
@@ -258,28 +258,26 @@ template <class InverseArray>
 static
 std::string
 decodeBase58Token (std::string const& s,
-    int type, InverseArray const& inv)
+    TokenType type, InverseArray const& inv)
 {
-    auto result = decodeBase58(s, inv);
-    if (result.empty())
-        return result;
+    auto ret = decodeBase58(s, inv);
+
     // Reject zero length tokens
-    if (result.size() < 6)
+    if (ret.size() < 6)
         return {};
-    if (result[0] != type)
+
+    // The type must match.
+    if (type != static_cast<TokenType>(ret[0]))
         return {};
+
+    // And the checksum must as well.
     std::array<char, 4> guard;
-    checksum(guard.data(),
-        result.data(), result.size() - 4);
-    if (std::memcmp(guard.data(),
-        result.data() +
-            result.size() - 4, 4) != 0)
+    checksum(guard.data(), ret.data(), ret.size() - guard.size());
+    if (!std::equal (guard.rbegin(), guard.rend(), ret.rbegin()))
         return {};
-    result.resize(result.size() - 4);
-    // Erase the type byte
-    // VFALCO This might cause problems later
-    result.erase(result.begin());
-    return result;
+
+    // Skip the leading type byte and the trailing checksum.
+    return ret.substr(1, ret.size() - 1 - guard.size());
 }
 
 //------------------------------------------------------------------------------
@@ -315,18 +313,16 @@ static InverseAlphabet bitcoinInverse(bitcoinAlphabet);
 
 std::string
 decodeBase58Token(
-    std::string const& s, int type)
+    std::string const& s, TokenType type)
 {
-    return decodeBase58Token(
-        s, type, rippleInverse);
+    return decodeBase58Token(s, type, rippleInverse);
 }
 
 std::string
 decodeBase58TokenBitcoin(
-    std::string const& s, int type)
+    std::string const& s, TokenType type)
 {
-    return decodeBase58Token(
-        s, type, bitcoinInverse);
+    return decodeBase58Token(s, type, bitcoinInverse);
 }
 
 } // ripple