Add logger for usr.bin/mdo/mdo.c: a logger like sudo.

[Rin0913]

We should add a logger for some MAC module. However, the logger for mac_do needs further discussions, and the tool use MAC framework should also be logging itself.

The output log format is like following:

Nov  4 03:46:54 bsd-workstation mdo[26618]: USER: rin; failed to call initgroups: 1
Nov  4 03:47:12 bsd-workstation mdo[26622]: USER: root; COMMAND=/bin/sh

[delphij]

I think this (adding logging support) is the right direction but logging in the utility is probably not a very good choice because when mdo is loaded and enabled, an approved users can simply call setuid() and/or setgid() in their applications to the allowed user / groups themselves, which will not trigger any logging...

Scratch that, I have misread the code. The module does require caller to be the hardcoded /usr/bin/mdo.

[delphij]

Please see comments inline.

[delphij]

Since this function is only used once, it can be folded into main()

[Rin0913]

I think the separation would make the code easier to read and without loss the original code logic?

[delphij]

I don't really have strong opinion here, it's more of a personal preference.

However, this approach also matches existing patterns of other similar situations: from readability point of view, while separating a more complex function is generally helpful (e.g. this program would do some complicated operations A, B, and C in order, and these operations are implemented as separate functions), it can actually hurt readability to overly wrap simple operations. My personal rule of thumb is to not extract an operation into a function if a) it's only used once, and b) the function itself plus the caller's code block would fit one page or screen (about 20-25 lines) worth of code block.

Note that there are other similar situations where it's done in place instead of in a separate function, for example in apply(1) and newsyslog(8).

[delphij]

Oops, previous comment was lost.

Note that allocating buffer in the function and returning is more prone to memory leaks, although for this particular use case it is Okay because we are calling execvp() right after, but it may make some static analyzers or tracers unhappy (because the memory allocated in the function was indeed lost, although they don't do any real harm).

Additionally you might want to use sbuf(9) to manage buffers. Or in other words something like:

struct sbuf *cmdbuf;

if ((cmdbuf = sbuf_new_auto()) == NULL)
    err(1, "sbuf_new_auto");

for (int i=0; i < argc; i++) {
    if (sbuf_cat(cmdbuf, argv[i]) != 0)
        err(1, "sbuf_cat");
    if (i < argc - 1)
        if (sbuf_cat(cmdbuf, " ") != 0)
            err(1, "sbuf_cat");
}

if (sbuf_finish(cmdbuf) != 0)
        err(1, "sbuf_finish");

syslog(LOG_AUTH | LOG_INFO, "USER: %s; COMMAND=%s",
    original_username,
    sbuf_data(cmdbuf));
execvp(argv[0], argv);
/* NOTREACHED */

[Rin0913]

Because the memory block of new generated command string is allocated by malloc, maybe I could just free the memory block after logging just like this?

As for using sbuf, I believe if we can guarantee that the join_strings function is well-implemented, we don't have to use it because there is no possibility of buffer overflows.

What do you think?

[delphij]

Since the VM space is going to be discarded anyway soon (with a successful execvp() or the err() below), the free() can be omitted here.

sbuf(9) is a "string builder", it keeps track of the length of string and handles buffer management. On the other hand, doing it your own, especially doing strcat() in a loop would have to iterate each characters in the string built so far in order to append to them. Therefore in most cases, using sbuf(9) is preferred for this kind of scenario.

[bapt]

open_memstream(3) can also be used instead of sbuf(9)

[rilysh]

There are several branches in these loops, also I think it's a good idea to print the function name where it fails.
As suggested by delphij, you can also use sbuf(9) so that you can probably eliminate this wrapper function.

Slightly changed

static char *join_strings2(char **arr, int nlen, const char *delim)
{
	char *buf;
	size_t dlen, tlen, i;

	tlen = 0;
	dlen = strlen(delim);
	for (i = 0; i < nlen; i++)
		tlen += strlen(arr[i]) + dlen;

	if ((buf = malloc(tlen)) == NULL)
		err(1, "malloc()");

	*buf = '\0';
        strcpy(buf, arr[0]);
	for (i = 1; i < nlen; i++) {
		strcpy(buf + strlen(buf), delim);
		strcpy(buf + strlen(buf), arr[i]);
	}

	return (buf);
}

[bapt]

In general I like the idea, I don't have a strong feeling about it, but still if we could use open_memstream(3) it would be nicer