Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions crates/lib/src/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -562,9 +562,15 @@ impl State {
}

fn stateroot(&self) -> &str {
// CLI takes precedence over config file
self.config_opts
.stateroot
.as_deref()
.or_else(|| {
self.install_config
.as_ref()
.and_then(|c| c.stateroot.as_deref())
})
Comment on lines +569 to +573
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

The stateroot value can now be supplied via the installation configuration file. This value is used to construct file system paths (e.g., ostree/deploy/{stateroot}) and is passed to libostree functions without validation. A malicious value containing path traversal sequences (e.g., ..) could be used to manipulate files outside the intended directory on the target system during installation. It is recommended to validate that the stateroot value is a simple alphanumeric string and does not contain path traversal sequences or directory separators.

.unwrap_or(ostree_ext::container::deploy::STATEROOT_DEFAULT)
}
}
Expand Down Expand Up @@ -2233,7 +2239,12 @@ pub(crate) async fn install_to_filesystem(
// We support overriding the mount specification for root (i.e. LABEL vs UUID versus
// raw paths).
// We also support an empty specification as a signal to omit any mountspec kargs.
let root_info = if let Some(s) = fsopts.root_mount_spec {
// CLI takes precedence over config file.
let config_root_mount_spec = state
.install_config
.as_ref()
.and_then(|c| c.root_mount_spec.as_ref());
let root_info = if let Some(s) = fsopts.root_mount_spec.as_ref().or(config_root_mount_spec) {
Comment on lines +2243 to +2247
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

The root_mount_spec value, which can now be supplied via the installation configuration file, is used to construct the root= kernel argument without proper validation. Because the kernel command line is a space-separated list of arguments, an attacker providing a malicious configuration file can include spaces in the root_mount_spec value to inject arbitrary additional kernel arguments (e.g., selinux=0, init=/bin/sh) into the target system. It is recommended to validate that the root_mount_spec value does not contain whitespace.

RootMountInfo {
mount_spec: s.to_string(),
kargs: Vec::new(),
Expand Down Expand Up @@ -2314,7 +2325,12 @@ pub(crate) async fn install_to_filesystem(
let device_info = bootc_blockdev::partitions_of(Utf8Path::new(&backing_device))?;

let rootarg = format!("root={}", root_info.mount_spec);
let mut boot = if let Some(spec) = fsopts.boot_mount_spec {
// CLI takes precedence over config file.
let config_boot_mount_spec = state
.install_config
.as_ref()
.and_then(|c| c.boot_mount_spec.as_ref());
let mut boot = if let Some(spec) = fsopts.boot_mount_spec.as_ref().or(config_boot_mount_spec) {
Comment on lines +2329 to +2333
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

The boot_mount_spec value, which can now be supplied via the installation configuration file, is used to construct an entry in the target system's /etc/fstab file without validation. An attacker can include newlines or extra fields in this value to inject arbitrary entries or malicious mount options into the target system's fstab. It is recommended to validate that the boot_mount_spec value does not contain newlines or unexpected whitespace.

// An empty boot mount spec signals to omit the mountspec kargs
// See https://github.com/bootc-dev/bootc/issues/1441
if spec.is_empty() {
Expand Down
92 changes: 92 additions & 0 deletions crates/lib/src/install/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,13 @@ pub(crate) struct InstallConfiguration {
pub(crate) match_architectures: Option<Vec<String>>,
/// Ostree repository configuration
pub(crate) ostree: Option<OstreeRepoOpts>,
/// The stateroot name to use. Defaults to `default`
pub(crate) stateroot: Option<String>,
/// Source device specification for the root filesystem.
/// For example, `UUID=2e9f4241-229b-4202-8429-62d2302382e1` or `LABEL=rootfs`.
pub(crate) root_mount_spec: Option<String>,
/// Mount specification for the /boot filesystem.
pub(crate) boot_mount_spec: Option<String>,
}

fn merge_basic<T>(s: &mut Option<T>, o: Option<T>, _env: &EnvProperties) {
Expand Down Expand Up @@ -150,6 +157,9 @@ impl Mergeable for InstallConfiguration {
merge_basic(&mut self.block, other.block, env);
self.filesystem.merge(other.filesystem, env);
self.ostree.merge(other.ostree, env);
merge_basic(&mut self.stateroot, other.stateroot, env);
merge_basic(&mut self.root_mount_spec, other.root_mount_spec, env);
merge_basic(&mut self.boot_mount_spec, other.boot_mount_spec, env);
if let Some(other_kargs) = other.kargs {
self.kargs
.get_or_insert_with(Default::default)
Expand Down Expand Up @@ -630,6 +640,7 @@ bls-append-except-default = "console=ttyS0"
let other = InstallConfiguration {
ostree: Some(OstreeRepoOpts {
bls_append_except_default: Some("console=tty0".to_string()),
..Default::default()
}),
..Default::default()
};
Expand All @@ -639,4 +650,85 @@ bls-append-except-default = "console=ttyS0"
"console=tty0"
);
}

#[test]
fn test_parse_stateroot() {
let c: InstallConfigurationToplevel = toml::from_str(
r#"[install]
stateroot = "custom"
"#,
)
.unwrap();
assert_eq!(c.install.unwrap().stateroot.unwrap(), "custom");
}

#[test]
fn test_merge_stateroot() {
let env = EnvProperties {
sys_arch: "x86_64".to_string(),
};
let mut install: InstallConfiguration = toml::from_str(
r#"stateroot = "original"
"#,
)
.unwrap();
let other = InstallConfiguration {
stateroot: Some("newroot".to_string()),
..Default::default()
};
install.merge(other, &env);
assert_eq!(install.stateroot.unwrap(), "newroot");
}

#[test]
fn test_parse_mount_specs() {
let c: InstallConfigurationToplevel = toml::from_str(
r#"[install]
root-mount-spec = "LABEL=rootfs"
boot-mount-spec = "UUID=abcd-1234"
"#,
)
.unwrap();
let install = c.install.unwrap();
assert_eq!(install.root_mount_spec.unwrap(), "LABEL=rootfs");
assert_eq!(install.boot_mount_spec.unwrap(), "UUID=abcd-1234");
}

#[test]
fn test_merge_mount_specs() {
let env = EnvProperties {
sys_arch: "x86_64".to_string(),
};
let mut install: InstallConfiguration = toml::from_str(
r#"root-mount-spec = "UUID=old"
boot-mount-spec = "UUID=oldboot"
"#,
)
.unwrap();
let other = InstallConfiguration {
root_mount_spec: Some("LABEL=newroot".to_string()),
..Default::default()
};
install.merge(other, &env);
// root_mount_spec should be overridden
assert_eq!(install.root_mount_spec.as_deref().unwrap(), "LABEL=newroot");
// boot_mount_spec should remain unchanged
assert_eq!(install.boot_mount_spec.as_deref().unwrap(), "UUID=oldboot");
}

/// Empty mount specs are valid and signal to omit mount kargs entirely.
/// See https://github.com/bootc-dev/bootc/issues/1441
#[test]
fn test_parse_empty_mount_specs() {
let c: InstallConfigurationToplevel = toml::from_str(
r#"[install]
root-mount-spec = ""
boot-mount-spec = ""
"#,
)
.unwrap();
let install = c.install.unwrap();
assert_eq!(install.root_mount_spec.as_deref().unwrap(), "");
assert_eq!(install.boot_mount_spec.as_deref().unwrap(), "");
}
}
13 changes: 13 additions & 0 deletions docs/src/man/bootc-install-config.5.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@ The `install` section supports these subfields:
- `kargs`: An array of strings; this will be appended to the set of kernel arguments.
- `match_architectures`: An array of strings; this filters the install config.
- `ostree`: See below.
- `stateroot`: The stateroot name to use. Defaults to `default`.
- `root-mount-spec`: A string specifying the root filesystem mount specification.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's debate this one a bit. I am OK to keep this as is under the concept that we're basically just supporting what bootc install already does.

But most cases now ideally use the DPS. When would one want to hardcode a different UUID?

For your use case you just need to support omitting it right? We could start with just root-karg-omit: true or so?

But I dunno.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But most cases now ideally use the DPS. When would one want to hardcode a different UUID?

I guess some people may need to, in some very specific cases? but yeah, I agree that simply having a omit flag probably works for most of the cases. It's also required for DPS, IIUC.

Also we recently moved to DPS in coreos-assembler as well, so it's worth investigating if that would work as-is.

For example, `UUID=2e9f4241-229b-4202-8429-62d2302382e1` or `LABEL=rootfs`.
If not provided, the UUID of the target filesystem will be used.
An empty string signals to omit boot mount kargs entirely.
- `boot-mount-spec`: A string specifying the /boot filesystem mount specification.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one heavily relates to #1388 among others - we need to change/strengthen how we do /boot in general.

Are you also just omitting this value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you also just omitting this value?

in the FCOS case yes, as we expect to find the partition with it's label during first boot.

If not provided and /boot is a separate mount, its UUID will be used.
An empty string signals to omit boot mount kargs entirely.

# filesystem

Expand All @@ -51,8 +59,13 @@ Configuration options for the ostree repository. There is one valid field:
```toml
[install.filesystem.root]
type = "xfs"

[install]
kargs = ["nosmt", "console=tty0"]
stateroot = "myos"
root-mount-spec = "LABEL=rootfs"
boot-mount-spec = "UUID=abcd-1234"

[install.ostree]
bls-append-except-default = 'grub_users=""'
```
Expand Down