View Issue Details

IDProjectCategoryView StatusLast Update
0002321unrealircdpublic2010-08-15 06:51
ReportersyzopAssigned Toohnobinki 
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product Version 
Target VersionFixed in Version3.2.9-RC1 
Summary0002321: ipv6 clones checking
DescriptionAFAIK we don't have this, but I think it's kinda needed:

we shouldn't be checking for clones on ipv6 addresses with an *EXACT* IP, but rather on.. well, I forgot, but.. say /64, like most ipv6 ircds do.
Else someone can easily launch like 1000+ clones, and if no oper is present (even if just for a few minutes) you can have not-so-nice-effects :p.
TagsNo tags attached.
3rd party modules

Relationships

child of 0003776 resolvedsyzop Unreal3.2.9 TODO 

Activities

codemastr

2005-02-04 00:27

reporter   ~0009024

Yeah, I guess that's a good idea.

stskeeps

2005-10-30 13:20

reporter   ~0010617

that's a bit problematic, since part of that can be encapsulated MAC address, which will be used A LOT in future IPv6 setups. We should invent filters/modules for noticing a sudden rise in connections from certain subnets instead. Perhaps something to kill mirkforce things too?

syzop

2010-07-02 10:01

administrator   ~0016134

got reminded by this by someone in #unreal-support.

Technically, no problem to implement this. Just need to think on how to do it configuration-wise (unrealircd.conf). As not everyone would want to limit per /whatever CIDR mask, some would still want to limit by-ip like now.

There are two options I can think of:
1) Global setting, of what is considered an ipv6 clone
2) Per-class setting

Since you might want to make a distinction between clone settings per-netmask, the per-class would be needed. Like, one subnet might have many with per-ip (->special class), while you would like to do per-netmask for all others (->regular client class).

Actually, how about doing both? Having a global setting which is the default for all classes, which can be overridden in each class.

Now as for the name... hmmm.. ipv6-clones-netmask, or something with cidr or bitmask...

set::ipv6-clones-bitmask 32;

not sure.. I'm open for suggestions.

since this only takes little coding to implement and might be quite an issue in some cases I think we should implement this for 3.2.9, if possible.

tabrisnet

2010-07-14 16:59

reporter   ~0016161

As the policy from various RIRs is that allocation to end-point (residential) will be /64, and to corporate entities will be /48, making the bitmask 32 is highly unlikely, albeit I wouldn't recommend making there be a minimum size of the mask as much as that we should make it clear in the docs that due to the RIR rules...

syzop

2010-07-14 17:13

administrator   ~0016167

I just picked a random value of 32 in my example, if that's what you mean :)
But what's your opinion on the general idea?

This issue is not 100% Release Critical, but is high on my "3.2.9 should have this" list.

tabrisnet

2010-07-14 17:19

reporter   ~0016169

No objection to the idea... hell, I'd expand it a bit and by default HIDE the bottom 64 bits including from the cloak. So then one only looks at the top 64 bits for purposes of channel bans & clone counting.

Not to say that we should drop the lower 64bits altogether if they umode -x, or hide it from opers.

ohnobinki

2010-08-11 05:41

reporter   ~0016258

So far for config naming, my thoughts are:
/* default of 96, assuming that most people have their own 32 bits */
set::default-ipv6-clones-mask 96;

/*
 * the allow blocks would default to set::default-ipv6-clones-mask.
 * they would have the option below to override the default.
 */
allow::ipv6-clones-mask <set::default-ipv6-clones-mask>;

For the masks, I would give the user an error if they set the mask to 0 and a warning if it was <= 32. Should I probably warn for <= 64?

syzop: Disallowing 0 would not hurt anyone and would be one way to implement a default allow::ipv6-clones-mask. (If the user doesn't set the allow::ipv6-clones-mask, I could set the variable to 0. After the entire config file is parsed, I would check for allow blocks that have a value of 0 and replace it with the set::default-ipv6-clones-mask. Is this the best/proper way to implement overridable defaults for this case? ;-) ).

syzop

2010-08-11 16:44

administrator   ~0016259

Sure, sounds fine.

If I understand correctly, if one would set it to 128 you get the current behavior, right? Just making sure that's still permitted, without warning.
The warning would be just there when illogical configuration is encountered.

I wouldn't mind us setting a default for set::default-ipv6-clones-mask, of like 96... [that would be done in config_setdefaultsettings()]

Yes, the way you describe would indeed be the way to go with regards to setting the allow::ipv6-clones-mask with the set::default-ipv6-clones-mask if the allow:: one is not set.
And yes, I don't see why to permit 0 either ;)

tabrisnet

2010-08-11 16:53

reporter   ~0016260

I think there are only 2 sane defaults: 64 and 128. I'm not aware of _any_ RIR that has a policy that ISPs should hand out longer prefixes than /64. Yes, I agree that a /64 is ridiculous, no one could ever use that much address space and a /96 would be more sensible... But the regulations from ARIN (and presumably other RIRs) remain.

Btw, what does this do to cloaking? How are the bits divided among the various components of the cloaks? I know that for IPv4 it was 16/8/8.

If your plan goes something like 16/16/16/16/64, that would probably be reasonable. But I don't seem to remember an IPv6 cloak being that long.

ohnobinki

2010-08-11 17:56

reporter   ~0016261

> If I understand correctly, if one would set it to 128 you get the current behavior, right?

Yes, this is perfectly valid. I intend to put this as an example in the documentation when I get this functional.

> I wouldn't mind us setting a default for set::default-ipv6-clones-mask, of like 96... [that would be done in config_setdefaultsettings()]

That's actually about all the progress I made last night ;-).

> Btw, what does this do to cloaking?

As far as I understand, this bug doesn't have anything to do with cloaking or channel bans. It just affects the code that enforces allow::maxperip. (at least, so far...)

syzop

2010-08-12 11:52

administrator   ~0016262

Ok, as for the default value for set::default-ipv6-clones-mask: users/admins usually get a /80 (which they subnet themselves to /64) or a /64 directly, right? So then 64 would be a good default?

As for cloaking, that is indeed not what this bugreport is about. tabrisnet: if you would like to put in a request for that, you'd have to open up a new one.

2010-08-13 06:52

 

unreal-2321-ipv6-clones.patch (10,828 bytes)

ohnobinki

2010-08-13 06:56

reporter   ~0016263

unreal-2321-ipv6-clones.patch: my first complete try at doing this. Tested with ipv6-clones-mask of 128 and 127 and by adding loopback addresses (for 127, ::2 and ::3 overlap, another set of overlapping addresses would be ::12 and ::13).

I changed the defaults mask to 64 bits. I poked at the docs.

Unfortunately, this change leaves AllowClient() in a less readable state.

Any comments more than welcome :-).

syzop

2010-08-13 11:00

administrator   ~0016264

Last edited: 2010-08-13 11:01

I was just taking a quick look, so I might overlook something, but...
the stuff with ipv6_mask_addr in AllowClient():
+#ifdef INET6
+ is_ipv4 = IN6_IS_ADDR_V4MAPPED(&cptr->ip);
+
+ if(is_ipv4)
+ {
+ /*
+ * initialize a mask for IPv6 clones detection. It has
+ * to be zeroed out after aconf->ipv6_clone_mask
+ * bits. Code inspired by match_ipv6().
+ */
+ memset(&ipv6_mask_addr, 0, sizeof(ipv6_mask_addr));
+ i = aconf->ipv6_clone_mask / 8;
+ memcpy(&ipv6_mask_addr, &cptr->ip, i);
+ /* support ipv6_clone_mask not being a multiple of 8 */
+ ipv6_mask_addr.s6_addr[i] |=
+ ~((1 << (8 - aconf->ipv6_clone_mask % 8)) - 1) & cptr->ip.s6_addr[i];
+ }
+#endif /* INET6 */

(after that) it doesn't seem to do anything with ipv6_mask_addr? So either it can be removed or something was forgotten :P. I think the former, since match_ipv6() does all the work?
EDIT: obviously with 'removed' I mean the 'if(is_ipv4)' block, and not the setting of 'is_ipv4' ;)

And something really tiny, but it would continue if I never mention it: I noticed you change 'i++' to 'i ++' here and there (same for errors ++), but since we use varname++ everywhere I would ask you not to (IOTW conform to current style) :)

Other than that, patch looks good, I think. I didn't test it, but it looks clean, has docs, etc... good job. Yet another item smashed, working towards a release :P

Now I should do something too...

ohnobinki

2010-08-13 15:45

reporter   ~0016265

> (after that) it doesn't seem to do anything with ipv6_mask_addr? So either it can be removed or something was forgotten :P.

This would be wonderful, but match_ipv6() only fixes up its first argument (addr), not its second argument (mask). It expects mask to be all zeros after the first number of bits specified by match_ipv6()'s third argument (bits). I actually realize that I had forgotten to tie in use of ipv6_mask_addr while waking up this morning ;-). Or that's at least the reason that I wrote that piece of code ;-).

There is an alternative and cleaner method with patching match_ipv6() in cidr.c:
@@ -311 +311,2 @@
- if ((addr->s6_addr[n] & ~((1 << (8 - m)) - 1)) == mask->s6_addr[n])
+ if ((addr->s6_addr[n] & ~((1 << (8 - m)) - 1))
+ == mask->s6_addr[n] & ~((1 << (8 - m)) - 1)))

In fact, I'm going to get rid of the humungous blob you quoted in favor of this ;-).

> I noticed you change 'i++' to 'i ++' here and there

Oops, I didn't realize i++ was the style even though I saw it everywhere.

Thanks for the review :-).

2010-08-13 16:34

 

unreal-2321-ipv6-clones-r1.patch (11,103 bytes)

ohnobinki

2010-08-13 16:35

reporter   ~0016266

unreal-2321-ipv6-clones-r1.patch: tested with mask of 127 and 96, I think this is ready to commit.

syzop

2010-08-14 19:30

administrator   ~0016267

Fine I think? :)

ohnobinki

2010-08-15 06:51

reporter   ~0016273

- IPv6 clones detection support (0002321). allow::ipv6-clone-mask determines the number of bits used when comparing two IPv6 addresses to determine if allow::maxperip is exceeded. This allows an admin to recognize that most IPv6 blocks are allocated to individuals, who might each get a /64 IPv6 block. set::default-ipv6-clone-mask defaults to 64 and provides default value for the allow blocks.

Issue History

Date Modified Username Field Change
2005-02-03 21:21 syzop New Issue
2005-02-04 00:27 codemastr Note Added: 0009024
2005-10-30 13:20 stskeeps Note Added: 0010617
2007-04-27 03:56 stskeeps Status new => acknowledged
2007-04-27 03:56 stskeeps View Status private => public
2010-07-02 09:54 syzop Relationship added child of 0003776
2010-07-02 10:01 syzop Note Added: 0016134
2010-07-14 16:59 tabrisnet Note Added: 0016161
2010-07-14 17:13 syzop Note Added: 0016167
2010-07-14 17:19 tabrisnet Note Added: 0016169
2010-08-11 04:32 ohnobinki Status acknowledged => assigned
2010-08-11 04:32 ohnobinki Assigned To => ohnobinki
2010-08-11 05:41 ohnobinki Note Added: 0016258
2010-08-11 16:44 syzop Note Added: 0016259
2010-08-11 16:53 tabrisnet Note Added: 0016260
2010-08-11 17:56 ohnobinki Note Added: 0016261
2010-08-12 11:52 syzop Note Added: 0016262
2010-08-13 06:52 ohnobinki File Added: unreal-2321-ipv6-clones.patch
2010-08-13 06:56 ohnobinki Note Added: 0016263
2010-08-13 11:00 syzop Note Added: 0016264
2010-08-13 11:01 syzop Note Edited: 0016264
2010-08-13 15:45 ohnobinki Note Added: 0016265
2010-08-13 16:34 ohnobinki File Added: unreal-2321-ipv6-clones-r1.patch
2010-08-13 16:35 ohnobinki Note Added: 0016266
2010-08-14 19:30 syzop Note Added: 0016267
2010-08-15 06:51 ohnobinki QA => Not touched yet by developer
2010-08-15 06:51 ohnobinki U4: Need for upstream patch => No need for upstream InspIRCd patch
2010-08-15 06:51 ohnobinki Note Added: 0016273
2010-08-15 06:51 ohnobinki Status assigned => resolved
2010-08-15 06:51 ohnobinki Fixed in Version => 3.2.9-RC1
2010-08-15 06:51 ohnobinki Resolution open => fixed