From 61d7aa04003637dd63de9708c5a5e72ad844620d Mon Sep 17 00:00:00 2001 From: Cameron Gutman Date: Sat, 22 Dec 2018 19:55:28 -0800 Subject: [PATCH] Require cert pinning for HTTPS --- app/backend/computermanager.cpp | 113 ++++++++++++++++++++----------- app/backend/nvhttp.cpp | 65 +++++++++++------- app/backend/nvhttp.h | 2 + app/backend/nvpairingmanager.cpp | 13 +++- 4 files changed, 128 insertions(+), 65 deletions(-) diff --git a/app/backend/computermanager.cpp b/app/backend/computermanager.cpp index d6357e47..f0370a49 100644 --- a/app/backend/computermanager.cpp +++ b/app/backend/computermanager.cpp @@ -538,12 +538,8 @@ signals: void computerStateChanged(NvComputer* computer); private: - void run() + QString fetchServerInfo(NvHTTP& http) { - NvHTTP http(m_Address, QSslCertificate()); - - qInfo() << "Processing new PC at" << m_Address << "from" << (m_Mdns ? "mDNS" : "user"); - QString serverInfo; try { // There's a race condition between GameStream servers reporting presence over @@ -564,15 +560,52 @@ private: throw e; } } + return serverInfo; } catch (...) { if (!m_Mdns) { emit computerAddCompleted(false); } + return QString(); + } + } + + void run() + { + NvHTTP http(m_Address, QSslCertificate()); + + qInfo() << "Processing new PC at" << m_Address << "from" << (m_Mdns ? "mDNS" : "user"); + + // Perform initial serverinfo fetch over HTTP since we don't know which cert to use + QString serverInfo = fetchServerInfo(http); + if (serverInfo.isEmpty()) { return; } + // Create initial newComputer using HTTP serverinfo with no pinned cert NvComputer* newComputer = new NvComputer(m_Address, serverInfo, QSslCertificate()); + // Check if we have a record of this host UUID to pull the pinned cert + NvComputer* existingComputer; + { + QReadLocker lock(&m_ComputerManager->m_Lock); + existingComputer = m_ComputerManager->m_KnownHosts[newComputer->uuid]; + if (existingComputer != nullptr) { + http.setServerCert(existingComputer->serverCert); + } + } + + // Fetch serverinfo again over HTTPS with the pinned cert + if (existingComputer != nullptr) { + serverInfo = fetchServerInfo(http); + if (serverInfo.isEmpty()) { + return; + } + + // Update the polled computer with the HTTPS information + NvComputer httpsComputer(m_Address, serverInfo, QSslCertificate()); + newComputer->update(httpsComputer); + } + // Update addresses depending on the context if (m_Mdns) { newComputer->localAddress = m_Address; @@ -591,46 +624,48 @@ private: newComputer->manualAddress = m_Address; } - // Check if this PC already exists - QWriteLocker lock(&m_ComputerManager->m_Lock); - NvComputer* existingComputer = m_ComputerManager->m_KnownHosts[newComputer->uuid]; - if (existingComputer != nullptr) { - // Fold it into the existing PC - bool changed = existingComputer->update(*newComputer); - delete newComputer; + { + // Check if this PC already exists + QWriteLocker lock(&m_ComputerManager->m_Lock); + NvComputer* existingComputer = m_ComputerManager->m_KnownHosts[newComputer->uuid]; + if (existingComputer != nullptr) { + // Fold it into the existing PC + bool changed = existingComputer->update(*newComputer); + delete newComputer; - // Drop the lock before notifying - lock.unlock(); + // Drop the lock before notifying + lock.unlock(); - // For non-mDNS clients, let them know it succeeded - if (!m_Mdns) { - emit computerAddCompleted(true); + // For non-mDNS clients, let them know it succeeded + if (!m_Mdns) { + emit computerAddCompleted(true); + } + + // Tell our client if something changed + if (changed) { + qInfo() << existingComputer->name << "is now at" << existingComputer->activeAddress; + emit computerStateChanged(existingComputer); + } } + else { + // Store this in our active sets + m_ComputerManager->m_KnownHosts[newComputer->uuid] = newComputer; - // Tell our client if something changed - if (changed) { - qInfo() << existingComputer->name << "is now at" << existingComputer->activeAddress; - emit computerStateChanged(existingComputer); + // Start polling if enabled (write lock required) + m_ComputerManager->startPollingComputer(newComputer); + + // Drop the lock before notifying + lock.unlock(); + + // For non-mDNS clients, let them know it succeeded + if (!m_Mdns) { + emit computerAddCompleted(true); + } + + // Tell our client about this new PC + emit computerStateChanged(newComputer); } } - else { - // Store this in our active sets - m_ComputerManager->m_KnownHosts[newComputer->uuid] = newComputer; - - // Start polling if enabled (write lock required) - m_ComputerManager->startPollingComputer(newComputer); - - // Drop the lock before notifying - lock.unlock(); - - // For non-mDNS clients, let them know it succeeded - if (!m_Mdns) { - emit computerAddCompleted(true); - } - - // Tell our client about this new PC - emit computerStateChanged(newComputer); - } } ComputerManager* m_ComputerManager; diff --git a/app/backend/nvhttp.cpp b/app/backend/nvhttp.cpp index edb6095c..cade2957 100644 --- a/app/backend/nvhttp.cpp +++ b/app/backend/nvhttp.cpp @@ -32,6 +32,11 @@ NvHTTP::NvHTTP(QString address, QSslCertificate serverCert) : m_Nam.setProxy(noProxy); } +void NvHTTP::setServerCert(QSslCertificate serverCert) +{ + m_ServerCert = serverCert; +} + QVector NvHTTP::parseQuad(QString quad) { @@ -74,36 +79,50 @@ NvHTTP::getServerInfo(NvLogLevel logLevel) { QString serverInfo; - try + // Check if we have a pinned cert for this host yet + if (!m_ServerCert.isNull()) { - // Always try HTTPS first, since it properly reports - // pairing status (and a few other attributes). - serverInfo = openConnectionToString(m_BaseUrlHttps, - "serverinfo", - nullptr, - true, - logLevel); - // Throws if the request failed - verifyResponseStatus(serverInfo); - } - catch (const GfeHttpResponseException& e) - { - if (e.getStatusCode() == 401) + try { - // Certificate validation error, fallback to HTTP - serverInfo = openConnectionToString(m_BaseUrlHttp, + // Always try HTTPS first, since it properly reports + // pairing status (and a few other attributes). + serverInfo = openConnectionToString(m_BaseUrlHttps, "serverinfo", nullptr, true, logLevel); + // Throws if the request failed verifyResponseStatus(serverInfo); } - else + catch (const GfeHttpResponseException& e) { - // Rethrow real errors - throw e; + if (e.getStatusCode() == 401) + { + // Certificate validation error, fallback to HTTP + serverInfo = openConnectionToString(m_BaseUrlHttp, + "serverinfo", + nullptr, + true, + logLevel); + verifyResponseStatus(serverInfo); + } + else + { + // Rethrow real errors + throw e; + } } } + else + { + // Only use HTTP prior to pairing + serverInfo = openConnectionToString(m_BaseUrlHttp, + "serverinfo", + nullptr, + true, + logLevel); + verifyResponseStatus(serverInfo); + } return serverInfo; } @@ -391,11 +410,7 @@ NvHTTP::openConnection(QUrl baseUrl, QNetworkReply* reply = m_Nam.get(request); - if (m_ServerCert.isNull()) { - // No server cert yet - reply->ignoreSslErrors(); - } - else { + if (!m_ServerCert.isNull()) { // Pin the server certificate received during pairing QList expectedSslErrors; expectedSslErrors.append(QSslError(QSslError::HostNameMismatch, m_ServerCert)); @@ -435,7 +450,7 @@ NvHTTP::openConnection(QUrl baseUrl, qWarning() << command << " request failed with error " << reply->error(); } - if (!m_ServerCert.isNull() && reply->error() == QNetworkReply::SslHandshakeFailedError) { + if (reply->error() == QNetworkReply::SslHandshakeFailedError) { // This will trigger falling back to HTTP for the serverinfo query // then pairing again to get the updated certificate. GfeHttpResponseException exception(401, "Server certificate mismatch"); diff --git a/app/backend/nvhttp.h b/app/backend/nvhttp.h index 9a59f777..014d87cf 100644 --- a/app/backend/nvhttp.h +++ b/app/backend/nvhttp.h @@ -152,6 +152,8 @@ public: bool enableTimeout, NvLogLevel logLevel = NvLogLevel::VERBOSE); + void setServerCert(QSslCertificate serverCert); + static QVector parseQuad(QString quad); diff --git a/app/backend/nvpairingmanager.cpp b/app/backend/nvpairingmanager.cpp index 5d195315..9e3e4455 100644 --- a/app/backend/nvpairingmanager.cpp +++ b/app/backend/nvpairingmanager.cpp @@ -208,6 +208,18 @@ NvPairingManager::pair(QString appVersion, QString pin, QSslCertificate& serverC return PairState::ALREADY_IN_PROGRESS; } + serverCert = QSslCertificate(serverCertStr); + if (serverCert.isNull()) { + Q_ASSERT(!serverCert.isNull()); + + qCritical() << "Failed to parse plaincert"; + m_Http.openConnectionToString(m_Http.m_BaseUrlHttp, "unpair", nullptr, true); + return PairState::FAILED; + } + + // Pin this cert for TLS + m_Http.setServerCert(serverCert); + QByteArray randomChallenge = generateRandomBytes(16); QByteArray encryptedChallenge = encrypt(randomChallenge, &encKey); QString challengeXml = m_Http.openConnectionToString(m_Http.m_BaseUrlHttp, @@ -309,6 +321,5 @@ NvPairingManager::pair(QString appVersion, QString pin, QSslCertificate& serverC return PairState::FAILED; } - serverCert = QSslCertificate(serverCertStr); return PairState::PAIRED; }