Fix coverity unbounded source buffer issues

[MarcinDigitic]

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:

1. string_size_argv: argv contains strings with unknown size.
   int main (int argc, char **argv)
   [...]
2. var_assign_var: Assigning: user = argv[optind]. Both are now tainted.
   user = argv[optind];
   [...]
   CID 5771784: (Retiring certain usernames. #1 of 1): Unbounded source buffer (STRING_SIZE)
3. 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
by adding a check for a valid user or group names.

[alejandro-colomar]

During coverity scan, there is reported an issue with unbounded source buffer for each usage of input arg directly with syslog function.

Please paste such a Coverity scan report in the commit message. It would be helpful when reviewing.

[alejandro-colomar]

Can you please rebase the branch?

---

Edit: I've done it myself for you.

v1b:

• Rebase

$ git range-diff md/master..md/topic/coverity/fix_unbound_input_buffer shadow/master..989 
1:  a90453e6 = 1:  bb741102 Fix coverity unbound buffer issues

[alejandro-colomar]

Please address the issues raised.

[alejandro-colomar]

I've converted your PR to draft, since it's not ready for review. Please address the comments.

[alejandro-colomar]

Hi, would you mind updating the PR?

[MarcinDigitic]

Hi, would you mind updating the PR?

Hi @alejandro-colomar , the PR is cleaned and rebased -> it should be ready for merging.

[alejandro-colomar]

Does this change remove all of the coverity reports? Which of them are still present after this change?

[MarcinDigitic]

Does this change remove all of the coverity reports? Which of them are still present after this change?

This will not remove the coverity reports issues. To remove the coverity report errors we would need to use xstrdup as it was in my original code. However, in this case I will prepare a separate PR after merging this one.
And also, after adding the conditions as in this PR we would be safe to ignore the coverity reports issues.

[alejandro-colomar]

Does this change remove all of the coverity reports? Which of them are still present after this change?

This will not remove the coverity reports issues. To remove the coverity report errors we would need to use xstrdup as it was in my original code.

Then the commit message should be updated to reflect what this commit does.

[MarcinDigitic]

The commit message is updated.

[alejandro-colomar]

The proposed changes add conditions verifing size of passed arguments
for users and groups names.

I don't understand that sentence. Could you please rephrase?

[MarcinDigitic]

Please, check now. The main reported issue by coverity is:

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: (#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));

Passing string user of unknown size to syslog
The added conditions check also the string sizes of the passed arguments.

[alejandro-colomar]

Please, check now.

The proposed changes add conditions, which verify
the user and group names arguments, including their sizes.

You should say lengths instead of sizes. is_valid_user_name() verifies the string length (strlen(3)).

$ grepc is_valid_user_name .
./lib/chkname.h:extern bool is_valid_user_name (const char *name);
./lib/chkname.c:bool
is_valid_user_name(const char *name)
{
	if (strlen(name) >= login_name_max_size()) {
		errno = EOVERFLOW;
		return false;
	}

	return is_valid_name(name);
}

[alejandro-colomar]

This will not fix the coverity reports, but the change causes
that they are irrelevant and could be ignored.

The report is actually fixed, I think. What the patch doesn't do is "silence" it. Please reword. Other than that, it more or less LGTM.

[MarcinDigitic]

Wording is corrected.

[alejandro-colomar]

LGTM. Thanks!