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

Fix more ident lookup bugs #1406

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

michaelortmann
Copy link
Member

@michaelortmann michaelortmann commented Feb 9, 2023

Found by: michaelortmann
Patch by: michaelortmann
Fixes:

One-line summary:
Fix more ident lookup bugs

Additional description (if needed):
Please misc/runautotools when testing / merging this PR.

If a socket is set O_NONBLOCK and connect() returns EINPROGRESS, it must be checked for ECONNREFUSED.
Here, this was done properly:

eggdrop/src/net.c

Lines 560 to 580 in 2a6a368

rc = connect(sock, &addr->addr.sa, addr->addrlen);
/* To minimize a proven race condition, call ident here (especially when
* rc < 0 and errno == EINPROGRESS)
*/
if (dcc[i].status & STAT_SERV) {
check_tcl_event("ident");
}
if (rc < 0) {
if (errno == EINPROGRESS) {
/* Async connection... don't return socket descriptor
* until after we confirm if it was successful or not */
tv.tv_sec = 1;
tv.tv_usec = 0;
FD_ZERO(&sockset);
FD_SET(sock, &sockset);
select(sock + 1, &sockset, NULL, NULL, &tv);
res_len = sizeof(res);
getsockopt(sock, SOL_SOCKET, SO_ERROR, &res, &res_len);
if (res == EINPROGRESS) /* Operation now in progress */
return sock; /* This could probably fail somewhere */
if (res == ECONNREFUSED) { /* Connection refused */

But here it was not and a write was committed:

eggdrop/src/dcc.c

Lines 1383 to 1404 in 2a6a368

if (connect(dcc[j].sock, &dcc[j].sockname.addr.sa,
dcc[j].sockname.addrlen) < 0 && (errno != EINPROGRESS)) {
killsock(dcc[j].sock);
lostdcc(j);
putlog(LOG_MISC, "*", DCC_IDENTFAIL, dcc[i].host, strerror(errno));
j = 0;
}
sock = dcc[j].sock;
}
}
if (j < 0) {
dcc_telnet_got_ident(i, userhost);
return;
}
dcc[j].sock = sock;
dcc[j].port = 113;
dcc[j].addr = dcc[i].addr;
strcpy(dcc[j].host, dcc[i].host);
strcpy(dcc[j].nick, "*");
dcc[j].u.ident_sock = dcc[i].sock;
dcc[j].timeval = now;
dprintf(j, "%d, %d\n", dcc[i].port, dcc[idx].port);

Additionally O_NONBLOCK is POSIX 2001 and old quirk code is removed, see: https://www.gaertner.de/~neitzel/susv3/basedefs/fcntl.h.html

Additionally also the proper code mentioned above had an issue, it did select() with an arbitrary timeout of 1 sec, when it should be nonblocking.

Additionally there was a bug here:

eggdrop/src/dcc.c

Lines 1388 to 1403 in 2a6a368

j = 0;
}
sock = dcc[j].sock;
}
}
if (j < 0) {
dcc_telnet_got_ident(i, userhost);
return;
}
dcc[j].sock = sock;
dcc[j].port = 113;
dcc[j].addr = dcc[i].addr;
strcpy(dcc[j].host, dcc[i].host);
strcpy(dcc[j].nick, "*");
dcc[j].u.ident_sock = dcc[i].sock;
dcc[j].timeval = now;

Were this line j = 0 ever executed, the gates of tartaros would have opened for dcc[0].

Test cases demonstrating functionality (if applicable):
Without this fix:
When telnetting to the bot and the user has no identd running, the bot would log like:

[21:01:18] Telnet connection: localhost.home.arpa/45052
[21:01:18] net: eof!(write) socket 13 (Broken pipe,32)

With this fix:
Test without identd:

[12:24:02] Telnet connection: localhost.home.arpa/52216
[12:24:02] net: attempted socket connection refused: 127.0.0.1:113
[12:24:02] Ident failed for localhost.home.arpa: Connection refused
[12:24:09] pbkdf2 method SHA256 rounds 16000, user 12.493ms sys 0.416ms
[12:24:09] Logged in: testuser ([email protected]/52216)
*** testuser joined the party line.

Test wih identd:

[12:24:25] Telnet connection: localhost.home.arpa/42944
[12:24:28] pbkdf2 method SHA256 rounds 16000, user 16.223ms sys 6.315ms
[12:24:28] Logged in: testuser ([email protected]/42944)
*** testuser joined the party line.

Additionally i applied this patch to windrop and it also works under cygwin.

@michaelortmann
Copy link
Member Author

Maybe related/fixing #809?

@vanosg vanosg added this to the v1.10 milestone Apr 4, 2023
@vanosg
Copy link
Member

vanosg commented Dec 30, 2023

Please resolve conflicts, thanks

@michaelortmann
Copy link
Member Author

Please resolve conflicts, thanks

done. ready for review again.

@michaelortmann michaelortmann changed the title Fix more ident lookup bugs (WIP) Fix more ident lookup bugs Jan 15, 2024
@michaelortmann michaelortmann changed the title (WIP) Fix more ident lookup bugs Fix more ident lookup bugs Jan 15, 2024
@vanosg vanosg modified the milestones: v1.10.0, v1.10.1 Feb 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants