From 73aa76dc5f4afb1b0b6f7637f571af7d3b470701 Mon Sep 17 00:00:00 2001 From: levlam Date: Thu, 9 Aug 2018 23:41:01 +0300 Subject: [PATCH] Pre-check DH parameters. GitOrigin-RevId: 98a488559b95651eab0c43b747a1f1fae2b716a5 --- benchmark/bench_handshake.cpp | 5 +++-- td/mtproto/crypto.cpp | 15 ++++++++------- td/mtproto/crypto.h | 6 +++--- td/telegram/CallActor.cpp | 12 +++++++++--- td/telegram/SecretChatActor.cpp | 9 +++++---- td/telegram/SecretChatActor.h | 2 +- 6 files changed, 29 insertions(+), 20 deletions(-) diff --git a/benchmark/bench_handshake.cpp b/benchmark/bench_handshake.cpp index 08d04f00..08845aed 100644 --- a/benchmark/bench_handshake.cpp +++ b/benchmark/bench_handshake.cpp @@ -54,13 +54,14 @@ class HandshakeBench : public Benchmark { DhHandshake a; DhHandshake b; auto prime = base64url_decode(prime_base64).move_as_ok(); + DhHandshake::check_config(g, prime, &dh_callback).ensure(); for (int i = 0; i < n; i += 2) { a.set_config(g, prime); b.set_config(g, prime); b.set_g_a(a.get_g_b()); a.set_g_a(b.get_g_b()); - a.run_checks(&dh_callback).ensure(); - b.run_checks(&dh_callback).ensure(); + a.run_checks(true, &dh_callback).ensure(); + b.run_checks(true, &dh_callback).ensure(); auto a_key = a.gen_key(); auto b_key = b.gen_key(); CHECK(a_key.first == b_key.first); diff --git a/td/mtproto/crypto.cpp b/td/mtproto/crypto.cpp index ce2fda5c..59e28799 100644 --- a/td/mtproto/crypto.cpp +++ b/td/mtproto/crypto.cpp @@ -208,10 +208,7 @@ Status DhHandshake::check_config(Slice prime_str, const BigNum &prime, int32 g_i return Status::OK(); } -Status DhHandshake::dh_check(Slice prime_str, const BigNum &prime, int32 g_int, const BigNum &g_a, const BigNum &g_b, - BigNumContext &ctx, DhCallback *callback) { - TRY_STATUS(check_config(prime_str, prime, g_int, ctx, callback)); - +Status DhHandshake::dh_check(const BigNum &prime, const BigNum &g_a, const BigNum &g_b) { // IMPORTANT: Apart from the conditions on the Diffie-Hellman prime dh_prime and generator g, both sides are // to check that g, g_a and g_b are greater than 1 and less than dh_prime - 1. // We recommend checking that g_a and g_b are between 2^{2048-64} and dh_prime - 2^{2048-64} as well. @@ -305,14 +302,18 @@ string DhHandshake::get_g_b_hash() const { return g_b_hash; } -Status DhHandshake::run_checks(DhCallback *callback) { +Status DhHandshake::run_checks(bool skip_config_check, DhCallback *callback) { CHECK(has_g_a_ && has_config_); if (has_g_a_hash_ && !ok_g_a_hash_) { return Status::Error("g_a_hash mismatch"); } - return dh_check(prime_str_, prime_, g_int_, g_a_, g_b_, ctx_, callback); + if (!skip_config_check) { + TRY_STATUS(check_config(prime_str_, prime_, g_int_, ctx_, callback)); + } + + return dh_check(prime_, g_a_, g_b_); } BigNum DhHandshake::get_g() const { @@ -354,7 +355,7 @@ Status dh_handshake(int g_int, Slice prime_str, Slice g_a_str, string *g_b_str, DhHandshake handshake; handshake.set_config(g_int, prime_str); handshake.set_g_a(g_a_str); - TRY_STATUS(handshake.run_checks(callback)); + TRY_STATUS(handshake.run_checks(false, callback)); *g_b_str = handshake.get_g_b(); *g_ab_str = handshake.gen_key().second; return Status::OK(); diff --git a/td/mtproto/crypto.h b/td/mtproto/crypto.h index 69614645..39e4cca7 100644 --- a/td/mtproto/crypto.h +++ b/td/mtproto/crypto.h @@ -73,7 +73,7 @@ class DhHandshake { string get_g_a() const; string get_g_b() const; string get_g_b_hash() const; - Status run_checks(DhCallback *callback) TD_WARN_UNUSED_RESULT; + Status run_checks(bool skip_config_check, DhCallback *callback) TD_WARN_UNUSED_RESULT; BigNum get_g() const; BigNum get_p() const; @@ -137,8 +137,8 @@ class DhHandshake { private: static Status check_config(Slice prime_str, const BigNum &prime, int32 g_int, BigNumContext &ctx, DhCallback *callback) TD_WARN_UNUSED_RESULT; - static Status dh_check(Slice prime_str, const BigNum &prime, int32 g_int, const BigNum &g_a, const BigNum &g_b, - BigNumContext &ctx, DhCallback *callback) TD_WARN_UNUSED_RESULT; + + static Status dh_check(const BigNum &prime, const BigNum &g_a, const BigNum &g_b) TD_WARN_UNUSED_RESULT; string prime_str_; BigNum prime_; diff --git a/td/telegram/CallActor.cpp b/td/telegram/CallActor.cpp index 203ba711..a2ffb72c 100644 --- a/td/telegram/CallActor.cpp +++ b/td/telegram/CallActor.cpp @@ -322,7 +322,7 @@ Status CallActor::do_update_call(telegram_api::phoneCallAccepted &call) { LOG(DEBUG) << "Do update call to Accepted"; dh_handshake_.set_g_a(call.g_b_.as_slice()); - TRY_STATUS(dh_handshake_.run_checks(DhCache::instance())); + TRY_STATUS(dh_handshake_.run_checks(true, DhCache::instance())); std::tie(call_state_.key_fingerprint, call_state_.key) = dh_handshake_.gen_key(); state_ = State::SendConfirmQuery; on_begin_exchanging_key(); @@ -348,7 +348,7 @@ Status CallActor::do_update_call(telegram_api::phoneCall &call) { LOG(DEBUG) << "Do update call to Ready from state " << static_cast(state_); if (state_ == State::WaitAcceptResult) { dh_handshake_.set_g_a(call.g_a_or_b_.as_slice()); - TRY_STATUS(dh_handshake_.run_checks(DhCache::instance())); + TRY_STATUS(dh_handshake_.run_checks(true, DhCache::instance())); std::tie(call_state_.key_fingerprint, call_state_.key) = dh_handshake_.gen_key(); } if (call_state_.key_fingerprint != call.key_fingerprint_) { @@ -430,8 +430,14 @@ void CallActor::on_dh_config(Result> r_dh_config, bool if (r_dh_config.is_error()) { return on_error(r_dh_config.move_as_error()); } - dh_config_ready_ = true; + dh_config_ = r_dh_config.move_as_ok(); + auto check_result = DhHandshake::check_config(dh_config_->g, dh_config_->prime, DhCache::instance()); + if (check_result.is_error()) { + return on_error(std::move(check_result)); + } + + dh_config_ready_ = true; yield(); } diff --git a/td/telegram/SecretChatActor.cpp b/td/telegram/SecretChatActor.cpp index 0ac3f377..09cdfe0b 100644 --- a/td/telegram/SecretChatActor.cpp +++ b/td/telegram/SecretChatActor.cpp @@ -574,7 +574,7 @@ Status SecretChatActor::run_auth() { if (!auth_state_.handshake.has_config()) { return Status::OK(); } - TRY_STATUS(auth_state_.handshake.run_checks(context_->dh_callback())); + TRY_STATUS(auth_state_.handshake.run_checks(true, context_->dh_callback())); auto id_and_key = auth_state_.handshake.gen_key(); pfs_state_.auth_key = mtproto::AuthKey(id_and_key.first, std::move(id_and_key.second)); calc_key_hash(); @@ -1859,7 +1859,7 @@ Status SecretChatActor::on_update_chat(telegram_api::encryptedChat &update) { TRY_STATUS(save_common_info(update)); if (auth_state_.state == State::WaitRequestResponse) { auth_state_.handshake.set_g_a(update.g_a_or_b_.as_slice()); - TRY_STATUS(auth_state_.handshake.run_checks(context_->dh_callback())); + TRY_STATUS(auth_state_.handshake.run_checks(true, context_->dh_callback())); auto id_and_key = auth_state_.handshake.gen_key(); pfs_state_.auth_key = mtproto::AuthKey(id_and_key.first, std::move(id_and_key.second)); calc_key_hash(); @@ -1974,6 +1974,7 @@ Status SecretChatActor::on_dh_config(NetQueryPtr query) { LOG(INFO) << "Got dh config"; TRY_RESULT(config, fetch_result(std::move(query))); downcast_call(*config, [&](auto &obj) { this->on_dh_config(obj); }); + TRY_STATUS(DhHandshake::check_config(auth_state_.dh_config.g, auth_state_.dh_config.prime, context_->dh_callback())); auth_state_.handshake.set_config(auth_state_.dh_config.g, auth_state_.dh_config.prime); return Status::OK(); } @@ -2147,7 +2148,7 @@ Status SecretChatActor::on_inbound_action(secret_api::decryptedMessageActionRequ pfs_state_.exchange_id = request_key.exchange_id_; pfs_state_.handshake.set_config(auth_state_.dh_config.g, auth_state_.dh_config.prime); pfs_state_.handshake.set_g_a(request_key.g_a_.as_slice()); - TRY_STATUS(pfs_state_.handshake.run_checks(context_->dh_callback())); + TRY_STATUS(pfs_state_.handshake.run_checks(true, context_->dh_callback())); auto id_and_key = pfs_state_.handshake.gen_key(); pfs_state_.other_auth_key = mtproto::AuthKey(id_and_key.first, std::move(id_and_key.second)); @@ -2167,7 +2168,7 @@ Status SecretChatActor::on_inbound_action(secret_api::decryptedMessageActionAcce return Status::Error("AcceptKey: exchange_id mismatch"); } pfs_state_.handshake.set_g_a(accept_key.g_b_.as_slice()); - TRY_STATUS(pfs_state_.handshake.run_checks(context_->dh_callback())); + TRY_STATUS(pfs_state_.handshake.run_checks(true, context_->dh_callback())); auto id_and_key = pfs_state_.handshake.gen_key(); if (static_cast(id_and_key.first) != accept_key.key_fingerprint_) { return Status::Error("AcceptKey: key_fingerprint mismatch"); diff --git a/td/telegram/SecretChatActor.h b/td/telegram/SecretChatActor.h index a93629c2..72bfcba6 100644 --- a/td/telegram/SecretChatActor.h +++ b/td/telegram/SecretChatActor.h @@ -137,7 +137,7 @@ class SecretChatActor : public NetQueryCallback { enum class State : int32 { Empty, SendRequest, SendAccept, WaitRequestResponse, WaitAcceptResponse, Ready, Closed }; static constexpr int32 MAX_RESEND_COUNT = 1000; - // We have git state that should be shynchronized with db. + // We have git state that should be synchronized with db. // It is splitted into several parts because: // 1. Some parts are BIG (auth_key, for example) and are rarely updated. // 2. Other are frequently updated, so probably should be as small as possible.