Skip to content

Fix WS_KeySignature leak on DoAsn1Key private-key error paths#1069

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_6513
Open

Fix WS_KeySignature leak on DoAsn1Key private-key error paths#1069
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_6513

Conversation

@yosuke-wolfssl

Copy link
Copy Markdown
Contributor

Description

DoAsn1Key (src/ssh.c) calls IdentifyAsn1Key(), which on success heap-allocates a WS_KeySignature and initializes an inner wolfCrypt key (wc_InitRsaKey/wc_ecc_init). That allocation is meant to be released at the end of the function via wolfSSH_KEY_clean(key) / WFREE(key, ...).

In the else if (ret > 0 && isPrivate) branch, two early return ret; statements bypassed that cleanup:

  • malloc-failure path (WS_MEMORY_E)
  • caller-buffer-too-small path (WS_BUFFER_E)

Both run after key has been allocated, so each leaks the WS_KeySignature plus the mp_int/ECC internals owned by the embedded wolfCrypt key. The *outSz < inSz path is reachable from the public wolfSSH_ReadKey_buffer / wolfSSH_ReadKey_buffer_ex APIs whenever a caller passes a fixed *out buffer smaller than the DER private key.

Addressed by f_6513.

Fix

Replaced the two early return ret; statements with fallthrough control flow so every error path reaches the existing wolfSSH_KEY_clean(key) / WFREE(key, ...) cleanup:

  • malloc failure and buffer-too-small now set ret instead of returning.
  • The success body is guarded with if (ret > 0), which also preserves the positive key-type id needed by IdToName(ret).
  • The existing tail if (*out == NULL) WFREE(newKey, ...) remains correct for both error paths.

Testing

Added a regression test in tests/api.c (test_wolfSSH_ReadKey) that drives the ASN.1/DER private-key error path with a caller-provided buffer one byte too small. Existing ASN.1 tests only used allocate-mode, leaving this path uncovered.

The test asserts:

  • ret == WS_BUFFER_E
  • caller buffer pointer is preserved (key == keyCheck)
  • output params are left untouched on the error path (keySz, keyType, keyTypeSz)

The WS_MEMORY_E early-return path is fixed the same way but is not directly tested (it requires allocation-failure injection).

Verification

  • ./configure --enable-all build + tests/api.test: pass.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Jun 29, 2026
Copilot AI review requested due to automatic review settings June 29, 2026 01:51

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #1069

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

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.

4 participants