Fix coverity unbounded source buffer issues #989
Conversation
During coverity scan, there is reported an issue with unbounded source buffer for each usage of input arg directly with syslog function. This commit is to limit the names of users and groups passed as input args to a fixed value of 256. The max length 256 is a typical POSIX login name length. This should improve also security.
|
|
||
| char * | ||
| xstrndup(const char *s, size_t max_length) { | ||
| size_t length = strlen(s); |
There was a problem hiding this comment.
I think strndup(3) is not allowed to read more than n bytes in the source string.
| strncpy(p, s, length); | ||
| p[length] = '\0'; |
There was a problem hiding this comment.
This is an antipattern that I've blown away from this project. Please have a look at the functions in lib/string/ to find something suitable.
There was a problem hiding this comment.
You can check that there are no direct calls to strncpy(3) at all:
$ grep -rn '\<strncpy *(' lib* src/
lib/string/strncpy.h:18:#define STRNCPY(dst, src) strncpy(dst, src, NITEMS(dst))And that macro is only used for other very specific purposes:
$ grep -rn 'STRNCPY *(' lib* src/
lib/utmp.c:279: STRNCPY(utent->ut_line, line);
lib/utmp.c:281: STRNCPY(utent->ut_id, ut->ut_id);
lib/utmp.c:284: STRNCPY(utent->ut_id, line + 3);
lib/utmp.c:287: STRNCPY(utent->ut_name, name);
lib/utmp.c:289: STRNCPY(utent->ut_user, name);
lib/utmp.c:293: STRNCPY(utent->ut_host, hostname);
lib/string/strncpy.h:18:#define STRNCPY(dst, src) strncpy(dst, src, NITEMS(dst))
lib/log.c:86: STRNCPY(newlog.ll_host, host);| user = argv[optind]; | ||
| user = xstrndup(argv[optind], USER_ENTRY_MAX_LENGTH); |
There was a problem hiding this comment.
Don't you free(3) the memory?
| * to syslog function. | ||
| * Typical POSIX login name max length is 256. | ||
| */ | ||
| #define USER_ENTRY_MAX_LENGTH 256 |
There was a problem hiding this comment.
There is LOGIN_NAME_MAX, defined by POSIX in <limits.h>.
However, we support unlimited user-name lengths. See this commit:
commit 6a1f45d932c859725bc175f96f7c42b9ccc20ea4
Author: Tobias Stoeckmann <[email protected]>
Date: Sat Feb 3 01:10:22 2024 +0100
lib/chkname.c: Support unlimited user name lengths
If the system does not have a user name length limit, support it
accordingly. If the system has no _SC_LOGIN_NAME_MAX, use
LOGIN_NAME_MAX constant instead.
Signed-off-by: Tobias Stoeckmann <[email protected]>
You should probably use a run-time-variable user-name length.
You'll need to write this function (extracted from the current code in is_valid_user_name() from <lib/chkname.c>):
size_t
username_maxlen(void)
{
long maxlen;
errno = 0;
maxlen = sysconf(_SC_LOGIN_NAME_MAX);
if (maxlen == -1 && errno != 0)
return LOGIN_NAME_MAX;
return maxlen;
}and use it in your cases.
There was a problem hiding this comment.
BTW, now I remember we use the term size, not len, since it includes the NUL byte.
| size_t length = strlen(s); | ||
| if (length > max_length) { | ||
| length = max_length; | ||
| } | ||
|
|
||
| char *p = malloc(length + 1); | ||
| if (!p) { | ||
| fprintf(log_get_logfd(), _("%s: %s\n"), | ||
| log_get_progname(), strerror(errno)); | ||
| exit(13); |
There was a problem hiding this comment.
Isn't this simpler as the following?:
{
char *dup;
dup = strndup(s, n);
if (dup == NULL) { ... }
return dup;
}Or am I missing something?
| length = max_length; | ||
| } | ||
|
|
||
| char *p = malloc(length + 1); |
There was a problem hiding this comment.
We don't call malloc(3) directly. Have a look at <lib/alloc.h>, and the MALLOC() macro.
There was a problem hiding this comment.
And you'll also want to check XMALLOC().
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]>
| @@ -498,7 +498,7 @@ int main (int argc, char **argv) | |||
| * name, or the name getlogin() returns. | |||
| */ | |||
| if (optind < argc) { | |||
| user = argv[optind]; | |||
| user = xstrndup(argv[optind], USER_ENTRY_MAX_LENGTH); | |||
There was a problem hiding this comment.
Why truncate it silently? If the input is invalid, we should just fail, no?
During coverity scan, there is reported an issue
with unbounded source buffer for each usage of input arg directly with syslog function.
This commit is to limit the names of users and groups passed as input args to a fixed value of 256.
The max length 256 is a typical POSIX login name length. This should improve also security.