Conversation
Reuse the stub already used for AES-GCM. Reported-by: afldl on github Ref: libssh2#2023 Follow-up to 492bc54 libssh2#1426
|
/cc @afldl |
There was a problem hiding this comment.
Pull request overview
Updates the MAC override logic so that when chacha20-poly1305@openssh.com is negotiated, libssh2 uses the same “integrated MAC” stub approach already used for AES-GCM, avoiding a separately negotiated MAC for an AEAD cipher.
Changes:
- Extend
ssh2_mac_override()to return the integrated MAC stub forchacha20-poly1305@openssh.com. - Reuse the existing integrated-MAC stub for both chacha20-poly1305 and AES-GCM.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static const struct mac_method mac_method_integrated = { | ||
| "INTEGRATED-MAC", /* made up name for display only */ | ||
| 16, | ||
| 16, | ||
| mac_method_none_init, | ||
| mac_method_hmac_none_hash, | ||
| mac_method_none_dtor, |
There was a problem hiding this comment.
Another pre-existing issue.
There was a problem hiding this comment.
Though it sounds really weird this may be necessary. Why isn't the dtor called on the existing mac on rekey, before rekeying?
There was a problem hiding this comment.
On a quick review, yes, this is possible, and to me puzzling.
kex_agree_mac() can be called via kex_agree_methods(), via ssh2_kex_exchange(),
from ssh2_transport_send()/ssh2_transport_read(), which then simply overwrites the callback
table with the new one. Then, kex_finish() dtors using the newly selected mac, and calls init
for it. It effectively means that an unmatched dtor is called on the mac_abstract structure.
a dtor may then free a buffer meant to be freed (free_key = 0, all non-AEAD
macs). This can't happen because the only other dtor is AEAD's where the dtor
is a no-op. Or memleak if the new dtor is a dummy one (for AEAD), and the
previous one was free_key == 0. That's reported by Copilot.
This means that the init/dtor code could possibly not be done via callbacks,
but by using the same pair of functions regardless of mac. Or kex_exchange()
save the new mac in a temp slot, kex_finish() dtor using the existing mac,
swap and call the new mac's init callback.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/mac.c:403
- mac_method_none_init() sets free_key=1 but leaves abstract untouched. During rekey, kex_agree_mac() can overwrite session->.mac with the integrated stub before the old MAC dtor runs, so the subsequent key-derivation step calls this init with a stale mac_abstract from the previous HMAC. If *abstract is not explicitly reset, the session can retain a dangling/leaked pointer for the remainder of the connection (and at shutdown, mac_method_none_dtor() currently won’t clear it).
static int mac_method_none_init(LIBSSH2_SESSION *session, unsigned char *key,
int *free_key, void **abstract)
{
*free_key = 1;
(void)session;
src/mac.c:413
- mac_method_none_dtor() is a no-op and does not clear/free *abstract. Because MAC negotiation (kex_agree_mac) can replace the active MAC method with this integrated stub before key-derivation calls the previous MAC's dtor, mac_abstract from the old HMAC can be leaked (and left dangling) across rekeys and at session shutdown. The integrated stub dtor should free and NULL out any existing abstract, mirroring mac_method_common_dtor().
static int mac_method_none_dtor(LIBSSH2_SESSION *session, void **abstract)
{
(void)session;
(void)abstract;
return 0;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/mac.c:423
- mac_method_none_dtor() is a no-op, but session->local.mac->dtor() is invoked during rekey and session cleanup using the current MAC method pointer. When switching from a real HMAC method (mac_method_common_init stores an allocated key in mac_abstract) to this integrated stub (now also used for chacha20-poly1305), the old mac_abstract allocation can be left unfreed. Make the stub dtor free any existing abstract and NULL it out to avoid leaks across algorithm changes/rekeys.
static int mac_method_none_dtor(LIBSSH2_SESSION *session, void **abstract)
{
(void)session;
(void)abstract;
return 0;
|
I started on this change a couple weeks ago and it broke some of our internal tests so I never finished it. I'll take a look at it again and make sure it's ok. |
Maybe unrelated, but the way the mac_method is pulled under the The rekey was not designed with macs with different inits/dtors |
|
I had noticed these issues and had also put in fixes for them but it was still failing our tests with a certain mix of ciphers which I'm forgetting off the top of my head. |
Reported by Copilot Bug: libssh2#2407 (comment)
Reuse the stub already used for AES-GCM.
Reported-by: afldl on github
Ref: #2023
Follow-up to 492bc54 #1426