Repository: SpectrumIM/spectrum2 (language: python)

Issue:
Passwordless libpurple backend (read: Telegram) allows easy identity stealing
1. I register with my Telegram phone number on a Spectrum Telegram transport and pass SMS authorization.
2. I log out of the telegram transport.
3. Someone else registers with my Telegram phone number on the same transport.
4. The libpurple backend reuses existing authorization information and allows that other user to connect to Telegram on my behalf!

This seems to be a huge security problem for all Spectrum Telegram transport users, and possibly for all other passwordless libpuprple backends (if any).

I'm using (gateway mode) Spectrum version 2.0.3, libpurple from pidgin 2.12.0, telegram-purple version 1.3.1, running on Gentoo. Jabber server is ejabberd version 16.09.


Discussion / hints:
Here is my transport configuration:
[telegram.cfg.zip](https://github.com/SpectrumIM/spectrum2/files/1196056/telegram.cfg.zip)

The only workaround I can think of would be to somehow completely disable registration with affected transport(s). (Is it even possible without changing the code?)
https://github.com/SpectrumIM/spectrum2/blob/master/spectrum/src/sample2.cfg#L104
The best correction for this would probably be adding the user JID to the names of account directories inside `/var/lib/spectrum2/<transport-jid>/telegram-purple`, so that they were called something like `+1234567890@user@jabber.server` instead of just `+1234567890`. This may turn out to be impossible if libpurple manages these directories without any control from spectrum2_libpurple_backend.

Another, less reliable correction would be to disallow registration if the legacy network username is already user by someone else. Also, when a user de-registers, the associated libpurple backend account needs to be deleted (right now, it's kept intact).
> https://github.com/SpectrumIM/spectrum2/blob/master/spectrum/src/sample2.cfg#L104

This controls public registration (from foreign Jabber servers), but if I disable it, registration from the "local" Jabber server will still be allowed, right?
AFAIR, it only allows to register by admin 
> AFAIR, it only allows to register by admin

Both you and me were wrong. If `registration.enable_public_registration` is zero, only users from Jabber servers listed in `service.allowed_servers` are allowed to register; if `service.allowed_servers` is not defined, even users from the local Jabber server can't register. (Well, probably admins can register anyway - I didn't check this.)

So, setting `registration.enable_public_registration` to zero and ensuring `service.allowed_servers` is not defined serves as a workaround, at least for personal or semi-personal transport instances, where registrations are seldom and can be made/assisted by admin.
Anything new about this issue?
Up

Fix patch:
diff --git a/include/transport/MySQLBackend.h b/include/transport/MySQLBackend.h
index 944299b3b..7256e6f6d 100644
--- a/include/transport/MySQLBackend.h
+++ b/include/transport/MySQLBackend.h
@@ -86,6 +86,8 @@ class MySQLBackend : public StorageBackend
 
 		bool getUsers(std::vector<std::string> &users);
 
+		bool getLegacyNetworkUsers(std::vector<std::string> &users);
+
 		long addBuddy(long userId, const BuddyInfo &buddyInfo);
 
 		void updateBuddy(long userId, const BuddyInfo &buddyInfo);
@@ -160,6 +162,7 @@ class MySQLBackend : public StorageBackend
 		Statement *m_setUserOnline;
 		Statement *m_getOnlineUsers;
 		Statement *m_getUsers;
+		Statement *m_getLegacyNetworkUsers;
 };
 
 }
diff --git a/include/transport/PQXXBackend.h b/include/transport/PQXXBackend.h
index 89ce1e175..8ce96eeef 100644
--- a/include/transport/PQXXBackend.h
+++ b/include/transport/PQXXBackend.h
@@ -85,6 +85,8 @@ class PQXXBackend : public StorageBackend
 
 		bool getUsers(std::vector<std::string> &users);
 
+		bool getLegacyNetworkUsers(std::vector<std::string> &users);
+
 		long addBuddy(long userId, const BuddyInfo &buddyInfo);
 
 		void updateBuddy(long userId, const BuddyInfo &buddyInfo);
diff --git a/include/transport/SQLite3Backend.h b/include/transport/SQLite3Backend.h
index 237b8c407..7a88a45a9 100644
--- a/include/transport/SQLite3Backend.h
+++ b/include/transport/SQLite3Backend.h
@@ -74,6 +74,8 @@ class SQLite3Backend : public StorageBackend
 
 		bool getUsers(std::vector<std::string> &users);
 
+		bool getLegacyNetworkUsers(std::vector<std::string> &users);
+
 		/// Removes user and all connected data from database.
 		/// \param id id of user - UserInfo.id
 		/// \return true if user has been found in database and removed
@@ -127,6 +129,7 @@ class SQLite3Backend : public StorageBackend
 		sqlite3_stmt *m_setUserOnline;
 		sqlite3_stmt *m_getOnlineUsers;
 		sqlite3_stmt *m_getUsers;
+		sqlite3_stmt *m_getLegacyNetworkUsers;
 };
 
 }
diff --git a/include/transport/StorageBackend.h b/include/transport/StorageBackend.h
index cf2f72386..e497588ed 100644
--- a/include/transport/StorageBackend.h
+++ b/include/transport/StorageBackend.h
@@ -125,6 +125,8 @@ class StorageBackend
 
 		virtual bool getUsers(std::vector<std::string> &users) = 0;
 
+		virtual bool getLegacyNetworkUsers(std::vector<std::string> &users) = 0;
+
 		virtual long addBuddy(long userId, const BuddyInfo &buddyInfo) = 0;
 		virtual void updateBuddy(long userId, const BuddyInfo &buddyInfo) = 0;
 		virtual void removeBuddy(long id) = 0;
diff --git a/libtransport/MySQLBackend.cpp b/libtransport/MySQLBackend.cpp
index d848954e0..8a5889454 100644
--- a/libtransport/MySQLBackend.cpp
+++ b/libtransport/MySQLBackend.cpp
@@ -256,6 +256,7 @@ void MySQLBackend::disconnect() {
 	delete m_setUserOnline;
 	delete m_getOnlineUsers;
 	delete m_getUsers;
+	delete m_getLegacyNetworkUsers;
 }
 
 bool MySQLBackend::connect() {
@@ -303,7 +304,7 @@ bool MySQLBackend::connect() {
 	m_setUserOnline = new Statement(&m_conn, "bi", "UPDATE " + m_prefix + "users SET online=?, last_login=NOW()  WHERE id=?");
 	m_getOnlineUsers = new Statement(&m_conn, "|s", "SELECT jid FROM " + m_prefix + "users WHERE online=1");
 	m_getUsers = new Statement(&m_conn, "|s", "SELECT jid FROM " + m_prefix + "users");
-
+	m_getLegacyNetworkUsers = new Statement(&m_conn, "|s", "SELECT uin FROM " + m_prefix + "users");
 	return true;
 }
 
@@ -451,6 +452,20 @@ bool MySQLBackend::getUsers(std::vector<std::string> &users) {
 	return true;
 }
 
+bool MySQLBackend::getLegacyNetworkUsers(std::vector<std::string> &users) {
+	EXEC(m_getLegacyNetworkUsers, getLegacyNetworkUsers(users));
+	if (!exec_ok)
+		return false;
+
+	std::string jid;
+	while (m_getLegacyNetworkUsers->fetch() == 0) {
+		*m_getLegacyNetworkUsers >> jid;
+		users.push_back(jid);
+	}
+
+	return true;
+}
+
 long MySQLBackend::addBuddy(long userId, const BuddyInfo &buddyInfo) {
 // 	"INSERT INTO " + m_prefix + "buddies (user_id, uin, subscription, groups, nickname, flags) VALUES (?, ?, ?, ?, ?, ?)"
 	std::string groups = StorageBackend::serializeGroups(buddyInfo.groups);
diff --git a/libtransport/PQXXBackend.cpp b/libtransport/PQXXBackend.cpp
index b0dccb16c..df148fac2 100644
--- a/libtransport/PQXXBackend.cpp
+++ b/libtransport/PQXXBackend.cpp
@@ -261,6 +261,23 @@ bool PQXXBackend::getUsers(std::vector<std::string> &users) {
 	return true;
 }
 
+bool PQXXBackend::getLegacyNetworkUsers(std::vector<std::string> &users) {
+	try {
+		pqxx::nontransaction txn(*m_conn);
+		pqxx::result r = txn.exec("SELECT uin FROM " + m_prefix + "users");
+
+		for (pqxx::result::const_iterator it = r.begin(); it != r.end(); it++)  {
+			users.push_back((*it)[0].as<std::string>());
+		}
+	}
+	catch (std::exception& e) {
+		LOG4CXX_ERROR(pxxLogger, e.what());
+		return false;
+	}
+
+	return true;
+}
+
 long PQXXBackend::addBuddy(long userId, const BuddyInfo &buddyInfo) {
 	try {
 		pqxx::nontransaction txn(*m_conn);
diff --git a/libtransport/SQLite3Backend.cpp b/libtransport/SQLite3Backend.cpp
index 6288b32ad..678310af7 100644
--- a/libtransport/SQLite3Backend.cpp
+++ b/libtransport/SQLite3Backend.cpp
@@ -102,6 +102,7 @@ SQLite3Backend::~SQLite3Backend(){
 		FINALIZE_STMT(m_setUserOnline);
 		FINALIZE_STMT(m_getOnlineUsers);
 		FINALIZE_STMT(m_getUsers);
+		FINALIZE_STMT(m_getLegacyNetworkUsers);
 		sqlite3_close(m_db);
 	}
 }
@@ -143,7 +144,7 @@ bool SQLite3Backend::connect() {
 	PREP_STMT(m_setUserOnline, "UPDATE " + m_prefix + "users SET online=?, last_login=DATETIME('NOW') WHERE id=?");
 	PREP_STMT(m_getOnlineUsers, "SELECT jid FROM " + m_prefix + "users WHERE online=1");
 	PREP_STMT(m_getUsers, "SELECT jid FROM " + m_prefix + "users");
-
+	PREP_STMT(m_getLegacyNetworkUsers, "SELECT uin FROM " + m_prefix + "users");
 	return true;
 }
 
@@ -301,6 +302,23 @@ bool SQLite3Backend::getUsers(std::vector<std::string> &users) {
 	return true;
 }
 
+bool SQLite3Backend::getLegacyNetworkUsers(std::vector<std::string> &users) {
+	sqlite3_reset(m_getLegacyNetworkUsers);
+
+	int ret;
+	while ((ret = sqlite3_step(m_getLegacyNetworkUsers)) == SQLITE_ROW) {
+		std::string jid = (const char *) sqlite3_column_text(m_getLegacyNetworkUsers, 0);
+		users.push_back(jid);
+	}
+
+	if (ret != SQLITE_DONE) {
+		LOG4CXX_ERROR(sqlite3Logger, "getLegacyNetworkUsers query"<< (sqlite3_errmsg(m_db) == NULL ? "" : sqlite3_errmsg(m_db)));
+		return false;
+	}
+
+	return true;
+}
+
 long SQLite3Backend::addBuddy(long userId, const BuddyInfo &buddyInfo) {
 // 	"INSERT INTO " + m_prefix + "buddies (user_id, uin, subscription, groups, nickname, flags) VALUES (?, ?, ?, ?, ?, ?)"
 	std::string groups = StorageBackend::serializeGroups(buddyInfo.groups);
diff --git a/spectrum/src/frontends/xmpp/XMPPUserRegistration.cpp b/spectrum/src/frontends/xmpp/XMPPUserRegistration.cpp
index e05103470..83758aa48 100644
--- a/spectrum/src/frontends/xmpp/XMPPUserRegistration.cpp
+++ b/spectrum/src/frontends/xmpp/XMPPUserRegistration.cpp
@@ -378,7 +378,9 @@ bool XMPPUserRegistration::handleSetRequest(const Swift::JID& from, const Swift:
 		}
 	}
 
-	if (!payload->getUsername() || (!payload->getPassword() && CONFIG_BOOL_DEFAULTED(m_config, "registration.needPassword", true))) {
+	bool needPassword = CONFIG_BOOL_DEFAULTED(m_config, "registration.needPassword", true);
+
+	if (!payload->getUsername() || (!payload->getPassword() && needPassword)) {
 		sendError(from, id, ErrorPayload::NotAcceptable, ErrorPayload::Modify);
 		return true;
 	}
@@ -393,18 +395,20 @@ bool XMPPUserRegistration::handleSetRequest(const Swift::JID& from, const Swift:
 		return true;
 	}
 
-	// TODO: Move this check to backend somehow
-	if (CONFIG_STRING(m_config, "service.protocol") == "prpl-jabber") {
-		// User tries to register himself.
+	// User tries to register himself (XMPP backends)
+	if (JID::parse(*payload->getUsername())) {
 		if ((Swift::JID(*payload->getUsername()).toBare() == from.toBare())) {
 			sendError(from, id, ErrorPayload::NotAcceptable, ErrorPayload::Modify);
 			return true;
 		}
+	}
 
-		// User tries to register someone who's already registered.
-		UserInfo user_row;
-		bool registered = m_storageBackend->getUser(Swift::JID(*payload->getUsername()).toBare().toString(), user_row);
-		if (registered) {
+	// User tries to register someone who's already registered (password-less backends)
+	if (needPassword) {
+		std::vector<std::string> legacyNetworkUsers;
+		m_storageBackend->getLegacyNetworkUsers(legacyNetworkUsers);
+		bool legacyUserExists = std::find(legacyNetworkUsers.begin(), legacyNetworkUsers.end(), *payload->getUsername()) != legacyNetworkUsers.end();
+		if (legacyUserExists) {
 			sendError(from, id, ErrorPayload::NotAcceptable, ErrorPayload::Modify);
 			return true;
 		}


Test patch:
diff --git a/tests/libtransport/basictest.h b/tests/libtransport/basictest.h
index 3807c75bd..26734cedf 100644
--- a/tests/libtransport/basictest.h
+++ b/tests/libtransport/basictest.h
@@ -177,6 +177,9 @@ class TestingStorageBackend : public StorageBackend {
 		virtual bool getUsers(std::vector<std::string> &users) {
 			return true;
 		}
+		virtual bool getLegacyNetworkUsers(std::vector<std::string> &users) {
+			return true;
+		}
 
 		virtual long addBuddy(long userId, const BuddyInfo &buddyInfo) {
 			return buddyid++;