Skip to content

Commit

Permalink
Fix a low risk race condition which could result in crashes, spotted …
Browse files Browse the repository at this point in the history
…by Szymek - big thanks! :)

git-svn-id: http://svn.inspircd.org/repository/branches/1_1_stable@10747 e03df62e-2008-0410-955e-edbf42e46eb7
  • Loading branch information
rburchell committed Oct 28, 2008
1 parent cdc458e commit 15013ec
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 13 deletions.
5 changes: 3 additions & 2 deletions include/users.h
Expand Up @@ -784,9 +784,10 @@ class CoreExport userrec : public connection
*/
void Oper(const std::string &opertype);

/** Call this method to find the matching <connect> for a user, and to check them against it.
/** Call this method to check a user against a connect class.
* @param a The connect class to match them against.
*/
void CheckClass();
void CheckClass(ConnectClass *a);

/** Use this method to fully connect a user.
* This will send the message of the day, check G/K/E lines, etc.
Expand Down
39 changes: 33 additions & 6 deletions src/modules/m_cgiirc.cpp
Expand Up @@ -351,8 +351,18 @@ class ModuleCgiIRC : public Module
user->Shrink("cgiirc_webirc_ip");
ServerInstance->AddLocalClone(user);
ServerInstance->AddGlobalClone(user);
user->CheckClass();
Recheck(user);

ConnectClass *i = user->GetClass();
if (!i)
{
userrec::QuitUser(ServerInstance, user, "Access denied by configuration");
return;
}
else
{
user->CheckClass(i);
}
}
}

Expand All @@ -379,7 +389,6 @@ class ModuleCgiIRC : public Module
#endif
ServerInstance->AddLocalClone(user);
ServerInstance->AddGlobalClone(user);
user->CheckClass();

if (valid)
{
Expand Down Expand Up @@ -409,6 +418,17 @@ class ModuleCgiIRC : public Module
/*if(NotifyOpers)
ServerInstance->WriteOpers("*** Connecting user %s detected as using CGI:IRC (%s), changing real host to %s from PASS", user->nick, user->host, user->password);*/

ConnectClass *i = user->GetClass();
if (!i)
{
userrec::QuitUser(ServerInstance, user, "Access denied by configuration");
return false;
}
else
{
user->CheckClass(i);
}

return true;
}

Expand Down Expand Up @@ -441,7 +461,6 @@ class ModuleCgiIRC : public Module
user->SetSockAddr(user->GetProtocolFamily(), newip, user->GetPort());
ServerInstance->AddLocalClone(user);
ServerInstance->AddGlobalClone(user);
user->CheckClass();
try
{
strlcpy(user->host, newip, 16);
Expand All @@ -462,9 +481,17 @@ class ModuleCgiIRC : public Module
if(NotifyOpers)
ServerInstance->WriteOpers("*** Connecting user %s detected as using CGI:IRC (%s), but i could not resolve their hostname!", user->nick, user->host);
}
/*strlcpy(user->host, newip, 16);
strlcpy(user->dhost, newip, 16);
strlcpy(user->ident, "~cgiirc", 8);*/

ConnectClass *i = user->GetClass();
if (!i)
{
userrec::QuitUser(ServerInstance, user, "Access denied by configuration");
return true;
}
else
{
user->CheckClass(i);
}

return true;
}
Expand Down
24 changes: 19 additions & 5 deletions src/users.cpp
Expand Up @@ -930,7 +930,7 @@ void userrec::AddClient(InspIRCd* Instance, int socket, int port, bool iscached,
* Check connect class settings and initialise settings into userrec.
* This will be done again after DNS resolution. -- w00t
*/
New->CheckClass();
New->CheckClass(i);

Instance->local_users.push_back(New);

Expand Down Expand Up @@ -1009,10 +1009,8 @@ unsigned long userrec::LocalCloneCount()
/*
* Check class restrictions
*/
void userrec::CheckClass()
void userrec::CheckClass(ConnectClass *a)
{
ConnectClass* a = this->GetClass();

if ((!a) || (a->GetType() == CC_DENY))
{
userrec::QuitUser(ServerInstance, this, "Unauthorised connection");
Expand Down Expand Up @@ -1045,13 +1043,29 @@ void userrec::FullConnect()
ServerInstance->stats->statsConnects++;
this->idle_lastmsg = ServerInstance->Time();


ConnectClass *a = this->GetClass();
if (!a)
{
/*
* This can happen:
* - user connects
* - /rehash, removing <connect> tag they matched
* - dns completes (putting us here) ...but they no longer have a connect tag
* Catch it here. Thanks Szymek!
* -- w00t
*/
userrec::QuitUser(ServerInstance, this, "Invalid password");
return;
}

/*
* You may be thinking "wtf, we checked this in userrec::AddClient!" - and yes, we did, BUT.
* At the time AddClient is called, we don't have a resolved host, by here we probably do - which
* may put the user into a totally seperate class with different restrictions! so we *must* check again.
* Don't remove this! -- w00t
*/
this->CheckClass();
this->CheckClass(a);

/* Check the password, if one is required by the user's connect class.
* This CANNOT be in CheckClass(), because that is called prior to PASS as well!
Expand Down

0 comments on commit 15013ec

Please sign in to comment.