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.

Sign up for GitHub

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

Jump to bottom

Fix coverity unbounded source buffer issues #989

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Fix coverity unbounded source buffer issues #989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/chfn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
#include "shadowlog.h"
#include "string/sprintf.h"
#include "string/strtcpy.h"

#include "chkname.h"

/*
* Global variables.
Expand Down Expand Up @@ -648,11 +648,17 @@ 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])) {
fprintf (stderr, _("%s: Provided user name is not a valid name\n"), Prog);
fail_exit (E_NOPERM);
}
user = xstrdup (argv[optind]);
Copy link
Collaborator

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?

Copy link
Author

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.

Copy link
Collaborator

@alejandro-colomar alejandro-colomar Jun 25, 2024 •
edited
Loading

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.


pw = xgetpwnam (user);
if (NULL == pw) {
fprintf (stderr, _("%s: user '%s' does not exist\n"), Prog,
user);
free (user);
Copy link
Collaborator

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.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Collaborator

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.

Copy link
Author

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?

Copy link
Author

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).

Copy link
Collaborator

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).

Copy link
Collaborator

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.

Copy link
Author

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.

Copy link
Collaborator

@alejandro-colomar alejandro-colomar Jun 25, 2024 •
edited
Loading

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.

fail_exit (E_NOPERM);
}
} else {
Expand All @@ -665,6 +671,7 @@ int main (int argc, char **argv)
(unsigned long) getuid ()));
fail_exit (E_NOPERM);
}

user = xstrdup (pw->pw_name);
}

Expand Down Expand Up @@ -708,6 +715,8 @@ int main (int argc, char **argv)

SYSLOG ((LOG_INFO, "changed user '%s' information", user));

free (user);

nscd_flush_cache ("passwd");
sssd_flush_cache (SSSD_DB_PASSWD);

Expand Down
12 changes: 11 additions & 1 deletion src/chsh.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#include "exitcodes.h"
#include "shadowlog.h"
#include "string/strtcpy.h"
#include "chkname.h"

#ifndef SHELLS_FILE
#define SHELLS_FILE "/etc/shells"
Expand Down Expand Up @@ -498,11 +499,17 @@ 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])) {
fprintf (stderr, _("%s: Provided user name is not a valid name\n"), Prog);
fail_exit (E_NOPERM);
}
user = xstrdup (argv[optind]);

pw = xgetpwnam (user);
if (NULL == pw) {
fprintf (stderr,
_("%s: user '%s' does not exist\n"), Prog, user);
free (user);
fail_exit (1);
}
} else {
Expand All @@ -515,6 +522,7 @@ int main (int argc, char **argv)
(unsigned long) getuid ()));
fail_exit (1);
}

user = xstrdup (pw->pw_name);
}

Expand Down Expand Up @@ -569,6 +577,8 @@ int main (int argc, char **argv)

SYSLOG ((LOG_INFO, "changed user '%s' shell to '%s'", user, loginsh));

free (user);

nscd_flush_cache ("passwd");
sssd_flush_cache (SSSD_DB_PASSWD);

Expand Down
26 changes: 22 additions & 4 deletions src/newgrp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
#include "exitcodes.h"
#include "shadowlog.h"
#include "string/sprintf.h"

#include "chkname.h"

/*
* Global variables
Expand Down Expand Up @@ -482,7 +482,13 @@ int main (int argc, char **argv)
* not "newgrp".
*/
if ((argc > 0) && (argv[0][0] != '-')) {
group = argv[0];
if (!is_valid_group_name (argv[optind])) {
fprintf (
stderr, _("%s: provided group is not a valid group name\n"),
Prog);
goto failure;
}
group = xstrdup (argv[optind]);
argc--;
argv++;
} else {
Expand Down Expand Up @@ -513,7 +519,13 @@ int main (int argc, char **argv)
usage ();
goto failure;
} else if (argv[0] != NULL) {
group = argv[0];
if (!is_valid_group_name (argv[optind])) {
fprintf (
stderr, _("%s: provided group is not a valid group name\n"),
Prog);
goto failure;
}
group = xstrdup (argv[optind]);
} else {
/*
* get the group file entry for her login group id.
Expand All @@ -531,7 +543,7 @@ int main (int argc, char **argv)
(unsigned long) pwd->pw_gid));
goto failure;
} else {
group = grp->gr_name;
group = xstrdup (grp->gr_name);
}
}
}
Expand Down Expand Up @@ -567,6 +579,7 @@ int main (int argc, char **argv)
"changing", NULL, getuid (), 0);
}
#endif
free (group);
exit (EXIT_FAILURE);
}
#endif /* HAVE_SETGROUPS */
Expand Down Expand Up @@ -722,6 +735,7 @@ int main (int argc, char **argv)
audit_logger (AUDIT_CHGRP_ID, Prog,
audit_buf, NULL, getuid (), 0);
#endif
free (group);
exit (EXIT_FAILURE);
}

Expand All @@ -732,6 +746,7 @@ int main (int argc, char **argv)
audit_logger (AUDIT_CHGRP_ID, Prog,
audit_buf, NULL, getuid (), 0);
#endif
free (group);
exit (EXIT_FAILURE);
}

Expand All @@ -748,6 +763,7 @@ int main (int argc, char **argv)
audit_buf, NULL, getuid (), 0);
#endif
perror (SHELL);
free (group);
exit ((errno == ENOENT) ? E_CMD_NOTFOUND : E_CMD_NOEXEC);
}

Expand Down Expand Up @@ -818,6 +834,7 @@ int main (int argc, char **argv)
* the previous environment which should be the user's login shell.
*/
err = shell (prog, initflag ? NULL : progbase, newenvp);
free (group);
exit ((err == ENOENT) ? E_CMD_NOTFOUND : E_CMD_NOEXEC);
/*@notreached@*/
failure:
Expand All @@ -843,6 +860,7 @@ int main (int argc, char **argv)
"changing", NULL, getuid (), 0);
}
#endif
free (group);
exit (EXIT_FAILURE);
}

23 changes: 20 additions & 3 deletions src/passwd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,7 @@
#include "shadowlog.h"
#include "string/strtcpy.h"
#include "time/day_to_str.h"


#include "chkname.h"

/*
* exit status values
Expand Down Expand Up @@ -907,7 +906,11 @@ int main (int argc, char **argv)
}
myname = xstrdup (pw->pw_name);
if (optind < argc) {
name = argv[optind];
if (!is_valid_user_name (argv[optind])) {
fprintf (stderr, _("%s: Provided user name is not a valid name\n"), Prog);
fail_exit (E_NOPERM);
}
name = xstrdup (argv[optind]);
} else {
name = myname;
}
Expand All @@ -931,13 +934,15 @@ int main (int argc, char **argv)
(void) fprintf (stderr,
_("%s: Permission denied.\n"),
Prog);
free (name);
exit (E_NOPERM);
}
prefix_setpwent ();
while ( (pw = prefix_getpwent ()) != NULL ) {
print_status (pw);
}
prefix_endpwent ();
free (name);
exit (E_SUCCESS);
}
#if 0
Expand Down Expand Up @@ -971,6 +976,7 @@ int main (int argc, char **argv)

if (anyflag && !amroot) {
(void) fprintf (stderr, _("%s: Permission denied.\n"), Prog);
free (name);
exit (E_NOPERM);
}

Expand All @@ -979,6 +985,7 @@ int main (int argc, char **argv)
(void) fprintf (stderr,
_("%s: user '%s' does not exist\n"),
Prog, name);
free (name);
exit (E_NOPERM);
}
#ifdef WITH_SELINUX
Expand All @@ -991,6 +998,7 @@ int main (int argc, char **argv)
(void) fprintf(stderr,
_("%s: root is not authorized by SELinux to change the password of %s\n"),
Prog, name);
free (name);
exit (E_NOPERM);
}
#endif /* WITH_SELINUX */
Expand All @@ -1007,11 +1015,13 @@ int main (int argc, char **argv)
"can't view or modify password information for %s",
name));
closelog ();
free (name);
exit (E_NOPERM);
}

if (Sflg) {
print_status (pw);
free (name);
exit (E_SUCCESS);
}
if (!use_pam)
Expand All @@ -1025,6 +1035,7 @@ int main (int argc, char **argv)
(void) fprintf (stderr,
_("%s: Permission denied.\n"),
Prog);
free (name);
exit (E_NOPERM);
}
sp = pwd_to_spwd (pw);
Expand Down Expand Up @@ -1056,6 +1067,7 @@ int main (int argc, char **argv)
_("The password for %s is unchanged.\n"),
name);
closelog ();
free (name);
exit (E_NOPERM);
}
do_update_pwd = true;
Expand All @@ -1079,20 +1091,23 @@ int main (int argc, char **argv)
if (sflg) {
cp = agetpass_stdin ();
if (cp == NULL) {
free (name);
exit (E_FAILURE);
}
do_pam_passwd_non_interactive ("passwd", name, cp);
erase_pass (cp);
} else {
do_pam_passwd (name, qflg, kflg);
}
free (name);
exit (E_SUCCESS);
}
#endif /* USE_PAM */
if (setuid (0) != 0) {
(void) fputs (_("Cannot change ID to root.\n"), stderr);
SYSLOG ((LOG_ERR, "can't setuid(0)"));
closelog ();
free (name);
exit (E_NOPERM);
}
if (spw_file_present ()) {
Expand All @@ -1106,6 +1121,8 @@ int main (int argc, char **argv)
sssd_flush_cache (SSSD_DB_PASSWD | SSSD_DB_GROUP);

SYSLOG ((LOG_INFO, "password for '%s' changed by '%s'", name, myname));

free (name);
closelog ();
if (!qflg) {
if (!anyflag) {
Expand Down