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

Client_Search: Avoid crash if Nick == NULL. #275

Closed
wants to merge 1 commit into from

Conversation

hillu
Copy link
Contributor

@hillu hillu commented May 10, 2020

This crash 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

Minimized input:

PASS pwd1 0 0
SERVER ngircd.test.server3 0
SERVER 0 0 0 0

Stacktrace from gdb:

#0  __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:65
#1  0x000000000044ac19 in strlcpy (dst=0x7fffffffd120 "@", src=0x0, size=64) at strlcpy.c:59
#2  0x000000000040f8eb in Client_Search (Nick=0x0) at client.c:605
#3  0x0000000000440207 in IRC_SERVER (Client=0x47e6b0, Req=0x7fffffffd260) at irc-server.c:189
#4  0x0000000000448d77 in Handle_Request (Idx=<optimized out>, Req=0x7fffffffd260) at parse.c:544
#5  Parse_Request (Idx=7, Request=<optimized out>) at parse.c:267
#6  0x000000000041f668 in Handle_Buffer (Idx=7) at conn.c:1783
#7  0x00000000004215f9 in Read_Request (Idx=7) at conn.c:1616
#8  cb_clientserver (sock=7, what=<optimized out>) at conn.c:296
#9  0x0000000000425ad0 in io_docallback (fd=7, what=<optimized out>) at io.c:924
#10 io_dispatch_epoll (tv=<optimized out>) at io.c:497
#11 io_dispatch (tv=<optimized out>) at io.c:896
#12 0x000000000041eddd in Conn_Handler () at conn.c:748
#13 0x0000000000405499 in main (argc=<optimized out>, argv=<optimized out>) at ngircd.c:316

This crash was found using American Fuzzy Lop.

Minimized input:

    PASS pwd1 0 0
    SERVER ngircd.test.server3 0
    SERVER 0 0 0 0

Stacktrace from gdb:

    #0  __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:65
    #1  0x000000000044ac19 in strlcpy (dst=0x7fffffffd120 "@", src=0x0, size=64) at strlcpy.c:59
    ngircd#2  0x000000000040f8eb in Client_Search (Nick=0x0) at client.c:605
    ngircd#3  0x0000000000440207 in IRC_SERVER (Client=0x47e6b0, Req=0x7fffffffd260) at irc-server.c:189
    ngircd#4  0x0000000000448d77 in Handle_Request (Idx=<optimized out>, Req=0x7fffffffd260) at parse.c:544
    ngircd#5  Parse_Request (Idx=7, Request=<optimized out>) at parse.c:267
    ngircd#6  0x000000000041f668 in Handle_Buffer (Idx=7) at conn.c:1783
    ngircd#7  0x00000000004215f9 in Read_Request (Idx=7) at conn.c:1616
    ngircd#8  cb_clientserver (sock=7, what=<optimized out>) at conn.c:296
    ngircd#9  0x0000000000425ad0 in io_docallback (fd=7, what=<optimized out>) at io.c:924
    ngircd#10 io_dispatch_epoll (tv=<optimized out>) at io.c:497
    ngircd#11 io_dispatch (tv=<optimized out>) at io.c:896
    ngircd#12 0x000000000041eddd in Conn_Handler () at conn.c:748
    ngircd#13 0x0000000000405499 in main (argc=<optimized out>, argv=<optimized out>) at ngircd.c:316
@alexbarton alexbarton added the bug Issue affects current expected functionality label May 10, 2020
@alexbarton
Copy link
Member

I think we shouldn’t change the calling convention of Client_Search() here (it doesn’t accept Nick=NULL which perfectly makes sense, I guess), but we should fix the caller not to call Client_Server when there is no „nick“:

Caller is IRC_SERVER(), see irc-server.c, line 189: we have to make sure that the request has a prefix.

I have a patch almost ready for this, will do some more tests …

alexbarton added a commit that referenced this pull request May 25, 2020
The SERVER command is only valid with a prefix when received from other
servers, so make sure that there is one and disconnect the peer if not
(instead of crashing ...).

This obsoletes PR #275.

Thanks Hilko Bengen (hillu) for finding & reporting this as well for the
patch & pull request! But I think this is the "more correct" fix.
@alexbarton
Copy link
Member

Thanks @hillu for finding & reporting this as well for the patch & pull request!

I think my fix in commit 02cf31c is „more correct“ as it detects and handles the root cause: no prefix sent by the remote server.

Again, thanks a lot!

@alexbarton alexbarton closed this May 25, 2020
@alexbarton alexbarton added this to the ngIRCd-26 milestone May 25, 2020
@alexbarton alexbarton self-assigned this May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue affects current expected functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants