Skip to content

fix: CountNameList undercounts leading-comma name-lists#1123

Open
MarkAtwood wants to merge 1 commit into
wolfSSL:masterfrom
MarkAtwood:fix/countnamelist-leading-comma
Open

fix: CountNameList undercounts leading-comma name-lists#1123
MarkAtwood wants to merge 1 commit into
wolfSSL:masterfrom
MarkAtwood:fix/countnamelist-leading-comma

Conversation

@MarkAtwood

Copy link
Copy Markdown
Contributor

Bug

CountNameList() (src/internal.c) sizes the allocation that GetNameListRaw()
then fills. It discounts both a leading and a trailing comma, but
GetNameListRaw() only strips a trailing comma — it still emits the empty
leading token (the idListIdx == 0 first-token case). For a name-list with a
leading comma such as ,ssh-rsa, CountNameList() returns 1 while
GetNameListRaw() needs 2 slots, so the fill hits the capacity guard and
returns WS_BUFFER_E.

The visible effect is in DoExtInfoServerSigAlgs(), which sizes the
server-sig-algs buffer from CountNameList(): a peer advertising a
leading-comma / empty-first-element name-list is spuriously rejected and the
connection fails. No memory-safety impact — the capacity guard fails closed.

Fix

Drop the leading-comma discount in CountNameList(). GetNameListRaw() keeps
that first token, so the count must include it. Over-counting an empty leading
element is harmless: GetNameListRaw() caps writes at the capacity and rewrites
the caller's size to the actual filled count.

Verification

Built --enable-all against wolfSSL master; compiles clean. Traced ,ssh-rsa,
ssh-rsa,ssh-dss (normal, unaffected), and ,,ssh-rsa (harmless over-alloc)
by hand against GetNameListRaw.

Reported by static analysis (Fenrir finding F-6817). Distinct from the
trailing-comma name-list issue tracked separately.

Copilot AI review requested due to automatic review settings July 23, 2026 18:36

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 an off-by-one allocation sizing bug in CountNameList() that could reject valid (but odd) SSH name-lists with a leading comma (empty first token), which in turn could cause DoExtInfoServerSigAlgs() to fail when parsing server-sig-algs.

Changes:

  • Stop discounting a leading comma in CountNameList() so its computed capacity matches what GetNameListRaw() can emit.
  • Preserve the existing behavior of discounting a trailing comma, consistent with GetNameListRaw() stripping only trailing commas.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.

3 participants