Skip to content
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

Fix incorrect return-value check for a scanf like function (CWE-253) #323

Merged
merged 1 commit into from
Jun 23, 2024

Conversation

gsnw
Copy link
Contributor

@gsnw gsnw commented Jun 14, 2024

This pull request fix the codeql security CWE-253 alert Incorrect return-value check for a 'scanf'-like function

@coveralls
Copy link

Coverage Status

coverage: 85.117%. remained the same
when pulling 5d0fdaa on gsnw:fix-scanf-cwe253
into 2f2ff0b on schweikert:develop.

Copy link
Collaborator

@auerswal auerswal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have looked at the changes, and reviewed the sscanf man page. This change seems correct, because only the return value of 1 represents matching the single expected value.

A value of 0 would mean that nothing matched the format string. The current code checks against this. A number greater than 1 seems unlikely with a format string that comprises a single conversion specification. In case of an error, EOF could also be returned, but the current code would not catch this, I think.

@gsnw
Copy link
Contributor Author

gsnw commented Jun 15, 2024

Fix issue #324

@auerswal
Copy link
Collaborator

sscanf() returns EOF when used with an empty string as input. This can be given as option argument, e.g., using fping -t '' …. Without the pull request, fping does not catch this invalid option argument.

@coveralls
Copy link

Coverage Status

coverage: 85.54% (+0.07%) from 85.469%
when pulling edffcc3 on gsnw:fix-scanf-cwe253
into 118cdc5 on schweikert:develop.

@auerswal
Copy link
Collaborator

I have looked at the changes again, IMHO they are "obviously correct", so I'll just go ahead and merge this.
@schweikert: Please let me know if this is OK.

@auerswal auerswal merged commit 94de791 into schweikert:develop Jun 23, 2024
9 checks passed
auerswal added a commit to auerswal/fping that referenced this pull request Jun 23, 2024
Giving an empty string instead of a number as option argument
was not reliably caught, see GH issue schweikert#324 and GH PR schweikert#323.
auerswal added a commit to auerswal/fping that referenced this pull request Jun 23, 2024
Giving an empty string instead of a number as option argument
was not reliably caught, see GH issue schweikert#324 and GH PR schweikert#323.
auerswal added a commit to auerswal/fping that referenced this pull request Jun 23, 2024
Giving an empty string instead of a number as option argument
was not reliably caught, see GH issue schweikert#324 and GH PR schweikert#323.
@gsnw gsnw deleted the fix-scanf-cwe253 branch June 24, 2024 04:27
@schweikert
Copy link
Owner

Looks good to me as well. Thanks!

auerswal added a commit that referenced this pull request Jun 28, 2024
Giving an empty string instead of a number as option argument
was not reliably caught, see GH issue #324 and GH PR #323.
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.

5 participants