Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid calling IRC_KillClient with Client == NULL #276

Closed
wants to merge 1 commit into from

Conversation

hillu
Copy link
Contributor

@hillu hillu commented May 10, 2020

I'm not familiar enough with the ngircd codebase, so I am not entirely
sure if Client is the right variable to pass to KillClient or if
it should be c instead.

This crash (segfault) was found using American Fuzzy Lop.

Build flags:

./configure --without-syslog --with-iconv CC=afl-clang

Run mode:

ngircd -n -f src/testsuite/ngircd-test1.conf

Manually inimized input:

PASS pwd1 0210-IRC+ Cd|t0:CHLMSX P
SERVER ngircd.test.server3 :on
:ngi 376 ngircd.test.server
:ngircd.test.server3 NICK NickNa.e 1 ~ locst 1 + :e
:ngircd.test.server3  JOIN #Channel :e.eEN
:ngircd.test.server3 NICK NickName 1 ~ locst 1 + :e
:ngircd.test.server3 NJOIN #Channel :@NickNamE,,,ngircd.test.server3!d

Stacktrace:

#0  Client_ID (Client=0x0) at client.c:707
#1  0x000000000042570c in IRC_KillClient (Client=0x0, From=0x0,
    Nick=0x7fffffffd00c "ngircd.test.server3!d", Reason=<optimized out>) at irc.c:379
#2  0x000000000044050a in IRC_NJOIN (Client=0x47d6b0, Req=0x7fffffffd260) at irc-server.c:291
#3  0x0000000000447eb7 in Handle_Request (Idx=<optimized out>, Req=0x7fffffffd260) at parse.c:544
#4  Parse_Request (Idx=7, Request=<optimized out>) at parse.c:267
#5  0x000000000041e788 in Handle_Buffer (Idx=7) at conn.c:1817
#6  0x00000000004206f9 in Read_Request (Idx=7) at conn.c:1650
#7  cb_clientserver (sock=7, what=<optimized out>) at conn.c:297
#8  0x0000000000424bd0 in io_docallback (fd=7, what=<optimized out>) at io.c:924
#9  io_dispatch_epoll (tv=<optimized out>) at io.c:497
#10 io_dispatch (tv=<optimized out>) at io.c:896
#11 0x000000000041defd in Conn_Handler () at conn.c:766
#12 0x0000000000405489 in main (argc=<optimized out>, argv=<optimized out>) at ngircd.c:317

I'm not familiar enough with the ngircd codebase, so I am not entirely
sure if `Client` is the right variable to pass to `KillClient` or if
it should be `c` instead.

This crash (segfault) was found using American Fuzzy Lop.

Build flags:

    ./configure --without-syslog --with-iconv CC=afl-clang

Run mode:

    ngircd -n -f src/testsuite/ngircd-test1.conf

Manually inimized input:

    PASS pwd1 0210-IRC+ Cd|t0:CHLMSX P
    SERVER ngircd.test.server3 :on
    :ngi 376 ngircd.test.server
    :ngircd.test.server3 NICK NickNa.e 1 ~ locst 1 + :e
    :ngircd.test.server3  JOIN #Channel :e.eEN
    :ngircd.test.server3 NICK NickName 1 ~ locst 1 + :e
    :ngircd.test.server3 NJOIN #Channel :@Nickname,,,ngircd.test.server3!d

Stacktrace:

    #0  Client_ID (Client=0x0) at client.c:707
    #1  0x000000000042570c in IRC_KillClient (Client=0x0, From=0x0,
        Nick=0x7fffffffd00c "ngircd.test.server3!d", Reason=<optimized out>) at irc.c:379
    ngircd#2  0x000000000044050a in IRC_NJOIN (Client=0x47d6b0, Req=0x7fffffffd260) at irc-server.c:291
    ngircd#3  0x0000000000447eb7 in Handle_Request (Idx=<optimized out>, Req=0x7fffffffd260) at parse.c:544
    ngircd#4  Parse_Request (Idx=7, Request=<optimized out>) at parse.c:267
    ngircd#5  0x000000000041e788 in Handle_Buffer (Idx=7) at conn.c:1817
    ngircd#6  0x00000000004206f9 in Read_Request (Idx=7) at conn.c:1650
    ngircd#7  cb_clientserver (sock=7, what=<optimized out>) at conn.c:297
    ngircd#8  0x0000000000424bd0 in io_docallback (fd=7, what=<optimized out>) at io.c:924
    ngircd#9  io_dispatch_epoll (tv=<optimized out>) at io.c:497
    ngircd#10 io_dispatch (tv=<optimized out>) at io.c:896
    ngircd#11 0x000000000041defd in Conn_Handler () at conn.c:766
    ngircd#12 0x0000000000405489 in main (argc=<optimized out>, argv=<optimized out>) at ngircd.c:317
@alexbarton alexbarton added the bug Issue affects current expected functionality label May 10, 2020
@alexbarton
Copy link
Member

I think this fix is wrong – as probably is the whole logic here …

Calling IRC_KillClient() with Client=NULL is perfectly fine and makes sense in IRC_NJOIN(), I think: when we kill the strange client, the server introducing it (=which is sending the NJOIN to us) must be informed about this as well (which Client=NULL basically means: don’t skip anyone).

So the bug is IRC_KillClient() calling Client_ID() with Client=NULL … and the only Client_ID() in IRC_KillClient() is in src/ngircd/irc.c, line 379:

return IRC_WriteErrClient(Client, msg, Client_ID(Client));

And here Client is NULL – because we pass Client=NULL to IRC_KillClient() … but the logic is totally bogus, as IRC_WriteErrClient() would be called with Client=NULL, too, which makes no sense at all!

I'll have to look into that, but I think the bug is in IRC_KillClient() – and looking at that function, I guess most(!) of the callers confuse the signature of this function (which is … „not ideal“ to begin with): they pass Client=Client From=NULL – but Client is completely ignored when From==NULL

So probably I’ll try to change the whole IRC_KillClient function to a more obvious signature? And update all callers? Hm …

@alexbarton alexbarton added this to the ngIRCd-26 milestone May 25, 2020
@alexbarton alexbarton self-assigned this May 25, 2020
@alexbarton
Copy link
Member

To be honest, this is a big mess …

IRC_NJOIN() isn’t designed to properly handle invalid input (so you can try to join servers to channels, like in your example above) and IRC_KillClient() is broken, too, as it doesn’t handle all possible cases.

So what needs to be done?

  1. For ngIRCd 26 … nothing, I guess: as this seems to only affect the server-server protocol (which is „trusted by design“, we don’t have to handle invalid input here, this is bad practice, but as already pointed out, „by design“ – so removing this bug from the milestone).

  2. IRC_KillClient() can’t handle the Client==NULL case when the Nick is something else than a more or less regular client (CLIENT_USER, CLIENT_GOTNICK or CLIENT_SERVICE). It will crash.

  3. ÌRC_KillClient() has a very confusing syntax, it should be rewritten, and all callers validated & fixed – probably even split into multiple functions handling different cases?

  4. And probably IRC_NJOIN() should/could do some more input validation, even being server-server links only. But then this would make sense in lots and lots of other places, too …

@alexbarton alexbarton removed this from the ngIRCd-26 milestone May 31, 2020
Copy link
Member

@alexbarton alexbarton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this doesn’t fix this bug properly: Most probably it will prevent the crash by sending a bogus ERR_CANTKILLSERVER_MSG (see IRC_KillClient() ) to the peer, but the IRC network still get’s out of sync …

@alexbarton
Copy link
Member

BTW: even the „client not found, let’s just skip it“ logic is bogus: if we do this, the global IRC network state is out of sync, as this client is joined to that channel on some servers, but not on all!

The only possible „solution“ is to enforce a net split, that is, disconnect the server from which we got commands that we weren’t able to handle …

@alexbarton alexbarton added s2s-bug This bug affects the server-server protocol only and removed bug Issue affects current expected functionality labels May 31, 2020
@dkoao
Copy link

dkoao commented Feb 3, 2021

Will this be fixed anytime soon? I'm kind of worried about using ngIRCd when there's a known security bug in it.

@alexbarton
Copy link
Member

Hi @DerDakon!

As far as I know no one is working on this, at least I’m not.

And I don’t see this as a security bug that much, as it only affects authenticated server-server connections: when you have a malicious but authenticated server in your network, all is lost anyway … the IRC server-server protocol is trusted by design, and this is especially true for ngIRCd.

Regards
Alex

@DerDakon
Copy link
Contributor

DerDakon commented Feb 8, 2021

You probably meant @dkoao instead of me, no?

@alexbarton
Copy link
Member

Whoops, sorry @DerDakon for bothering you, I'll blame the autocompletion … yes, I meant @dkoao.

@dkoao
Copy link

dkoao commented Feb 8, 2021

Ah nice, thank you for elaboration.

@alexbarton alexbarton removed their assignment Dec 17, 2022
@alexbarton
Copy link
Member

Closing this pull request as "won't fix": this patch doesn't fix the issue, and the issue only affects the server-server protocol and can't happen during normal operation (without "faking" a buggy server). And the server-server protocol is "secure by definition".

@alexbarton alexbarton closed this Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s2s-bug This bug affects the server-server protocol only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants