UBSan null pointer violation in ESIGN + Fix for Issue #1338
9 views
Skip to first unread message
Oscar Yang
Nov 13, 2025, 5:58:12 AMNov 13
to cryptop...@googlegroups.com
Hello Crypto++ maintainers and community,
I'm scc, and I've discovered an UndefinedBehaviorSanitizer (UBSan) violation in the ESIGN implementation while working on Issue #1338 (https://github.com/weidai11/cryptopp/issues/1338). I've developed a comprehensive fix and validation infrastructure that I'd like to contribute.
Issue Summary
- Primary Issue: UBSan detects null pointer passed to memcpy in esign.cpp:118 when an empty seed (size=0) is provided to InvertibleESIGNFunction::GenerateRandom.
- Secondary Issue: This fix also resolves Issue #1338 (clang22 fortify warning) by using .data() and an explicit bounds assertion.
Environment
- Crypto++ Version: Current master (commit 60f81a77)
- Operating System: Ubuntu (GitHub Actions: ubuntu-latest)
- Compiler: Clang (default) and Clang 22
- Build Flags: -fsanitize=address,undefined for sanitizer testing; -O3 -D_FORTIFY_SOURCE=2 for fortify validation
Exact Error Message
esign.cpp:118:25: runtime error: null pointer passed as argument 2,
which is declared to never be null
/usr/include/string.h:44:28: note: nonnull attribute specified here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior esign.cpp:118:25
Stack Trace
#0 in CryptoPP::InvertibleESIGNFunction::GenerateRandom(...)
/home/runner/work/cryptopp/cryptopp/esign.cpp:118:3
#1 in run_one(unsigned long, unsigned int)
/home/runner/work/cryptopp/cryptopp/tests/esign_memcpy_asan.cpp:30:9
#2 in main
/home/runner/work/cryptopp/cryptopp/tests/esign_memcpy_asan.cpp:47:17
Root Cause
When seedParam.size() == 0, seedParam.begin() may return a null pointer, which violates the C++ standard requirement that memcpy must receive non-null pointers even when the copy size is 0.
Minimal Reproduction
A focused test exercising this path is available here:
https://github.com/scc-tw/cryptopp/blob/asan-esign-test/tests/esign_memcpy_asan.cpp
The test validates 17 seed lengths (including 0) across 3 modulus sizes for comprehensive edge case coverage.
Evidence
Commit demonstrating the issue:
- https://github.com/scc-tw/cryptopp/commit/f47507e6beb94c97cdda84f0d0d06f4299cc51d9
- CI failure: https://github.com/scc-tw/cryptopp/actions/runs/19327409311/job/55281830058
Commit with UBSan fix:
- https://github.com/scc-tw/cryptopp/commit/db3951aa00258c2789c2eab519b312de6157f948
- CI success: https://github.com/scc-tw/cryptopp/actions/runs/19327600421/job/55282868767
Complete fix (UBSan + Issue #1338):
- https://github.com/scc-tw/cryptopp/commit/ab1878dde7658ff8f18482d9944f6996bf3155a6
- CI validation: https://github.com/scc-tw/cryptopp/actions/runs/19327831662/job/55283667912
Pull Request #1340
I've opened Pull Request #1340 with this fix:
https://github.com/weidai11/cryptopp/pull/1340
The fix applies three changes to esign.cpp:117-122:
```cpp
seed.resize(seedParam.size() + 4);
// Help static analyzer verify bounds (Issue #1338)
CRYPTOPP_ASSERT(seed.size() >= seedParam.size() + 4);
// Guard against null pointer when size is 0 (UBSan)
if (seedParam.size() > 0)
std::memcpy(seed.data() + 4, seedParam.begin(), seedParam.size());
```
Key changes:
1. Use seed.data() instead of seed + 4: Clearer for static analyzers, resolves clang22 fortify warning
2. Add CRYPTOPP_ASSERT: Explicit bounds verification helps compiler verify safety
3. Guard with if (size > 0): Prevents null pointer UB, handles empty seed edge case
This approach is inspired by the cryptopp-modern fork fix (commit 79ecd1f0) with additional UBSan protection.
Testing
The fix has been validated with:
- ✅ ASan/UBSan (no violations)
- ✅ Clang22 -O3 with fortify (no warnings)
- ✅ Full cryptest.exe validation suite
- ✅ 51 ESIGN edge cases (0-128 byte seeds, 96-384 bit moduli)
Questions
1. Is the fix approach acceptable? (seed.data() + assert + size guard)
2. Would you like CI validation infrastructure (ASan/UBSan + clang22 fortify) added to the repository?
3. Any additional testing or changes needed for the PR?
I'm happy to make any adjustments based on your feedback. Thank you for maintaining this excellent library!
Best regards,
scc
I'm scc, and I've discovered an UndefinedBehaviorSanitizer (UBSan) violation in the ESIGN implementation while working on Issue #1338 (https://github.com/weidai11/cryptopp/issues/1338). I've developed a comprehensive fix and validation infrastructure that I'd like to contribute.
Issue Summary
- Primary Issue: UBSan detects null pointer passed to memcpy in esign.cpp:118 when an empty seed (size=0) is provided to InvertibleESIGNFunction::GenerateRandom.
- Secondary Issue: This fix also resolves Issue #1338 (clang22 fortify warning) by using .data() and an explicit bounds assertion.
Environment
- Crypto++ Version: Current master (commit 60f81a77)
- Operating System: Ubuntu (GitHub Actions: ubuntu-latest)
- Compiler: Clang (default) and Clang 22
- Build Flags: -fsanitize=address,undefined for sanitizer testing; -O3 -D_FORTIFY_SOURCE=2 for fortify validation
Exact Error Message
esign.cpp:118:25: runtime error: null pointer passed as argument 2,
which is declared to never be null
/usr/include/string.h:44:28: note: nonnull attribute specified here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior esign.cpp:118:25
Stack Trace
#0 in CryptoPP::InvertibleESIGNFunction::GenerateRandom(...)
/home/runner/work/cryptopp/cryptopp/esign.cpp:118:3
#1 in run_one(unsigned long, unsigned int)
/home/runner/work/cryptopp/cryptopp/tests/esign_memcpy_asan.cpp:30:9
#2 in main
/home/runner/work/cryptopp/cryptopp/tests/esign_memcpy_asan.cpp:47:17
Root Cause
When seedParam.size() == 0, seedParam.begin() may return a null pointer, which violates the C++ standard requirement that memcpy must receive non-null pointers even when the copy size is 0.
Minimal Reproduction
A focused test exercising this path is available here:
https://github.com/scc-tw/cryptopp/blob/asan-esign-test/tests/esign_memcpy_asan.cpp
The test validates 17 seed lengths (including 0) across 3 modulus sizes for comprehensive edge case coverage.
Evidence
Commit demonstrating the issue:
- https://github.com/scc-tw/cryptopp/commit/f47507e6beb94c97cdda84f0d0d06f4299cc51d9
- CI failure: https://github.com/scc-tw/cryptopp/actions/runs/19327409311/job/55281830058
Commit with UBSan fix:
- https://github.com/scc-tw/cryptopp/commit/db3951aa00258c2789c2eab519b312de6157f948
- CI success: https://github.com/scc-tw/cryptopp/actions/runs/19327600421/job/55282868767
Complete fix (UBSan + Issue #1338):
- https://github.com/scc-tw/cryptopp/commit/ab1878dde7658ff8f18482d9944f6996bf3155a6
- CI validation: https://github.com/scc-tw/cryptopp/actions/runs/19327831662/job/55283667912
Pull Request #1340
I've opened Pull Request #1340 with this fix:
https://github.com/weidai11/cryptopp/pull/1340
The fix applies three changes to esign.cpp:117-122:
```cpp
seed.resize(seedParam.size() + 4);
// Help static analyzer verify bounds (Issue #1338)
CRYPTOPP_ASSERT(seed.size() >= seedParam.size() + 4);
// Guard against null pointer when size is 0 (UBSan)
if (seedParam.size() > 0)
std::memcpy(seed.data() + 4, seedParam.begin(), seedParam.size());
```
Key changes:
1. Use seed.data() instead of seed + 4: Clearer for static analyzers, resolves clang22 fortify warning
2. Add CRYPTOPP_ASSERT: Explicit bounds verification helps compiler verify safety
3. Guard with if (size > 0): Prevents null pointer UB, handles empty seed edge case
This approach is inspired by the cryptopp-modern fork fix (commit 79ecd1f0) with additional UBSan protection.
Testing
The fix has been validated with:
- ✅ ASan/UBSan (no violations)
- ✅ Clang22 -O3 with fortify (no warnings)
- ✅ Full cryptest.exe validation suite
- ✅ 51 ESIGN edge cases (0-128 byte seeds, 96-384 bit moduli)
Questions
1. Is the fix approach acceptable? (seed.data() + assert + size guard)
2. Would you like CI validation infrastructure (ASan/UBSan + clang22 fortify) added to the repository?
3. Any additional testing or changes needed for the PR?
I'm happy to make any adjustments based on your feedback. Thank you for maintaining this excellent library!
Best regards,
scc
|
|
Reply all
Reply to author
Forward
0 new messages