-
Notifications
You must be signed in to change notification settings - Fork 41
Enhance password memory security using char[] with explicit clearing #228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: devondragon <1254537+devondragon@users.noreply.github.com>
Co-authored-by: devondragon <1254537+devondragon@users.noreply.github.com>
Co-authored-by: devondragon <1254537+devondragon@users.noreply.github.com>
Co-authored-by: devondragon <1254537+devondragon@users.noreply.github.com>
Co-authored-by: devondragon <1254537+devondragon@users.noreply.github.com>
Co-authored-by: devondragon <1254537+devondragon@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances password memory security by implementing char[] instead of String for password handling throughout the application, allowing explicit memory clearing to reduce password exposure time and protect against memory dump attacks.
Key Changes:
- New
PasswordSecurityUtilclass providing constant-time comparison and secure password clearing utilities - DTOs enhanced with
char[]support andAutoCloseableinterface for automatic cleanup - Service methods updated with
char[]overloads while maintaining backward compatibility
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| PasswordSecurityUtil.java | New utility class for secure password handling with constant-time comparison and memory clearing |
| UserDto.java | Enhanced with char[] fields, AutoCloseable implementation, and secure cleanup methods |
| PasswordDto.java | Enhanced with char[] fields, AutoCloseable implementation, and secure cleanup methods |
| SavePasswordDto.java | Enhanced with char[] fields, AutoCloseable implementation, and secure cleanup methods |
| UserService.java | Added char[] overloads for password operations with immediate clearing |
| PasswordPolicyService.java | Added char[] overload for validation with temporary String clearing |
| UserAPI.java | Added finally blocks to ensure password cleanup in all controller endpoints |
| PasswordSecurityUtilTest.java | Comprehensive test coverage for security utility methods |
| PasswordDtoCharArrayTest.java | Test coverage for DTO char[] handling and cleanup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Clear the temporary String reference | ||
| password = null; |
Copilot
AI
Oct 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting a String reference to null does not clear the String's contents from memory, as Strings are immutable. This comment in the code is misleading as the actual String data remains in the string pool until garbage collected. Consider removing this line or updating the comment to clarify that this only clears the reference, not the String's memory.
| // Clear the temporary String reference | |
| password = null; | |
| // Note: Setting the String reference to null does not remove the password from memory. |
| // Clear the temporary String reference | ||
| oldPassword = null; |
Copilot
AI
Oct 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting a String reference to null does not clear the String's contents from memory, as Strings are immutable. This comment in the code is misleading as the actual String data remains in the string pool until garbage collected. Consider removing this line or updating the comment to clarify that this only clears the reference, not the String's memory.
| // Clear the temporary String reference | |
| oldPassword = null; | |
| // Note: Setting oldPassword to null does not clear its contents from memory, as Strings are immutable in Java. |
| // Clear the temporary String from memory (best effort) | ||
| // Note: String is immutable so this won't actually clear the String, | ||
| // but it helps signal intent and clears the reference | ||
| password = null; |
Copilot
AI
Oct 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the comment correctly acknowledges that setting a String to null doesn't clear its memory, the practice of nulling the reference adds no security value and creates code clutter. Consider removing this line entirely since the comment already explains the limitation.
| password = null; |
|
After comprehensive code review and architectural analysis, I've decided not to |
Addresses CWE-316 by enabling explicit password clearing from memory. String-based password handling remains in memory until GC; this adds char[] support with deterministic clearing to reduce exposure windows.
Changes
New Security Utility
Arrays.fill()DTO Layer
AutoCloseablefor try-with-resources patternclearPasswords()method zeros char arrays immediately after useService Layer
validate(User, char[], String, Locale)overloadchangeUserPassword(User, char[])andcheckIfValidOldPassword(User, char[])overloadsController Layer
registerUserAccount(),savePassword(),updatePassword()clearPasswords()execution on all paths (success/error/exception)Usage
Security Impact
Compatibility
Zero breaking changes. All existing tests pass. char[] methods are opt-in enhancements.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.