Skip to content

Fix ssl read#1506

Merged
vanosg merged 1 commit intoeggheads:developfrom
michaelortmann:SSL_read
Jan 2, 2024
Merged

Fix ssl read#1506
vanosg merged 1 commit intoeggheads:developfrom
michaelortmann:SSL_read

Conversation

@michaelortmann
Copy link
Copy Markdown
Member

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

One-line summary:
This is a more clean/efficient fix than #1501

Additional description (if needed):
Instead of fixing and using SSL_pending(), this PR fixes the problem by upping the read buffer size for SSL_read() and read() to 16K, the maximum according to SSL spec and to openssls internal buffer size.

Test cases demonstrating functionality (if applicable):
It fixes the IRC server connect mentioned in #1501
It also fixes a user file transfer over tls. I tested with a userfile < 16K - 1, one with == 16K and one > 16K

@michaelortmann michaelortmann mentioned this pull request Nov 28, 2023
@michaelortmann
Copy link
Copy Markdown
Member Author

Please everyone "battletest" this PR as soon and as much as possible, for its modifying the main-select-loop.

@vanosg vanosg added this to the v1.10.0 milestone Dec 1, 2023
@vanosg
Copy link
Copy Markdown
Member

vanosg commented Dec 2, 2023

I just tested against the server @PeGaSuS-Coder provided the other week- a TLS connection now works properly. Nice work and persistence to track this one down!

[00:31:08] [@] :server.irc CAP * LS * :unrealircd.org/link-security=2 unrealircd.org/plaintext-policy=user=warn,oper=deny,server=warn unrealircd.org/history-storage=memory letspiss.net/hiddenoper letspiss.net/id solanum.chat/oper away-notify invite-notify extended-join userhost-in-names multi-prefix cap-notify sasl=EXTERNAL,PLAIN setname tls chghost account-notify message-tags batch server-time account-tag echo-message labeled-response draft/chathistory extended-monitor standard-replies
[00:31:08] [@] :server.irc CAP * LS :unrealircd.org/json-log
[00:31:08] [!m] CAP REQ : userhost-in-names account-tag
[00:31:08] [m->] CAP REQ : userhost-in-names account-tag
[00:31:08] [@] PING :2249BEEC
[00:31:08] [m->] PONG :2249BEEC
[00:31:09] [@] :server.irc CAP BeerBot ACK :userhost-in-names account-tag 
[00:31:09] [!m] CAP END
[00:31:09] [m->] CAP END

Note the server no longer hangs after the first long CAP LS line

@thommey
Copy link
Copy Markdown
Member

thommey commented Dec 2, 2023

Are we sure this is right?

"TLS versions 1.2 [RFC5246] and earlier permit senders to generate records 16384 octets in size, plus any expansion from compression and protection up to 2048 octets (though typically this expansion is only 16 octets)."

@michaelortmann
Copy link
Copy Markdown
Member Author

michaelortmann commented Dec 2, 2023

Are we sure this is right?

"TLS versions 1.2 [RFC5246] and earlier permit senders to generate records 16384 octets in size, plus any expansion from compression and protection up to 2048 octets (though typically this expansion is only 16 octets)."

eggdrop doesnt receive raw tls data from server, it receives from open-/libre-ssl, which already unwarps (decrypt, decompresses, ...) the raw data before it hands the max 16kb payload to eggdrop. the max buffer size is hardcoded in openssl. other ssl libs do the same, here is such an example from wolfssl limiting the output of wolfSSL_read() to 16kb: wolfssl/internal.h MAX_RECORD_SIZE 16384

@vanosg vanosg merged commit 6bc6560 into eggheads:develop Jan 2, 2024
@michaelortmann michaelortmann deleted the SSL_read branch January 3, 2024 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

3 participants