v0,28,2

src/tls/external_paths.rs

@@ -0,0 +1,174 @@
+pub fn certificate_files_are_external(cert_file: &str, key_file: &str) -> Result<bool> {
+    let cert_external = Path::new(cert_file).is_absolute();
+    let key_external = Path::new(key_file).is_absolute();
+    ensure!(
+        cert_external == key_external,
+        "certificate and private key must both use filenames from the RustProxy certificate directory or both use absolute external paths"
+    );
+    if cert_external {
+        ensure!(
+            !cert_file.is_empty()
+                && !key_file.is_empty()
+                && cert_file.len() <= MAX_CERTIFICATE_PATH
+                && key_file.len() <= MAX_CERTIFICATE_PATH,
+            "external certificate paths must be absolute paths up to 4096 bytes"
+        );
+        Ok(true)
+    } else {
+        safe_name(cert_file)?;
+        safe_name(key_file)?;
+        Ok(false)
+    }
+}
+
+#[cfg(unix)]
+fn validate_external_parent_chain(path: &Path) -> Result<()> {
+    use std::os::unix::fs::{MetadataExt, PermissionsExt};
+
+    let parent = path
+        .parent()
+        .context("external certificate path has no parent directory")?;
+    for directory in parent.ancestors() {
+        if directory.as_os_str().is_empty() {
+            continue;
+        }
+        let meta = fs::symlink_metadata(directory).with_context(|| {
+            format!(
+                "cannot inspect external certificate parent directory {}",
+                directory.display()
+            )
+        })?;
+        ensure!(
+            meta.is_dir(),
+            "external certificate parent path must contain real directories, not symlinks or special files: {}",
+            directory.display()
+        );
+        let mode = meta.permissions().mode();
+        let owner = meta.uid();
+        let service_uid = unsafe { libc::geteuid() };
+        ensure!(
+            owner == 0 || owner == service_uid,
+            "external certificate parent directory must be owned by root or the RustProxy service user: {}",
+            directory.display()
+        );
+        if mode & 0o022 != 0 {
+            // Root-owned sticky directories such as /tmp and /var/tmp cannot have
+            // entries renamed/unlinked by arbitrary users, so they are safe as an
+            // ancestor of a private service-owned test/custom directory.
+            ensure!(
+                owner == 0 && mode & 0o1000 != 0,
+                "external certificate parent directory must not be group/world-writable unless it is a root-owned sticky directory: {}",
+                directory.display()
+            );
+        }
+    }
+    Ok(())
+}
+
+#[cfg(not(unix))]
+fn validate_external_parent_chain(_path: &Path) -> Result<()> {
+    Ok(())
+}
+
+fn configured_certificate_path(directory: &Path, value: &str) -> Result<PathBuf> {
+    if Path::new(value).is_absolute() {
+        ensure!(
+            !value.is_empty() && value.len() <= MAX_CERTIFICATE_PATH,
+            "external certificate path must be an absolute path up to 4096 bytes"
+        );
+        let configured = Path::new(value);
+        // The configured path itself may be Certbot's final live/*.pem symlink, but
+        // every parent directory must be a stable, non-writable directory. This
+        // prevents a writable parent from redirecting canonicalize() to attacker-
+        // controlled material between validation and open.
+        validate_external_parent_chain(configured)?;
+        let resolved = fs::canonicalize(configured)
+            .with_context(|| format!("cannot resolve external certificate path {value}"))?;
+        // Validate the resolved archive/custom path as well. Certbot's final file
+        // symlink is intentionally followed on each reload; intermediate symlinks
+        // and group/world-writable parent directories are rejected.
+        validate_external_parent_chain(&resolved)?;
+        return Ok(resolved);
+    }
+    safe_name(value)?;
+    Ok(directory.join(value))
+}
+
+fn read_configured_pem(directory: &Path, value: &str, private: bool) -> Result<Zeroizing<Vec<u8>>> {
+    let external = Path::new(value).is_absolute();
+    let path = configured_certificate_path(directory, value)?;
+    read_pem_with_policy(&path, private, external)
+}
+
+fn read_pem(path: &Path, private: bool) -> Result<Zeroizing<Vec<u8>>> {
+    read_pem_with_policy(path, private, false)
+}
+
+fn read_pem_with_policy(
+    path: &Path,
+    private: bool,
+    external_group_read: bool,
+) -> Result<Zeroizing<Vec<u8>>> {
+    let mut options = OpenOptions::new();
+    options.read(true);
+    #[cfg(unix)]
+    {
+        use std::os::unix::fs::OpenOptionsExt;
+        options.custom_flags(libc::O_NOFOLLOW);
+    }
+    let file = options
+        .open(path)
+        .with_context(|| format!("cannot open certificate input {}", path.display()))?;
+    let meta = file.metadata()?;
+    ensure!(
+        meta.is_file() && meta.len() <= MAX_PEM,
+        "certificate inputs must be regular files up to 128 KiB"
+    );
+    #[cfg(unix)]
+    {
+        use std::os::unix::fs::{MetadataExt, PermissionsExt};
+        let mode = meta.permissions().mode();
+        if external_group_read {
+            let owner = meta.uid();
+            let service_uid = unsafe { libc::geteuid() };
+            ensure!(
+                owner == 0 || owner == service_uid,
+                "external certificate/key must be owned by root or the RustProxy service user: {}",
+                path.display()
+            );
+            // POSIX ACL masks are reflected in the traditional group mode bits.
+            // Therefore do not require an exact chmod such as 0600/0640 and do
+            // not reject harmless execute-mask bits. What matters here is that
+            // an untrusted group/other identity cannot modify the material and
+            // that the private key is never readable by "other". A named ACL
+            // such as `u:rustproxy:r--` is intentionally supported.
+            ensure!(
+                mode & 0o022 == 0,
+                "external certificate/key must not be group/world-writable: {}",
+                path.display()
+            );
+            if private {
+                ensure!(
+                    mode & 0o004 == 0,
+                    "external private key must not be world-readable; use a restricted group or POSIX ACL for the rustproxy user: {}",
+                    path.display()
+                );
+            }
+        } else if private {
+            ensure!(
+                mode & 0o077 == 0,
+                "private key permissions must not grant group/world access: {}",
+                path.display()
+            );
+        }
+    }
+    #[cfg(not(unix))]
+    let _ = (private, external_group_read);
+    let mut data = Zeroizing::new(Vec::new());
+    file.take(MAX_PEM + 1).read_to_end(&mut data)?;
+    ensure!(
+        data.len() <= MAX_PEM as usize,
+        "PEM file grew beyond its limit"
+    );
+    Ok(data)
+}

src/tls/tests.rs

@@ -162,68 +162,8 @@ mod tests {
         assert!(certificate_files_are_external("app.pem", "/etc/tls/app.key").is_err());
     }
 
-    #[cfg(unix)]
-    #[test]
-    fn external_absolute_paths_follow_certbot_style_symlinks_on_each_reload() {
-        use std::os::unix::fs::{symlink, PermissionsExt};
+    include!("tests/external_paths.rs");
 
-        let root = tempfile::tempdir().unwrap();
-        let cert_dir = root.path().join("rustproxy-certs");
-        fs::create_dir(&cert_dir).unwrap();
-        initialize_default(&cert_dir).unwrap();
-
-        let archive = root.path().join("archive/example.test");
-        let live = root.path().join("live/example.test");
-        fs::create_dir_all(&archive).unwrap();
-        fs::create_dir_all(&live).unwrap();
-
-        let first = rcgen::generate_simple_self_signed(vec!["example.test".into()]).unwrap();
-        fs::write(archive.join("fullchain1.pem"), first.cert.pem()).unwrap();
-        fs::write(archive.join("privkey1.pem"), first.key_pair.serialize_pem()).unwrap();
-        fs::set_permissions(archive.join("privkey1.pem"), fs::Permissions::from_mode(0o640)).unwrap();
-        symlink("../../archive/example.test/fullchain1.pem", live.join("fullchain.pem")).unwrap();
-        symlink("../../archive/example.test/privkey1.pem", live.join("privkey.pem")).unwrap();
-
-        let cert_path = live.join("fullchain.pem").display().to_string();
-        let key_path = live.join("privkey.pem").display().to_string();
-        assert!(certificate_files_are_external(&cert_path, &key_path).unwrap());
-        let settings = Settings {
-            default_certificate: Some("letsencrypt".into()),
-            certificates: vec![Certificate {
-                issuer: None,
-                id: "letsencrypt".into(),
-                covered_hosts: Vec::new(),
-                hosts: vec!["example.test".into()],
-                cert_file: cert_path.clone(),
-                key_file: key_path.clone(),
-            }],
-            ..Settings::default()
-        };
-        assert!(compile(&settings, Some(&cert_dir)).unwrap().is_some());
-        assert_eq!(
-            certificate_metadata(&cert_dir, &cert_path).unwrap().covered_hosts,
-            vec!["example.test"]
-        );
-
-        let second = rcgen::generate_simple_self_signed(vec![
-            "example.test".into(),
-            "www.example.test".into(),
-        ])
-        .unwrap();
-        fs::write(archive.join("fullchain2.pem"), second.cert.pem()).unwrap();
-        fs::write(archive.join("privkey2.pem"), second.key_pair.serialize_pem()).unwrap();
-        fs::set_permissions(archive.join("privkey2.pem"), fs::Permissions::from_mode(0o640)).unwrap();
-        fs::remove_file(live.join("fullchain.pem")).unwrap();
-        fs::remove_file(live.join("privkey.pem")).unwrap();
-        symlink("../../archive/example.test/fullchain2.pem", live.join("fullchain.pem")).unwrap();
-        symlink("../../archive/example.test/privkey2.pem", live.join("privkey.pem")).unwrap();
-
-        assert!(compile(&settings, Some(&cert_dir)).unwrap().is_some());
-        assert_eq!(
-            certificate_metadata(&cert_dir, &cert_path).unwrap().covered_hosts,
-            vec!["example.test", "www.example.test"]
-        );
-    }
     #[test]
     fn default_is_persistent_and_tls_advertises_both_protocols() {
         let dir = tempfile::tempdir().unwrap();

src/tls/tests/external_paths.rs

@@ -0,0 +1,98 @@
+#[cfg(unix)]
+#[test]
+fn external_absolute_paths_follow_certbot_style_symlinks_on_each_reload() {
+    use std::os::unix::fs::{symlink, PermissionsExt};
+
+    let root = tempfile::tempdir().unwrap();
+    let cert_dir = root.path().join("rustproxy-certs");
+    fs::create_dir(&cert_dir).unwrap();
+    initialize_default(&cert_dir).unwrap();
+
+    let archive = root.path().join("archive/example.test");
+    let live = root.path().join("live/example.test");
+    fs::create_dir_all(&archive).unwrap();
+    fs::create_dir_all(&live).unwrap();
+
+    let first = rcgen::generate_simple_self_signed(vec!["example.test".into()]).unwrap();
+    fs::write(archive.join("fullchain1.pem"), first.cert.pem()).unwrap();
+    fs::write(archive.join("privkey1.pem"), first.key_pair.serialize_pem()).unwrap();
+    fs::set_permissions(archive.join("privkey1.pem"), fs::Permissions::from_mode(0o650)).unwrap();
+    symlink("../../archive/example.test/fullchain1.pem", live.join("fullchain.pem")).unwrap();
+    symlink("../../archive/example.test/privkey1.pem", live.join("privkey.pem")).unwrap();
+
+    let cert_path = live.join("fullchain.pem").display().to_string();
+    let key_path = live.join("privkey.pem").display().to_string();
+    assert!(certificate_files_are_external(&cert_path, &key_path).unwrap());
+    let settings = Settings {
+        default_certificate: Some("letsencrypt".into()),
+        certificates: vec![Certificate {
+            issuer: None,
+            id: "letsencrypt".into(),
+            covered_hosts: Vec::new(),
+            hosts: vec!["example.test".into()],
+            cert_file: cert_path.clone(),
+            key_file: key_path.clone(),
+        }],
+        ..Settings::default()
+    };
+    assert!(compile(&settings, Some(&cert_dir)).unwrap().is_some());
+    assert_eq!(
+        certificate_metadata(&cert_dir, &cert_path).unwrap().covered_hosts,
+        vec!["example.test"]
+    );
+
+    let second = rcgen::generate_simple_self_signed(vec![
+        "example.test".into(),
+        "www.example.test".into(),
+    ])
+    .unwrap();
+    fs::write(archive.join("fullchain2.pem"), second.cert.pem()).unwrap();
+    fs::write(archive.join("privkey2.pem"), second.key_pair.serialize_pem()).unwrap();
+    fs::set_permissions(archive.join("privkey2.pem"), fs::Permissions::from_mode(0o640)).unwrap();
+    fs::remove_file(live.join("fullchain.pem")).unwrap();
+    fs::remove_file(live.join("privkey.pem")).unwrap();
+    symlink("../../archive/example.test/fullchain2.pem", live.join("fullchain.pem")).unwrap();
+    symlink("../../archive/example.test/privkey2.pem", live.join("privkey.pem")).unwrap();
+
+    assert!(compile(&settings, Some(&cert_dir)).unwrap().is_some());
+    assert_eq!(
+        certificate_metadata(&cert_dir, &cert_path).unwrap().covered_hosts,
+        vec!["example.test", "www.example.test"]
+    );
+}
+
+#[cfg(unix)]
+#[test]
+fn external_key_permissions_accept_acl_style_group_mask_but_reject_unsafe_write_or_world_read() {
+    use std::os::unix::fs::PermissionsExt;
+
+    let root = tempfile::tempdir().unwrap();
+    let key = root.path().join("external.key");
+    fs::write(&key, b"not-a-real-key").unwrap();
+
+    // POSIX ACL masks can surface through the traditional group class bits.
+    // Group read/execute is not itself a disclosure when `other` has no read
+    // and neither group nor other can write the file.
+    fs::set_permissions(&key, fs::Permissions::from_mode(0o650)).unwrap();
+    assert!(read_pem_with_policy(&key, true, true).is_ok());
+
+    fs::set_permissions(&key, fs::Permissions::from_mode(0o660)).unwrap();
+    assert!(read_pem_with_policy(&key, true, true).is_err());
+
+    fs::set_permissions(&key, fs::Permissions::from_mode(0o644)).unwrap();
+    assert!(read_pem_with_policy(&key, true, true).is_err());
+}
+
+#[cfg(unix)]
+#[test]
+fn external_paths_reject_writable_non_sticky_parent_directories() {
+    use std::os::unix::fs::PermissionsExt;
+
+    let root = tempfile::tempdir().unwrap();
+    let unsafe_parent = root.path().join("unsafe");
+    fs::create_dir(&unsafe_parent).unwrap();
+    fs::set_permissions(&unsafe_parent, fs::Permissions::from_mode(0o770)).unwrap();
+    let cert = unsafe_parent.join("cert.pem");
+    fs::write(&cert, b"dummy").unwrap();
+    assert!(configured_certificate_path(root.path(), cert.to_str().unwrap()).is_err());
+}

src/tls/types.rs

@@ -330,106 +330,6 @@ pub fn safe_name(name: &str) -> Result<()> {
 /// remain private to `<data-dir>/certs`; an explicitly selected external pair uses
 /// two absolute paths. Mixing the two modes is rejected so the source is never
 /// ambiguous.
-pub fn certificate_files_are_external(cert_file: &str, key_file: &str) -> Result<bool> {
-    let cert_external = Path::new(cert_file).is_absolute();
-    let key_external = Path::new(key_file).is_absolute();
-    ensure!(
-        cert_external == key_external,
-        "certificate and private key must both use filenames from the RustProxy certificate directory or both use absolute external paths"
-    );
-    if cert_external {
-        ensure!(
-            !cert_file.is_empty()
-                && !key_file.is_empty()
-                && cert_file.len() <= MAX_CERTIFICATE_PATH
-                && key_file.len() <= MAX_CERTIFICATE_PATH,
-            "external certificate paths must be absolute paths up to 4096 bytes"
-        );
-        Ok(true)
-    } else {
-        safe_name(cert_file)?;
-        safe_name(key_file)?;
-        Ok(false)
-    }
-}
-
-fn configured_certificate_path(directory: &Path, value: &str) -> Result<PathBuf> {
-    if Path::new(value).is_absolute() {
-        ensure!(
-            !value.is_empty() && value.len() <= MAX_CERTIFICATE_PATH,
-            "external certificate path must be an absolute path up to 4096 bytes"
-        );
-        // Certbot's live/ entries are symlinks to archive/. Resolve them on every
-        // reload, then open the final regular file with O_NOFOLLOW in read_pem.
-        return fs::canonicalize(value)
-            .with_context(|| format!("cannot resolve external certificate path {value}"));
-    }
-    safe_name(value)?;
-    Ok(directory.join(value))
-}
-
-fn read_configured_pem(
-    directory: &Path,
-    value: &str,
-    private: bool,
-) -> Result<Zeroizing<Vec<u8>>> {
-    let external = Path::new(value).is_absolute();
-    let path = configured_certificate_path(directory, value)?;
-    read_pem_with_policy(&path, private, external)
-}
-
-fn read_pem(path: &Path, private: bool) -> Result<Zeroizing<Vec<u8>>> {
-    read_pem_with_policy(path, private, false)
-}
-
-fn read_pem_with_policy(
-    path: &Path,
-    private: bool,
-    external_group_read: bool,
-) -> Result<Zeroizing<Vec<u8>>> {
-    let mut options = OpenOptions::new();
-    options.read(true);
-    #[cfg(unix)]
-    {
-        use std::os::unix::fs::OpenOptionsExt;
-        options.custom_flags(libc::O_NOFOLLOW);
-    }
-    let file = options
-        .open(path)
-        .with_context(|| format!("cannot open certificate input {}", path.display()))?;
-    let meta = file.metadata()?;
-    ensure!(
-        meta.is_file() && meta.len() <= MAX_PEM,
-        "certificate inputs must be regular files up to 128 KiB"
-    );
-    #[cfg(unix)]
-    if private {
-        use std::os::unix::fs::PermissionsExt;
-        let mode = meta.permissions().mode();
-        if external_group_read {
-            ensure!(
-                mode & 0o137 == 0,
-                "external private key may be owner/group-readable but must not be executable, group-writable, or world-accessible: {}",
-                path.display()
-            );
-        } else {
-            ensure!(
-                mode & 0o077 == 0,
-                "private key permissions must not grant group/world access: {}",
-                path.display()
-            );
-        }
-    }
-    #[cfg(not(unix))]
-    let _ = (private, external_group_read);
-    let mut data = Zeroizing::new(Vec::new());
-    file.take(MAX_PEM + 1).read_to_end(&mut data)?;
-    ensure!(
-        data.len() <= MAX_PEM as usize,
-        "PEM file grew beyond its limit"
-    );
-    Ok(data)
-}
 fn der_tlv<'a>(input: &'a [u8], offset: &mut usize) -> Result<(u8, &'a [u8])> {
     ensure!(*offset < input.len(), "truncated DER object");
     let tag = input[*offset];