If SVSNICK is sent to the user's server is this actually an issue? A NICK sent from the user would go to the local server, which would have the correct ban information due to your patch.
Or are we talking about non SVSNICK events... Where a user has changed to a nick that matches a ban on their own?
------Original Message------ From: Ned T. Crigler To: kevin@buley.org Cc: dalnet-src@lists.dal.net Sent: Apr 23, 2009 19:07 Subject: Re: [DALnet-src] SVSNICK and cached ban status handling
On Thu, Apr 23, 2009 at 10:25:07PM +0000, kevin@buley.org wrote:
I like this! Too many times have I set a ban on guest*!*@somedomain only to watch an endless nick->guest->nick cycle. There are times when we don't want to remove a user from the channel, but we DO want to stop their lame script. Currently the only options are to kick the user or remove and re-set the ban... Both are undesdirable.
In our off-list discussion you mentioned a case where this may still happen... Is there a solution other than changing banserial for every NICK event?
That problem is because SVSNICK is relayed to the person's server, which then does the forced nick change locally, and then sends out a standard NICK protocol message to the other servers.
The other servers cannot tell the difference between a normal nick change and a forced nick change. This will cause remote servers to have the old user's banserial cached, since a normal NICK change doesn't change banserial right now.
I think a flush of the banserial in m_nick in addition to m_svsnick is the easiest way to fix this, unless maybe SVSNICK is changed to not use a regular NICK protocol message to other servers.
That unfortunately, would probably involve backwards-compatibility protocol issues. With the m_nick change, unpatched servers would just have a out-of-date banserial cached.
On Thu, Apr 23, 2009 at 11:27:47PM +0000, kevin@buley.org wrote:
If SVSNICK is sent to the user's server is this actually an issue? A NICK sent from the user would go to the local server, which would have the correct ban information due to your patch.
Or are we talking about non SVSNICK events... Where a user has changed to a nick that matches a ban on their own?
With my patch so far, only the user's local server will have the correct cached ban information, while all other servers on the network will still have the old cached ban information. This means, for example, when someone is SVSNICKed from a banned nickname to a unbanned nickname (bannednick to Guest12345 for instance), only their local server will believe that they are now unbanned.
If they then try to talk in the channel they are in, the local server will allow them to do so, while other servers on the network will silently drop whatever they say (since the other servers believe they are still banned.)
Good conversation on this one - this would be something good to get fixed. A few comments on the thread so far...
On Thu, Apr 23, 2009 at 8:12 PM, Ned T. Crigler crigler@gmail.com wrote:
With my patch so far, only the user's local server will have the correct cached ban information, while all other servers on the network will still have the old cached ban information. This means, for example, when someone is SVSNICKed from a banned nickname to a unbanned nickname (bannednick to Guest12345 for instance), only their local server will believe that they are now unbanned.
If a normal NICK doesn't update the banserial, how does bsilent currently work on a nick change? With the current SVSNICK method of sending the change to the users' server and having the nick change propagate out to the rest of the network as a NICK, does that create a desync in what servers think the user can speak on the change?
You're on the right track here though. One request about your patch - can you use git? If you pull down our git tree and send a git formatted patch, I can properly attribute the fix to you in the commit history. The git tree is at http://code.dal.net/bahamut.git
This goes for any patches that are submitted. I'll definitely review anything that goes through this list, but having the patches git-formatted will make attribution and tracking easier.
Thanks! -epi
On Fri, Apr 24, 2009 at 09:10:20AM -0400, epiphani wrote:
If a normal NICK doesn't update the banserial, how does bsilent currently work on a nick change? With the current SVSNICK method of sending the change to the users' server and having the nick change propagate out to the rest of the network as a NICK, does that create a desync in what servers think the user can speak on the change?
Currently, without the patch, there shouldn't be a desync happening because every server will have the same idea of the users' cached ban status afterwards. The status they had before the SVSNICK occurred is what is used, and the users' server forbids a nick change or any talking according to that status, which matches what the rest of the network believes.
With the patch and without NICK being fixed to update the banserial (which an addition to the patch could do), we create a desync as the users' server may have a different idea of what the user can do than the rest of the network.
The goal is for every server to have the cached ban status reflect the new actual ban status for the user after the SVSNICK is finished.
You're on the right track here though. One request about your patch - can you use git? If you pull down our git tree and send a git formatted patch, I can properly attribute the fix to you in the commit history. The git tree is at http://code.dal.net/bahamut.git
Sure, I can send a git formatted patch to you (or to the list as well.)
After services does a SVSNICK on a user, if the new nickname is banned in a channel, they are still allowed to talk and change their nick if they were not banned previously.
For example:
*** Mode change "+b testing!*@*" on #test by RuneB *** Mode change "-o RuneB" on #test by RuneB <RuneB> I can talk. Now I'll force a SVSNICK on me to testing. *** RuneB is now known as testing <testing> can I still talk? <testing> yep.
This is because is_banned doesn't recheck the ban list if the banserial has not changed for the user (instead it returns immediately). SVSNICK forces a nick change, but doesn't change the banserial in any of the channels the user is in. This causes is_banned to think the ban status has not changed. --- include/h.h | 1 + src/channel.c | 20 ++++++++++++++++++++ src/m_nick.c | 7 +++++++ src/m_services.c | 1 + 4 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/include/h.h b/include/h.h index 26465cd..1d4cce7 100644 --- a/include/h.h +++ b/include/h.h @@ -91,6 +91,7 @@ extern int bootopt; extern char *canonize(char *); extern void check_fdlists(); extern aChannel *find_channel(char *, aChannel *); +extern void flush_user_banserial(aClient *); extern aBan *nick_is_banned(aChannel *, char *, aClient *); extern void remove_matching_bans(aChannel *, aClient *, aClient *); #ifdef EXEMPT_LISTS diff --git a/src/channel.c b/src/channel.c index 642d7f1..1662f33 100644 --- a/src/channel.c +++ b/src/channel.c @@ -537,6 +537,26 @@ static int is_banned(aClient *cptr, aChannel *chptr, chanMember *cm) return 0; }
+/* + * Forces the cached banned status for a user to be flushed in all the channels + * they are in. + */ +void flush_user_banserial(aClient *cptr) +{ + Link *ptr; + + if (!IsPerson(cptr)) + return; + for (ptr = cptr->user->channel; ptr; ptr = ptr->next) + { + aChannel *chptr = ptr->value.chptr; + chanMember *cm = find_user_member(chptr->members, cptr); + + if (cm) + cm->banserial = chptr->banserial - 1; + } +} + aBan *nick_is_banned(aChannel *chptr, char *nick, aClient *cptr) { aBan *ban; diff --git a/src/m_nick.c b/src/m_nick.c index 79eab36..6eee775 100644 --- a/src/m_nick.c +++ b/src/m_nick.c @@ -575,6 +575,13 @@ int m_nick(aClient *cptr, aClient *sptr, int parc, char *parv[]) /* If it changed nicks, -r it */ if (mycmp(parv[0], nick)) sptr->umode &= ~UMODE_r; + + /* + * Flush the banserial for the channels the user is in, since this + * could be a SVSNICK induced nick change, which overrides any ban + * checking on the originating server. + */ + flush_user_banserial(sptr); } } else diff --git a/src/m_services.c b/src/m_services.c index 50e0b87..bc97958 100644 --- a/src/m_services.c +++ b/src/m_services.c @@ -230,6 +230,7 @@ int m_svsnick(aClient *cptr, aClient *sptr, int parc, char *parv[]) strcpy(acptr->name, newnick); add_to_client_hash_table(acptr->name, acptr); hash_check_watch(acptr, RPL_LOGON); + flush_user_banserial(acptr);
return 0; }
I like. Will apply this.
One question, for future thought - since we're flushing banserial now on nick changes, does it really make sense to limit nick changes based on is_banned status? It might make sense to just let users change their nicks, and let them be quiet if they want to change to a banned nickname...
-Aaron
On Mon, May 4, 2009 at 6:00 PM, Ned T. Crigler crigler@gmail.com wrote:
After services does a SVSNICK on a user, if the new nickname is banned in a channel, they are still allowed to talk and change their nick if they were not banned previously.
For example:
*** Mode change "+b testing!*@*" on #test by RuneB *** Mode change "-o RuneB" on #test by RuneB <RuneB> I can talk. Now I'll force a SVSNICK on me to testing. *** RuneB is now known as testing <testing> can I still talk? <testing> yep.
This is because is_banned doesn't recheck the ban list if the banserial has not changed for the user (instead it returns immediately). SVSNICK forces a nick change, but doesn't change the banserial in any of the channels the user is in. This causes is_banned to think the ban status has not changed.
include/h.h | 1 + src/channel.c | 20 ++++++++++++++++++++ src/m_nick.c | 7 +++++++ src/m_services.c | 1 + 4 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/include/h.h b/include/h.h index 26465cd..1d4cce7 100644 --- a/include/h.h +++ b/include/h.h @@ -91,6 +91,7 @@ extern int bootopt; extern char *canonize(char *); extern void check_fdlists(); extern aChannel *find_channel(char *, aChannel *); +extern void flush_user_banserial(aClient *); extern aBan *nick_is_banned(aChannel *, char *, aClient *); extern void remove_matching_bans(aChannel *, aClient *, aClient *); #ifdef EXEMPT_LISTS diff --git a/src/channel.c b/src/channel.c index 642d7f1..1662f33 100644 --- a/src/channel.c +++ b/src/channel.c @@ -537,6 +537,26 @@ static int is_banned(aClient *cptr, aChannel *chptr, chanMember *cm) return 0; }
+/*
- Forces the cached banned status for a user to be flushed in all the channels
- they are in.
- */
+void flush_user_banserial(aClient *cptr) +{
- Link *ptr;
- if (!IsPerson(cptr))
- return;
- for (ptr = cptr->user->channel; ptr; ptr = ptr->next)
- {
- aChannel *chptr = ptr->value.chptr;
- chanMember *cm = find_user_member(chptr->members, cptr);
- if (cm)
- cm->banserial = chptr->banserial - 1;
- }
+}
aBan *nick_is_banned(aChannel *chptr, char *nick, aClient *cptr) { aBan *ban; diff --git a/src/m_nick.c b/src/m_nick.c index 79eab36..6eee775 100644 --- a/src/m_nick.c +++ b/src/m_nick.c @@ -575,6 +575,13 @@ int m_nick(aClient *cptr, aClient *sptr, int parc, char *parv[]) /* If it changed nicks, -r it */ if (mycmp(parv[0], nick)) sptr->umode &= ~UMODE_r;
- /*
- * Flush the banserial for the channels the user is in, since this
- * could be a SVSNICK induced nick change, which overrides any ban
- * checking on the originating server.
- */
- flush_user_banserial(sptr);
} } else diff --git a/src/m_services.c b/src/m_services.c index 50e0b87..bc97958 100644 --- a/src/m_services.c +++ b/src/m_services.c @@ -230,6 +230,7 @@ int m_svsnick(aClient *cptr, aClient *sptr, int parc, char *parv[]) strcpy(acptr->name, newnick); add_to_client_hash_table(acptr->name, acptr); hash_check_watch(acptr, RPL_LOGON);
- flush_user_banserial(acptr);
return 0; } -- 1.6.2.4
I like being able to stop someone from changing TO a nick. I have some users that annoyingly change to "away" nicks or some other nick change when they disconnect from a bnc. I can let the user use the channel, but stop them from using the annoying away/disconnected nick by banning it.
Aaron Wiebe wrote:
I like. Will apply this.
One question, for future thought - since we're flushing banserial now on nick changes, does it really make sense to limit nick changes based on is_banned status? It might make sense to just let users change their nicks, and let them be quiet if they want to change to a banned nickname...
-Aaron
On Mon, May 4, 2009 at 6:00 PM, Ned T. Crigler crigler@gmail.com wrote:
After services does a SVSNICK on a user, if the new nickname is banned in a channel, they are still allowed to talk and change their nick if they were not banned previously.
For example:
*** Mode change "+b testing!*@*" on #test by RuneB *** Mode change "-o RuneB" on #test by RuneB <RuneB> I can talk. Now I'll force a SVSNICK on me to testing. *** RuneB is now known as testing <testing> can I still talk? <testing> yep.
This is because is_banned doesn't recheck the ban list if the banserial has not changed for the user (instead it returns immediately). SVSNICK forces a nick change, but doesn't change the banserial in any of the channels the user is in. This causes is_banned to think the ban status has not changed.
include/h.h | 1 + src/channel.c | 20 ++++++++++++++++++++ src/m_nick.c | 7 +++++++ src/m_services.c | 1 + 4 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/include/h.h b/include/h.h index 26465cd..1d4cce7 100644 --- a/include/h.h +++ b/include/h.h @@ -91,6 +91,7 @@ extern int bootopt; extern char *canonize(char *); extern void check_fdlists(); extern aChannel *find_channel(char *, aChannel *); +extern void flush_user_banserial(aClient *); extern aBan *nick_is_banned(aChannel *, char *, aClient *); extern void remove_matching_bans(aChannel *, aClient *, aClient *); #ifdef EXEMPT_LISTS diff --git a/src/channel.c b/src/channel.c index 642d7f1..1662f33 100644 --- a/src/channel.c +++ b/src/channel.c @@ -537,6 +537,26 @@ static int is_banned(aClient *cptr, aChannel *chptr, chanMember *cm) return 0; }
+/*
- Forces the cached banned status for a user to be flushed in all the channels
- they are in.
- */
+void flush_user_banserial(aClient *cptr) +{
Link *ptr;if (!IsPerson(cptr))return;for (ptr = cptr->user->channel; ptr; ptr = ptr->next){aChannel *chptr = ptr->value.chptr;chanMember *cm = find_user_member(chptr->members, cptr);if (cm)cm->banserial = chptr->banserial - 1;}+}
aBan *nick_is_banned(aChannel *chptr, char *nick, aClient *cptr) { aBan *ban; diff --git a/src/m_nick.c b/src/m_nick.c index 79eab36..6eee775 100644 --- a/src/m_nick.c +++ b/src/m_nick.c @@ -575,6 +575,13 @@ int m_nick(aClient *cptr, aClient *sptr, int parc, char *parv[]) /* If it changed nicks, -r it */ if (mycmp(parv[0], nick)) sptr->umode &= ~UMODE_r;
/** Flush the banserial for the channels the user is in, since this* could be a SVSNICK induced nick change, which overrides any ban* checking on the originating server.*/flush_user_banserial(sptr); }diff --git a/src/m_services.c b/src/m_services.c index 50e0b87..bc97958 100644 --- a/src/m_services.c +++ b/src/m_services.c @@ -230,6 +230,7 @@ int m_svsnick(aClient *cptr, aClient *sptr, int parc, char *parv[]) strcpy(acptr->name, newnick); add_to_client_hash_table(acptr->name, acptr); hash_check_watch(acptr, RPL_LOGON);
flush_user_banserial(acptr);
return 0;
}
1.6.2.4
In addition... that could cause problems, a user could change to a banned nick without knowing it, then wonder why they suddenly can't NICK or speak.
Aaron Wiebe wrote:
I like. Will apply this.
One question, for future thought - since we're flushing banserial now on nick changes, does it really make sense to limit nick changes based on is_banned status? It might make sense to just let users change their nicks, and let them be quiet if they want to change to a banned nickname...
-Aaron
On Mon, May 4, 2009 at 6:00 PM, Ned T. Crigler crigler@gmail.com wrote:
After services does a SVSNICK on a user, if the new nickname is banned in a channel, they are still allowed to talk and change their nick if they were not banned previously.
For example:
*** Mode change "+b testing!*@*" on #test by RuneB *** Mode change "-o RuneB" on #test by RuneB <RuneB> I can talk. Now I'll force a SVSNICK on me to testing. *** RuneB is now known as testing <testing> can I still talk? <testing> yep.
This is because is_banned doesn't recheck the ban list if the banserial has not changed for the user (instead it returns immediately). SVSNICK forces a nick change, but doesn't change the banserial in any of the channels the user is in. This causes is_banned to think the ban status has not changed.
include/h.h | 1 + src/channel.c | 20 ++++++++++++++++++++ src/m_nick.c | 7 +++++++ src/m_services.c | 1 + 4 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/include/h.h b/include/h.h index 26465cd..1d4cce7 100644 --- a/include/h.h +++ b/include/h.h @@ -91,6 +91,7 @@ extern int bootopt; extern char *canonize(char *); extern void check_fdlists(); extern aChannel *find_channel(char *, aChannel *); +extern void flush_user_banserial(aClient *); extern aBan *nick_is_banned(aChannel *, char *, aClient *); extern void remove_matching_bans(aChannel *, aClient *, aClient *); #ifdef EXEMPT_LISTS diff --git a/src/channel.c b/src/channel.c index 642d7f1..1662f33 100644 --- a/src/channel.c +++ b/src/channel.c @@ -537,6 +537,26 @@ static int is_banned(aClient *cptr, aChannel *chptr, chanMember *cm) return 0; }
+/*
- Forces the cached banned status for a user to be flushed in all the channels
- they are in.
- */
+void flush_user_banserial(aClient *cptr) +{
Link *ptr;if (!IsPerson(cptr))return;for (ptr = cptr->user->channel; ptr; ptr = ptr->next){aChannel *chptr = ptr->value.chptr;chanMember *cm = find_user_member(chptr->members, cptr);if (cm)cm->banserial = chptr->banserial - 1;}+}
aBan *nick_is_banned(aChannel *chptr, char *nick, aClient *cptr) { aBan *ban; diff --git a/src/m_nick.c b/src/m_nick.c index 79eab36..6eee775 100644 --- a/src/m_nick.c +++ b/src/m_nick.c @@ -575,6 +575,13 @@ int m_nick(aClient *cptr, aClient *sptr, int parc, char *parv[]) /* If it changed nicks, -r it */ if (mycmp(parv[0], nick)) sptr->umode &= ~UMODE_r;
/** Flush the banserial for the channels the user is in, since this* could be a SVSNICK induced nick change, which overrides any ban* checking on the originating server.*/flush_user_banserial(sptr); }diff --git a/src/m_services.c b/src/m_services.c index 50e0b87..bc97958 100644 --- a/src/m_services.c +++ b/src/m_services.c @@ -230,6 +230,7 @@ int m_svsnick(aClient *cptr, aClient *sptr, int parc, char *parv[]) strcpy(acptr->name, newnick); add_to_client_hash_table(acptr->name, acptr); hash_check_watch(acptr, RPL_LOGON);
flush_user_banserial(acptr);
return 0;
}
1.6.2.4
Yeah, good point. Lets leave it as is.
-Aaron
On Tue, May 5, 2009 at 2:05 PM, Kevin Buley kevin@buley.org wrote:
In addition... that could cause problems, a user could change to a banned nick without knowing it, then wonder why they suddenly can't NICK or speak.
Aaron Wiebe wrote:
I like. Will apply this.
One question, for future thought - since we're flushing banserial now on nick changes, does it really make sense to limit nick changes based on is_banned status? It might make sense to just let users change their nicks, and let them be quiet if they want to change to a banned nickname...
-Aaron
On Mon, May 4, 2009 at 6:00 PM, Ned T. Crigler crigler@gmail.com wrote:
After services does a SVSNICK on a user, if the new nickname is banned in a channel, they are still allowed to talk and change their nick if they were not banned previously.
For example:
*** Mode change "+b testing!*@*" on #test by RuneB *** Mode change "-o RuneB" on #test by RuneB <RuneB> I can talk. Now I'll force a SVSNICK on me to testing. *** RuneB is now known as testing <testing> can I still talk? <testing> yep.
This is because is_banned doesn't recheck the ban list if the banserial has not changed for the user (instead it returns immediately). SVSNICK forces a nick change, but doesn't change the banserial in any of the channels the user is in. This causes is_banned to think the ban status has not changed.
include/h.h | 1 + src/channel.c | 20 ++++++++++++++++++++ src/m_nick.c | 7 +++++++ src/m_services.c | 1 + 4 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/include/h.h b/include/h.h index 26465cd..1d4cce7 100644 --- a/include/h.h +++ b/include/h.h @@ -91,6 +91,7 @@ extern int bootopt; extern char *canonize(char *); extern void check_fdlists(); extern aChannel *find_channel(char *, aChannel *); +extern void flush_user_banserial(aClient *); extern aBan *nick_is_banned(aChannel *, char *, aClient *); extern void remove_matching_bans(aChannel *, aClient *, aClient *); #ifdef EXEMPT_LISTS diff --git a/src/channel.c b/src/channel.c index 642d7f1..1662f33 100644 --- a/src/channel.c +++ b/src/channel.c @@ -537,6 +537,26 @@ static int is_banned(aClient *cptr, aChannel *chptr, chanMember *cm) return 0; }
+/*
- Forces the cached banned status for a user to be flushed in all the channels
- they are in.
- */
+void flush_user_banserial(aClient *cptr) +{
- Link *ptr;
- if (!IsPerson(cptr))
- return;
- for (ptr = cptr->user->channel; ptr; ptr = ptr->next)
- {
- aChannel *chptr = ptr->value.chptr;
- chanMember *cm = find_user_member(chptr->members, cptr);
- if (cm)
- cm->banserial = chptr->banserial - 1;
- }
+}
aBan *nick_is_banned(aChannel *chptr, char *nick, aClient *cptr) { aBan *ban; diff --git a/src/m_nick.c b/src/m_nick.c index 79eab36..6eee775 100644 --- a/src/m_nick.c +++ b/src/m_nick.c @@ -575,6 +575,13 @@ int m_nick(aClient *cptr, aClient *sptr, int parc, char *parv[]) /* If it changed nicks, -r it */ if (mycmp(parv[0], nick)) sptr->umode &= ~UMODE_r;
- /*
- * Flush the banserial for the channels the user is in, since this
- * could be a SVSNICK induced nick change, which overrides any ban
- * checking on the originating server.
- */
- flush_user_banserial(sptr);
} } else diff --git a/src/m_services.c b/src/m_services.c index 50e0b87..bc97958 100644 --- a/src/m_services.c +++ b/src/m_services.c @@ -230,6 +230,7 @@ int m_svsnick(aClient *cptr, aClient *sptr, int parc, char *parv[]) strcpy(acptr->name, newnick); add_to_client_hash_table(acptr->name, acptr); hash_check_watch(acptr, RPL_LOGON);
- flush_user_banserial(acptr);
return 0; } -- 1.6.2.4
-- This message has been scanned for viruses and dangerous content by MailScanner, and is believed to be clean.
If they then try to talk in the channel they are in, the local server will allow them to do so, while other servers on the network will silently drop whatever they say (since the other servers believe they are still banned.)
I wonder... is it even really good for other servers to attempt to capture 'client banned' status and suppress messages? It seems like only the client's locally connected server should really be concerned about enforcing ban restrictions, like suppressing messages.
The result of suppressing on a remote server is inconsistent operation if different servers have different opinions about a user's banned status; this is not very robust.
Consider the case where baduser!*@* is banned. All the channels ops are on Server2. Most of the other channel members are on Server1, which is also the server baduser is on.
Server1 and Server2 banlists are in sync, but cached member status is not -- Server1 believes baduser is not banned, Server2 believes baduser is banned.
As a result, baduser can flood and send other nasty messages to his heart's content. None of the ops can see it, but all the members on Server1 have to endure it.
What should happen is Server2 should either pass the messages or detect them as a sign of desync, and take some sort of remediary action....
But just silently discarding messages is pretty bad; I wouldn't count that as remediary action, it just serves to partially conceal (from some channel members) that something is going wrong...
-- -J
On Fri, Apr 24, 2009 at 11:15:24PM -0500, James Hess wrote:
Consider the case where baduser!*@* is banned. All the channels ops are on Server2. Most of the other channel members are on Server1, which is also the server baduser is on.
Server1 and Server2 banlists are in sync, but cached member status is not -- Server1 believes baduser is not banned, Server2 believes baduser is banned.
As a result, baduser can flood and send other nasty messages to his heart's content. None of the ops can see it, but all the members on Server1 have to endure it.
This is a good point, especially since SVSNICK handling will be inconsistent until every server on the network is upgraded.
NICK can be changed to update the banserial on a nick change, and the code to drop messages from banned non-local users can be removed.
-
Aaron Wiebe -
epiphani -
James Hess -
Kevin Buley -
kevin＠buley.org
-
Ned T. Crigler