From 871bffc2d8203073da09b7ac16eec2e8a589725a Mon Sep 17 00:00:00 2001 From: nisse Date: Fri, 10 Mar 2017 00:03:10 -0800 Subject: [PATCH] Replace rtc::CryptString with std::string CryptString is intended for secret data, such as passwords, and the memory is cleared on deallocation. However, where it is used in libjingle_xmpp, the password is copied to other objects without any magic clearing, so the security benefit is questionable. In addition, CryptString is yet another string class, it's unused within webrtc, and essentially unmaintained. If we can replace its use in Chrome, it will be deleted. BUG=webrtc:6424 Review-Url: https://codereview.chromium.org/2738973004 Cr-Commit-Position: refs/heads/master@{#456021} --- .../notifier/base/gaia_token_pre_xmpp_auth.cc | 2 +- jingle/notifier/base/gaia_token_pre_xmpp_auth.h | 2 +- .../notifier/base/xmpp_connection_unittest.cc | 3 +-- .../libjingle_xmpp/xmpp/plainsaslhandler.h | 5 ++--- third_party/libjingle_xmpp/xmpp/prexmppauth.h | 3 +-- .../libjingle_xmpp/xmpp/saslplainmechanism.h | 17 ++++++++--------- third_party/libjingle_xmpp/xmpp/xmppclient.cc | 6 +++--- .../libjingle_xmpp/xmpp/xmppclientsettings.h | 13 ++++++------- .../libjingle_xmpp/xmpp/xmppengine_unittest.cc | 5 ++--- .../xmpp/xmpplogintask_unittest.cc | 6 ++---- 10 files changed, 27 insertions(+), 35 deletions(-) diff --git a/jingle/notifier/base/gaia_token_pre_xmpp_auth.cc b/jingle/notifier/base/gaia_token_pre_xmpp_auth.cc index 26fbfc4375d28b..dec9aebeff52d6 100644 --- a/jingle/notifier/base/gaia_token_pre_xmpp_auth.cc +++ b/jingle/notifier/base/gaia_token_pre_xmpp_auth.cc @@ -65,7 +65,7 @@ GaiaTokenPreXmppAuth::~GaiaTokenPreXmppAuth() { } void GaiaTokenPreXmppAuth::StartPreXmppAuth( const buzz::Jid& jid, const rtc::SocketAddress& server, - const rtc::CryptString& pass, + const std::string& pass, const std::string& auth_mechanism, const std::string& auth_token) { SignalAuthDone(); diff --git a/jingle/notifier/base/gaia_token_pre_xmpp_auth.h b/jingle/notifier/base/gaia_token_pre_xmpp_auth.h index 86da7a04c1aea7..a2c3b3491613bc 100644 --- a/jingle/notifier/base/gaia_token_pre_xmpp_auth.h +++ b/jingle/notifier/base/gaia_token_pre_xmpp_auth.h @@ -29,7 +29,7 @@ class GaiaTokenPreXmppAuth : public buzz::PreXmppAuth { // this point. void StartPreXmppAuth(const buzz::Jid& jid, const rtc::SocketAddress& server, - const rtc::CryptString& pass, + const std::string& pass, const std::string& auth_mechanism, const std::string& auth_token) override; diff --git a/jingle/notifier/base/xmpp_connection_unittest.cc b/jingle/notifier/base/xmpp_connection_unittest.cc index 4e07fcf76f2f25..28a0d713c71887 100644 --- a/jingle/notifier/base/xmpp_connection_unittest.cc +++ b/jingle/notifier/base/xmpp_connection_unittest.cc @@ -30,7 +30,6 @@ class Jid; } // namespace buzz namespace rtc { -class CryptString; class SocketAddress; class Task; } // namespace rtc @@ -52,7 +51,7 @@ class MockPreXmppAuth : public buzz::PreXmppAuth { MOCK_METHOD5(StartPreXmppAuth, void(const buzz::Jid&, const rtc::SocketAddress&, - const rtc::CryptString&, + const std::string&, const std::string&, const std::string&)); MOCK_CONST_METHOD0(IsAuthDone, bool()); diff --git a/third_party/libjingle_xmpp/xmpp/plainsaslhandler.h b/third_party/libjingle_xmpp/xmpp/plainsaslhandler.h index 894570e9c77254..a25d54fbe1cc9b 100644 --- a/third_party/libjingle_xmpp/xmpp/plainsaslhandler.h +++ b/third_party/libjingle_xmpp/xmpp/plainsaslhandler.h @@ -14,13 +14,12 @@ #include #include "third_party/libjingle_xmpp/xmpp/saslhandler.h" #include "third_party/libjingle_xmpp/xmpp/saslplainmechanism.h" -#include "third_party/webrtc/base/cryptstring.h" namespace buzz { class PlainSaslHandler : public SaslHandler { public: - PlainSaslHandler(const Jid & jid, const rtc::CryptString & password, + PlainSaslHandler(const Jid & jid, const std::string & password, bool allow_plain) : jid_(jid), password_(password), allow_plain_(allow_plain) {} @@ -54,7 +53,7 @@ class PlainSaslHandler : public SaslHandler { private: Jid jid_; - rtc::CryptString password_; + std::string password_; bool allow_plain_; }; diff --git a/third_party/libjingle_xmpp/xmpp/prexmppauth.h b/third_party/libjingle_xmpp/xmpp/prexmppauth.h index 92cb6db62a6a73..89d327bca5216f 100644 --- a/third_party/libjingle_xmpp/xmpp/prexmppauth.h +++ b/third_party/libjingle_xmpp/xmpp/prexmppauth.h @@ -12,7 +12,6 @@ #define WEBRTC_LIBJINGLE_XMPP_PREXMPPAUTH_H_ #include "third_party/libjingle_xmpp/xmpp/saslhandler.h" -#include "third_party/webrtc/base/cryptstring.h" #include "third_party/webrtc/base/sigslot.h" namespace rtc { @@ -51,7 +50,7 @@ class PreXmppAuth : public SaslHandler { virtual void StartPreXmppAuth( const Jid& jid, const rtc::SocketAddress& server, - const rtc::CryptString& pass, + const std::string& pass, const std::string& auth_mechanism, const std::string& auth_token) = 0; diff --git a/third_party/libjingle_xmpp/xmpp/saslplainmechanism.h b/third_party/libjingle_xmpp/xmpp/saslplainmechanism.h index b173c524f9f7d1..d7319b483c55a2 100644 --- a/third_party/libjingle_xmpp/xmpp/saslplainmechanism.h +++ b/third_party/libjingle_xmpp/xmpp/saslplainmechanism.h @@ -12,14 +12,13 @@ #define WEBRTC_LIBJINGLE_XMPP_SASLPLAINMECHANISM_H_ #include "third_party/libjingle_xmpp/xmpp/saslmechanism.h" -#include "third_party/webrtc/base/cryptstring.h" namespace buzz { class SaslPlainMechanism : public SaslMechanism { public: - SaslPlainMechanism(const buzz::Jid user_jid, const rtc::CryptString & password) : + SaslPlainMechanism(const buzz::Jid user_jid, const std::string & password) : user_jid_(user_jid), password_(password) {} virtual std::string GetMechanismName() { return "PLAIN"; } @@ -29,18 +28,18 @@ class SaslPlainMechanism : public SaslMechanism { XmlElement * el = new XmlElement(QN_SASL_AUTH, true); el->AddAttr(QN_MECHANISM, "PLAIN"); - rtc::FormatCryptString credential; - credential.Append("\0", 1); - credential.Append(user_jid_.node()); - credential.Append("\0", 1); - credential.Append(&password_); - el->AddText(Base64EncodeFromArray(credential.GetData(), credential.GetLength())); + std::stringstream ss; + ss.write("\0", 1); + ss << user_jid_.node(); + ss.write("\0", 1); + ss << password_; + el->AddText(Base64EncodeFromArray(ss.str().data(), ss.str().length())); return el; } private: Jid user_jid_; - rtc::CryptString password_; + std::string password_; }; } diff --git a/third_party/libjingle_xmpp/xmpp/xmppclient.cc b/third_party/libjingle_xmpp/xmpp/xmppclient.cc index 4860b5b5232f1e..975887382a408b 100644 --- a/third_party/libjingle_xmpp/xmpp/xmppclient.cc +++ b/third_party/libjingle_xmpp/xmpp/xmppclient.cc @@ -50,7 +50,7 @@ class XmppClient::Private : std::unique_ptr socket_; std::unique_ptr engine_; std::unique_ptr pre_auth_; - rtc::CryptString pass_; + std::string pass_; std::string auth_mechanism_; std::string auth_token_; rtc::SocketAddress server_; @@ -208,13 +208,13 @@ int XmppClient::ProcessStart() { d_->pre_auth_->StartPreXmppAuth( d_->engine_->GetUser(), d_->server_, d_->pass_, d_->auth_mechanism_, d_->auth_token_); - d_->pass_.Clear(); // done with this; + d_->pass_.clear(); // done with this; return STATE_PRE_XMPP_LOGIN; } else { d_->engine_->SetSaslHandler(new PlainSaslHandler( d_->engine_->GetUser(), d_->pass_, d_->allow_plain_)); - d_->pass_.Clear(); // done with this; + d_->pass_.clear(); // done with this; return STATE_START_XMPP_LOGIN; } } diff --git a/third_party/libjingle_xmpp/xmpp/xmppclientsettings.h b/third_party/libjingle_xmpp/xmpp/xmppclientsettings.h index a60ab9782d3381..f3ed3a22b02102 100644 --- a/third_party/libjingle_xmpp/xmpp/xmppclientsettings.h +++ b/third_party/libjingle_xmpp/xmpp/xmppclientsettings.h @@ -13,7 +13,6 @@ #include "third_party/webrtc/p2p/base/port.h" #include "third_party/libjingle_xmpp/xmpp/xmppengine.h" -#include "third_party/webrtc/base/cryptstring.h" namespace buzz { @@ -26,7 +25,7 @@ class XmppUserSettings { void set_user(const std::string& user) { user_ = user; } void set_host(const std::string& host) { host_ = host; } - void set_pass(const rtc::CryptString& pass) { pass_ = pass; } + void set_pass(const std::string& pass) { pass_ = pass; } void set_auth_token(const std::string& mechanism, const std::string& token) { auth_mechanism_ = mechanism; @@ -44,7 +43,7 @@ class XmppUserSettings { const std::string& user() const { return user_; } const std::string& host() const { return host_; } - const rtc::CryptString& pass() const { return pass_; } + const std::string& pass() const { return pass_; } const std::string& auth_mechanism() const { return auth_mechanism_; } const std::string& auth_token() const { return auth_token_; } const std::string& resource() const { return resource_; } @@ -56,7 +55,7 @@ class XmppUserSettings { private: std::string user_; std::string host_; - rtc::CryptString pass_; + std::string pass_; std::string auth_mechanism_; std::string auth_token_; std::string resource_; @@ -84,7 +83,7 @@ class XmppClientSettings : public XmppUserSettings { void set_proxy_port(int port) { proxy_port_ = port; }; void set_use_proxy_auth(bool f) { use_proxy_auth_ = f; } void set_proxy_user(const std::string& user) { proxy_user_ = user; } - void set_proxy_pass(const rtc::CryptString& pass) { proxy_pass_ = pass; } + void set_proxy_pass(const std::string& pass) { proxy_pass_ = pass; } const rtc::SocketAddress& server() const { return server_; } cricket::ProtocolType protocol() const { return protocol_; } @@ -93,7 +92,7 @@ class XmppClientSettings : public XmppUserSettings { int proxy_port() const { return proxy_port_; } bool use_proxy_auth() const { return use_proxy_auth_; } const std::string& proxy_user() const { return proxy_user_; } - const rtc::CryptString& proxy_pass() const { return proxy_pass_; } + const std::string& proxy_pass() const { return proxy_pass_; } private: rtc::SocketAddress server_; @@ -103,7 +102,7 @@ class XmppClientSettings : public XmppUserSettings { int proxy_port_; bool use_proxy_auth_; std::string proxy_user_; - rtc::CryptString proxy_pass_; + std::string proxy_pass_; }; } diff --git a/third_party/libjingle_xmpp/xmpp/xmppengine_unittest.cc b/third_party/libjingle_xmpp/xmpp/xmppengine_unittest.cc index 5b9ff966ec2942..5e399154e40b78 100644 --- a/third_party/libjingle_xmpp/xmpp/xmppengine_unittest.cc +++ b/third_party/libjingle_xmpp/xmpp/xmppengine_unittest.cc @@ -62,14 +62,13 @@ class XmppEngineTest : public testing::Test { handler_.reset(new XmppTestHandler(engine_.get())); Jid jid("david@my-server"); - rtc::InsecureCryptStringImpl pass; - pass.password() = "david"; + std::string pass("david"); engine_->SetSessionHandler(handler_.get()); engine_->SetOutputHandler(handler_.get()); engine_->AddStanzaHandler(handler_.get()); engine_->SetUser(jid); engine_->SetSaslHandler( - new buzz::PlainSaslHandler(jid, rtc::CryptString(pass), true)); + new buzz::PlainSaslHandler(jid, pass, true)); } virtual void TearDown() { handler_.reset(); diff --git a/third_party/libjingle_xmpp/xmpp/xmpplogintask_unittest.cc b/third_party/libjingle_xmpp/xmpp/xmpplogintask_unittest.cc index 17bced53b754ee..99bf934b22441d 100644 --- a/third_party/libjingle_xmpp/xmpp/xmpplogintask_unittest.cc +++ b/third_party/libjingle_xmpp/xmpp/xmpplogintask_unittest.cc @@ -19,7 +19,6 @@ #include "third_party/libjingle_xmpp/xmpp/saslplainmechanism.h" #include "third_party/libjingle_xmpp/xmpp/util_unittest.h" #include "third_party/libjingle_xmpp/xmpp/xmppengine.h" -#include "third_party/webrtc/base/cryptstring.h" #include "third_party/webrtc/base/gunit.h" #include "third_party/webrtc/typedefs.h" @@ -52,14 +51,13 @@ class XmppLoginTaskTest : public testing::Test { handler_.reset(new XmppTestHandler(engine_.get())); Jid jid("david@my-server"); - rtc::InsecureCryptStringImpl pass; - pass.password() = "david"; + std::string pass("david"); engine_->SetSessionHandler(handler_.get()); engine_->SetOutputHandler(handler_.get()); engine_->AddStanzaHandler(handler_.get()); engine_->SetUser(jid); engine_->SetSaslHandler( - new buzz::PlainSaslHandler(jid, rtc::CryptString(pass), true)); + new buzz::PlainSaslHandler(jid, pass, true)); } virtual void TearDown() { handler_.reset();