View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0003907 | unreal | ircd | public | 2010-05-18 22:59 | 2010-06-10 05:20 |
| Reporter | tabrisnet | Assigned To | ohnobinki | ||
| Priority | normal | Severity | major | Reproducibility | random |
| Status | resolved | Resolution | fixed | ||
| Platform | Linux/x86 | OS | Debian Linux | OS Version | testing |
| Product Version | 3.2.8 | ||||
| Fixed in Version | 3.2.9-RC1 | ||||
| Summary | 0003907: IPv6/4 addrs (::ffff:w.x.y.z) break NICK connection parsing | ||||
| Description | With a custom CGI-IRC client connecting from localhost and passing a WEBIRC with an IPv4 addr, the IRCd sometimes replaces the IPv4 addr w.x.y.z with ::ffff:w.x.y.z. This then produces the following & usmb_h96 1 !1Byl9B htIRC-0min ::ffff:41.103.69.234 9 0 +ix * A9694D2B:80B733CB:872E6D4A:IP KWdF6g== :WWW User This then gets misparsed with the leading colon, such that everything after it is taken as a single param, and thus invalid. | ||||
| Steps To Reproduce | No known way to reproduce intentionally. Does happen semi-regularly on SurrealChat.net, and as far as can be determined, only with the CGI-IRC client. | ||||
| Additional Information | This has been extensively discussed and analyzed with binki | ||||
| Tags | No tags attached. | ||||
| Attached Files | |||||
| 3rd party modules | |||||
|
|
Regardless of whether or not my patch fixes this problem, it does at least do a few positive things. - In m_pass.c, Inet_ia2p(&cptr->ip) should be used instead of the local variable `ip' or `ipbuff' - In m_nick.c:_register_user(), the protection against evil DNS resolvers does not filter out `:', which means that if the resolver accepted a ``:blah.net'' as a hostname, then unrealircd would be confused by the `:'. The patch does add a sendto_realops() which should be removed after tabrisnet has tested the patch _unless_ if others think that the message would be useful. |
|
|
Ok, I've seen it happen now with non-cgiirc users (we had a spamflood today). But so far only (afaik) on one server. This server is probably the only one I have running Debian Squeeze AMD64. Is that relevant? |
|
|
I don't see this stated anywhere, but... the ircd with the CGI:IRC client connected to it is IPv6 I presume? :P How about the 'next hop'? As for you saying you got it with non-CGI:IRC users. Are you sure about that? Got some pastable proof perhaps? You had this before with 3.2.8.1 (non-cvs) too? (no need to downgrade/test, just wondering if you know...) ohnobinki: we already highly filter DNS results from our resolver in verify_hostname() in res.c. Perhaps we could use it for things passed to us by CGI:IRC as well? :) [note: we must still allow ipv4&ipv6 ips {in other words: ips, not resolved hostnames} as well] Did not review or test your patch.. yet.. |
|
|
Trying to reproduce on 3 server network: A-B-C All IPv6 Command on A is: WEBIRC eleet "cgiirc" 1.2.3.4 1.2.3.4 did not cause any issues on A->B or B->C (forwarded) traffic: & hai 2 !1Bz0YC ~x 1.2.3.4 x 0 +i * AQIDBA== :x. Linux (Debian Lenny 5.0), 32 bits. |
|
|
On an unrelated side note... isdigit(*tmpstr - 1) (which is also in the original code) that can never work right :P most likely isdigit(tmpstr[-1]) was intended, but a length check before that would be needed [tmpstr > sptr->sockhost]. So, to summarize: binki, go with the patch, sounds like a good check, but.. - add a fix for this silly isdigit *tmpstr -1 thing as well - add a verify_host() check prior to unreal_create_hostent() on line 191 of m_pass.c [verify_host might need to be made extern.. didn't check] - I don't know if Inet_ia2p() can return NULL or something invalid? If so, perhaps we could check on that... and reject the user. as for the sendto_realops(), yeah ehm... useful for tabrisnet, but something that needs to be removed before commit :P. sorry, as you can see I couldn't dug into it much deeper at the moment. theory: tabrisnet, if you use CGI:IRC, with non-WEBIRC (old method), and it omits the host, causes host to be NULL, then ip can be sent as ::ffff:1.2.3.4 and then perhaps this might happen. In WEBIRC this is not possible as host must be equal to ip to cause the 'unresolved ip', which is not possible as this is a line which cannot not be sent: {EDIT: you know what i mean, it can be sent, but it would be rejected due to too few params due to the : ;p} WEBIRC eleet "cgiirc" ::ffff:1.2.3.4 ::ffff:1.2.3.4 but as said, with the old method.. strchr on _ etc blabla... it might be possible :) |
|
|
yes, I did say that the reproducibility is RANDOM. I actually had a WEBIRC user that connected with the 'funny IP syndrome' that then later (minutes) reconnected w/o it. as to the next hops being IPv6, i played with it either way, did not make any difference. the server in question has listen addrs in both IPv4 and IPv6. Does not matter if the CGIIRC connects to the IPv6 or IPv4 listen port. This is a user connecting with WEBIRC (3.2.8.1 with DEBUGMODE enabled). Parsing: WEBIRC secret java 72.186.67.59 72.186.67.59 (from *) FindCommand WEBIRC Parsing: NICK :Guest34031 (from *) FindCommand NICK <snip> Parsing: PONG :D9739324 (from Guest34031) FindCommand PONG NOSPOOFch_cl: check access for Guest34031[0.0.0.0] ch_cl: access ok: Guest34031[::ffff:72.186.67.59]Sending [:hanashi.surrealchat.net 001 Guest34031 :Welcome to the SurrealChat.net IRC Network Guest34031!htIRC-0h2x@::ffff:72.186.67.59] to Guest34031 Sending [:hanashi.surrealchat.net 002 Guest34031 :Your host is hanashi.surrealchat.net, running version Unreal3.2.8.1+(debug)] to Guest34031 <snip> Sending [& Guest34031 1 !1BykmU htIRC-0h2x ::ffff:72.186.67.59 9 0 +ix * SLpDOw== :WWW User ] to yuzuki.surrealchat.net Sending [& Guest34031 1 !1BykmU htIRC-0h2x ::ffff:72.186.67.59 9 0 +ix * 18400517:CCA1E371:FF712BE3:IP SLpDOw== :WWW User ] to bugserv.SC.net Sending [& Guest34031 1 !1BykmU htIRC-0h2x ::ffff:72.186.67.59 9 0 +ix * SLpDOw== :WWW User ] to bardiel.surrealchat.net Sending [& Guest34031 1 !1BykmU htIRC-0h2x ::ffff:72.186.67.59 9 0 +ix * SLpDOw== :WWW User ] to alderaan.surrealchat.net Sending [& Guest34031 1 !1BykmU htIRC-0h2x ::ffff:72.186.67.59 9 0 +ix * SLpDOw== :WWW User ] to leviathan.surrealchat.net I have other logs from a 'monitoring server'. they doesn't show all of the inputs, but do show how often it happens and how sometimes a user can come back later w/o the funny IP. I can present some of those logs later. |
|
|
Proof it's 'random'. scnet@athena:~/srsv/logs$ grep usmb_h96 netdump.log-2010-05-18|grep " & " 20:44:00 & usmb_h96 1 !1Byl9B htIRC-0min ::ffff:41.103.69.234 9 0 +ix * A9694D2B:80B733CB:872E6D4A:IP KWdF6g== :WWW User 20:46:10 & usmb_h96 1 !1ByloG htIRC-0min 41.103.69.234 9 0 +ix * EE8A66E0.2DA25264.A4CF9A19.IP KWdF6g== :WWW User scnet@athena:~/srsv/logs$ Beginning to wonder if it really does happen when not htIRC user. I just took it as given when I saw the same error message during the spamflood. maybe not. netdump.log-2010-05-19:00:31:01 & gfq6126 1 !1Byp54 gfq6126 c-68-55-19-150.hsd1.md.comcast.net 9 0 +ix * SCnet-5D036553.hsd1.md.comcast.net RDcTlg== :gfq6126 netdump.log-2010-05-19:00:31:01 :ConnectServ ! #Diagnostics :SIGNED ON user: gfq6126 ([email protected] - gfq6126) at hanashi.surrealchat.net netdump.log-2010-05-19:00:31:32 @9 ~ !18WLLY #pokemonlake :gfq6126 netdump.log-2010-05-19:00:31:38 :gfq6126 & xpute4138 1274229098 netdump.log-2010-05-19:00:31:38 :ConnectServ ! #Diagnostics :NICK CHANGE user: gfq6126 ([email protected]) changed their nick to xpute4138 netdump.log-2010-05-19:00:31:43 :ConnectServ ! #Diagnostics :SIGNED OFF user: xpute4138 ([email protected] - gfq6126) at hanashi.surrealchat.net - Client exited netdump.log-2010-05-19:00:31:48 :ServServ ! #Diagnostics :INFO: (ServServ) gfq6126 is connecting from United States (Manchester, Maryland) [Baltimore, MD] netdump.log-2010-05-19:00:31:48 :ConnectServ ! #Diagnostics :GLOBAL KILL user: gfq6126 (@) killed by NickServ - Session Limit Exceeded netdump.log-2010-05-19:00:35:48 :services.SC.net ! #Diagnostics :gfq6126 has a NULL user->{__ID} in user_join_multi(#pokemonlake, ... netdump.log-2010-05-19:00:44:51 :ServServ ! #Diagnostics :INFO: (NickServ) gfq6126!@ - registered gfq6126 (email: [email protected]) requires email validation code |
|
|
Thanks, output is helpful, though yeah.. the randomness is.. interesting. Try replacing part of the code in docgiirc() (in m_pass.c) with this: #ifdef INET6 char ipbuf[64], crap[32]; #endif int ret; <-- add this and then.. replace the step 1 part with this: /* STEP 1: Update cptr->ip */ #ifdef INET6 /* Transform ipv4 to ::ffff:ipv4 if needed */ if (inet_pton(AF_INET, ip, crap) != 0) { snprintf(ipbuf, sizeof(ipbuf), "::ffff:%s", ip); ret = inet_pton(AFINET, ipbuf, &cptr->ip); } else /* else.. do like we normally do.. */ #endif ret = inet_pton(AFINET, ip, &cptr->ip); if (ret <= 0) return exit_client(cptr, cptr, &me, "Invalid IP address"); /* STEP 2: Update GetIP() */ that might be the bug. or in any case it is *A* bug. we put the ::ffff: prefix in for inet_pton, but it must NOT be used for all the rest. |
|
|
in addition to the fix above, the way to reproduce this (again, this particular bug, don't know if it's your bug) is sending USER prior to WEBIRC, instead of first WEBIRC and then NICK & USER. Unfortunately you snipped that part of your log out, so I couldn't see if USER was sent prior to WEBIRC or after. |
|
|
WEBIRC is sent by the perl-code, NICK is sent by the perl-code, and then USER is sent by the javascript code. So it would be _really_ weird if those got reordered (WEBIRC and NICK are part of the same string, like so: "WEBIRC ... blah blah blah\nNICK blah") I'll see when I can try applying your patch. |
|
|
Patch applied yesterday, the bug hasn't asserted itself yet. |
|
|
Uploaded the patch I have (which is presumably a combination of syzop & binki's patches) |
|
|
unreal-3.2.9-cvs-ipv6-cgiirc.patch gets rid of char crap[32] and, IMO, makes the code a bit more readable (even though the patch itself is ugly ;-) ). AFAIK, this does exactly the same thing as tabrisnet's patch and tabrisnet claims that his patch fixes his issue. I'll commit this soon if this is good. |
|
|
looks good, it includes all things I mentioned.. except one, maybe you could do that as well? the verify_host() thing. I know, not related to this bugreport, but if you're at it.. why not :P Oh, and the tmpstr -1 thing. You can always split it in two commits / patches, if you want. |
|
|
Oops! I skipped over your entire comment on tmpstr-1, I'll try to address that too. |
|
|
Hopefully the following two commits fixed the problem: (commitid: zJkXriiFw1tRNAAu, OCqfoAJHNezAx5zu) - Partially fixed bug where IPv4 addresses were randomly mishandled by the cgiirc code, resulting in the sockhost/hostmask being set to something like ::ffff:127.0.0.1, which confused the s2s protocol. Reported by tabrisnet (0003907). Also, reject incorrectly formed hostnames from WEBIRC command. - More strict sockhost (hostmask) checking in m_nick.c:_register_user(). Fixed some bad string handling as well. See comments in bug (0003907). |
|
|
looks fine |
|
|
Have run the CVS for 24 hours, the bug has not shown up that I can see. |
|
|
Thanks, tabrisnet :-) |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2010-05-18 22:59 | tabrisnet | New Issue | |
| 2010-05-19 02:04 | ohnobinki | File Added: unreal-3.2.9-CVS-ipv6-cgiirc-sockhost-colon.patch | |
| 2010-05-19 02:08 | ohnobinki | Note Added: 0016088 | |
| 2010-05-19 03:21 | tabrisnet | Note Added: 0016090 | |
| 2010-05-19 03:35 | ohnobinki | Relationship added | child of 0003776 |
| 2010-05-19 04:04 | ohnobinki | File Deleted: unreal-3.2.9-CVS-ipv6-cgiirc-sockhost-colon.patch | |
| 2010-05-19 04:04 | ohnobinki | File Added: unreal-3.2.9-CVS-ipv6-cgiirc-sockhost-colon.patch | |
| 2010-05-19 17:39 | syzop | Note Added: 0016092 | |
| 2010-05-19 17:52 | syzop | Note Added: 0016093 | |
| 2010-05-19 18:12 | syzop | Note Added: 0016094 | |
| 2010-05-19 18:12 | syzop | Note Edited: 0016094 | |
| 2010-05-19 18:22 | tabrisnet | Note Added: 0016095 | |
| 2010-05-19 19:19 | tabrisnet | Note Added: 0016096 | |
| 2010-05-19 20:55 | syzop | Note Added: 0016097 | |
| 2010-05-19 21:13 | syzop | Note Added: 0016098 | |
| 2010-05-19 21:30 | tabrisnet | Note Added: 0016099 | |
| 2010-05-22 19:48 | tabrisnet | Note Added: 0016100 | |
| 2010-05-24 20:11 | tabrisnet | File Added: ipv64.diff | |
| 2010-05-24 20:12 | tabrisnet | Note Added: 0016106 | |
| 2010-05-27 07:09 | ohnobinki | File Added: unreal-3.2.9-cvs-ipv6-cgiirc.patch | |
| 2010-05-27 07:12 | ohnobinki | Note Added: 0016107 | |
| 2010-05-27 09:40 | syzop | Note Added: 0016108 | |
| 2010-05-27 17:42 | ohnobinki | Note Added: 0016109 | |
| 2010-05-28 23:34 | ohnobinki | Note Added: 0016110 | |
| 2010-05-28 23:35 | ohnobinki | Assigned To | => ohnobinki |
| 2010-05-28 23:35 | ohnobinki | Status | new => feedback |
| 2010-05-29 12:22 | syzop | Note Added: 0016111 | |
| 2010-06-10 05:19 | tabrisnet | Note Added: 0016114 | |
| 2010-06-10 05:20 | ohnobinki | QA | => Not touched yet by developer |
| 2010-06-10 05:20 | ohnobinki | U4: Need for upstream patch | => No need for upstream InspIRCd patch |
| 2010-06-10 05:20 | ohnobinki | Note Added: 0016115 | |
| 2010-06-10 05:20 | ohnobinki | Status | feedback => resolved |
| 2010-06-10 05:20 | ohnobinki | Fixed in Version | => 3.2.9-RC1 |
| 2010-06-10 05:20 | ohnobinki | Resolution | open => fixed |
