security(xtask): web --serve serves only plain files inside the site; linear asset globs; exclusive lock owner file (#3682)

* security(xtask): web --serve serves only plain files inside the site; linear asset globs; exclusive lock owner file

* address review: say so when the ci lock owner file cannot be written

xtask/src/assets.rs

@@ -21,15 +21,24 @@ pub fn glob(pat: &str, path: &str) -> bool {
     if let Some(dir) = pat.strip_suffix('/') {
         return path.starts_with(dir) && path[dir.len()..].starts_with('/');
     }
-    fn rec(p: &[u8], s: &[u8]) -> bool {
-        match (p.first(), s.first()) {
-            (None, None) => true,
-            (Some(b'*'), _) => rec(&p[1..], s) || (!s.is_empty() && s[0] != b'/' && rec(p, &s[1..])),
-            (Some(a), Some(b)) if a == b => rec(&p[1..], &s[1..]),
-            _ => false,
+    // (pattern, path) positions already found not to match: each is tried once, so overlapping
+    // `*`s can't make the search exponential
+    fn rec(p: &[u8], s: &[u8], failed: &mut std::collections::HashSet<(usize, usize)>) -> bool {
+        if failed.contains(&(p.len(), s.len())) {
+            return false;
         }
+        let ok = match (p.first(), s.first()) {
+            (None, None) => true,
+            (Some(b'*'), _) => rec(&p[1..], s, failed) || (!s.is_empty() && s[0] != b'/' && rec(p, &s[1..], failed)),
+            (Some(a), Some(b)) if a == b => rec(&p[1..], &s[1..], failed),
+            _ => false,
+        };
+        if !ok {
+            failed.insert((p.len(), s.len()));
+        }
+        ok
     }
-    rec(pat.as_bytes(), path.as_bytes())
+    rec(pat.as_bytes(), path.as_bytes(), &mut std::collections::HashSet::new())
 }
 
 fn backticked(cell: &str) -> impl Iterator<Item = &str> {
@@ -117,6 +126,13 @@ mod tests {
         assert!(glob("docs/images/*-tetons.jpg", "docs/images/ba-tetons.jpg"));
         assert!(!glob("docs/images/*.jpg", "docs/images/sub/x.jpg"));
         assert!(glob("crates/scenes/", "crates/scenes/a/b.png"));
+        // forty `*` against a long name that fails at its very end: exponential without the memo
+        let pat = format!("{}x", "*a".repeat(40));
+        let name = "a".repeat(200);
+        let t = std::time::Instant::now();
+        assert!(!glob(&pat, &name));
+        assert!(t.elapsed().as_secs() < 5, "took {:?}", t.elapsed());
+        assert!(glob("*a*b*", "xaybz") && !glob("*a*b*", "xa/b"));
         assert!(!glob("crates/scenes/", "crates/scenesx/b.png"));
     }
 

xtask/src/ci_lock.rs

@@ -74,7 +74,17 @@ pub fn try_acquire(dir: &Path, slots: usize, owner: &str) -> Result<Option<Held>
             Ok(()) => {
                 // a sidecar file, not the lock file: Windows locks are mandatory, so others couldn't read it
                 let owner_file = owner_path(&path);
-                let _ = std::fs::File::create(&owner_file).and_then(|mut f| f.write_all(owner.as_bytes()));
+                // a new file: an entry left under the name (a link planted in a shared lock folder)
+                // is removed, never written through
+                let removed = std::fs::remove_file(&owner_file);
+                let written = OpenOptions::new().write(true).create_new(true).open(&owner_file).and_then(|mut f| f.write_all(owner.as_bytes()));
+                // when the file could be neither replaced nor written it may still name an earlier
+                // holder: say so rather than let `holders` report it as this run's slot's owner
+                if let (Err(e), Err(w)) = (&removed, &written)
+                    && e.kind() != std::io::ErrorKind::NotFound
+                {
+                    eprintln!("ci lock: could not record the owner in {} ({e}; {w}): waiting runs may show a stale owner", owner_file.display());
+                }
                 return Ok(Some(Held { _file: file, owner: owner_file }));
             }
             Err(TryLockError::WouldBlock) => continue,

xtask/src/web.rs

@@ -232,6 +232,21 @@ fn serve_dir(dir: &Path, port: u16) -> Result<(), String> {
     Ok(())
 }
 
+/// Whether a request path names a file inside the site folder: relative, made of plain names
+/// only (no `..`, no root, no `C:` drive or other `:`, which `Path::join` on Windows would take as
+/// the whole path).
+fn safe_site_path(rel: &str) -> bool {
+    !rel.contains(['\\', ':']) && Path::new(rel).components().all(|c| matches!(c, std::path::Component::Normal(_)))
+}
+
+/// A regular file's bytes (a symlink, folder or device under the name is refused, not read).
+fn read_site_file(path: &Path) -> std::io::Result<Vec<u8>> {
+    if !std::fs::symlink_metadata(path)?.is_file() {
+        return Err(std::io::Error::other("not a file"));
+    }
+    std::fs::read(path)
+}
+
 fn handle(mut stream: TcpStream, dir: &Path) -> std::io::Result<()> {
     let mut reader = BufReader::new(stream.try_clone()?);
     let mut line = String::new();
@@ -253,11 +268,14 @@ fn handle(mut stream: TcpStream, dir: &Path) -> std::io::Result<()> {
     let rel = path.trim_start_matches('/');
     let rel = if rel.is_empty() { "index.html" } else { rel };
     let file = dir.join(rel);
-    let safe = !rel.split('/').any(|c| c == ".." || c.contains('\\'));
+    let safe = safe_site_path(rel);
     let encoding = if safe { negotiate(&accept_encoding, |suffix| dir.join(format!("{rel}{suffix}")).is_file()) } else { None };
-    let read = match encoding {
-        Some((_, suffix)) => std::fs::read(dir.join(format!("{rel}{suffix}"))),
-        None => std::fs::read(&file),
+    // only a plain file of the site, read after the path check: a device or a path outside the
+    // folder is never opened
+    let read = match (safe, encoding) {
+        (false, _) => Err(std::io::Error::other("unsafe path")),
+        (true, Some((_, suffix))) => read_site_file(&dir.join(format!("{rel}{suffix}"))),
+        (true, None) => read_site_file(&file),
     };
     let (status, body, ctype) = match (method, safe, read) {
         ("GET" | "HEAD", true, Ok(b)) => ("200 OK", b, mime(&file)),
@@ -288,6 +306,14 @@ fn handle(mut stream: TcpStream, dir: &Path) -> std::io::Result<()> {
 mod tests {
     use super::*;
 
+    #[test]
+    fn served_paths_stay_inside_the_site() {
+        assert!(safe_site_path("index.html") && safe_site_path("pkg/lightcraft.js"));
+        for bad in ["../Cargo.toml", "a/../../x", "/etc/passwd", "C:/Windows/win.ini", "C:\\x", "a\\..\\b", "./x"] {
+            assert!(!safe_site_path(bad), "{bad}");
+        }
+    }
+
     #[test]
     fn bindgen_version_is_locked() {
         let v = locked_bindgen_version().unwrap();