Skip to content

Deduplication and type checking - #1159

Open
stenslae wants to merge 1 commit into
wolfSSL:masterfrom
stenslae:followup-review-findings
Open

Deduplication and type checking#1159
stenslae wants to merge 1 commit into
wolfSSL:masterfrom
stenslae:followup-review-findings

Conversation

@stenslae

Copy link
Copy Markdown
Member
  • Added authorized_key type checking extract the embedded key type directly from the SSH wire-format blob rather than using wolfSSH_QueryKey().
  • Added a makeKey function pointer to CompositeTradOps.
  • Deduplicated ECDSA Encoding
  • Introduced WOLFSSH_MLDSA_COMPOSITE_ID_CASES for redundant switch cases.
  • Centralized maximum public key sizing behind a new WOLFSSH_MLDSA_MAX_PUB_KEY_SZ macro
  • Switched to dynamically allocating MlDsaKey and large public key buffers in wolfSSH_MakeMlDsaCompositeKey() and GetOpenSshKeyMlDsaComposite() when WOLFSSH_SMALL_STACK is defined.
  • Added test coverage

@stenslaestenslae self-assigned this Aug 11, 2026

@wolfSSL-Fenrir-botwolfSSL-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 #1159

Scan targets checked:wolfssh-bugs, wolfssh-src

Findings: 5
5 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

Comment threadsrc/internal.c
Comment threadsrc/ossh.c
Comment threadapps/wolfsshd/test/test_configuration.c Outdated
Comment threadapps/wolfsshd/auth.c Outdated
Comment threadapps/wolfsshd/test/test_configuration.c
Comment threadapps/wolfsshd/test/test_configuration.c
Comment threadapps/wolfsshd/auth.c Outdated
Comment threadapps/wolfsshd/test/test_configuration.c Outdated
Comment threadsrc/ossh.c
Comment threadsrc/internal.c

@wolfSSL-Fenrir-botwolfSSL-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 #1159

Scan targets checked:wolfssh-bugs, wolfssh-src

Findings: 7
7 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

Comment threadapps/wolfsshd/auth.c Outdated
Comment threadtests/unit.c
Comment threadsrc/keygen.c
Comment threadapps/wolfsshd/test/test_configuration.c
Comment threadapps/wolfsshd/test/test_configuration.c Outdated
Comment threadapps/wolfsshd/test/test_configuration.c
@stenslae
stenslaeforce-pushed the followup-review-findings branch from 8000746 to 818f77bCompareAugust 12, 2026 21:02
Comment threadapps/wolfsshd/auth.c Outdated
Comment threadtests/unit.c
Comment threadapps/wolfsshd/test/test_configuration.c
Comment threadsrc/keygen.c
Comment threadapps/wolfsshd/test/test_configuration.c Outdated
Comment threadapps/wolfsshd/test/test_configuration.c

@wolfSSL-Fenrir-botwolfSSL-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 #1159

Scan targets checked:wolfssh-bugs, wolfssh-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

Comment threadapps/wolfsshd/test/test_configuration.c
Comment threadapps/wolfsshd/test/test_configuration.c

@wolfSSL-Fenrir-botwolfSSL-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 #1159

Scan targets checked:wolfssh-bugs, wolfssh-src

Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

/* Declared type length (12) matches the token and the 8 bytes that
* follow are the token's own prefix, so the XMEMCMP can't reject on
* content: only the typeStrSz > keyCandSz - 4 bound can. */
static const char truncType[] = "rsa-sha2-256";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 [Low] Truncated-blob test never reaches the bound it claims to cover · Weak or missing assertions

The line token rsa-sha2-256 is mapped by AuthKeysTokenKeyType() to ssh-rsa, so keyTypeSz is 7 while the blob declares 12; typeStrSz != keyTypeSz rejects first and the typeStrSz > keyCandSz - 4 bound in CheckAuthKeysLine() stays untested. Deleting that bound leaves the suite green.

Fix: Use a non-aliased token whose length equals the declared type length, e.g. token "ssh-rsa" with blob { 0,0,0,7, 's','s','h' }.

Comment threadsrc/internal.c
/* ML-DSA listed first (post-quantum priority), then ECDSA, ED25519, RSA. */
static const char cannedKeyAlgoNames[] =
#ifndef WOLFSSH_NO_MLDSA_COMPOSITES
#if !defined(WOLFSSH_NO_MLDSA87) && defined(HAVE_ED448)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 [Low] WOLFSSH_NO_MLDSA_COMPOSITES not applied to cannedKeyAlgoClient · Logic errors

The new WOLFSSH_NO_MLDSA_COMPOSITES option guards cannedKeyAlgoNames and cannedKeyAlgoNamesHostKey but not cannedKeyAlgoClient, so composite IDs still match in DoUserAuthRequestPublicKey() and DoExtInfoServerSigAlgs() while WS_GetCompositeParams() now always returns WS_NOT_COMPILED.

Fix: Wrap the six composite ID entries in cannedKeyAlgoClient with #ifndef WOLFSSH_NO_MLDSA_COMPOSITES.

* content: only the typeStrSz > keyCandSz - 4 bound can. */
static const char truncType[] = "rsa-sha2-256";
static const byte truncatedBlob[] = {
0x00, 0x00, 0x00, 0x0c, 'r', 's', 'a', '-', 's', 'h', 'a', '2'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 [Low] Truncated-blob test never reaches the new out-of-bounds guard in CheckAuthKeysLine · Missing edge-case coverage on a function the PR also changed

The vector declares a 12-byte type length while the normalized line type ssh-rsa is 7, so typeStrSz != keyTypeSz short-circuits first and the typeStrSz > keyCandSz - 4 guard at auth.c:332 is never exercised. That guard is the only thing preventing XMEMCMP from reading past the decoded keyCand heap allocation.

Fix: Use a vector where the declared type length equals the line type length but exceeds keyCandSz - 4, e.g. line type ssh-ed25519 with blob {0,0,0,11,'x','x','x','x'}.

Comment threadapps/wolfsshd/auth.c
ret = WS_FATAL_ERROR;
/* Skip, don't abort: an option-prefixed line puts a
* non-base64 token here (e.g. "no-pty ssh-rsa ..."). */
ret = WSSHD_AUTH_FAILURE;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 [Low] Fail-soft malformed-line handling applied only to the Base64 branch · Privilege escalation in wolfsshd

Base64 decode failure now yields WSSHD_AUTH_FAILURE so the scan continues, but the sibling tokenization failure at lines 293–298 still returns WS_FATAL_ERROR, which makes SearchKeysFile abort the whole file. A single whitespace-free non-comment line in authorized_keys therefore denies auth for every key listed after it; the new SearchForPubKey malformed-line test uses a two-token line and does not cover this.

Fix: Return WSSHD_AUTH_FAILURE for tokenization failures as well, and add a single-token-line case to test_SearchForPubKey.

Sign up for freeto 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

@stenslae@wolfSSL-Fenrir-bot