diff --git a/src/ripple/app/misc/Manifest.h b/src/ripple/app/misc/Manifest.h index 766ef8bcc4b..ad5e6a6d5ea 100644 --- a/src/ripple/app/misc/Manifest.h +++ b/src/ripple/app/misc/Manifest.h @@ -105,6 +105,9 @@ struct Manifest @param s Serialized manifest string @return `boost::none` if string is invalid + + @note This does not verify manifest signatures. + `Manifest::verify` should be called after constructing manifest. */ static boost::optional make_Manifest(std::string s); diff --git a/src/ripple/app/misc/impl/Manifest.cpp b/src/ripple/app/misc/impl/Manifest.cpp index 64ce9d8dbc6..4ef6109ee40 100644 --- a/src/ripple/app/misc/impl/Manifest.cpp +++ b/src/ripple/app/misc/impl/Manifest.cpp @@ -41,27 +41,32 @@ Manifest::make_Manifest (std::string s) STObject st (sfGeneric); SerialIter sit (s.data (), s.size ()); st.set (sit); - auto const opt_pk = get(st, sfPublicKey); + auto const pk = st.getFieldVL (sfPublicKey); + if (! publicKeyType (makeSlice(pk))) + return boost::none; + auto const opt_seq = get (st, sfSequence); auto const opt_msig = get (st, sfMasterSignature); - if (!opt_pk || !opt_seq || !opt_msig) + if (!opt_seq || !opt_msig) return boost::none; // Signing key and signature are not required for // master key revocations if (*opt_seq != std::numeric_limits::max ()) { - auto const opt_spk = get(st, sfSigningPubKey); + auto const spk = st.getFieldVL (sfSigningPubKey); + if (! publicKeyType (makeSlice(spk))) + return boost::none; auto const opt_sig = get (st, sfSignature); - if (!opt_spk || !opt_sig) - { + if (! opt_sig) return boost::none; - } - return Manifest (std::move (s), *opt_pk, *opt_spk, *opt_seq); + return Manifest (std::move (s), PublicKey (makeSlice(pk)), + PublicKey (makeSlice(spk)), *opt_seq); } - return Manifest (std::move (s), *opt_pk, PublicKey(), *opt_seq); + return Manifest (std::move (s), PublicKey (makeSlice(pk)), + PublicKey(), *opt_seq); } catch (std::exception const&) { diff --git a/src/ripple/app/misc/impl/ValidatorKeys.cpp b/src/ripple/app/misc/impl/ValidatorKeys.cpp index c658269f6ad..599d83c567b 100644 --- a/src/ripple/app/misc/impl/ValidatorKeys.cpp +++ b/src/ripple/app/misc/impl/ValidatorKeys.cpp @@ -24,6 +24,7 @@ #include #include #include +#include namespace ripple { ValidatorKeys::ValidatorKeys(Config const& config, beast::Journal j) @@ -42,9 +43,23 @@ ValidatorKeys::ValidatorKeys(Config const& config, beast::Journal j) if (auto const token = ValidatorToken::make_ValidatorToken( config.section(SECTION_VALIDATOR_TOKEN).lines())) { - secretKey = token->validationSecret; - publicKey = derivePublicKey(KeyType::secp256k1, secretKey); - manifest = std::move(token->manifest); + auto const pk = derivePublicKey( + KeyType::secp256k1, token->validationSecret); + auto const m = Manifest::make_Manifest( + beast::detail::base64_decode(token->manifest)); + + if (! m || pk != m->signingKey) + { + configInvalid_ = true; + JLOG(j.fatal()) + << "Invalid token specified in [" SECTION_VALIDATOR_TOKEN "]"; + } + else + { + secretKey = token->validationSecret; + publicKey = pk; + manifest = std::move(token->manifest); + } } else { diff --git a/src/test/app/Manifest_test.cpp b/src/test/app/Manifest_test.cpp index 899c09fa732..ffa6a6b2e09 100644 --- a/src/test/app/Manifest_test.cpp +++ b/src/test/app/Manifest_test.cpp @@ -461,6 +461,195 @@ class Manifest_test : public beast::unit_test::suite } } + void testMakeManifest() + { + testcase ("make_Manifest"); + + std::array const keyTypes {{ + KeyType::ed25519, + KeyType::secp256k1 }}; + + std::uint32_t sequence = 0; + + // public key with invalid type + auto const ret = strUnHex("9930E7FC9D56BB25D6893BA3F317AE5BCF33B3291BD63DB32654A313222F7FD020"); + auto const badKey = Slice{ret.first.data(), ret.first.size()}; + + // short public key + auto const retShort = strUnHex("0330"); + auto const shortKey = Slice{retShort.first.data(), retShort.first.size()}; + + auto toString = [](STObject const& st) + { + Serializer s; + st.add(s); + + return std::string (static_cast (s.data()), s.size()); + }; + + for (auto const keyType : keyTypes) + { + auto const sk = generateSecretKey (keyType, randomSeed ()); + auto const pk = derivePublicKey(keyType, sk); + + for (auto const sKeyType : keyTypes) + { + auto const ssk = generateSecretKey (sKeyType, randomSeed ()); + auto const spk = derivePublicKey(sKeyType, ssk); + + auto buildManifestObject = [&]( + std::uint32_t const& seq, + bool noSigningPublic = false, + bool noSignature = false) + { + STObject st(sfGeneric); + st[sfSequence] = seq; + st[sfPublicKey] = pk; + + if (! noSigningPublic) + st[sfSigningPubKey] = spk; + + sign(st, HashPrefix::manifest, keyType, sk, + sfMasterSignature); + + if (! noSignature) + sign(st, HashPrefix::manifest, sKeyType, ssk); + + return st; + }; + + auto const st = buildManifestObject(++sequence); + + { + // valid manifest + auto const m = toString(st); + + auto const manifest = Manifest::make_Manifest (m); + + BEAST_EXPECT(manifest); + BEAST_EXPECT(manifest->masterKey == pk); + BEAST_EXPECT(manifest->signingKey == spk); + BEAST_EXPECT(manifest->sequence == sequence); + BEAST_EXPECT(manifest->serialized == m); + BEAST_EXPECT(manifest->verify()); + } + { + // valid manifest with invalid signature + auto badSigSt = st; + badSigSt[sfPublicKey] = badSigSt[sfSigningPubKey]; + + auto const m = toString(badSigSt); + auto const manifest = Manifest::make_Manifest (m); + + BEAST_EXPECT(manifest); + BEAST_EXPECT(manifest->masterKey == spk); + BEAST_EXPECT(manifest->signingKey == spk); + BEAST_EXPECT(manifest->sequence == sequence); + BEAST_EXPECT(manifest->serialized == m); + BEAST_EXPECT(! manifest->verify()); + } + { + // reject missing sequence + auto badSt = st; + BEAST_EXPECT(badSt.delField(sfSequence)); + BEAST_EXPECT(! Manifest::make_Manifest (toString(badSt))); + } + { + // reject missing public key + auto badSt = st; + BEAST_EXPECT(badSt.delField(sfPublicKey)); + BEAST_EXPECT(! Manifest::make_Manifest (toString(badSt))); + } + { + // reject invalid public key type + auto badSt = st; + badSt[sfPublicKey] = badKey; + BEAST_EXPECT(! Manifest::make_Manifest (toString(badSt))); + } + { + // reject short public key + auto badSt = st; + badSt[sfPublicKey] = shortKey; + BEAST_EXPECT(! Manifest::make_Manifest (toString(badSt))); + } + { + // reject missing signing public key + auto badSt = st; + BEAST_EXPECT(badSt.delField(sfSigningPubKey)); + BEAST_EXPECT(! Manifest::make_Manifest (toString(badSt))); + } + { + // reject invalid signing public key type + auto badSt = st; + badSt[sfSigningPubKey] = badKey; + BEAST_EXPECT(! Manifest::make_Manifest (toString(badSt))); + } + { + // reject short signing public key + auto badSt = st; + badSt[sfSigningPubKey] = shortKey; + BEAST_EXPECT(! Manifest::make_Manifest (toString(badSt))); + } + { + // reject missing signature + auto badSt = st; + BEAST_EXPECT(badSt.delField(sfMasterSignature)); + BEAST_EXPECT(! Manifest::make_Manifest (toString(badSt))); + } + { + // reject missing signing key signature + auto badSt = st; + BEAST_EXPECT(badSt.delField(sfSignature)); + BEAST_EXPECT(! Manifest::make_Manifest (toString(badSt))); + } + + // test revocations (max sequence revoking the master key) + auto testRevocation = [&](STObject const& st) + { + auto const m = toString(st); + auto const manifest = Manifest::make_Manifest (m); + + BEAST_EXPECT(manifest); + BEAST_EXPECT(manifest->masterKey == pk); + BEAST_EXPECT(manifest->signingKey == PublicKey()); + BEAST_EXPECT(manifest->sequence == + std::numeric_limits::max ()); + BEAST_EXPECT(manifest->serialized == m); + BEAST_EXPECT(manifest->verify()); + }; + + // valid revocation + { + auto const revSt = buildManifestObject( + std::numeric_limits::max ()); + testRevocation(revSt); + } + + // signing key and signature are optional in revocation + { + auto const revSt = buildManifestObject( + std::numeric_limits::max (), + true /* no signing key */); + testRevocation(revSt); + } + { + auto const revSt = buildManifestObject( + std::numeric_limits::max (), + false, true /* no signature */); + testRevocation(revSt); + + } + { + auto const revSt = buildManifestObject( + std::numeric_limits::max (), + true /* no signing key */, + true /* no signature */); + testRevocation(revSt); + } + } + } + } + void run() override { @@ -523,6 +712,7 @@ class Manifest_test : public beast::unit_test::suite testGetSignature (); testGetKeys (); testValidatorToken (); + testMakeManifest (); } }; diff --git a/src/test/app/ValidatorKeys_test.cpp b/src/test/app/ValidatorKeys_test.cpp index 7fb8d32bfdc..62c59dc2af9 100644 --- a/src/test/app/ValidatorKeys_test.cpp +++ b/src/test/app/ValidatorKeys_test.cpp @@ -57,6 +57,17 @@ class ValidatorKeys_test : public beast::unit_test::suite "gBD67kMaRFGvmpATHlGKJdvDFlWPYy5AqDedFv5TJa2w0i21eq3MYywLVJZnFOr7C0kw" "2AiTzSCjIzditQ8="; + // Manifest does not match private key + const std::vector invalidTokenBlob = { + "eyJtYW5pZmVzdCI6IkpBQUFBQVZ4SWUyOVVBdzViZFJudHJ1elVkREk4aDNGV1JWZl\n", + "k3SXVIaUlKQUhJd3MxdzZzM01oQWtsa1VXQWR2RnFRVGRlSEpvS1pNY0hlS0RzOExo\n", + "b3d3bDlHOEdkVGNJbmFka1l3UkFJZ0h2Q01lQU1aSzlqQnV2aFhlaFRLRzVDQ3BBR1\n", + "k0bGtvZHRXYW84UGhzR3NDSUREVTA1d1c3bWNiMjlVNkMvTHBpZmgvakZPRGhFR21i\n", + "NWF6dTJMVHlqL1pjQkpBbitmNGhtQTQ0U0tYbGtTTUFqak1rSWRyR1Rxa21SNjBzVG\n", + "JaTjZOOUYwdk9UV3VYcUZ6eDFoSGIyL0RqWElVZXhDVGlITEcxTG9UdUp1eXdXbk55\n", + "RFE9PSIsInZhbGlkYXRpb25fc2VjcmV0X2tleSI6IjkyRDhCNDBGMzYwMTc5MTkwMU\n", + "MzQTUzMzI3NzBDMkUwMTA4MDI0NTZFOEM2QkI0NEQ0N0FFREQ0NzJGMDQ2RkYifQ==\n"}; + public: void run() override @@ -141,6 +152,17 @@ class ValidatorKeys_test : public beast::unit_test::suite BEAST_EXPECT(k.manifest.empty()); } + { + // Token manifest and private key must match + Config c; + c.section(SECTION_VALIDATOR_TOKEN).append(invalidTokenBlob); + ValidatorKeys k{c, j}; + + BEAST_EXPECT(k.configInvalid()); + BEAST_EXPECT(k.publicKey.size() == 0); + BEAST_EXPECT(k.manifest.empty()); + } + } }; // namespace test