From 26ad3277e87d407b11fe8a11f3359ab7bed6bc24 Mon Sep 17 00:00:00 2001
From: Adrien Beraud <adrien.beraud@savoirfairelinux.com>
Date: Fri, 3 Mar 2017 20:51:05 +0100
Subject: [PATCH] device revocation: refuse loading revoked device

The missing piece for working device revocation.
* cert store: always use certificate chain match to check
  parent certificate.
  We used to allow a certificate if the parent certificate
  was explicitly allowed. This doesn't check for revoked
  device certificate. Instead, always check certificates
  using the standard certificate chain method except if
  the certificate itself is whitelisted.
* ringaccount: check the account own device certificate chain
  This cause a revoked account to fail loading and propose
  the user to generate a new device from the archive
  through the standard account migration procedure.
* ringaccount: check device loaded from disc using
  the common routine to discard devices revoked since the last
  save.

Tuleap: #1457
Change-Id: I03f015e78d1d14f5f2e9f99a6d3dd47a6f2c5bfe
---
 src/ringdht/ringaccount.cpp | 57 +++++++++++++------------------------
 src/ringdht/ringaccount.h   |  4 ++-
 src/security/certstore.cpp  | 10 -------
 src/security/certstore.h    | 17 +++++++++++
 4 files changed, 39 insertions(+), 49 deletions(-)

diff --git a/src/ringdht/ringaccount.cpp b/src/ringdht/ringaccount.cpp
index 197045ca99..2b6314bcc4 100644
--- a/src/ringdht/ringaccount.cpp
+++ b/src/ringdht/ringaccount.cpp
@@ -821,7 +821,15 @@ RingAccount::useIdentity(const dht::crypto::Identity& identity)
 
     auto accountCertificate = identity.second->issuer;
     if (not accountCertificate) {
-        RING_ERR("Device certificate must be issued by the account certificate");
+        RING_ERR("[Account %s] device certificate must be issued by the account certificate", getAccountID().c_str());
+        return false;
+    }
+
+    // match certificate chain
+    tls::TrustStore account_trust;
+    account_trust.setCertificateStatus(accountCertificate, tls::TrustStore::PermissionStatus::ALLOWED, false);
+    if (not account_trust.isAllowed(*identity.second)) {
+        RING_ERR("[Account %s] can't use identity: device certificate chain can't be verified", getAccountID().c_str());
         return false;
     }
 
@@ -871,13 +879,14 @@ RingAccount::useIdentity(const dht::crypto::Identity& identity)
 
     // success, make use of this identity (certificate chain and private key)
     identity_ = identity;
+    accountTrust_ = std::move(account_trust);
     ringAccountId_ = id;
     ringDeviceId_ = identity.first->getPublicKey().getId().toString();
     username_ = RING_URI_PREFIX + id;
     announce_ = std::make_shared<dht::Value>(std::move(announce_val));
     ethAccount_ = root["eth"].asString();
 
-    RING_DBG("[Account %s] device receipt checked successfully", getAccountID().c_str());
+    RING_DBG("[Account %s] ring:%s device %s receipt checked successfully", getAccountID().c_str(), id.c_str(), ringDeviceId_.c_str());
     return true;
 }
 
@@ -2331,12 +2340,8 @@ RingAccount::foundAccountDevice(const std::shared_ptr<dht::crypto::Certificate>&
     if (not crt)
         return false;
 
-    // Trust store with Ring account main certificate as the only CA
-    tls::TrustStore account_trust;
-    account_trust.setCertificateStatus(identity_.second->issuer, tls::TrustStore::PermissionStatus::ALLOWED, false);
-
     // match certificate chain
-    if (not account_trust.isAllowed(*crt)) {
+    if (not accountTrust_.isAllowed(*crt)) {
         RING_WARN("[Account %s] Found invalid account device: %s", getAccountID().c_str(), crt->getId().toString().c_str());
         return false;
     }
@@ -2606,28 +2611,9 @@ RingAccount::saveTreatedMessages() const
     saveIdList(cachePath_+DIR_SEPARATOR_STR "treatedMessages", treatedMessages_);
 }
 
-void
-RingAccount::loadKnownDevicesOld()
-{
-    auto knownDevices = loadIdList<dht::InfoHash>(idPath_+DIR_SEPARATOR_STR "knownDevices");
-    for (const auto& d : knownDevices) {
-        RING_DBG("[Account %s]: loaded known account device %s", getAccountID().c_str(), d.toString().c_str());
-        if (auto crt = tls::CertificateStore::instance().getCertificate(d.toString()))
-            if (crt->issuer and crt->issuer->getId() == identity_.second->issuer->getId())
-                knownDevices_.emplace(d, KnownDevice{crt});
-            else
-                RING_ERR("Known device certificate not matching identity.");
-        else
-            RING_WARN("Can't find known device certificate.");
-    }
-    fileutils::remove(idPath_+DIR_SEPARATOR_STR "knownDevices");
-}
-
 void
 RingAccount::loadKnownDevices()
 {
-    loadKnownDevicesOld();
-
     std::map<dht::InfoHash, std::pair<std::string, uint64_t>> knownDevices;
     try {
         // read file
@@ -2644,18 +2630,13 @@ RingAccount::loadKnownDevices()
         RING_DBG("[Account %s] loading known account device %s %s", getAccountID().c_str(),
                                                                     d.second.first.c_str(),
                                                                     d.first.toString().c_str());
-        if (auto crt = tls::CertificateStore::instance().getCertificate(d.first.toString()))
-            if (crt->issuer and crt->issuer->getId() == identity_.second->issuer->getId())
-                knownDevices_.emplace(d.first,
-                    KnownDevice {
-                        crt,
-                        d.second.first,
-                        clock::from_time_t(d.second.second)
-                    });
-            else
-                RING_WARN("[Account %s] known device certificate not matching identity", getAccountID().c_str());
-        else
-            RING_WARN("[Account %s] can't find known device certificate", getAccountID().c_str());
+        if (auto crt = tls::CertificateStore::instance().getCertificate(d.first.toString())) {
+            if (not foundAccountDevice(crt, d.second.first, clock::from_time_t(d.second.second)))
+                RING_WARN("[Account %s] can't add device %s", getAccountID().c_str(), d.first.toString().c_str());
+        }
+        else {
+            RING_WARN("[Account %s] can't find certificate for device %s", getAccountID().c_str(), d.first.toString().c_str());
+        }
     }
 }
 
diff --git a/src/ringdht/ringaccount.h b/src/ringdht/ringaccount.h
index 77ee97fdcc..92448abee4 100644
--- a/src/ringdht/ringaccount.h
+++ b/src/ringdht/ringaccount.h
@@ -470,6 +470,9 @@ class RingAccount : public SIPAccountBase {
         void saveContacts() const;
         void updateContact(const dht::InfoHash&, const Contact&);
 
+        // Trust store with Ring account main certificate as the only CA
+        tls::TrustStore accountTrust_;
+        // Trust store for to match peer certificates
         tls::TrustStore trust_;
 
         std::shared_ptr<dht::Value> announce_;
@@ -512,7 +515,6 @@ class RingAccount : public SIPAccountBase {
         void loadTreatedMessages();
         void saveTreatedMessages() const;
 
-        void loadKnownDevicesOld();
         void loadKnownDevices();
         void saveKnownDevices() const;
 
diff --git a/src/security/certstore.cpp b/src/security/certstore.cpp
index 818056fedf..3f09c59d90 100644
--- a/src/security/certstore.cpp
+++ b/src/security/certstore.cpp
@@ -538,15 +538,6 @@ TrustStore::isAllowed(const crypto::Certificate& crt)
     else if (status == PermissionStatus::BANNED)
         return false;
 
-    // Match by certificate pinning (Ring account)
-    if (crt.issuer) {
-        status = getCertificateStatus(crt.issuer->getId().toString());
-        if (status == PermissionStatus::ALLOWED)
-            return true;
-        else if (status == PermissionStatus::BANNED)
-            return false;
-    }
-
     // Match by certificate chain
     updateKnownCerts();
     return matchTrustStore(getChain(crt), allowed_);
@@ -566,7 +557,6 @@ TrustStore::getTrustedCertificates() const
 bool
 TrustStore::matchTrustStore(std::vector<gnutls_x509_crt_t>&& crts, gnutls_x509_trust_list_st* store)
 {
-    RING_ERR("TrustStore::matchTrustStore: %lu", crts.size());
     unsigned result = 0;
 
 #if GNUTLS_VERSION_NUMBER > 0x030308
diff --git a/src/security/certstore.h b/src/security/certstore.h
index f8591916e3..8d0f4ede0d 100644
--- a/src/security/certstore.h
+++ b/src/security/certstore.h
@@ -110,6 +110,23 @@ class TrustStore {
 public:
     TrustStore();
     virtual ~TrustStore();
+    TrustStore(TrustStore&& o) :
+        unknownCertStatus_(std::move(o.unknownCertStatus_)),
+        certStatus_(std::move(o.certStatus_)),
+        revokedList_(std::move(o.revokedList_)),
+        allowed_(std::move(o.allowed_))
+    {
+        o.allowed_ = nullptr;
+    }
+
+    TrustStore& operator=(TrustStore&& o) {
+        unknownCertStatus_ = std::move(o.unknownCertStatus_);
+        certStatus_ = std::move(o.certStatus_);
+        revokedList_ = std::move(o.revokedList_),
+        allowed_ = std::move(o.allowed_);
+        o.allowed_ = nullptr;
+        return *this;
+    }
 
     enum class PermissionStatus {
         UNDEFINED = 0,
-- 
GitLab