Skip to content

ossh: check the key-type parse before the ID lookup - #1193

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

ossh: check the key-type parse before the ID lookup#1193
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_11650

Conversation

@yosuke-wolfssl

Copy link
Copy Markdown
Contributor

Problem

GetOpenSshPublicKey() overwrote the result of GetStringRef() before checking it:

ret = GetStringRef(&publicKeyTypeSz, &publicKeyType, buf, len, idx);
keyId = NameToId((const char*)publicKeyType, publicKeyTypeSz);

GetStringRef() sets *strSz inside GetUint32() before its bounds check, so a
truncated key-type string returns WS_BUFFER_E with *str never assigned. The
caller's publicKeyType is an uninitialized local, so NameToId() compared through
an indeterminate pointer using a length taken from the wire. The unchecked ret was
then overwritten by the switch, so the parse failure was reported as
WS_UNIMPLEMENTED_E instead of WS_BUFFER_E.

Compiled only under WOLFSSH_TPM. The single call site is
PrepareUserAuthRequestRsa(), on the branch where the private key lives in the TPM
and only a public key blob is available — the input is the local user's public key
material, not a remote peer's. Closes f-11650.

Fix (src/ossh.c)

  • NameToId() and the key-type switch run only when GetStringRef() returns
    WS_SUCCESS; that result is returned otherwise.
  • publicKeyType starts NULL and keyId starts ID_UNKNOWN.

The switch body is unchanged apart from indentation. Every other GetStringRef() /
GetMpint() call in the file already gated on ret == WS_SUCCESS; this was the only
one that did not.

Tests (tests/api.c)

test_GetOpenSshPublicKey_type(), gated on WOLFSSH_TPM + WOLFSSH_TEST_INTERNAL:

Blob Expected
"ssh" under a length of 7 WS_BUFFER_E
Length prefix truncated WS_BUFFER_E
Empty type name WS_UNIMPLEMENTED_E
ssh-dss (well-formed, unsupported) WS_UNIMPLEMENTED_E
ssh-rsa + mpint e + mpint n WS_SUCCESS, idx fully consumed

Runs upstream in the tpm-ssh.yml ecc/ibmswtpm2/raw cell, which builds the
check_PROGRAMs with -DWOLFSSH_TPM and runs make check TESTS=tests/api.test.

Verification

  • --enable-tpm --enable-certs: build clean, tests/api.test passes.
  • Negative control: with the src/ossh.c hunk reverted, the truncated-type case
    fails — -1017 != -1004.
  • gcc-13 -O2 -Werror sweep: clean across 6 configs (enable-all, zephyr-defines,
    sftp-only, scp-only, default, smallstack).

- GetOpenSshPublicKey() calls NameToId() and enters the key-type
  switch only when GetStringRef() returns WS_SUCCESS, and returns
  that result otherwise.
- publicKeyType starts NULL and keyId starts ID_UNKNOWN.
- tests/api.c adds test_GetOpenSshPublicKey_type(), covering a
  truncated type string, a truncated length prefix, an empty type,
  an unsupported type and a well-formed ssh-rsa blob.

Issue: F-11650
@yosuke-wolfssl yosuke-wolfssl self-assigned this Aug 24, 2026
Copilot AI lite review requested due to automatic review settings August 24, 2026 08:09

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.

Pull request overview

Fixes TPM-only OpenSSH public-key parsing by preserving malformed key-type errors and adding regression coverage.

Changes:

  • Gates key lookup on successful parsing.
  • Initializes parser state safely.
  • Adds malformed and valid key tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
tests/api.c Adds TPM-gated regression tests.
src/ossh.c Fixes key-type parsing and error handling.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@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 #1193

Scan targets checked: wolfssh-bugs, wolfssh-src

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

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