Skip to content

Conversation

@douglaz
Copy link
Owner

@douglaz douglaz commented Jul 28, 2025

Summary

This PR addresses multiple security vulnerabilities and code quality issues identified during a comprehensive code review. The changes improve the robustness and usability of WSKDF while maintaining backward compatibility.

Security Fixes

1. Integer Overflow Protection

  • Added checked_mul() for mem_limit_kbytes * 1024 calculation to prevent overflow
  • Added checked_mul() for iterations * threads calculation in benchmark command
  • These prevent potential panics or undefined behavior when using large values

2. Input Validation in Release Builds

  • Changed debug_assert\! to assert\! for n_bits validation to ensure checks run in release builds
  • Added explicit n_bits validation in the find-key command
  • Prevents arithmetic underflow when n_bits = 0

3. Comprehensive Input Validation

  • Added validation functions for common checks (threads, iterations, n_bits)
  • Set reasonable limits: MAX_THREADS = 10,000, MAX_ITERATIONS = 1,000,000
  • Added validation for empty/whitespace hex strings with clear error messages
  • All hex parsing functions now trim input and check for empty strings

Code Quality Improvements

1. Reduced Code Duplication

  • Refactored validation logic into reusable functions
  • Centralized validation constants for easy maintenance
  • Consistent error messages across all commands

2. Convention Compliance

  • Fixed string interpolation to use named placeholders throughout
  • Improved error messages with specific values included
  • All code now follows project conventions for error handling and formatting

3. Enhanced CLI Documentation

  • Added comprehensive help text for all commands (benchmark, generate-salt were missing)
  • Documented expected formats (hex-encoded) and sizes (8-byte preimage, 16-byte salt, 32-byte key)
  • Improved argument descriptions with constraints and usage instructions
  • Added detailed program description explaining WSKDF's purpose

Changes Made

  • wskdf-core/src/lib.rs: Fixed integer overflow and assertion issues
  • wskdf-cli/src/main.rs: Added validation, improved documentation, fixed string interpolation
  • CONVENTIONS.md: Added file documenting project coding standards
  • flake.nix: Added gh to development environment

Testing

  • All existing tests pass
  • Code formatted with cargo fmt
  • No warnings from cargo clippy
  • Manually tested edge cases (n_bits=0, large thread counts, empty files)

Breaking Changes

None. All changes maintain backward compatibility.

Checklist

  • Code follows project conventions
  • All tests pass
  • Documentation updated
  • No new warnings from clippy
  • Security vulnerabilities addressed

douglaz added 2 commits July 28, 2025 23:14
This commit addresses multiple security and code quality issues identified
during code review:

Security Fixes:
- Fix integer overflow risks by using checked_mul() for memory limit calculations
- Replace debug_assert\! with assert\! to ensure n_bits validation in release builds
- Add comprehensive input validation for threads, iterations, and n_bits
- Add validation for empty/whitespace hex strings with clear error messages

Code Quality Improvements:
- Refactor validation logic into reusable functions to eliminate duplication
- Fix string interpolation to use named placeholders per conventions
- Improve error messages to be more specific and include actual values
- Add constants for maximum threads (10,000) and iterations (1,000,000)

CLI Documentation:
- Add comprehensive help text for all CLI commands
- Document expected file formats and byte sizes for all arguments
- Improve argument descriptions to clarify usage and constraints
- Add long description explaining WSKDF's purpose and design

All changes maintain backward compatibility while significantly improving
the security and usability of the tool.
@douglaz douglaz enabled auto-merge July 28, 2025 23:51
@douglaz douglaz merged commit 76dadce into master Jul 28, 2025
1 check passed
@douglaz douglaz deleted the fix/security-and-validation-improvements branch July 29, 2025 22:17
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