From f398952a6c03574e5947f6dfe5ea0f7b0f0b5224 Mon Sep 17 00:00:00 2001 From: Adam Ierymenko Date: Tue, 7 Jul 2015 08:14:41 -0700 Subject: Revert some bad docs in Packet -- I think we will still use that. Also rename addMembershipCertificate to more security-descriptive validateAndAddMembershipCertificate, give it a return value, and drop unused force parameter. --- node/Network.cpp | 54 ++++++++++++++++++++++++++++-------------------------- 1 file changed, 28 insertions(+), 26 deletions(-) (limited to 'node/Network.cpp') diff --git a/node/Network.cpp b/node/Network.cpp index 4414e4d1..84b8d320 100644 --- a/node/Network.cpp +++ b/node/Network.cpp @@ -267,53 +267,55 @@ void Network::requestConfiguration() RR->sw->send(outp,true,_id); } -void Network::addMembershipCertificate(const CertificateOfMembership &cert,bool forceAccept) +bool Network::validateAndAddMembershipCertificate(const CertificateOfMembership &cert) { if (!cert) // sanity check - return; + return false; Mutex::Lock _l(_lock); CertificateOfMembership &old = _membershipCertificates[cert.issuedTo()]; // Nothing to do if the cert hasn't changed -- we get duplicates due to zealous cert pushing if (old == cert) - return; + return true; // but if it's a duplicate of one we already accepted, return is 'true' // Check signature, log and return if cert is invalid - if (!forceAccept) { - if (cert.signedBy() != controller()) { - TRACE("rejected network membership certificate for %.16llx signed by %s: signer not a controller of this network",(unsigned long long)_id,cert.signedBy().toString().c_str()); - return; + if (cert.signedBy() != controller()) { + TRACE("rejected network membership certificate for %.16llx signed by %s: signer not a controller of this network",(unsigned long long)_id,cert.signedBy().toString().c_str()); + return false; // invalid signer + } + + if (cert.signedBy() == RR->identity.address()) { + + // We are the controller: RR->identity.address() == controller() == cert.signedBy() + // So, verify that we signed th cert ourself + if (!cert.verify(RR->identity)) { + TRACE("rejected network membership certificate for %.16llx self signed by %s: signature check failed",(unsigned long long)_id,cert.signedBy().toString().c_str()); + return false; // invalid signature } - if (cert.signedBy() == RR->identity.address()) { - // We are the controller: RR->identity.address() == controller() == cert.signedBy() - // So, verify that we signed th cert ourself - if (!cert.verify(RR->identity)) { - TRACE("rejected network membership certificate for %.16llx self signed by %s: signature check failed",(unsigned long long)_id,cert.signedBy().toString().c_str()); - return; - } - } else { + } else { - SharedPtr signer(RR->topology->getPeer(cert.signedBy())); + SharedPtr signer(RR->topology->getPeer(cert.signedBy())); - if (!signer) { - // This would be rather odd, since this is our controller... could happen - // if we get packets before we've gotten config. - RR->sw->requestWhois(cert.signedBy()); - return; - } + if (!signer) { + // This would be rather odd, since this is our controller... could happen + // if we get packets before we've gotten config. + RR->sw->requestWhois(cert.signedBy()); + return false; // signer unknown + } - if (!cert.verify(signer->identity())) { - TRACE("rejected network membership certificate for %.16llx signed by %s: signature check failed",(unsigned long long)_id,cert.signedBy().toString().c_str()); - return; - } + if (!cert.verify(signer->identity())) { + TRACE("rejected network membership certificate for %.16llx signed by %s: signature check failed",(unsigned long long)_id,cert.signedBy().toString().c_str()); + return false; // invalid signature } } // If we made it past authentication, update cert if (cert.revision() != old.revision()) old = cert; + + return true; } bool Network::peerNeedsOurMembershipCertificate(const Address &to,uint64_t now) -- cgit v1.2.3 From 07ea4fd4f92d7d5aa8573525b11bf7821b9acb19 Mon Sep 17 00:00:00 2001 From: Adam Ierymenko Date: Tue, 7 Jul 2015 10:02:48 -0700 Subject: Fix potential bug in controller config request. --- node/Network.cpp | 2 +- node/Switch.hpp | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) (limited to 'node/Network.cpp') diff --git a/node/Network.cpp b/node/Network.cpp index 84b8d320..adc8e1b8 100644 --- a/node/Network.cpp +++ b/node/Network.cpp @@ -264,7 +264,7 @@ void Network::requestConfiguration() outp.append((uint64_t)_config->revision()); else outp.append((uint64_t)0); } - RR->sw->send(outp,true,_id); + RR->sw->send(outp,true,0); } bool Network::validateAndAddMembershipCertificate(const CertificateOfMembership &cert) diff --git a/node/Switch.hpp b/node/Switch.hpp index 95ca362c..1b29d050 100644 --- a/node/Switch.hpp +++ b/node/Switch.hpp @@ -110,7 +110,8 @@ public: * won't be encrypted right. (This is not used for relaying.) * * The network ID should only be specified for frames and other actual - * network traffic. + * network traffic. Other traffic such as controller requests and regular + * protocol messages should specify zero. * * @param packet Packet to send * @param encrypt Encrypt packet payload? (always true except for HELLO) -- cgit v1.2.3