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 ssl read #1501

Closed
wants to merge 4 commits into from
Closed

Conversation

michaelortmann
Copy link
Member

@michaelortmann michaelortmann commented Nov 21, 2023

Found by: pym67 and PeGaSuS
Patch by: michaelortmann with help from the whole eggheads crew
Fixes: #1496

One-line summary:
When there are more than 511 bytes available to SSL_read() from an ssl sock, eggdrop would not SSL_read() the next bytes, until more data arrives for this sock.

Additional description (if needed):
For ssl connections, eggdrop grabs max 511 bytes via SSL_read(). If there where more bytes, the next call to select() in sockread() does not fire for such sock. After select() there is a call to SSL_pending(), but thats not reached, because select doesnt fire for the sock, but only for the "secondly" select() timeout (heartbeat) und thus returns 0. Only, when new data arrives, like in our example, when an IRC server sends a "Registration Timeout" message (because eggy didnt read and respond to the PING), only then eggy reads again, and again only up to those 511 bytes. Now it will read the PING the server was waiting for us to respond to, but its too late and the server already disconnects us.

In out testcase the server (UnrealIRCd-6.1.1.1) sends more than 511 bytes because of a long "CAP 302 multiline reply".

This patch checks for SSL_pending() before the select() and then treats the sock as if select() would have fired for it.

The bug was probably effecting many ssl conections, but its hard to notice, if the other end of the ssl connection constantly sends traffic triggering new SSL_read()s. In such case i would expect only a delay here and there.

This patch is likely to fix #1496, because the commit f9d8af4 referenced there introduced a change to not handle SSL_pending() for when select() doesnt fire for the sock:
f9d8af4#diff-e83dab64ed96e8a50f94260f6a3dea5712ef4150a7c2aa3c1c282d4227c93b66R923-R924

So, the bug was introduced with eggdrop 1.9.3rc1

Now, that the bug got fixed, if we look at the size of the buffer that we read into in sockread(): Some time ago the buffer size was upped because of tags and is currently above 8k. SSL_read() will return max 16K, see man page. So, this PR now also upps the "grab" value from 511 to RECVLINEMAX-1. This is more efficient for SSL_read(), read() and eggdrops IO buffer handling.

Please, can anyone check, that #1496 is indeed fixed by this PR?

Test cases demonstrating functionality (if applicable):
For the following tests, finegrained timestamps, see #1087 and #1441 and additional debug was added.
Before:
https://pastebin.com/A7HFC9jV
After:
https://pastebin.com/0aCMzgY4

Copy link
Member

@skralg skralg left a comment

Choose a reason for hiding this comment

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

I have not tested this, but it look plausible.

…already way above 512 due to tag functionality, so, lets grab as much as fits into that buffer instead of limiting to 511 here
@michaelortmann
Copy link
Member Author

this PR is obsolete by #1506

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.

botnet userfile transfer issue since v1.9.3, due to commit f9d8af4
2 participants