Skip to content

Set keyAllocated=1 when initializing any key#1038

Open
padelsbach wants to merge 2 commits into
wolfSSL:masterfrom
padelsbach:keyallocated-flag
Open

Set keyAllocated=1 when initializing any key#1038
padelsbach wants to merge 2 commits into
wolfSSL:masterfrom
padelsbach:keyallocated-flag

Conversation

@padelsbach

@padelsbach padelsbach commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Fixes a possible memory leak if parsing fails. Only a problem with --enable-heapmath since normal spmath does not allocate buffers for the key.

Added unit tests and heapmath variant to CI.

Also, adding heapmath+asan uncovered a second unrelated leak when freeing the KEX key. Fix included as a second commit.

@padelsbach padelsbach marked this pull request as ready for review June 16, 2026 06:22
@padelsbach padelsbach requested a review from ejohnstown June 16, 2026 06:22

@ejohnstown ejohnstown left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ignore this comment.

Comment thread src/internal.c
wc_FreeDhKey(&hs->privKey.dh);
}
#endif
#ifndef WOLFSSH_NO_ECDH

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's a bug in SendKexDhInit() that should be fixed with this PR. When KEX is ID_CURVE25519_MLKEM768_SHA256, useEccMlKem and useCurve25519MlKem are set. This will cause a double free of hs->privKey, once as each of ECC and Curve25519.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This was fine here. The bug is in SendKexDhInit(). The case where ID_CURVE25519_MLKEM768_SHA256 is handled, both useCurve25519MlKem (correct) and useEccMlKem (incorrect) are getting set true.

Comment thread src/internal.c Outdated
WFREE(hs->primeGroup, heap, DYNTYPE_MPINT);
WFREE(hs->generator, heap, DYNTYPE_MPINT);
#endif
#ifndef WOLFSSH_NO_DH

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This #ifndef WOLFSSH_NO_DH is adjacent to another one.

@ejohnstown ejohnstown self-requested a review June 19, 2026 00:09
Comment thread compile_commands.json Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please remove this file from the PR.

Comment thread src/internal.c
wc_FreeDhKey(&hs->privKey.dh);
}
#endif
#ifndef WOLFSSH_NO_ECDH

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This was fine here. The bug is in SendKexDhInit(). The case where ID_CURVE25519_MLKEM768_SHA256 is handled, both useCurve25519MlKem (correct) and useEccMlKem (incorrect) are getting set true.

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