oopsbot should have flooder defences

VERIFIED FIXED in 2.2

Status

Webtools
Mozbot
--
enhancement
VERIFIED FIXED
17 years ago
9 years ago

People

(Reporter: Hixie (not reading bugmail), Assigned: Jacob Steenhagen)

Tracking

other
Other
Other

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Reporter)

Description

17 years ago
From the wishlist: 

<David> lordpixel wants a mzobot attack asshole command for flooders

Comment 1

17 years ago
lol. That's the first bug I've ever had filed on the basis of a joke I made. 

I'm trying to remember the context so I can suggest something useful, but I 
suspect someone much more well versed in IRC could do a better job.

IIRC, someone had just come in to do a literal flood attack, so lots of very 
similar alphanumeric logins (from multiple hosts, I assume they were compromised) 
and each proceeded to generate a ton of traffic, outstripping the ability of the 
human channel operators to kick/ban them. (what does kline mean anyhow?)

Some sort of tool to allow a channel op or irc op to automatically shut down a 
flood attack is desirable, but given a well structured flood is also a DDOS 
(multiple compromised hosts) this is probably quite hard...

Comment 2

17 years ago
easiest solution is to respond to join flood w/ +i and then respond to channel 
flood (by two-three people) w/ +m.  
(Reporter)

Comment 3

17 years ago
This should be quite easy.

If a channel has more than <n> successive joins in period <t>, then make it +i
for a period of time <l>.

If a channel has more than <N> lines of traffic in period <T>, then make it +m 
for a period of time <L>.

How does that sound?

cc'ing Pavlov and Dawn for their input.

Comment 4

17 years ago
sounds good.
...
depending on how stupid we think the flooders are, we might consider using +k 
before +i, so:

if n joinflooders in time t, +k random_or_fixed_string
if p (p is probably smaller than n) joinflooders (preferably stricter 
interpretation of the term) in time u w/in v of t, -k+i
this gives normal users on the channel a way to help other normal users join 
but makes unattended attacks fail.  smart attacks will trigger the stricter 
mode at only a slight additional cost to normal people.

I'd suggest k interim be an optional feature (probably disabled by default, but 
possibly tested on #mozillazine)
(Reporter)

Comment 5

17 years ago
timeless: it would really save everyone a lot of effort if you would use whole
sentences when commenting, and I'm sure it wouldn't take you much longer.

Yes, I think making a channel +k first would not be a bad idea.
New QA.
QA Contact: matty → timeless
(Assignee)

Comment 7

17 years ago
Wow... I was thinking about this yesterday and just started coding... when I was
"done" and gonna submit my work, I figured I should look fist and see if anybody
had requested it... then I found this bug :)

What this (forthcoming) modules does is after <n> (numberOfJoins - default 7)
successive joins in period <t> (secondsToTrigger - default 2), then make it +i
for a period of time <l> (minutesToProtect - default 5).

If numberOfJoins or secondsToTrigger are not positive integers, it essentially
does nothing.  If minutesToProtect is not a positive integer, it never sets the
mode -i (an op must do so manually).

Like I said, I had finished coding before I found this bug so It doesn't do any
of the other possibilies mentioned here.  It is currently running in ssdbot in
#mozwebtools with the default values, but we get very few flood attacks :)
(Assignee)

Comment 8

17 years ago
Created attachment 38630 [details]
Flood.bm
(Reporter)

Comment 9

17 years ago
Some review comments:

Your |RegisterConfig| method doesn't call it's inherited method. From the docs:
    Always call inherited function!

The condition |$variable !~ m/[1-9][0-9]*/| will be false when $variable has the
value |-1|. You probably want to check for |$variable !~ /^[0-9]+$/|. Also,
don't forget to add |/o| to regexp matches (o for optimise).

Why do you return |5| in |SpottedJoin|? You should call the inherited method if
you cannot deal with the command. (Returning 5 would result in you being called
5 times, in fact...)

Don't do:
    $channel =~ s/^#//;
...since channels can start with many characters other than "#", as timeless 
will happily tell you. Is there any reason to take the "#" off at all? I would
strongly recommend leaving it. (You do this twice.)

Your comment has the string |canfigure| which is missing a space (|can figure|).

The condition |$self->{'minutesToProtect'} =~ m/[1-9][0-9]*/| is again wrong and
is missing the |/o|. Try |$self->{'minutesToProtect'} =~ /^[0-9]+$/o|.

Any reason for the following line to be split in two? I would embed the |@mode|
definition inside the method call:
                my @mode = ('mode', $event->{'channel'}, '-i');
                $self->schedule($event, $seconds, 1, @mode);
That's just a very minor performance nit though.

Your |Scheduled| method doesn't call its inherited method.

You have a lot of whitespace at the bottom of your file.

BTW, I like the wy you call |registerVariables| from outside the 
|RegisterConfig| method -- I hadn't thought of doing that before! It's a very
neat trick. The only problem with it is that you if join some channels, tweak
the config, and then leave the channels, the data will never be loaded back in
(since the variables are registered at that point). Of course, in this scenario
it doesn't matter (since for that to happen some time has to elapse, and thus
the count will automatically be incorrect), but it is something to bear in mind
for future modules. You might want to put a comment about that somewhere.

Overall, looks nice!
Assignee: ian → jake
(Assignee)

Comment 10

17 years ago
Hmmm... I missed a lot of inherited methods... in fact, I don't think I did a
single one :(

Regarding the m/[1-9][0-9]*/ expressions, I used those there for the same reason
Myk used them in ValidateBugID() [bugzilla's CGI.pl].  I don't want '0', '-1',
or 'a' to be considered valid (and in fact setting it to any of those values
will be like turning off that part of the module).

Regarding $channel =~ s/^#//, I don't remember why I did that.  I did it once
and then just had to keep it up.

Regarding my @mode, I thought it made it a bit easier for readability (makes it
more obvious why the Scheduled sub is going to see)... but maybe that's just me.

I'll upload an updated version for review momentarily.
Keywords: review
(Assignee)

Comment 11

17 years ago
Created attachment 38761 [details]
Flood.bm v2
(Reporter)

Comment 12

17 years ago
Much better, still some nits though. :-)

In |JoinedChannel|, you call the inherited method but lose the return value --
what you want to do is:
    return $self->SUPER::JoinedChannel($event, $channel);
...as that the return value is preserved.

Regarding the regular expression match -- yes, I agree entirely on what you are
trying to do, but the problem is your code is wrong! :-) The condition you use, 
namely |$variable !~ m/[1-9][0-9]*/|, will be *false* when $variable has the
value |-1|. I think you want to check for |$variable !~ /^[0-9]+$/| or maybe
the quicker |$variable =~ /[^0-9]/|.

Finally, in |Scheduled| you call the inherited method too much now. :-) If you
can handle the event, then you know that the parent doesn't want to, since it
can't know how to handle it. I would recommend using a construct such as:

   my $what = shift(@data);
   if ($what eq 'mode') {
       $self->mode($event, @data);
   } else {
       # Call the inherited event
       $self->SUPER::Schedule(@_);
   }

(However you do it, don't forget to pass the arguments as well.)
(Assignee)

Comment 13

17 years ago
Created attachment 38953 [details]
Flood.bm v3
(Assignee)

Comment 14

17 years ago
OK, I fixed the inherited method stuff.

As for the regular expression... I finally know why -1 was being approved... the
same reason hj5ui would've made it through... I forget the '^' and '$'!  To find
(and fix this, I did this tiny perl script:
--------------------------------
#!/usr/bin/perl

my $numberOfJoins = -1;
my $secondsToTrigger = 1;

if ($numberOfJoins !~ m/^[1-9][0-9]*$/o 
    || $secondsToTrigger !~ m/^[1-9][0-9]*$/o) {
    print "Returning....\n";
} else {
    print "Doing the rest of this stuff\n";
}
--------------------------------
and fiddled with the values of my variables.  After adding '^' and '$' I got the
responce I was expecting each time.

I also added a (rather lengthy) comment describing what is currently the only
remaining issue I know of.
(Reporter)

Comment 15

17 years ago
I think you attached the wrong patch...
(Assignee)

Comment 16

17 years ago
Created attachment 39259 [details]
Flood.bm v3 (for real this time)
(Assignee)

Comment 17

17 years ago
I sure enough did... sorry about that...
(Assignee)

Comment 18

17 years ago
Hixie, any chance of getting an r= on this so it can be checked in?
(Reporter)

Comment 19

17 years ago
Regarding the comment, there is indeed a way to hook in:

 # Set - called to set a variable to a particular value. 
 sub Set {
     my $self = shift;
     my ($event, $variable, $value) = @_;
     if ($variable eq 'numberOfJoins') {
          # XXX do your stuff here
     }
     # now actually do the setting of the variable
     return $self->SUPER::Set($event, $variable, $value);
 }

Once you've done that, you can get rid of the resetting code in JoinedChannel,
since we don't really want someone flooding oopsbot off causing oopsbot to think
it's ok. ;-)

Other than that, looks pretty good!
(Reporter)

Comment 20

17 years ago
RE your comment on IRC, once you've fixed that and I've looked at it one more 
time to be sure, then to get this into CVS you need a= from me and r= from some-
one else, e.g. zach or timeless (shouldn't be a problem, the code is simple).
(Assignee)

Comment 21

17 years ago
Created attachment 42150 [details]
Flood.bm v4
(Reporter)

Comment 22

17 years ago
(Just a stylistic nit, btw, the following:
   @{$self->{"join_$chan"}} = ();
...could also be written using slightly less punctuation as:
   $self->{"join_$chan"} = [];
...without omitting any optional punctuation.)

Looks good to me, assuming you can get a review from zach or timeless, a=hixie.
Assuming you are ok with using an MPL/GPL dual license, feel free to then check 
it in as is into the "BotModules" directory in CVS. (If you want another license
then speak to me about it on IRC.)
Keywords: approval, patch

Comment 23

17 years ago
This looks good. I'll do some testing and probably r= it.

Zach
(Assignee)

Comment 24

17 years ago
Got an r= zach@zachlipton.com in IRC.

Checked in.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 25

17 years ago
mozilla/webtools/mozbot/BotModules/Flood.bm
Status: RESOLVED → VERIFIED
QA Contact: timeless → timeless
(Reporter)

Comment 26

17 years ago
Thanks Jake, you rock!
QA Contact: timeless → mozbot

Updated

9 years ago
Target Milestone: --- → 2.2
You need to log in before you can comment on or make changes to this bug.