Skip to content

Add sha3 and icsf tests#594

Draft
1000TurquoisePogs wants to merge 1 commit into
v3.x/stagingfrom
feature/v3/sha3
Draft

Add sha3 and icsf tests#594
1000TurquoisePogs wants to merge 1 commit into
v3.x/stagingfrom
feature/v3/sha3

Conversation

@1000TurquoisePogs

Copy link
Copy Markdown
Member

Proposed changes

Fix three bugs in the ICSF digest wrapper (icsf.h/icsf.c) and add SHA-3 algorithm support.

Bug fixes

  1. SHA-256 typo (icsf.c): The icsfDigestInit rule array for ICSF_DIGEST_SHA256 contained "SHA246" instead of "SHA256". This caused SHA-256 hashing to fail at the ICSF callable service level with a bad rule array keyword. Fixed by correcting the string to "SHA256 FIRST ".

  2. Hash buffer overflow (icsf.h): The hash field in ICSFDigest was declared as char hash[32], which is too small for SHA-384 (48 bytes) and SHA-512 (64 bytes). Writing the hash output would overflow into adjacent struct memory. Fixed by increasing to char hash[64].

  3. Context buffer too small for SHA-3 (icsf.h): ICSF_HASH_CONTEXT_LENGTH was 128 bytes, which is insufficient for SHA-3's Keccak state (requires ~200+ bytes). Increased to 512 bytes to accommodate all current and foreseeable ICSF algorithm context sizes.

New feature: SHA-3 support

Added SHA-3 (Keccak) family algorithms to the ICSF digest wrapper:

Constant Value ICSF keyword Hash length
ICSF_DIGEST_SHA3_224 7 SHA3-224 28 bytes
ICSF_DIGEST_SHA3_256 8 SHA3-256 32 bytes
ICSF_DIGEST_SHA3_384 9 SHA3-384 48 bytes
ICSF_DIGEST_SHA3_512 10 SHA3-512 64 bytes

Cases added to all three streaming functions: icsfDigestInit, icsfDigestUpdate, icsfDigestFinish.

Files changed

File Change
h/icsf.h Fixed ICSF_HASH_CONTEXT_LENGTH (128->512), hash[32]->hash[64], added SHA-3 constants
c/icsf.c Fixed "SHA246" typo, added SHA-3 cases to Init/Update/Finish

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

PR Checklist

  • If the changes in this PR are meant for the next release / mainline, this PR targets the "staging" branch.
  • My code follows the style guidelines of this project (see: Contributing guideline)
  • I have commented my code, particularly in hard-to-understand areas
  • Relevant update to CHANGELOG.md
  • My changes generate no new warnings

Testing

Try out tests/icsftest.c

Further comments

The SHA-256 typo ("SHA246") has been present since the original code. Any existing callers using ICSF_DIGEST_SHA256 through this wrapper were silently getting ICSF errors. The SHA-384/SHA-512 buffer overflow was a latent bug that could cause memory corruption when those algorithms were used.

The context length increase from 128 to 512 bytes increases the ICSFDigest struct size. Callers allocating this struct on the stack should have sufficient stack space (the struct is now approximately 640 bytes total).

Signed-off-by: 1000TurquoisePogs <sgrady@rocketsoftware.com>
@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
6.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Comment thread CHANGELOG.md
Comment on lines +4 to +6
- Bugfix: `icsfDigestInit` in `icsf.c` contained a typo in the SHA-256 ICSF rule array keyword (`"SHA246"` instead of `"SHA256"`), causing SHA-256 hashing to fail.
- Bugfix: `ICSFDigest.hash` buffer in `icsf.h` was 32 bytes, too small for SHA-384 (48 bytes) and SHA-512 (64 bytes). Increased to 64 bytes to prevent buffer overflow.
- Enhancement: Added SHA-3 (Keccak) algorithm support to the ICSF digest wrapper: `ICSF_DIGEST_SHA3_224`, `ICSF_DIGEST_SHA3_256`, `ICSF_DIGEST_SHA3_384`, `ICSF_DIGEST_SHA3_512` with corresponding cases in `icsfDigestInit`, `icsfDigestUpdate`, and `icsfDigestFinish`.

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.

Could you, please, add the PR or issue number?

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.

2 participants