View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0002992 | unreal | ircd | public | 2006-07-09 00:22 | 2006-08-03 05:48 |
| Reporter | tabrisnet | Assigned To | syzop | ||
| Priority | normal | Severity | major | Reproducibility | N/A |
| Status | resolved | Resolution | fixed | ||
| Platform | Linux/x86 | OS | Debian Linux | OS Version | testing |
| Product Version | 3.2.5 | ||||
| Fixed in Version | 3.2.6 | ||||
| Summary | 0002992: Desync when mixing +Q & KICKs | ||||
| Description | Assume 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 Information | I 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. | ||||
| Tags | No tags attached. | ||||
| Attached Files | |||||
| 3rd party modules | |||||
|
|
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. |
|
|
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. |
|
|
MyClient is a check on client FD being >= 0 I think |
|
|
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. |
|
|
Someone remind me again why we still have +Q? |
|
|
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. |
|
|
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 :] |
|
|
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) |
|
|
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. |
|
|
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). |
|
|
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. |
|
|
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. |
|
|
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. |
|
|
oh I should have told you, I've a fix in my commit queue now (inspired by your patches ;p). |
|
|
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. |
|
|
Should be fixed in .548 EDIT: yes, the wonderful world of hotfixing ;) |
| 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 |
|
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 |
