From 25b002b37fb639898a10f5d2e120ef040ca381e5 Mon Sep 17 00:00:00 2001 From: wilsonianb Date: Mon, 27 Nov 2017 13:20:38 -0600 Subject: [PATCH] Add make_Manifest test Reject manifest with invalid public key type RIPD-1560 --- src/ripple/app/misc/Manifest.h | 3 + src/ripple/app/misc/impl/Manifest.cpp | 23 ++-- src/test/app/Manifest_test.cpp | 190 ++++++++++++++++++++++++++ 3 files changed, 207 insertions(+), 9 deletions(-) diff --git a/src/ripple/app/misc/Manifest.h b/src/ripple/app/misc/Manifest.h index 766ef8bcc..ad5e6a6d5 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 64ce9d8db..4ef6109ee 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 opt_sig = get (st, sfSignature); - if (!opt_spk || !opt_sig) - { + auto const spk = st.getFieldVL (sfSigningPubKey); + if (! publicKeyType (makeSlice(spk))) + return boost::none; + auto const opt_sig = get (st, sfSignature); + 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/test/app/Manifest_test.cpp b/src/test/app/Manifest_test.cpp index 899c09fa7..ffa6a6b2e 100644 --- a/src/test/app/Manifest_test.cpp +++ b/src/test/app/Manifest_test.cpp @@ -461,6 +461,195 @@ public: } } + 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 @@ public: testGetSignature (); testGetKeys (); testValidatorToken (); + testMakeManifest (); } };