ensure 0644 perms for /etc/pam.d/system

[cocoa-xu]

Hi, I noticed that the file permission of /etc/pam.d/system is incorrect when I was using the minimal disk image. It prompts that:

2024-01-28T07:59:02.046161+00:00 - login 83 - - in openpam_check_desc_owner_perms(): /etc/pam.d/system: insecure ownership or permissions
2024-01-28T07:59:02.059751+00:00 - login 83 - - pam_start(): System error

And if I log out the system, I can never log in back.

This PR tries to fix this issue by ensuring the file permission of /etc/pam.d/system to be 0644 when building the rootfs. After applying this patch, I can log out and log in again without any issue.

Logging in as root...
2024-01-28T09:27:25.194438+00:00 - login 83 - - login on console as root
exec /bin/sh
# ^D

CheriBSD/arm64 (Amnesiac) (ttyu0)

login: root
2024-01-28T09:32:54.297221+00:00 - login 86 - - login on ttyu0 as root
2024-01-28T09:32:54.305668+00:00 - login 86 - - ROOT LOGIN (root) ON ttyu0
#

[jrtc27]

Doing this as a one-off for /etc/pam.d/system sounds wrong, surely we should set the right permissions for other files too, i.e. give create_file_for_image a sensible default?

[cocoa-xu]

Hi @jrtc27, I'm not sure if give create_file_for_image a default would break other things, and it might be okay to add this one-off for the reasons below.

Firstly, there're already quite a few one-offs in the codebase, such as:

cheribuild/pycheribuild/projects/disk_image.py

Lines 356 to 379 in af47e1d

	self.create_file_for_image("/sbin/qemu-mount-rootfs.sh", contents=mount_rootfs_script,
	mode=0o755, show_contents_non_verbose=False)
	mount_sources_script = include_local_file("files/cheribsd/qemu-mount-sources.sh.in").format(
	SRCPATH=self.config.source_root, ROOTFS_DIR=self.rootfs_dir)
	self.create_file_for_image("/sbin/qemu-mount-sources.sh", contents=mount_sources_script,
	mode=0o755, show_contents_non_verbose=False)
	do_reroot_script = include_local_file("files/cheribsd/qemu-do-reroot.sh.in").format(
	SRCPATH=self.config.source_root, ROOTFS_DIR=self.rootfs_dir)
	self.create_file_for_image("/sbin/qemu-do-reroot.sh", contents=do_reroot_script,
	mode=0o755, show_contents_non_verbose=False)
	self.create_file_for_image("/sbin/startup-benchmark.sh", mode=0o755, show_contents_non_verbose=False,
	contents=include_local_file("files/cheribsd/startup-benchmark.sh"))
	# Add a script to launch gdb, run a program and get a backtrace:
	self.create_file_for_image("/usr/bin/gdb-run.sh", contents=include_local_file("files/cheribsd/gdb-run.sh"),
	mode=0o755, show_contents_non_verbose=False)
	# And another one for non-interactive use:
	self.create_file_for_image("/usr/bin/gdb-run-noninteractive.sh",
	contents=include_local_file("files/cheribsd/gdb-run-noninteractive.sh"),
	mode=0o755, show_contents_non_verbose=False)
	# Add a script to turn of network and stop running services:
	self.create_file_for_image("/usr/bin/prepare-benchmark-environment.sh",
	contents=include_local_file("files/cheribsd/prepare-benchmark-environment.sh"),
	mode=0o755, show_contents_non_verbose=False)

cheribuild/pycheribuild/projects/disk_image.py

Line 421 in af47e1d

self.create_file_for_image("/root/.ssh/authorized_keys", contents=contents, mode=0o600)

cheribuild/pycheribuild/projects/disk_image.py

Line 440 in af47e1d

self.create_file_for_image("/boot/loader.conf", contents=loader_conf_contents, mode=0o644)

Meanwhile, there're also a dozen of calls to this function didn't specify the desired file permissions, i.e.,

cheribuild/pycheribuild/projects/disk_image.py

Lines 309 to 329 in af47e1d

	self.create_file_for_image("/etc/fstab", contents=fstab_contents, show_contents_non_verbose=True)
	# enable ssh and set hostname
	# TODO: use separate file in /etc/rc.conf.d/ ?
	rc_conf_contents = self.file_templates.get_rc_conf_template().format(hostname=self.hostname)
	self.create_file_for_image("/etc/rc.conf", contents=rc_conf_contents, show_contents_non_verbose=False)
	cshrc_contents = self.file_templates.get_cshrc_template().format(SRCPATH=self.config.source_root,
	ROOTFS_DIR=self.rootfs_dir)
	self.create_file_for_image("/etc/csh.cshrc", contents=cshrc_contents)
	# Basic .bashrc/.bash_profile template
	dot_bashrc_contents = self.file_templates.get_dot_bashrc_template().format(SRCPATH=self.config.source_root,
	ROOTFS_DIR=self.rootfs_dir)
	self.create_file_for_image("/root/.bashrc", contents=dot_bashrc_contents)
	self.create_file_for_image("/usr/share/skel/dot.bashrc", contents=dot_bashrc_contents)
	dot_bash_profile_contents = self.file_templates.get_dot_bash_profile_template().format(
	SRCPATH=self.config.source_root,
	ROOTFS_DIR=self.rootfs_dir)
	self.create_file_for_image("/root/.bash_profile", contents=dot_bash_profile_contents)
	self.create_file_for_image("/usr/share/skel/dot.bash_profile", contents=dot_bash_profile_contents)

And for these files that without being explicitly set permissions, they will be created by

cheribuild/pycheribuild/filesystemutils.py

Line 215 in af47e1d

with file.open("w", encoding="utf-8") as f:

which sets default permissions based on current umask value.

And that leads to the final reason that, if the umask value is changed somewhere else during the build, and they depend on this default behaviour, then giving create_file_for_image a default could cause incorrect file permissions.

[arichardson]

Since there aren't that many calls I think requiring the mode argument probably makes sense and avoids problems like this in the future

[arichardson]

@cocoa-xu would you be able to update this PR with add an explicit argument added to the other calls as well?

[cocoa-xu]

@cocoa-xu would you be able to update this PR with add an explicit argument added to the other calls as well?

Sure thing! I'll do that in a minute

[jrtc27]

You seem to have typo'ed 644 as 664 in some places

[cocoa-xu]

Hi @jrtc27, sorry I was copying file permissions from the system image built by the script. They should all be corrected now, but please do let me know if there're any more mistakes, and I'll correct them as soon as possible.

And I guess this also suggests that these files have wrong file permissions set all the time, and we indeed need to explicitly specify their permissions.

[arichardson]

Thanks this looks good now, @jrtc27 do you agree?

I can remove the default argument in a future change.

[jrtc27]

LGTM subject to cleaning up the commit message on squash