View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0002321 | unreal | ircd | public | 2005-02-03 21:21 | 2010-08-15 06:51 |
| Reporter | syzop | Assigned To | ohnobinki | ||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | resolved | Resolution | fixed | ||
| Fixed in Version | 3.2.9-RC1 | ||||
| Summary | 0002321: ipv6 clones checking | ||||
| Description | AFAIK 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. | ||||
| Tags | No tags attached. | ||||
| Attached Files | |||||
| 3rd party modules | |||||
|
|
Yeah, I guess that's a good idea. |
|
|
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? |
|
|
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. |
|
|
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... |
|
|
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. |
|
|
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. |
|
|
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? ;-) ). |
|
|
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 ;) |
|
|
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. |
|
|
> 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...) |
|
|
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. |
|
|
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 :-). |
|
|
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... |
|
|
> (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 :-). |
|
|
unreal-2321-ipv6-clones-r1.patch: tested with mask of 127 and 96, I think this is ready to commit. |
|
|
Fine I think? :) |
|
|
- 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. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2005-02-03 21:21 | syzop | New Issue | |
| 2005-02-04 00:27 |
|
Note Added: 0009024 | |
| 2005-10-30 13:20 |
|
Note Added: 0010617 | |
| 2007-04-27 03:56 |
|
Status | new => acknowledged |
| 2007-04-27 03:56 |
|
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 |
