src/vipw.c: Superfluous file copy in TCB mode?

[alejandro-colomar]

shadow/src/vipw.c

Lines 414 to 443 in a49d2ac

	#ifdef WITH_TCB
	if (tcb_mode) {
	f = fopen (fileedit, "r");
	if (NULL == f) {
	vipwexit (_("failed to open scratch file"), errno, 1);
	}
	if (unlink (fileedit) != 0) {
	vipwexit (_("failed to unlink scratch file"), errno, 1);
	}
	if (shadowtcb_drop_priv () == SHADOWTCB_FAILURE) {
	vipwexit (_("failed to drop privileges"), errno, 1);
	}
	if (stat (file, &st1) != 0) {
	vipwexit (_("failed to stat edited file"), errno, 1);
	}
	to_rename = aprintf("%s,XXXXXX", file);
	if (to_rename == NULL)
	vipwexit (_("aprintf() failed"), errno, 1);
	if (create_backup_file (f, to_rename, &st1) != 0) {
	free(to_rename);
	vipwexit (_("failed to create backup file"), errno, 1);
	}
	(void) fclose (f);
	} else {
	#endif /* WITH_TCB */
	to_rename = fileedit;
	#ifdef WITH_TCB
	}
	#endif /* WITH_TCB */

I'm trying to understand this code, but can't make sense of it.

• We already created, edited, and closed fileedit, which is the temporary file used by the editor.
• We now open it again, unlink it, drop privileges, and copy the entire file to a new different temporary file.

Later we will rename the temporary file to file.

So, my question is, what is the point of this entire block? Why do we need a second temporary file? Can't we directly rename the temporary file used by the editor, like we do in the non-TCB case? Am I missing some detail inherent to TCB that makes this block necessary or beneficial?

Cc: @stoeckmann , @sem-gh, @ldv-alt

[stoeckmann]

So, my question is, what is the point of this entire block? Why do we need a second temporary file? Can't we directly rename the temporary file used by the editor, like we do in the non-TCB case? Am I missing some detail inherent to TCB that makes this block necessary or beneficial?

In short: File permissions.

The temporary file is located in /etc/tcb/:tmp according to SHADOWTCB_SCRATCHDIR and created and owned by root.
Then, when editing was successful:

1. re-open the temporary file (after it was edited)
2. unlink temporary file owned by root
3. shadowtcb_drop_priv
4. create a new file in /etc/tcb as target user with proper permissions
5. rename that file

Note: While I was tempted to use : as separator in front of XXXXXX as well, I know bash can act really weird if a username resembles a protocol etc. Don't know how I triggered a strange behavior in the past, but since then I avoid :.

[alejandro-colomar]

So, my question is, what is the point of this entire block? Why do we need a second temporary file? Can't we directly rename the temporary file used by the editor, like we do in the non-TCB case? Am I missing some detail inherent to TCB that makes this block necessary or beneficial?

In short: File permissions.

The temporary file is located in /etc/tcb/:tmp according to SHADOWTCB_SCRATCHDIR and created and owned by root. Then, when editing was successful:

1. re-open the temporary file (after it was edited)

2. unlink temporary file owned by root

3. `shadowtcb_drop_priv`

4. create a new file in /etc/tcb as target user with proper permissions

5. rename that file

Hmmm, how about changing the permissions while being root? Wouldn't that be simpler (and more readable) than doing a full file copy?

[stoeckmann]

Definitely simpler, but then we skip tcb_drop_priv and even though we could read the library code as it is today and do permission adjustments on our own, we might skip something that's done in the future (or is done with an older version).

It's not that I like the code as it is. Even though I like the idea of TCB to have finer granularity of access to password hashes by users themselves, having the same restrictions in user names as in filenames is way stricter (and closer to POSIX) than what shadow and others might allow.

In summary: It's TCB. I'm not touching it (mostly due to lack of experience with it). :)

[sem-gh]

Hmmm, how about changing the permissions while being root? Wouldn't that be simpler (and more readable) than doing a full file copy?

Moreover,create_backup_file()already does this by setting the owner/permissions the same as those of the original file. The second backup file is created with dropped privileges, but I'm not sure if that matters in this case.

Definitely simpler, but then we skip tcb_drop_priv and even though we could read the library code as it is today and do permission adjustments on our own, we might skip something that's done in the future (or is done with an older version).

Good point, but perhaps it really would be better to drop the second backup file if the result would be the same without it. I'll try this and test .

[stoeckmann]

Good point, but perhaps it really would be better to drop the second backup file if the result would be the same without it. I'll try this and test .

By any chance, can you test the --prefix option with TCB as well?

Something like:

BASE=$(mktemp -d)
mkdir -p $BASE/etc/tcb
touch $BASE/etc/passwd
useradd --prefix=$BASE testuser

From my understanding of the code, shadow's TCB implementation does not support the --prefix option. I would assume that this new user ends up in /etc/tcb instead.

[sem-gh]

By any chance, can you test the --prefix option with TCB as well?

There's no point in testing it, I already know it doesn't work. With the --prefix option, the TCB support is completely broken and /etc/tcb will be used .

Actually, in the patch I promised to make a long time ago (in #1046 (comment)), that support will be added. Right now, I'm in the process of updating the shadow package in ALT, so I'm in the right context and really hope I'll finally get around to doing what I promised, as well.