View Issue Details

IDProjectCategoryView StatusLast Update
0003907unrealircdpublic2010-06-10 05:20
Reportertabrisnet Assigned Toohnobinki  
PrioritynormalSeveritymajorReproducibilityrandom
Status resolvedResolutionfixed 
PlatformLinux/x86OSDebian LinuxOS Versiontesting
Product Version3.2.8 
Fixed in Version3.2.9-RC1 
Summary0003907: IPv6/4 addrs (::ffff:w.x.y.z) break NICK connection parsing
DescriptionWith 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 ReproduceNo 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 InformationThis has been extensively discussed and analyzed with binki
TagsNo tags attached.
Attached Files
ipv64.diff (1,637 bytes)
3rd party modules

Relationships

child of 0003776 resolvedsyzop Unreal3.2.9 TODO 

Activities

ohnobinki

2010-05-19 02:08

reporter   ~0016088

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.

tabrisnet

2010-05-19 03:21

reporter   ~0016090

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?

syzop

2010-05-19 17:39

administrator   ~0016092

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..

syzop

2010-05-19 17:52

administrator   ~0016093

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.

syzop

2010-05-19 18:12

administrator   ~0016094

Last edited: 2010-05-19 18:12

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 :)

tabrisnet

2010-05-19 18:22

reporter   ~0016095

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.

tabrisnet

2010-05-19 19:19

reporter   ~0016096

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

syzop

2010-05-19 20:55

administrator   ~0016097

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.

syzop

2010-05-19 21:13

administrator   ~0016098

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.

tabrisnet

2010-05-19 21:30

reporter   ~0016099

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.

tabrisnet

2010-05-22 19:48

reporter   ~0016100

Patch applied yesterday, the bug hasn't asserted itself yet.

tabrisnet

2010-05-24 20:12

reporter   ~0016106

Uploaded the patch I have (which is presumably a combination of syzop & binki's patches)

ohnobinki

2010-05-27 07:12

reporter   ~0016107

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.

syzop

2010-05-27 09:40

administrator   ~0016108

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.

ohnobinki

2010-05-27 17:42

reporter   ~0016109

Oops! I skipped over your entire comment on tmpstr-1, I'll try to address that too.

ohnobinki

2010-05-28 23:34

reporter   ~0016110

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).

syzop

2010-05-29 12:22

administrator   ~0016111

looks fine

tabrisnet

2010-06-10 05:19

reporter   ~0016114

Have run the CVS for 24 hours, the bug has not shown up that I can see.

ohnobinki

2010-06-10 05:20

reporter   ~0016115

Thanks, tabrisnet :-)

Issue History

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