Skip to content

Remove FIPS guards in GetASN_BitString length check#10027

Merged
dgarske merged 1 commit intowolfSSL:masterfrom
embhorn:zd21394
Mar 24, 2026
Merged

Remove FIPS guards in GetASN_BitString length check#10027
dgarske merged 1 commit intowolfSSL:masterfrom
embhorn:zd21394

Conversation

@embhorn
Copy link
Copy Markdown
Member

@embhorn embhorn commented Mar 20, 2026

Description

Remove FIPS guards in GetASN_BitString length check

Fixes zd21394

Testing

Build test

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@embhorn embhorn self-assigned this Mar 20, 2026
Copilot AI review requested due to automatic review settings March 20, 2026 14:29
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Removes conditional compilation guards so GetASN_BitString always rejects zero-length BIT STRING content, aligning behavior across FIPS/non-FIPS builds.

Changes:

  • Removed #if/#endif gating around the length == 0 validation in GetASN_BitString.
  • Ensured the “one or more octets” check runs in all build configurations.
Comments suppressed due to low confidence (2)

wolfcrypt/src/asn.c:1

  • length is an int, but the new guard only rejects length == 0. If length can ever be negative (e.g., propagated from a decode error), this check won’t trigger and the function will continue and potentially read input[idx] despite an invalid length. Consider changing the condition to reject all non-positive lengths (e.g., length <= 0) so invalid/underflowed lengths fail fast as well.
    wolfcrypt/src/asn.c:1
  • Removing the HAVE_SELFTEST/HAVE_FIPS guards changes behavior specifically for those build modes (previously, length == 0 would not return ASN_PARSE_E in those configurations). If consumers rely on the prior behavior in FIPS/selftest builds, this is a behavioral breaking change; consider documenting it (release notes / migration note) or clarifying in the PR description why this is safe/required for those configurations.

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

Copy link
Copy Markdown
Contributor

@kaleb-himes kaleb-himes left a comment

Choose a reason for hiding this comment

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

This was only compiled out for ctaocrypt (cert 2425) but we're no longer actively supporting that very old FIPS module at this point so this is a fine change from my perspective.

I'll ask @cconlon to also review for the self-test use-case. Good from my side though.

@dgarske dgarske merged commit cf6c172 into wolfSSL:master Mar 24, 2026
542 of 600 checks passed
@embhorn embhorn deleted the zd21394 branch April 24, 2026 21:33
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.

5 participants