Merge branch 'insp3' into master.

6 files changed, 184 insertions, 104 deletions

diff --git a/src/coremods/core_dns.cpp b/src/coremods/core_dns.cpp
index fb83ed0a5..c23f43c17 100644
--- a/src/coremods/core_dns.cpp
+++ b/src/coremods/core_dns.cpp
@@ -303,7 +303,7 @@ class Packet final
 	unsigned short Pack(unsigned char* output, unsigned short output_size)
 	{
 		if (output_size < HEADER_LENGTH)
-			throw Exception("Unable to pack packet");
+			throw Exception("Unable to pack oversized packet header");
 
 		unsigned short pos = 0;
 
@@ -326,7 +326,8 @@ class Packet final
 			if (q.type == QUERY_PTR)
 			{
 				irc::sockets::sockaddrs ip;
-				irc::sockets::aptosa(q.name, 0, ip);
+				if (!irc::sockets::aptosa(q.name, 0, ip))
+					throw Exception("Unable to pack packet with malformed IP for PTR lookup");
 
 				if (q.name.find(':') != std::string::npos)
 				{
@@ -357,7 +358,7 @@ class Packet final
 			this->PackName(output, output_size, pos, q.name);
 
 			if (pos + 4 >= output_size)
-				throw Exception("Unable to pack packet");
+				throw Exception("Unable to pack oversized packet body");
 
 			short s = htons(q.type);
 			memcpy(&output[pos], &s, 2);
diff --git a/src/coremods/core_hostname_lookup.cpp b/src/coremods/core_hostname_lookup.cpp
index c4e81f4d4..287685cbe 100644
--- a/src/coremods/core_hostname_lookup.cpp
+++ b/src/coremods/core_hostname_lookup.cpp
@@ -28,19 +28,24 @@ namespace
 	BoolExtItem* dl;
 }
 
-/** Derived from Resolver, and performs user forward/reverse lookups.
- */
-class UserResolver final
+class UserResolver
 	: public DNS::Request
 {
- private:
-	/** The socket address that the user we are looking up is connected from. */
+ protected:
+	// The socket address that the associated user was connected from at the time of lookup.
 	const irc::sockets::sockaddrs sa;
 
-	/** UUID we are looking up */
+	// The UUID of the user we are looking up the hostname of.
 	const std::string uuid;
 
-	/** Handles errors which happen during DNS resolution. */
+	UserResolver(DNS::Manager* mgr, Module* me, LocalUser* user, const std::string& to_resolve, DNS::QueryType qt)
+		: DNS::Request(mgr, me, to_resolve, qt)
+		, sa(user->client_sa)
+		, uuid(user->uuid)
+	{
+	}
+
+	// Handles errors which happen during DNS resolution.
 	static void HandleError(LocalUser* user, const std::string& message)
 	{
 		user->WriteNotice("*** " + message + "; using your IP address (" + user->GetIPString() + ") instead.");
@@ -51,104 +56,138 @@ class UserResolver final
 		dl->Unset(user);
 	}
 
+	/** Logs the result of a DNS lookup. */
+	inline void LogLookup(const DNS::ResourceRecord& rr, bool cached) const
+	{
+		ServerInstance->Logs.Log(MODNAME, LOG_DEBUG, "DNS %s result for %s: '%s' -> '%s'%s",
+			manager->GetTypeStr(question.type).c_str(), uuid.c_str(), rr.name.c_str(),
+			rr.rdata.c_str(), cached ? " (cached)" : "");
+	}
+
  public:
-	/** Create a resolver.
-	 * @param mgr DNS Manager
-	 * @param me this module
-	 * @param user The user to begin lookup on
-	 * @param to_resolve The IP or host to resolve
-	 * @param qt The query type
-	 */
-	UserResolver(DNS::Manager* mgr, Module* me, LocalUser* user, const std::string& to_resolve, DNS::QueryType qt)
-		: DNS::Request(mgr, me, to_resolve, qt)
-		, sa(user->client_sa)
-		, uuid(user->uuid)
+	void OnError(const DNS::Query* query) override
 	{
+		LocalUser* user = IS_LOCAL(ServerInstance->Users.FindUUID(uuid));
+		if (user && user->client_sa == sa)
+			HandleError(user, "Could not resolve your hostname: " + this->manager->GetErrorStr(query->error));
 	}
+};
 
-	/** Called on successful lookup
-	 * if a previous result has already come back.
-	 * @param r The finished query
-	 */
-	void OnLookupComplete(const DNS::Query* r) override
+// Handles checking the retrieved PTR record against its A/AAAA records.
+class UserIPResolver final
+	: public UserResolver
+{
+ public:
+	UserIPResolver(DNS::Manager* mgr, Module* me, LocalUser* user, const std::string& host)
+		: UserResolver(mgr, me, user, host, user->client_sa.family() == AF_INET6 ? DNS::QUERY_AAAA : DNS::QUERY_A)
 	{
-		LocalUser* bound_user = IS_LOCAL(ServerInstance->Users.FindUUID(uuid));
-		if (!bound_user || bound_user->client_sa != sa)
-			return;
+	}
 
-		const DNS::ResourceRecord* ans_record = r->FindAnswerOfType(this->question.type);
-		if (!ans_record)
-		{
-			HandleError(bound_user, "Could not resolve your hostname: No " + this->manager->GetTypeStr(this->question.type) + " records found");
+	void OnLookupComplete(const DNS::Query* query) override
+	{
+		LocalUser* user = IS_LOCAL(ServerInstance->Users.FindUUID(uuid));
+		if (!user || user->client_sa != sa)
 			return;
-		}
-
-		ServerInstance->Logs.Log(MODNAME, LOG_DEBUG, "DNS %s result for %s: '%s' -> '%s'%s",
-			this->manager->GetTypeStr(question.type).c_str(), uuid.c_str(),
-			ans_record->name.c_str(), ans_record->rdata.c_str(),
-			r->cached ? " (cached)" : "");
 
-		if (this->question.type == DNS::QUERY_PTR)
+		bool hasrecord = false;
+		bool matches = false;
+		const DNS::QueryType qt = user->client_sa.family() == AF_INET6 ? DNS::QUERY_AAAA : DNS::QUERY_A;
+		for (const auto& rr : query->answers)
 		{
-			const DNS::QueryType qt = bound_user->client_sa.family() == AF_INET6 ? DNS::QUERY_AAAA : DNS::QUERY_A;
-			UserResolver* res_forward = new UserResolver(this->manager, this->creator, bound_user, ans_record->rdata, qt);
-			try
-			{
-				this->manager->Process(res_forward);
-			}
-			catch (DNS::Exception& e)
-			{
-				delete res_forward;
-				ServerInstance->Logs.Log(MODNAME, LOG_DEBUG, "Error in resolver: " + e.GetReason());
+			if (rr.type != qt)
+				continue;
 
-				HandleError(bound_user, "There was an internal error resolving your host");
-			}
-		}
-		else if (this->question.type == DNS::QUERY_A || this->question.type == DNS::QUERY_AAAA)
-		{
-			/* Both lookups completed */
+			// We've seen an A/AAAA record.
+			hasrecord = true;
 
-			irc::sockets::sockaddrs* user_ip = &bound_user->client_sa;
-			bool rev_match = false;
-			if (user_ip->family() == AF_INET6)
+			switch (user->client_sa.family())
 			{
-				struct in6_addr res_bin;
-				if (inet_pton(AF_INET6, ans_record->rdata.c_str(), &res_bin))
+				case AF_INET:
 				{
-					rev_match = !memcmp(&user_ip->in6.sin6_addr, &res_bin, sizeof(res_bin));
+					// Does this result match the user's IPv4 address?
+					struct in_addr v4addr;
+					if (inet_pton(AF_INET, rr.rdata.c_str(), &v4addr) == 1)
+						matches = !memcmp(&user->client_sa.in4.sin_addr, &v4addr, sizeof(v4addr));
+					break;
 				}
-			}
-			else
-			{
-				struct in_addr res_bin;
-				if (inet_pton(AF_INET, ans_record->rdata.c_str(), &res_bin))
+
+				case AF_INET6:
 				{
-					rev_match = !memcmp(&user_ip->in4.sin_addr, &res_bin, sizeof(res_bin));
+					// Does this result match the user's IPv6 address?
+					struct in6_addr v6addr;
+					if (inet_pton(AF_INET6, rr.rdata.c_str(), &v6addr) == 1)
+						matches = !memcmp(&user->client_sa.in6.sin6_addr, &v6addr, sizeof(v6addr));
+					break;
 				}
 			}
 
-			if (rev_match)
+			// If we've found a valid match then stop processing records.
+			if (matches)
 			{
-				bound_user->WriteNotice("*** Found your hostname (" + this->question.name + (r->cached ? ") -- cached" : ")"));
-				bool display_is_real = bound_user->GetDisplayedHost() == bound_user->GetRealHost();
-				bound_user->ChangeRealHost(this->question.name, display_is_real);
-				dl->Unset(bound_user);
-			}
-			else
-			{
-				HandleError(bound_user, "Your hostname does not match up with your IP address");
+				LogLookup(rr, query->cached);
+				break;
 			}
 		}
+
+		if (matches)
+		{
+			// We have found the hostname for the user.
+			user->WriteNotice("*** Found your hostname (" + this->question.name + (query->cached ? ") -- cached" : ")"));
+			bool display_is_real = user->GetDisplayedHost() == user->GetRealHost();
+			user->ChangeRealHost(this->question.name, display_is_real);
+			dl->Unset(user);
+		}
+		else if (hasrecord)
+		{
+			// The hostname has A/AAAA records but none of them are the IP address of the user.
+			HandleError(user, "Your hostname does not match up with your IP address");
+		}
+		else
+		{
+			// The hostname has no A/AAAA records.
+			HandleError(user, "Could not resolve your hostname: No " + this->manager->GetTypeStr(this->question.type) + " records found");
+		}
 	}
+};
 
-	/** Called on failed lookup
-	 * @param query The errored query
-	 */
-	void OnError(const DNS::Query* query) override
+// Handles retrieving the PTR record for users.
+class UserHostResolver final
+	: public UserResolver
+{
+ public:
+	UserHostResolver(DNS::Manager* mgr, Module* me, LocalUser* user)
+		: UserResolver(mgr, me, user, user->GetIPString(), DNS::QUERY_PTR)
 	{
-		LocalUser* bound_user = IS_LOCAL(ServerInstance->Users.FindUUID(uuid));
-		if (bound_user && bound_user->client_sa == sa)
-			HandleError(bound_user, "Could not resolve your hostname: " + this->manager->GetErrorStr(query->error));
+	}
+
+	void OnLookupComplete(const DNS::Query* query) override
+	{
+		LocalUser* user = IS_LOCAL(ServerInstance->Users.FindUUID(uuid));
+		if (!user || user->client_sa != sa)
+			return;
+
+		// An IP can have multiple PTR records but it is considered bad practise to have
+		// more than one so to simplify the lookup logic we only use the first.
+		const DNS::ResourceRecord* rr = query->FindAnswerOfType(DNS::QUERY_PTR);
+		if (!rr)
+		{
+			HandleError(user, "Could not resolve your hostname: No " + this->manager->GetTypeStr(this->question.type) + " records found");
+			return;
+		}
+
+		LogLookup(*rr, query->cached);
+		UserResolver* res_forward = new UserIPResolver(this->manager, this->creator, user, rr->rdata);
+		try
+		{
+			this->manager->Process(res_forward);
+		}
+		catch (const DNS::Exception& e)
+		{
+			delete res_forward;
+			ServerInstance->Logs.Log(MODNAME, LOG_DEBUG, "Error in resolver: " + e.GetReason());
+
+			HandleError(user, "There was an internal error resolving your host");
+		}
 	}
 };
 
@@ -181,7 +220,7 @@ class ModuleHostnameLookup final
 
 		user->WriteNotice("*** Looking up your hostname...");
 
-		UserResolver* res_reverse = new UserResolver(*this->DNS, this, user, user->GetIPString(), DNS::QUERY_PTR);
+		UserResolver* res_reverse = new UserHostResolver(*this->DNS, this, user);
 		try
 		{
 			/* If both the reverse and forward queries are cached, the user will be able to pass DNS completely
diff --git a/src/modules/m_spanningtree/main.cpp b/src/modules/m_spanningtree/main.cpp
index 93afcd237..02668d190 100644
--- a/src/modules/m_spanningtree/main.cpp
+++ b/src/modules/m_spanningtree/main.cpp
@@ -853,7 +853,7 @@ void ModuleSpanningTree::OnDecodeMetaData(Extensible* target, const std::string&
 	// HACK: this should use automatically synced user metadata in v4.
 	User* dest = static_cast<User*>(target);
 	if (dest && (extname == "uniqueusername"))
-		dest->uniqueusername = true;
+		dest->uniqueusername = (extdata != "0");
 }
 
 Cullable::Result ModuleSpanningTree::Cull()
diff --git a/src/modules/m_spanningtree/resolvers.cpp b/src/modules/m_spanningtree/resolvers.cpp
index 9bf5a4b17..c827b89f5 100644
--- a/src/modules/m_spanningtree/resolvers.cpp
+++ b/src/modules/m_spanningtree/resolvers.cpp
@@ -113,6 +113,25 @@ SecurityIPResolver::SecurityIPResolver(Module* me, DNS::Manager* mgr, const std:
 {
 }
 
+bool SecurityIPResolver::CheckIPv4()
+{
+	// We only check IPv4 addresses if we have checked IPv6.
+	if (query != DNS::QUERY_AAAA)
+		return false;
+
+	SecurityIPResolver* res = new SecurityIPResolver(mine, this->manager, host, MyLink, DNS::QUERY_A);
+	try
+	{
+		this->manager->Process(res);
+		return true;
+	}
+	catch (const DNS::Exception&)
+	{
+		delete res;
+		return false;
+	}
+}
+
 void SecurityIPResolver::OnLookupComplete(const DNS::Query *r)
 {
 	for (std::shared_ptr<Link> L : Utils->LinkBlocks)
@@ -121,31 +140,27 @@ void SecurityIPResolver::OnLookupComplete(const DNS::Query *r)
 		{
 			for (const auto& ans_record : r->answers)
 			{
-				if (ans_record.type == this->question.type)
-					Utils->ValidIPs.push_back(ans_record.rdata);
+				if (ans_record.type != this->question.type)
+					continue;
+
+				Utils->ValidIPs.push_back(ans_record.rdata);
+				ServerInstance->Logs.Log(MODNAME, LOG_DEFAULT, "Resolved '%s' as a valid IP address for link '%s'",
+					ans_record.rdata.c_str(), MyLink->Name.c_str());
 			}
 			break;
 		}
 	}
+
+	CheckIPv4();
 }
 
 void SecurityIPResolver::OnError(const DNS::Query *r)
 {
 	// This can be called because of us being unloaded but we don't have to do anything differently
-	if (query == DNS::QUERY_AAAA)
-	{
-		SecurityIPResolver* res = new SecurityIPResolver(mine, this->manager, host, MyLink, DNS::QUERY_A);
-		try
-		{
-			this->manager->Process(res);
-			return;
-		}
-		catch (DNS::Exception &)
-		{
-			delete res;
-		}
-	}
-	ServerInstance->Logs.Log(MODNAME, LOG_DEFAULT, "Could not resolve IP associated with Link '%s': %s",
+	if (CheckIPv4())
+		return;
+
+	ServerInstance->Logs.Log(MODNAME, LOG_DEBUG, "Could not resolve IP associated with link '%s': %s",
 		MyLink->Name.c_str(), this->manager->GetErrorStr(r->error).c_str());
 }
 
diff --git a/src/modules/m_spanningtree/resolvers.h b/src/modules/m_spanningtree/resolvers.h
index 1bf3ce14a..a748537be 100644
--- a/src/modules/m_spanningtree/resolvers.h
+++ b/src/modules/m_spanningtree/resolvers.h
@@ -41,6 +41,7 @@ class SecurityIPResolver final
 	Module* mine;
 	std::string host;
 	DNS::QueryType query;
+	bool CheckIPv4();
  public:
 	SecurityIPResolver(Module* me, DNS::Manager* mgr, const std::string& hostname, std::shared_ptr<Link> x, DNS::QueryType qt);
 	void OnLookupComplete(const DNS::Query *r) override;
diff --git a/src/modules/m_starttls.cpp b/src/modules/m_starttls.cpp
index 590da9410..116f3e792 100644
--- a/src/modules/m_starttls.cpp
+++ b/src/modules/m_starttls.cpp
@@ -82,19 +82,43 @@ class CommandStartTLS final
 	}
 };
 
+class TLSCap final
+	: public Cap::Capability
+{
+ private:
+	dynamic_reference_nocheck<IOHookProvider>& sslref;
+
+	bool OnList(LocalUser* user) override
+	{
+		return sslref;
+	}
+
+	bool OnRequest(LocalUser* user, bool adding) override
+	{
+		return sslref;
+	}
+
+ public:
+	TLSCap(Module* mod, dynamic_reference_nocheck<IOHookProvider>& ssl)
+		: Cap::Capability(mod, "tls")
+		, sslref(ssl)
+	{
+	}
+};
+
 class ModuleStartTLS final
 	: public Module
 {
  private:
 	CommandStartTLS starttls;
-	Cap::Capability tls;
+	TLSCap tls;
 	dynamic_reference_nocheck<IOHookProvider> ssl;
 
  public:
 	ModuleStartTLS()
 		: Module(VF_VENDOR, "Provides the IRCv3 tls client capability.")
 		, starttls(this, ssl)
-		, tls(this, "tls")
+		, tls(this, ssl)
 		, ssl(this, "ssl")
 	{
 	}