fix: secure down HTTP pair process

- Block out of order requests to bypass pairing
- Prevent MITM attacks
- Avoid sigsegv when passing malicious payloads
- Refactored code to better test this

src/moonlight-protocol/crypto/src/crypto.cpp

@@ -168,6 +168,9 @@ std::string sign(std::string_view msg, std::string_view private_key) {
 
 bool verify(std::string_view msg, std::string_view signature, std::string_view public_key) {
   auto p_key = signature::create_key(public_key, false);
+  if (!p_key) {
+    return false;
+  }
   return signature::verify(msg, signature, p_key.get(), EVP_sha256());
 }
 

src/moonlight-protocol/crypto/src/x509.cpp

@@ -192,6 +192,9 @@ std::string get_pkey_content(pkey_ptr pkey) {
 
 std::string get_cert_public_key(x509_ptr cert) {
   auto pkey = X509_get_pubkey(cert.get());
+  if (!pkey) {
+    return "";
+  }
   return get_key_content(pkey_ptr(pkey, EVP_PKEY_free), false);
 }
 

src/moonlight-protocol/moonlight.cpp

@@ -133,23 +133,33 @@ XML client_pair(const std::string &aes_key,
                 const std::string &client_public_cert_signature,
                 const std::string &client_cert_public_key) {
   XML resp;
-  resp.put("root.<xmlattr>.status_code", 200);
   auto digest_size = 256;
 
   auto pairing_secret = crypto::hex_to_str(client_pairing_secret, true);
+  if (pairing_secret.size() < digest_size) {
+    resp.put("root.paired", 0);
+    resp.put("root.<xmlattr>.status_code", 400);
+    resp.put("root.<xmlattr>.status_message", "Invalid pairing secret");
+    return resp;
+  }
   auto client_secret = pairing_secret.substr(0, 16);
   auto client_signature = pairing_secret.substr(16, digest_size);
 
   auto hash = crypto::hex_to_str(crypto::sha256(server_challenge + client_public_cert_signature + client_secret), true);
   if (hash != client_hash) {
     resp.put("root.paired", 0);
+    resp.put("root.<xmlattr>.status_code", 400);
+    resp.put("root.<xmlattr>.status_message", "Invalid client hash");
     return resp;
   }
 
   if (crypto::verify(client_secret, client_signature, client_cert_public_key)) {
     resp.put("root.paired", 1);
+    resp.put("root.<xmlattr>.status_code", 200);
   } else {
     resp.put("root.paired", 0);
+    resp.put("root.<xmlattr>.status_code", 400);
+    resp.put("root.<xmlattr>.status_message", "Invalid client signature");
   }
   return resp;
 }

src/moonlight-server/rest/endpoints.hpp

@@ -79,11 +79,139 @@ void serverinfo(const std::shared_ptr<typename SimpleWeb::Server<T>::Response> &
   send_xml<T>(response, SimpleWeb::StatusCode::success_ok, xml);
 }
 
-template <class T>
-void pair(const std::shared_ptr<typename SimpleWeb::Server<T>::Response> &response,
-          const std::shared_ptr<typename SimpleWeb::Server<T>::Request> &request,
+void remove_pair_session(const immer::box<state::AppState> &state, const std::string &cache_key) {
+  state->pairing_cache->update([&cache_key](const immer::map<std::string, state::PairCache> &pairing_cache) {
+    return pairing_cache.erase(cache_key);
+  });
+}
+
+XML fail_pair(const std::string &status_msg) {
+  logs::log(logs::warning, "Failed pairing: {}", status_msg);
+
+  XML tree;
+  tree.put("root.paired", 0);
+  tree.put("root.<xmlattr>.status_code", 400);
+  tree.put("root.<xmlattr>.status_message", status_msg);
+
+  return tree;
+}
+
+struct XMLResult {
+  SimpleWeb::StatusCode status;
+  XML xml;
+};
+
+std::shared_ptr<boost::promise<XMLResult>> pair_phase1(const immer::box<state::AppState> &state,
+                                                       const std::string &client_ip,
+                                                       const std::string &host_ip,
+                                                       const std::string &client_cert_str,
+                                                       const std::string &salt,
+                                                       const std::string &cache_key) {
+  auto future_result = std::make_shared<boost::promise<XMLResult>>();
+  if (state->pairing_cache->load()->find(cache_key)) {
+    future_result->set_value(
+        {SimpleWeb::StatusCode::client_error_bad_request, fail_pair("Out of order pair request (phase 1)")});
+    remove_pair_session(state, cache_key);
+    return future_result;
+  }
+
+  auto future_pin = std::make_shared<boost::promise<std::string>>();
+  state->event_bus->fire_event( // Emit a signal and wait for the promise to be fulfilled
+      immer::box<events::PairSignal>(
+          events::PairSignal{.client_ip = client_ip, .host_ip = host_ip, .user_pin = future_pin}));
+
+  future_pin->get_future().then(
+      [state, salt, client_cert_str, cache_key, future_result](boost::future<std::string> fut_pin) {
+        auto server_pem = x509::get_cert_pem(state->host->server_cert);
+        auto result = moonlight::pair::get_server_cert(fut_pin.get(), salt, server_pem);
+
+        auto client_cert_parsed = crypto::hex_to_str(client_cert_str, true);
+
+        state->pairing_cache->update([&](const immer::map<std::string, state::PairCache> &pairing_cache) {
+          return pairing_cache.set(cache_key,
+                                   {.client_cert = client_cert_parsed,
+                                    .aes_key = result.second,
+                                    .last_phase = state::PAIR_PHASE::GETSERVERCERT});
+        });
+
+        future_result->set_value({SimpleWeb::StatusCode::success_ok, result.first});
+      });
+
+  return future_result;
+}
+
+XMLResult pair_phase2(const immer::box<state::AppState> &state,
+                      state::PairCache &client_cache,
+                      const std::string &client_challenge,
+                      const std::string &cache_key) {
+  if (client_cache.last_phase != state::PAIR_PHASE::GETSERVERCERT) {
+    return {SimpleWeb::StatusCode::client_error_bad_request, fail_pair("Out of order pair request (phase 2)")};
+  }
+  client_cache.last_phase = state::PAIR_PHASE::CLIENTCHALLENGE;
+
+  auto server_cert_signature = x509::get_cert_signature(state->host->server_cert);
+  auto [xml, server_secret_pair] =
+      moonlight::pair::send_server_challenge(client_cache.aes_key, client_challenge, server_cert_signature);
+
+  auto [server_secret, server_challenge] = server_secret_pair;
+  client_cache.server_secret = server_secret;
+  client_cache.server_challenge = server_challenge;
+  state->pairing_cache->update([&](const immer::map<std::string, state::PairCache> &pairing_cache) {
+    return pairing_cache.set(cache_key, client_cache);
+  });
+
+  return {SimpleWeb::StatusCode::success_ok, xml};
+}
+
+XMLResult pair_phase3(const immer::box<state::AppState> &state,
+                      state::PairCache &client_cache,
+                      const std::string &server_challenge,
+                      const std::string &cache_key) {
+  if (client_cache.last_phase != state::PAIR_PHASE::CLIENTCHALLENGE) {
+    return {SimpleWeb::StatusCode::client_error_bad_request, fail_pair("Out of order pair request (phase 3)")};
+  }
+  client_cache.last_phase = state::PAIR_PHASE::SERVERCHALLENGERESP;
+
+  auto [xml, client_hash] = moonlight::pair::get_client_hash(client_cache.aes_key,
+                                                             client_cache.server_secret.value(),
+                                                             server_challenge,
+                                                             x509::get_pkey_content(state->host->server_pkey));
+  client_cache.client_hash = client_hash;
+  state->pairing_cache->update([&](const immer::map<std::string, state::PairCache> &pairing_cache) {
+    return pairing_cache.set(cache_key, client_cache);
+  });
+  return {SimpleWeb::StatusCode::success_ok, xml};
+}
+
+XMLResult pair_phase4(state::PairCache &client_cache, const std::string &client_secret) {
+  if (client_cache.last_phase != state::PAIR_PHASE::SERVERCHALLENGERESP) {
+    return {SimpleWeb::StatusCode::client_error_bad_request, fail_pair("Out of order pair request (phase 4)")};
+  }
+  client_cache.last_phase = state::PAIR_PHASE::CLIENTPAIRINGSECRET;
+
+  auto client_cert = x509::cert_from_string(client_cache.client_cert);
+
+  if (!client_cert) {
+    return {SimpleWeb::StatusCode::client_error_bad_request, fail_pair("Unable to parse client certificate")};
+  }
+
+  auto client_sig = x509::get_cert_signature(client_cert);
+  auto public_key = x509::get_cert_public_key(client_cert);
+  auto xml = moonlight::pair::client_pair(client_cache.aes_key,
+                                          client_cache.server_challenge.value(),
+                                          client_cache.client_hash.value(),
+                                          client_secret,
+                                          client_sig,
+                                          public_key);
+
+  auto is_paired = xml.get<int>("root.paired") == 1;
+  return {is_paired ? SimpleWeb::StatusCode::success_ok : SimpleWeb::StatusCode::client_error_bad_request, xml};
+}
+
+void pair(const std::shared_ptr<typename SimpleWeb::Server<SimpleWeb::HTTP>::Response> &response,
+          const std::shared_ptr<typename SimpleWeb::Server<SimpleWeb::HTTP>::Request> &request,
           const immer::box<state::AppState> &state) {
-  log_req<T>(request);
+  log_req<SimpleWeb::HTTP>(request);
 
   SimpleWeb::CaseInsensitiveMultimap headers = request->parse_query_string();
   auto salt = get_header(headers, "salt");
@@ -92,7 +220,9 @@ void pair(const std::shared_ptr<typename SimpleWeb::Server<T>::Response> &respon
   auto client_ip = request->remote_endpoint().address().to_string();
 
   if (!client_id) {
-    logs::log(logs::warning, "Received pair request without uniqueid, stopping.");
+    send_xml<SimpleWeb::HTTP>(response,
+                              SimpleWeb::StatusCode::client_error_bad_request,
+                              fail_pair("Received pair request without uniqueid, stopping."));
     return;
   }
 
@@ -101,90 +231,58 @@ void pair(const std::shared_ptr<typename SimpleWeb::Server<T>::Response> &respon
 
   // PHASE 1
   if (client_id && salt && client_cert_str) {
-    auto future_pin = std::make_shared<boost::promise<std::string>>();
-    state->event_bus->fire_event( // Emit a signal and wait for the promise to be fulfilled
-        immer::box<events::PairSignal>(events::PairSignal{.client_ip = client_ip,
-                                                          .host_ip = get_host_ip<T>(request, state),
-                                                          .user_pin = future_pin}));
-
-    future_pin->get_future().then(
-        [state, salt, client_cert_str, cache_key, client_id, response](boost::future<std::string> fut_pin) {
-          auto server_pem = x509::get_cert_pem(state->host->server_cert);
-          auto result = moonlight::pair::get_server_cert(fut_pin.get(), salt.value(), server_pem);
-
-          auto client_cert_parsed = crypto::hex_to_str(client_cert_str.value(), true);
-
-          state->pairing_cache->update([&](const immer::map<std::string, state::PairCache> &pairing_cache) {
-            return pairing_cache.set(cache_key, {.client_cert = client_cert_parsed, .aes_key = result.second});
-          });
-
-          send_xml<T>(response, SimpleWeb::StatusCode::success_ok, result.first);
-        });
-
+    auto future_result = pair_phase1(state,
+                                     client_ip,
+                                     get_host_ip<SimpleWeb::HTTP>(request, state),
+                                     client_cert_str.value(),
+                                     salt.value(),
+                                     cache_key);
+    future_result->get_future().then([response](boost::future<XMLResult> result) {
+      auto [status, xml] = result.get();
+      send_xml<SimpleWeb::HTTP>(response, status, xml);
+    });
     return;
   }
 
   auto client_cache_it = state->pairing_cache->load()->find(cache_key);
   if (client_cache_it == nullptr) {
-    logs::log(logs::warning, "Unable to find {} {} in the pairing cache", client_id.value(), client_ip);
+    send_xml<SimpleWeb::HTTP>(
+        response,
+        SimpleWeb::StatusCode::client_error_bad_request,
+        fail_pair(fmt::format("Unable to find {} {} in the pairing cache", client_id.value(), client_ip)));
     return;
   }
   auto client_cache = *client_cache_it;
 
   // PHASE 2
   auto client_challenge = get_header(headers, "clientchallenge");
   if (client_challenge) {
-
-    auto server_cert_signature = x509::get_cert_signature(state->host->server_cert);
-    auto [xml, server_secret_pair] =
-        moonlight::pair::send_server_challenge(client_cache.aes_key, client_challenge.value(), server_cert_signature);
-
-    auto [server_secret, server_challenge] = server_secret_pair;
-    client_cache.server_secret = server_secret;
-    client_cache.server_challenge = server_challenge;
-    state->pairing_cache->update([&](const immer::map<std::string, state::PairCache> &pairing_cache) {
-      return pairing_cache.set(cache_key, client_cache);
-    });
-
-    send_xml<T>(response, SimpleWeb::StatusCode::success_ok, xml);
+    auto [status, xml] = pair_phase2(state, client_cache, client_challenge.value(), cache_key);
+    send_xml<SimpleWeb::HTTP>(response, status, xml);
+    if (status != SimpleWeb::StatusCode::success_ok) {
+      remove_pair_session(state, cache_key); // security measure, remove the session if the pairing failed
+    }
     return;
   }
 
   // PHASE 3
   auto server_challenge = get_header(headers, "serverchallengeresp");
   if (server_challenge && client_cache.server_secret) {
-
-    auto [xml, client_hash] = moonlight::pair::get_client_hash(client_cache.aes_key,
-                                                               client_cache.server_secret.value(),
-                                                               server_challenge.value(),
-                                                               x509::get_pkey_content(state->host->server_pkey));
-
-    client_cache.client_hash = client_hash;
-
-    state->pairing_cache->update([&](const immer::map<std::string, state::PairCache> &pairing_cache) {
-      return pairing_cache.set(cache_key, client_cache);
-    });
-
-    send_xml<T>(response, SimpleWeb::StatusCode::success_ok, xml);
+    auto [status, xml] = pair_phase3(state, client_cache, server_challenge.value(), cache_key);
+    send_xml<SimpleWeb::HTTP>(response, status, xml);
+    if (status != SimpleWeb::StatusCode::success_ok) {
+      remove_pair_session(state, cache_key); // security measure, remove the session if the pairing failed
+    }
     return;
   }
 
   // PHASE 4
   auto client_secret = get_header(headers, "clientpairingsecret");
   if (client_secret && client_cache.server_challenge && client_cache.client_hash) {
-    auto client_cert = x509::cert_from_string({client_cache.client_cert});
-
-    auto xml = moonlight::pair::client_pair(client_cache.aes_key,
-                                            client_cache.server_challenge.value(),
-                                            client_cache.client_hash.value(),
-                                            client_secret.value(),
-                                            x509::get_cert_signature(client_cert),
-                                            x509::get_cert_public_key(client_cert));
+    auto [status, xml] = pair_phase4(client_cache, client_secret.value());
+    send_xml<SimpleWeb::HTTP>(response, status, xml);
 
-    send_xml<T>(response, SimpleWeb::StatusCode::success_ok, xml);
-
-    auto is_paired = xml.template get<int>("root.paired");
-    if (is_paired == 1) {
+    if (status == SimpleWeb::StatusCode::success_ok) {
       state::pair(
           state->config,
           state::PairedClient{.client_cert = client_cache.client_cert,
@@ -193,31 +291,34 @@ void pair(const std::shared_ptr<typename SimpleWeb::Server<T>::Response> &respon
     } else {
       logs::log(logs::warning, "Failed pairing with {}", client_ip);
     }
+
+    remove_pair_session(state, cache_key); // Either case, this session is done
     return;
   }
 
-  // PHASE 5 (over HTTPS)
+  logs::log(logs::warning, "Unable to match pair with any phase, you can retry pairing from Moonlight");
+}
+
+namespace https {
+
+/**
+ * The check here is implicit, by running over HTTPS we are checking the client certificate
+ */
+void pair(const std::shared_ptr<typename SimpleWeb::Server<SimpleWeb::HTTPS>::Response> &response,
+          const std::shared_ptr<typename SimpleWeb::Server<SimpleWeb::HTTPS>::Request> &request) {
+  SimpleWeb::CaseInsensitiveMultimap headers = request->parse_query_string();
   auto phrase = get_header(headers, "phrase");
+  // PHASE 5 (over HTTPS)
   if (phrase && phrase.value() == "pairchallenge") {
     XML xml;
 
     xml.put("root.paired", 1);
     xml.put("root.<xmlattr>.status_code", 200);
 
-    // Cleanup temporary pairing_cache
-    state->pairing_cache->update([&cache_key](const immer::map<std::string, state::PairCache> &pairing_cache) {
-      return pairing_cache.erase(cache_key);
-    });
-
-    send_xml<T>(response, SimpleWeb::StatusCode::success_ok, xml);
-    return;
+    send_xml<SimpleWeb::HTTPS>(response, SimpleWeb::StatusCode::success_ok, xml);
   }
-
-  logs::log(logs::warning, "Unable to match pair with any phase, you can retry pairing from Moonlight");
 }
 
-namespace https {
-
 void applist(const std::shared_ptr<typename SimpleWeb::Server<SimpleWeb::HTTPS>::Response> &response,
              const std::shared_ptr<typename SimpleWeb::Server<SimpleWeb::HTTPS>::Request> &request,
              const immer::box<state::AppState> &state) {

src/moonlight-server/rest/servers.cpp

@@ -29,9 +29,7 @@ void startServer(HttpServer *server, const immer::box<state::AppState> state, in
     endpoints::serverinfo<SimpleWeb::HTTP>(resp, req, state);
   };
 
-  server->resource["^/pair$"]["GET"] = [&state](auto resp, auto req) {
-    endpoints::pair<SimpleWeb::HTTP>(resp, req, state);
-  };
+  server->resource["^/pair$"]["GET"] = [&state](auto resp, auto req) { endpoints::pair(resp, req, state); };
 
   auto pairing_atom = state->pairing_atom;
 
@@ -121,7 +119,7 @@ void startServer(HttpsServer *server, const immer::box<state::AppState> state, i
 
   server->resource["^/pair$"]["GET"] = [&state](auto resp, auto req) {
     if (get_client_if_paired(state, req)) {
-      endpoints::pair<SimpleWeb::HTTPS>(resp, req, state);
+      endpoints::https::pair(resp, req);
     } else {
       reply_unauthorized(req, resp);
     }