-
Notifications
You must be signed in to change notification settings - Fork 229
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 coverity unbounded source buffer issues #989
base: master
Are you sure you want to change the base?
Fix coverity unbounded source buffer issues #989
Conversation
Please paste such a Coverity scan report in the commit message. It would be helpful when reviewing. |
It encapsulates some logic that we may want to reuse elsewhere. Link: <shadow-maint#989> Signed-off-by: Alejandro Colomar <[email protected]>
It encapsulates some logic that we may want to reuse elsewhere. Link: <shadow-maint#989> Signed-off-by: Alejandro Colomar <[email protected]>
It encapsulates some logic that we may want to reuse elsewhere. Link: <shadow-maint#989> Signed-off-by: Alejandro Colomar <[email protected]>
It encapsulates some logic that we may want to reuse elsewhere. Link: <#989> Signed-off-by: Alejandro Colomar <[email protected]>
Can you please rebase the branch? Edit: I've done it myself for you. v1b:
|
a90453e
to
bb74110
Compare
Please address the issues raised. |
I've converted your PR to draft, since it's not ready for review. Please address the comments. |
bb74110
to
7b8d2f7
Compare
7b8d2f7
to
21af660
Compare
src/chfn.c
Outdated
@@ -648,11 +648,19 @@ int main (int argc, char **argv) | |||
* name, or the name getlogin() returns. | |||
*/ | |||
if (optind < argc) { | |||
user = argv[optind]; | |||
if (is_valid_user_name (argv[optind])) { | |||
user = xstrdup (argv[optind]); |
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.
Why do we want a copy?
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.
Copy will make sure that the program can accommodate proper string length, which is passed further to syslog function. This should prevent any further issues with unbound buffers during static analyses.
Also, notice that user is allocated further in the code in other branch:
user = xstrdup (pw->pw_name);
So, with the proposed change user variable will contain allocated value in each case. And then we can safely make free before exiting.
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.
Copy will make sure that the program can accommodate proper string length, which is passed further to syslog function.
The previous call to is_valid_user_name() already makes sure that the length is good.
This should prevent any further issues with unbound buffers during static analyses.
Also, notice that user is allocated further in the code in other branch: user = xstrdup (pw->pw_name); So, with the proposed change user variable will contain allocated value in each case. And then we can safely make free before exiting.
This should be fixed in a separate commit, since it's a separate problem.
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.
This is not exactly. The static analyses tools sees that the user pointer is assigned to the program argument and as is cannot be passed to syslog. If we do only check then it does not help for the main issue: unbounded buffer passed to syslog. The xstrdup is mandatory to get rid of the static analyses tool issues. However, after adding the check, we can always mark the reported issue as false positive.
Just, it is a general good practice to make a copy of passed arguments for further usage (at least form my experience). Please, confirm that we should not do copy in this case, so I will remove any copies from the proposed PR. And I can create a separate PR with that implementation.
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.
This is not exactly. The static analyses tools sees that the user pointer is assigned to the program argument and as is cannot be passed to syslog. If we do only check then it does not help for the main issue: unbounded buffer passed to syslog. The xstrdup is mandatory to get rid of the static analyses tool issues. However, after adding the check, we can always mark the reported issue as false positive. Just, it is a general good practice to make a copy of passed arguments for further usage (at least form my experience). Please, confirm that we should not do copy in this case, so I will remove any copies from the proposed PR. And I can create a separate PR with that implementation.
I don't want to silence the warning; I want to fix the bug. And if we do merge the second patch, we will have the side-effect that the warning will vanish.
So, I confirm
pw = xgetpwnam (user); | ||
if (NULL == pw) { | ||
fprintf (stderr, _("%s: user '%s' does not exist\n"), Prog, | ||
user); | ||
free (user); |
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.
We don't need to free(3) in these early exit() points.
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.
Fixed
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.
Not really. I still see it there.
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.
You are correct, just the memory was already allocated, so I added free before exiting. Why we should not do free at early exit stage?
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.
If we do not do free for any allocated memory, we get issues in other code analyses tools (for example using gcc compiler with allocation checks flags).
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.
You are correct, just the memory was already allocated, so I added free before exiting. Why we should not do free at early exit stage?
Because exit(3) makes sure all memory is released. And it's just simpler (no need for having dozens of free(3) calls on the same resource).
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.
If we do not do free for any allocated memory, we get issues in other code analyses tools (for example using gcc compiler with allocation checks flags).
Please show those warnings.
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.
Some comment to exit(3):
While it is true that exit(3) ensures that all memory is freed, it is still considered good practice to explicitly free dynamically allocated memory using free() before calling exit(3) or before the program exits normally. This practice has several benefits:
-
Portability: On some non-Linux systems, exit(3) might not guarantee that all resources are properly cleaned up.
-
Code Maintainability: Explicitly freeing memory makes the code easier to understand and maintain. It shows where and why resources are being allocated and deallocated.
-
Debugging: Explicitly freeing memory can help catch memory leaks earlier during program execution, making debugging easier.
Anyway, in this exact sample from the proposed changes, I can do remove free from the code. Please, confirm that this is the general practice for that repo.
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.
Some comment to exit(3): While it is true that exit(3) ensures that all memory is freed, it is still considered good practice to explicitly free dynamically allocated memory using free() before calling exit(3) or before the program exits normally. This practice has several benefits:
* ``` Portability: On some non-Linux systems, exit(3) might not guarantee that all resources are properly cleaned up. ```
free(3) is guaranteed by ISO C to free all allocated memory, IIRC.
* ``` Code Maintainability: Explicitly freeing memory makes the code easier to understand and maintain. It shows where and why resources are being allocated and deallocated. ```
Having 13 different points where we free(name);
in the same function is exactly the opposite of code maintainability.
During coverity scan, there are reported four issues with unbounded source buffer for each usage of input arg directly with syslog function. Sample coverity test report for chsh.c file: 1. string_size_argv: argv contains strings with unknown size. int main (int argc, char **argv) [...] 4. var_assign_var: Assigning: user = argv[optind]. Both are now tainted. user = argv[optind]; [...] CID 5771784: (shadow-maint#1 of 1): Unbounded source buffer (STRING_SIZE) 15. string_size: Passing string user of unknown size to syslog. SYSLOG ((LOG_INFO, "changed user '%s' shell to '%s'", user, loginsh)); Similar issue is reported three times more: File: chfn.c, function: main, variable: user File: passwd.c, function: main, variable: name File: newgrp.c, function: main, variable: group The proposed commit is a try to fix the reported issues.
21af660
to
1351181
Compare
fprintf (stderr, _("%s: Provided user name is not a valid name\n"), Prog); | ||
fail_exit (E_NOPERM); | ||
} | ||
user = xstrdup (argv[optind]); |
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.
Why do we want a copy?
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.
Copy will make sure that the program can accommodate proper string length, which is passed further to syslog function. This should prevent any further issues with unbound buffers during static analyses.
Also, notice that user is allocated further in the code in other branch:
user = xstrdup (pw->pw_name);
So, with the proposed change user variable will contain allocated value in each case. And then we can safely make free before exiting.
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.
Copy will make sure that the program can accommodate proper string length, which is passed further to syslog function.
I don't think this is true. The check right above already makes sure the length is good.
This should prevent any further issues with unbound buffers during static analyses. Also, notice that user is allocated further in the code in other branch: user = xstrdup (pw->pw_name); So, with the proposed change user variable will contain allocated value in each case. And then we can safely make free before exiting.
This makes sense, but please fix it in a separate PR after we merge the first one.
Fix coverity unbound buffer issues
During coverity scan, there are reported four issues
with unbounded source buffer for each usage of input arg
directly with syslog function.
Sample coverity test report for chsh.c file:
int main (int argc, char **argv)
[...]
user = argv[optind];
[...]
CID 5771784: (Retiring certain usernames. #1 of 1): Unbounded source buffer (STRING_SIZE)
SYSLOG ((LOG_INFO, "changed user '%s' shell to '%s'", user, loginsh));
Similar issue is reported three times more:
File: chfn.c, function: main, variable: user
File: passwd.c, function: main, variable: name
File: newgrp.c, function: main, variable: group
The proposed commit is a try to fix the reported issues
by adding a check for a valid user or group names.