Open
Conversation
When making a copy of a string, check the destination pointer of the copy rather than the original. Affected function: CheckPasswordUnix.
There was a problem hiding this comment.
Pull request overview
This PR fixes five distinct bugs identified by static analysis in the wolfSSH library, addressing critical security and stability issues related to null pointer checks, integer overflow, and incorrect bitwise operations.
Changes:
- Fixed null pointer dereference when channel lookup fails by properly structuring the error handling with braces and else-if
- Corrected bitwise operator from OR to AND when testing FILE_ATTRIBUTE_READONLY flag on Windows
- Added integer overflow protection for peer window size adjustment by checking bounds before addition
- Fixed incorrect pointer null check after CTX allocation (checking dereferenced pointer instead of pointer itself)
- Fixed incorrect variable null check after string duplication (checking source instead of destination)
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/wolfscp.c | Fixed null dereference on channel EOF check and corrected bitwise operator for file attribute testing |
| src/internal.c | Added overflow protection for peer window size adjustment with bounds checking |
| apps/wolfsshd/wolfsshd.c | Fixed null check to validate dereferenced CTX pointer after allocation |
| apps/wolfsshd/auth.c | Fixed null check to validate newly duplicated string instead of source string |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Bounds check the bytes to add from the peer against the window size. Affected function: DoChannelWindowAdjust.
When looking up the channel object for the current channel ID, if the lookup failed, we still checked if the channel had an EOF with a null pointer. That function, does check for NULL and error, but it is better to error out sooner. Affected function: ReceiveScpMessage.
After creating a new SSH context, the pointer returned wasn't checked; the pointer to the pointer was checked. Changed to the correct pointer. Affected function: SetupCTX.
Was using OR to check if a bit was set in the read-only file attribute. This was always succeeding. Needed to change to an AND to see if it is set. Affected function: GetFileStats.
ejohnstown
force-pushed
the
static-fixes
branch
from
cddb9c9 to
961810b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix some bugs found by the wolfSSL static analyzer: