View Issue Details

IDProjectCategoryView StatusLast Update
0002992unrealircdpublic2006-08-03 05:48
ReportertabrisnetAssigned Tosyzop 
PrioritynormalSeveritymajorReproducibilityN/A
Status resolvedResolutionfixed 
PlatformLinux/x86OSDebian LinuxOS Versiontesting
Product Version3.2.5 
Target VersionFixed in Version3.2.6 
Summary0002992: Desync when mixing +Q & KICKs
DescriptionAssume the following obvious race.
Services mlocks +Q
User sets -Q, KICKs, Services resets +Q

Aparrently this [can] cause[s] a desync. Server of kicker sees user kicked. Server kick-target is on does not.

I agree that users shouldn't be racing this, as it's rather bad form, but it does occur. I was in the process of coding services to counteract this (KILL kicker, fjoin kick-target back into channel).

Additional InformationI have a two server testnet, both running 3.2.5 (one compiled with IPv6, one not. Should be irrelevant).

The fact of the zero lag (testnet on LAN) may contribute to the race/desync, but I don't know.
TagsNo tags attached.
3rd party modules

Activities

w00t

2006-07-09 10:18

reporter   ~0012061

Unless I'm wrong, remote KICK is always to be accepted (least, I remember a comment from my recent chainsawing) - so that would be rather a weird one, or human error in the code.

tabrisnet

2006-07-09 12:26

reporter   ~0012063

Looking at the code, it does look like it's a missing && MyClient(sptr) check.

The funny thing is... just ONE block above is the comment that it must do exactly that, but the two blocks below it do NOT.

The simplest fix would be like this:

modify if (IsUline(sptr) || IsServer(sptr)) goto attack;
to this: if (IsUline(sptr) || IsServer(sptr) || !MyClient(sptr)) goto attack;

then we can drop below and remove the MyClient(sptr) check one below.

The alternative is to add another MyClient(sptr) check to the if (chptr->mode.mode & MODE_NOKICKS). I'm not sure I like that or not. I haven't looked at the macro to see if MyClient is just another flag check or more involved.

w00t

2006-07-27 13:35

reporter   ~0012087

MyClient is a check on client FD being >= 0 I think

tabrisnet

2006-07-28 00:29

reporter   ~0012092

Last edited: 2006-07-28 01:10

I looked at the code more carefully, and I don't think I can do the simple solution as it would skip all of the OperOverride checks.

Attached is a patch that appears to fix the bug, and also eliminates some of the spurious OperOverride notices. I am not sure yet if it's safe however.

It is compile and runtested, but I would like it to have some more eyes look at it.

2006-07-28 01:09

 

kick_NOKICK_fix.diff (2,846 bytes)

codemastr

2006-07-29 17:24

reporter   ~0012094

Someone remind me again why we still have +Q?

tabrisnet

2006-07-29 18:05

reporter   ~0012099

My understanding is that it's to try to keep 'peace' in a channel.

And I just implemented (well, did it 2 weeks ago or so) a feature whereby the ChanServ kick system refuses to kick someone who has channel-access if +Q is mlocked.

Only trouble is, b/c this is raceable, it's still not quite done. We can't eliminate the race for mlock, but we can counteract it. Which I also implemented.

Furthermore, this bug I tracked down is true regardless of my 'feature' as it is still raceable, and will still cause a desync.

I'd still like to keep it.

Bock

2006-07-30 03:56

reporter   ~0012100

tabrisnet, I see, that you can do somth with this: http://bugs.unrealircd.org/view.php?id=2889
Imho, if look at your patch, you don't verify if the person who kicks is a halfop and if he kicks hime/herself. Sorry, if this is offtopic :]

tabrisnet

2006-07-30 04:05

reporter   ~0012101

I'll look at it later, my main devel box just coughed up a lung (harddrive said it failed, tho after cleaning the inside of the case it seems to be OK now).

I guess I could try stricmp(sptr->name, who->name)

All I was going for at the time was to a) make operoverride checks local only [and broadcast them via sendsno) b) fix this bug

and tbh, (a) only came about b/c it was the cleanest way to do (b)

tabrisnet

2006-07-30 04:51

reporter   ~0012103

Last edited: 2006-07-30 04:53

Previous note deleted b/c it was wrong... I have multiple leaves on my testnet, but only one has that patch.

It's 7 to 6AM... clearly I should be sleeping and not coding.

2006-07-30 05:07

 

kick_NOKICK_fix-v2.diff (3,217 bytes)

tabrisnet

2006-07-30 05:08

reporter   ~0012104

Ok, this patch addresses the issue of kicking oneself as well.

2889 is about MODE, and tbh I'm not touching MODE right now. I'll leave that for another day. Besides, I think that services should have an unrestricted command for removing channel op (my services package does, as did Auspice).

syzop

2006-08-02 09:44

administrator   ~0012110

Last edited: 2006-08-02 09:45

I agree we should let ULines do whatever they want, and that +Q checking should be done locally (race condition fun), what I don't agree with though is operoverride being done entirely locally. Whenever possible, and not too much of an issue, doing it remotely is safer (like with tweaked servers and such), and to my knowledge remote servers have all the info they need to decide whether or not it was an override. The only thing I can see is a small race condition (+Q, arriving kick) resulting in a override message when it shouldn't, but I think we can live with that.

Anyway, I'm going to give my thoughts a second thought (doesn't that sounds nicely?). I'll also have to take the factor of double operoverride notices into account, and things like that.

tabrisnet

2006-08-02 15:05

reporter   ~0012111

I admit to having a vested interest in local-only override-checks (distributing the SENDSNO instead), as it then generates a SENDSNO, and I want to log override messages.

The patch may be rewritable w/ the override checks being distributed, I'll have to look over the code again and rewrite the patch.

2006-08-02 17:47

 

kick_NOKICK-fix-v3.diff (1,573 bytes)

tabrisnet

2006-08-02 17:48

reporter   ~0012115

v3 should make no appreciable changes to override-logging, but should fix both bugs, the original and the one Bock mentioned...
wait, I might have forgotten to do it for +a kicking self too.
oh well. That is a trivial add.

syzop

2006-08-02 17:54

administrator   ~0012116

oh I should have told you, I've a fix in my commit queue now (inspired by your patches ;p).

tabrisnet

2006-08-02 17:59

reporter   ~0012119

Damn you ;)

I would also like to see it (pull it from CVS or something) so I can put it on my network ASAP. Yet another 'hotpatch/hotfix'. Not a security bug admittedly, but it is a desync, and right now I cannot prevent my users from abusing it.

syzop

2006-08-03 05:48

administrator   ~0012127

Last edited: 2006-08-03 05:48

Should be fixed in .548

EDIT: yes, the wonderful world of hotfixing ;)

Issue History

Date Modified Username Field Change
2006-07-09 00:22 tabrisnet New Issue
2006-07-09 10:18 w00t Note Added: 0012061
2006-07-09 12:26 tabrisnet Note Added: 0012063
2006-07-27 13:35 w00t Note Added: 0012087
2006-07-28 00:29 tabrisnet Note Added: 0012092
2006-07-28 01:09 tabrisnet File Added: kick_NOKICK_fix.diff
2006-07-28 01:10 tabrisnet Note Edited: 0012092
2006-07-29 17:24 codemastr Note Added: 0012094
2006-07-29 18:05 tabrisnet Note Added: 0012099
2006-07-30 03:56 Bock Note Added: 0012100
2006-07-30 04:06 tabrisnet Note Added: 0012101
2006-07-30 04:51 tabrisnet Note Added: 0012103
2006-07-30 04:53 tabrisnet Note Edited: 0012103
2006-07-30 05:07 tabrisnet File Added: kick_NOKICK_fix-v2.diff
2006-07-30 05:08 tabrisnet Note Added: 0012104
2006-08-02 09:44 syzop Note Added: 0012110
2006-08-02 09:45 syzop Note Edited: 0012110
2006-08-02 15:05 tabrisnet Note Added: 0012111
2006-08-02 17:47 tabrisnet File Added: kick_NOKICK-fix-v3.diff
2006-08-02 17:48 tabrisnet Note Added: 0012115
2006-08-02 17:54 syzop Note Added: 0012116
2006-08-02 17:59 tabrisnet Note Added: 0012119
2006-08-03 05:48 syzop Status new => resolved
2006-08-03 05:48 syzop Fixed in Version => 3.2.6
2006-08-03 05:48 syzop Resolution open => fixed
2006-08-03 05:48 syzop Assigned To => syzop
2006-08-03 05:48 syzop Note Added: 0012127
2006-08-03 05:48 syzop Note Edited: 0012127