Avoid TOCTOU race when serving console assets

[david-crespo]

Closes #927

I was cleaning up old issues linked to the prod eng huddle and figured I could knock this out. This is out of my area to feel confident in on my own, but the code, tests, and summary pass the sniff test for me. Basically this does what Dave recommended in #927 (comment), blocking symlinks segment by segment rather than checking for them once up front.

🤖 summary

find_file used to walk the requested path doing a symlink_metadata check on each component, then return a Utf8PathBuf that a later File::open re-resolved from scratch — so every check could be invalidated between check and use. Now find_file walks the path with openat and returns the open file. Nothing is ever re-resolved by pathname: every property we care about is either enforced by the kernel atomically at open time or checked by fstat on the already-open fd.

• Each component is opened with openat(dirfd, segment, O_RDONLY | O_NOFOLLOW | O_CLOEXEC) relative to the directory fd from the previous step. Since each openat gets exactly one component, O_NOFOLLOW applies to every component: a symlink at any position fails the open. The no-symlink check and the open are the same syscall, so there is no window to swap in a symlink after the check. illumos documents the failure as ELOOP, same as Linux (macOS reports ENOTDIR when O_DIRECTORY is also set; either way we 404):

  https://github.com/illumos/illumos-gate/blob/a293484/usr/src/man/man2/open.2#L292-L298

• Once a directory is open, renaming it doesn't matter: the next openat resolves through the fd, which is pinned to the inode. The scenario from the issue — b passes the check, then is swapped for a symlink before c/d resolve — now just means we keep reading from the original b, which we hold open.

• Intermediate components also pass O_DIRECTORY, so "must be a directory" is likewise enforced by the kernel at open time rather than by a separate stat.

• find_file now validates segments itself: it iterates Utf8Path::components() and 404s anything that isn't a Normal component. This matters more than it used to, because a .. reaching openat would escape the root and an absolute path would ignore the dirfd entirely. Dropshot rejects .. before we get here (covered by existing integration tests), but find_file no longer assumes it.

• The final component gets O_NONBLOCK so the open can't hang if the path names a FIFO; it's a no-op for regular files.

The new test test_find_file_race_does_not_escape_root demonstrates the fix directly: open a file via find_file, replace its parent directory with a symlink pointing outside the static root, then read. The returned handle yields the original contents; the old code, re-opening the checked path, followed the swapped symlink out of the root and fails this test.

[david-crespo]

The tokio docs recommend against blocking like this ("This line could block. It is not recommended to do this on the Tokio runtime."), but we were already doing it before. Not sure if it's worth wrapping the find_file calls in spawn_blocking.

What the spawn_blocking change would look like

diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs
index d6c84ec88e..24b86ead61 100644
--- a/nexus/src/external_api/console_api.rs
+++ b/nexus/src/external_api/console_api.rs
@@ -374,15 +374,18 @@
         .get(http::header::ACCEPT_ENCODING)
         .and_then(|v| v.to_str().ok())
         .unwrap_or_default();
-    let file = match accept_gz(accept_encoding)
-        .then(|| find_file(&with_gz_ext(&path), static_dir))
-    {
-        Some(Ok(gzipped_file)) => {
+    let gzipped_file = if accept_gz(accept_encoding) {
+        find_file_async(with_gz_ext(&path), static_dir.to_owned()).await.ok()
+    } else {
+        None
+    };
+    let file = match gzipped_file {
+        Some(gzipped_file) => {
             resp = resp
                 .header(http::header::CONTENT_ENCODING, CONTENT_ENCODING_GZIP);
             gzipped_file
         }
-        _ => find_file(&path, static_dir)?,
+        None => find_file_async(path.to_owned(), static_dir.to_owned()).await?,
     };
 
     let file = File::from_std(file);
@@ -439,6 +442,19 @@
     HttpError::for_not_found(None, internal_msg.to_string())
 }
 
+/// Run [`find_file`] on the blocking thread pool so its synchronous open
+/// syscalls don't block the runtime thread.
+async fn find_file_async(
+    path: Utf8PathBuf,
+    root_dir: Utf8PathBuf,
+) -> Result<std::fs::File, HttpError> {
+    tokio::task::spawn_blocking(move || find_file(&path, &root_dir))
+        .await
+        .map_err(|e| {
+            HttpError::for_internal_error(format!("error finding file: {e}"))
+        })?
+}
+
 /// Open `path` beneath `root_dir` without following symlinks. Reject paths
 /// containing anything other than normal segments (e.g., `..` or a leading
 /// `/`). Dropshot is expected to have rejected those already, but we don't