security: Validator::verifySignature don't throw on Interest with malformed signature
refs #3723
Change-Id: I7de6ab667ffbcc531a5ea8bccae1551e1699cefd
diff --git a/src/security/security-common.hpp b/src/security/security-common.hpp
index 8594581..542e169 100644
--- a/src/security/security-common.hpp
+++ b/src/security/security-common.hpp
@@ -33,8 +33,16 @@
const ssize_t POS_RANDOM_VAL = -3;
const ssize_t POS_TIMESTAMP = -4;
+/** \brief minimal number of components for Command Interest
+ * \sa https://redmine.named-data.net/projects/ndn-cxx/wiki/CommandInterest
+ */
const size_t MIN_LENGTH = 4;
+/** \brief minimal number of components for Signed Interest
+ * \sa https://redmine.named-data.net/projects/ndn-cxx/wiki/SignedInterest
+ */
+const size_t MIN_LENGTH_SIG_ONLY = 2;
+
} // namespace signed_interest
enum class KeyType {
diff --git a/src/security/validator.cpp b/src/security/validator.cpp
index 488d0d7..1ef7a05 100644
--- a/src/security/validator.cpp
+++ b/src/security/validator.cpp
@@ -22,8 +22,6 @@
* @author Jeff Thompson <jefft0@remap.ucla.edu>
*/
-#include "common.hpp"
-
#include "validator.hpp"
#include "../util/crypto.hpp"
@@ -115,27 +113,27 @@
bool
Validator::verifySignature(const Interest& interest, const PublicKey& key)
{
- const Name& interestName = interest.getName();
+ const Name& name = interest.getName();
- if (interestName.size() < 2)
+ if (name.size() < signed_interest::MIN_LENGTH_SIG_ONLY)
return false;
+ Signature sig;
try {
- const Block& nameBlock = interestName.wireEncode();
-
- Signature sig(interestName[signed_interest::POS_SIG_INFO].blockFromValue(),
- interestName[signed_interest::POS_SIG_VALUE].blockFromValue());
-
- if (!sig.hasKeyLocator())
- return false;
-
- return verifySignature(nameBlock.value(),
- nameBlock.value_size() - interestName[signed_interest::POS_SIG_VALUE].size(),
- sig, key);
+ sig.setInfo(name[signed_interest::POS_SIG_INFO].blockFromValue());
+ sig.setValue(name[signed_interest::POS_SIG_VALUE].blockFromValue());
}
- catch (const Block::Error& e) {
+ catch (const tlv::Error&) {
return false;
}
+
+ if (!sig.hasKeyLocator())
+ return false;
+
+ const Block& nameWire = name.wireEncode();
+ return verifySignature(nameWire.value(),
+ nameWire.value_size() - name[signed_interest::POS_SIG_VALUE].size(),
+ sig, key);
}
bool
diff --git a/src/security/validator.hpp b/src/security/validator.hpp
index bf8e1ca..1f06c83 100644
--- a/src/security/validator.hpp
+++ b/src/security/validator.hpp
@@ -25,9 +25,6 @@
#ifndef NDN_SECURITY_VALIDATOR_HPP
#define NDN_SECURITY_VALIDATOR_HPP
-#include "../common.hpp"
-
-#include "../data.hpp"
#include "../face.hpp"
#include "public-key.hpp"
#include "signature-sha256-with-rsa.hpp"
@@ -39,9 +36,7 @@
namespace ndn {
/**
- * @brief Validator is one of the main classes of the security library.
- *
- * The Validator class provides the interfaces for packet validation.
+ * @brief provides the interfaces for packet validation.
*/
class Validator
{
diff --git a/tests/unit-tests/make-interest-data.hpp b/tests/unit-tests/make-interest-data.hpp
index 36c8deb..8dea8bf 100644
--- a/tests/unit-tests/make-interest-data.hpp
+++ b/tests/unit-tests/make-interest-data.hpp
@@ -81,6 +81,30 @@
lp::Nack
makeNack(const Name& name, uint32_t nonce, lp::NackReason reason);
+/** \brief replace a name component
+ * \param[inout] name name
+ * \param index name component index
+ * \param a arguments to name::Component constructor
+ */
+template<typename...A>
+void
+setNameComponent(Name& name, ssize_t index, const A& ...a)
+{
+ Name name2 = name.getPrefix(index);
+ name2.append(name::Component(a...));
+ name2.append(name.getSubName(name2.size()));
+ name = name2;
+}
+
+template<typename PKT, typename...A>
+void
+setNameComponent(PKT& pkt, ssize_t index, const A& ...a)
+{
+ Name name = pkt.getName();
+ setNameComponent(name, index, a...);
+ pkt.setName(name);
+}
+
} // namespace tests
} // namespace ndn
diff --git a/tests/unit-tests/security/command-interest-validator.t.cpp b/tests/unit-tests/security/command-interest-validator.t.cpp
index 6173274..9a9d1e8 100644
--- a/tests/unit-tests/security/command-interest-validator.t.cpp
+++ b/tests/unit-tests/security/command-interest-validator.t.cpp
@@ -112,16 +112,6 @@
unique_ptr<CommandInterestValidator> validator;
};
-template<typename...A>
-void
-setNameComponent(Name& name, ssize_t index, const A& ...a)
-{
- Name name2 = name.getPrefix(index);
- name2.append(name::Component(a...));
- name2.append(name.getSubName(index + 1));
- name = name2;
-}
-
BOOST_AUTO_TEST_SUITE(Security)
BOOST_FIXTURE_TEST_SUITE(TestCommandInterestValidator, CommandInterestValidatorFixture)
@@ -162,57 +152,47 @@
BOOST_AUTO_TEST_CASE(BadTimestamp)
{
auto i1 = makeCommandInterest();
- Name n1 = i1->getName();
- setNameComponent(n1, signed_interest::POS_TIMESTAMP, "not-timestamp");
- i1->setName(n1);
+ setNameComponent(*i1, signed_interest::POS_TIMESTAMP, "not-timestamp");
assertReject(*i1, CommandInterestValidator::ErrorCode::BAD_TIMESTAMP);
}
BOOST_AUTO_TEST_CASE(BadSigInfo)
{
auto i1 = makeCommandInterest();
- Name n1 = i1->getName();
- setNameComponent(n1, signed_interest::POS_SIG_INFO, "not-SignatureInfo");
- i1->setName(n1);
+ setNameComponent(*i1, signed_interest::POS_SIG_INFO, "not-SignatureInfo");
assertReject(*i1, CommandInterestValidator::ErrorCode::BAD_SIG_INFO);
}
BOOST_AUTO_TEST_CASE(MissingKeyLocator)
{
auto i1 = makeCommandInterest();
- Name n1 = i1->getName();
SignatureInfo sigInfo;
- setNameComponent(n1, signed_interest::POS_SIG_INFO,
+ setNameComponent(*i1, signed_interest::POS_SIG_INFO,
sigInfo.wireEncode().begin(), sigInfo.wireEncode().end());
- i1->setName(n1);
assertReject(*i1, CommandInterestValidator::ErrorCode::MISSING_KEY_LOCATOR);
}
BOOST_AUTO_TEST_CASE(BadKeyLocatorType)
{
auto i1 = makeCommandInterest();
- Name n1 = i1->getName();
KeyLocator kl;
kl.setKeyDigest(makeBinaryBlock(tlv::KeyDigest, "\xDD\xDD\xDD\xDD\xDD\xDD\xDD\xDD", 8));
SignatureInfo sigInfo;
sigInfo.setKeyLocator(kl);
- setNameComponent(n1, signed_interest::POS_SIG_INFO,
+ setNameComponent(*i1, signed_interest::POS_SIG_INFO,
sigInfo.wireEncode().begin(), sigInfo.wireEncode().end());
- i1->setName(n1);
assertReject(*i1, CommandInterestValidator::ErrorCode::BAD_KEY_LOCATOR_TYPE);
}
BOOST_AUTO_TEST_CASE(BadCertName)
{
auto i1 = makeCommandInterest();
- Name n1 = i1->getName();
KeyLocator kl;
kl.setName("/bad/cert/name");
SignatureInfo sigInfo;
sigInfo.setKeyLocator(kl);
- setNameComponent(n1, signed_interest::POS_SIG_INFO,
+ setNameComponent(*i1, signed_interest::POS_SIG_INFO,
sigInfo.wireEncode().begin(), sigInfo.wireEncode().end());
- i1->setName(n1);
assertReject(*i1, CommandInterestValidator::ErrorCode::BAD_CERT_NAME);
}
@@ -265,12 +245,9 @@
auto i1 = makeCommandInterest(); // signed at 0s
assertAccept(*i1);
- auto i2 = makeCommandInterest();
- Name n1 = i1->getName();
- Name n2 = i2->getName();
- setNameComponent(n2, signed_interest::POS_TIMESTAMP,
- n1[signed_interest::POS_TIMESTAMP]);
- i2->setName(n2); // signed at 0s
+ auto i2 = makeCommandInterest(); // signed at 0s
+ setNameComponent(*i2, signed_interest::POS_TIMESTAMP,
+ i1->getName()[signed_interest::POS_TIMESTAMP]);
assertReject(*i2, CommandInterestValidator::ErrorCode::TIMESTAMP_REORDER);
advanceClocks(time::seconds(2));
@@ -320,10 +297,8 @@
auto i1 = makeCommandInterest(1); // signed at 0ms
auto i2 = makeCommandInterest(2); // signed at 0ms
for (auto interest : {i1, i2}) {
- Name name = interest->getName();
- setNameComponent(name, signed_interest::POS_TIMESTAMP,
+ setNameComponent(*interest, signed_interest::POS_TIMESTAMP,
name::Component::fromNumber(time::toUnixTimestamp(time::system_clock::now()).count()));
- interest->setName(name);
} // ensure timestamps are exactly 0ms
assertAccept(*i1); // verifying at 0ms
diff --git a/tests/unit-tests/security/validator.t.cpp b/tests/unit-tests/security/validator.t.cpp
index dd2ecd1..8fa26ec 100644
--- a/tests/unit-tests/security/validator.t.cpp
+++ b/tests/unit-tests/security/validator.t.cpp
@@ -21,16 +21,16 @@
#include "security/validator-null.hpp"
#include "security/key-chain.hpp"
-#include "util/time.hpp"
-#include "identity-management-fixture.hpp"
+
#include "boost-test.hpp"
+#include "identity-management-fixture.hpp"
+#include "../make-interest-data.hpp"
namespace ndn {
namespace tests {
-using std::string;
-
-BOOST_FIXTURE_TEST_SUITE(SecurityValidator, IdentityManagementFixture)
+BOOST_AUTO_TEST_SUITE(Security)
+BOOST_FIXTURE_TEST_SUITE(TestValidator, IdentityManagementFixture)
void
onValidated(const shared_ptr<const Data>& data)
@@ -39,7 +39,7 @@
}
void
-onValidationFailed(const shared_ptr<const Data>& data, const string& failureInfo)
+onValidationFailed(const shared_ptr<const Data>& data, const std::string& failureInfo)
{
BOOST_CHECK(false);
}
@@ -236,7 +236,30 @@
BOOST_CHECK(Validator::verifySignature(*testInterestRsa, rsaCert->getPublicKeyInfo()));
}
-BOOST_AUTO_TEST_SUITE_END()
+BOOST_AUTO_TEST_CASE(MalformedInterestSigInfo)
+{
+ auto interest = make_shared<Interest>("/prefix");
+ m_keyChain.sign(*interest);
+
+ setNameComponent(*interest, signed_interest::POS_SIG_INFO, "not-SignatureInfo");
+
+ PublicKey pubkey = m_keyChain.getDefaultCertificate()->getPublicKeyInfo();
+ BOOST_CHECK_EQUAL(Validator::verifySignature(*interest, pubkey), false);
+}
+
+BOOST_AUTO_TEST_CASE(MalformedInterestSigValue)
+{
+ auto interest = make_shared<Interest>("/prefix");
+ m_keyChain.sign(*interest);
+
+ setNameComponent(*interest, signed_interest::POS_SIG_VALUE, "bad-signature-bits");
+
+ PublicKey pubkey = m_keyChain.getDefaultCertificate()->getPublicKeyInfo();
+ BOOST_CHECK_EQUAL(Validator::verifySignature(*interest, pubkey), false);
+}
+
+BOOST_AUTO_TEST_SUITE_END() // TestValidator
+BOOST_AUTO_TEST_SUITE_END() // Security
} // namespace tests
} // namespace ndn