Deep Review: 20260519-084740-pr-5013

          <LocalAccount wcm:action="add">
            <Name>limauser</Name>
            <DisplayName>limauser</DisplayName>
            <Group>Administrators</Group>
            <Password>
              <Value>limauser</Value>
              <PlainText>true</PlainText>
            </Password>
          </LocalAccount>
        </LocalAccounts>
      </UserAccounts>

      <AutoLogon>
        <Enabled>true</Enabled>
        <Username>limauser</Username>
        <LogonCount>1</LogonCount>
        <Password>
          <Value>limauser</Value>
          <PlainText>true</PlainText>
        </Password>
      </AutoLogon>

      <OOBE>
        <HideEULAPage>true</HideEULAPage>
        <HideLocalAccountScreen>true</HideLocalAccountScreen>

The Go package encodes passwords (template.xml:138-141, PlainText>false</PlainText>) and the test suite contains an explicit guard (TestGenerate_PasswordNeverPlaintext). The committed manual PoC undoes that guarantee twice: LocalAccount and AutoLogon both carry <Value>limauser</Value> with <PlainText>true</PlainText>. Running the PoC creates an Administrators-group account whose password equals the username, then forwards SSH to 127.0.0.1:60022 via run.sh:110.

        <LocalAccounts>
          <LocalAccount wcm:action="add">
            <Name>{{.Username}}</Name>
            <DisplayName>{{.Username}}</DisplayName>
            <Group>Administrators</Group>
            <Password>
              <Value>{{encodePassword .Password}}</Value>
              <PlainText>false</PlainText>
            </Password>
          </LocalAccount>
        </LocalAccounts>
      </UserAccounts>

      <AutoLogon>

        -blockdev driver=raw,file=cdrom1-storage,node-name=cdrom1,read-only=true \
        -device ide-cd,bus=ide.2,drive=cdrom1 \
        -blockdev driver=file,filename="$CIDATA_ISO",node-name=cdrom2-storage,read-only=true \
        -blockdev driver=raw,file=cdrom2-storage,node-name=cdrom2,read-only=true \
        -device ide-cd,bus=ide.3,drive=cdrom2 \
        -netdev user,id=net0,net=192.168.10.0/24,dhcpstart=192.168.10.15,hostfwd=tcp:127.0.0.1:60022-:22 \
        -device virtio-net-pci,netdev=net0 \
        -chardev socket,id=chr-vu-fs0,path="$SCRIPT_DIR/virtiofsd.sock" \
        -device vhost-user-fs-pci,chardev=chr-vu-fs0,tag=lima \
        -device virtio-rng-pci
}

Fix: generate the PoC answer file from pkg/windows/autounattend at run time instead of maintaining two divergent copies. run.sh can invoke a small Go binary that calls autounattend.Generate before staging the ISO.

		return fmt.Errorf("cidata: finalising ISO: %w", err)
	}
	return nil
}

func detectISOTool() (string, error) {
	for _, candidate := range []string{"genisoimage", "mkisofs", "xorriso"} {
		if p, err := exec.LookPath(candidate); err == nil {
			return p, nil
		}
	}
	return "", fmt.Errorf(
		"cidata: no ISO tool found; install one of: genisoimage, mkisofs, xorriso\n" +
			"  Ubuntu/Debian: sudo apt install genisoimage\n" +
			"  Fedora/RHEL:   sudo dnf install genisoimage",
	)
}

func runISOTool(toolPath, stagingDir, outputPath string) error {
	cmd := exec.Command(toolPath,
		"-output", outputPath,
		"-joliet",
		"-rock",
		"-volid", "cidata",
		stagingDir,
	)
	out, err := cmd.CombinedOutput()
	if err != nil {
		return fmt.Errorf("cidata: %s failed: %w\noutput: %s", filepath.Base(toolPath), err, out)
	}
	return nil
}

Two compounding problems:

• The detection set names xorriso, but the args use the mkisofs emulation dialect that xorriso accepts only with -as mkisofs prepended. Without that, xorriso parses -output as its own command and fails.
• Long-form -output, -joliet, -rock, -volid are not standard for genisoimage or mkisofs — those use -o, -J, -r, -V. The advertised fallback only works on xorrisofs (the mkisofs-emulation binary), which the code does not detect.

The existing helper at pkg/iso9660util/joliet.go:57-67 already handles macOS (hdiutil makehybrid), the correct binary set (xorrisofs, not xorriso), and short flags all three tools accept.

		return fmt.Errorf("failed to run %v: %w (output=%q)", cmd.Args, err, string(b))
	}
	return nil
}

func writeJolietCommand(isoPath, label, workDir string) ([]string, error) {
	if runtime.GOOS == "darwin" {
		return []string{"hdiutil", "makehybrid", "-o", isoPath, "-iso", "-joliet", "-default-volume-name", label, workDir}, nil
	}
	candidates := []string{"xorrisofs", "genisoimage", "mkisofs"}
	for _, cmd := range candidates {
		if cmdAbs, err := exec.LookPath(cmd); err == nil {
			return []string{cmdAbs, "-o", isoPath, "--norock", "-J", "-V", label, workDir}, nil
		}
	}
	return nil, fmt.Errorf("none of %v is available", candidates)
}

Fix: call pkg/iso9660util.Write(..., iso9660util.WithJoliet()) from BuildISO. Drop detectISOTool and runISOTool. Extend the existing helper rather than duplicate it.

      <FirstLogonCommands>
        <SynchronousCommand wcm:action="add">
          <Order>1</Order>
          <RequiresUserInput>false</RequiresUserInput>
          <CommandLine>powershell -NoProfile -NonInteractive -ExecutionPolicy Bypass -File "F:\first_logon.ps1" -SSHKey "{{joinLines .SSHPublicKeys}}"</CommandLine>
        </SynchronousCommand>
        {{range $i, $cmd := .ExtraCommands}}
        <SynchronousCommand wcm:action="add">
          <Order>{{inc $i 1 | inc}}</Order>
          <RequiresUserInput>false</RequiresUserInput>

Three failure modes:

• joinLines joins multiple keys with \n. XML preserves the literal newline; Windows Setup forwards it to cmd.exe, which truncates or rejects multi-line command strings. The single-key test in autounattend_test.go:15 hides the problem.
• An SSH key comment containing ", backtick, or $() survives XML escaping and is interpreted by PowerShell. A malicious or unusual comment can break argument parsing or trigger expression evaluation as Administrator.
• In run.sh, sed substitutes the key through a | delimiter. A key containing |, \, or & corrupts the XML before boot.

func baseConfig() autounattend.Config {
	return autounattend.Config{
		Hostname:            "lima-win",
		Username:            "limauser",
		Password:            "T3stP@ssw0rd!",
		SSHPublicKeys:       []string{"ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAA user@host"},
		WindowsEditionIndex: 1,
	}
}

func TestGenerate_ContainsRequiredPasses(t *testing.T) {

Gemini graded this Critical; Codex and Claude graded it Important. Kept Important because the PR controls both ends; severity rises once external SSH keys are accepted from limayaml.

Fix: stop passing the key on the command line. Stage an authorized_keys file on the cidata ISO. Have first_logon.ps1 read it from $PSScriptRoot and copy it to C:\ProgramData\ssh\administrators_authorized_keys. Removes the quoting hazard, the newline issue, and the sed substitution risk in one change.

		case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9':
			out = append(out, byte(r))
		default:
			out = append(out, '-')
		}
	}
	s := strings.Trim(string(out), "-")
	if len(s) > 15 {
		s = s[:15]
	}
	if s == "" {
		return "lima-windows"
	}
	return s
}

Trim runs once, before truncation. If the 15-byte truncation lands inside a hyphen run, the result has a trailing -. Concrete repro: "abcdefghijklmn-xyz" filters and trims to "abcdefghijklmn-xyz", then [:15] is "abcdefghijklmn-". Windows NetBIOS rejects names ending in -, and Sysprep fails on first boot with the answer file's <ComputerName> rejected.

The test table at autounattend_test.go:91-97 has "this-is-way-too-long-for-netbios" — that input truncates cleanly because the 15th byte is o, not -. The boundary case is untested.

		})
	}
}

func TestSanitiseHostname(t *testing.T) {
	cases := []struct{ in, want string }{
		{"my-vm", "my-vm"},
		{"my_special_vm!", "my-special-vm"},
		{"this-is-way-too-long-for-netbios", "this-is-way-too"},
		{"---", "lima-windows"},
		{"", "lima-windows"},
	}
	for _, tc := range cases {
		got := autounattend.SanitiseHostname(tc.in)
		if got != tc.want {
			t.Errorf("SanitiseHostname(%q) = %q, want %q", tc.in, got, tc.want)
		}

Fix: re-trim after truncation.

 s := strings.Trim(string(out), "-")
 if len(s) > 15 {
-    s = s[:15]
+    s = strings.TrimRight(s[:15], "-")
 }

Add {"abcdefghijklmn-xyz", "abcdefghijklmn"} to the test table.

    $keyFile = Join-Path $sshDir 'administrators_authorized_keys'
    if (-not (Test-Path $sshDir)) {
        New-Item -ItemType Directory -Path $sshDir -Force | Out-Null
    }
    Set-Content -Path $keyFile -Value $SSHKey -Encoding UTF8 -Force
    icacls $keyFile /inheritance:r                | Out-Null
    icacls $keyFile /grant 'SYSTEM:(F)'           | Out-Null
    icacls $keyFile /grant 'Administrators:(F)'   | Out-Null
}

Invoke-Step 'Set default SSH shell to PowerShell' {
    $regPath = 'HKLM:\SOFTWARE\OpenSSH'
    if (-not (Test-Path $regPath)) {

$ErrorActionPreference = 'Stop' does not promote native-executable non-zero exit codes to PowerShell exceptions on Windows PowerShell 5.1 (the default shell on Server 2025). If icacls fails to grant SYSTEM:(F) or choco install winfsp fails (transient network error, package signing change), the enclosing Invoke-Step still prints OK. The script then continues to the next step, register the VirtIO-FS service against a missing WinFSP install. The provisioning log claims success while the guest comes up broken.

Fix: wrap native commands in a helper that checks $LASTEXITCODE immediately.

function Invoke-Native {
    param([string]$FilePath, [string[]]$Arguments)
    & $FilePath @Arguments
    if ($LASTEXITCODE -ne 0) {
        throw "$FilePath exited with $LASTEXITCODE"
    }
}

    New-ItemProperty -Path $regPath -Name DefaultShell `
        -Value 'C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe' `
        -PropertyType String -Force | Out-Null
}

Invoke-Step 'Install WinFSP (required for VirtIO-FS)' {
    [Net.ServicePointManager]::SecurityProtocol =
        [Net.ServicePointManager]::SecurityProtocol -bor [Net.SecurityProtocolType]::Tls12

    $chocoExe = 'C:\ProgramData\chocolatey\bin\choco.exe'
    if (-not (Test-Path $chocoExe)) {
        Set-ExecutionPolicy Bypass -Scope Process -Force
        $installScript = (New-Object Net.WebClient).DownloadString(
            'https://community.chocolatey.org/install.ps1'
        )
        Invoke-Expression $installScript
    }
    & $chocoExe install winfsp -y --pre --no-progress
}

Invoke-Step 'Register and start VirtIO-FS service' {
    $virtiofsBin = 'E:\viofs\2k25\amd64\virtiofs.exe'
    if (-not (Get-Service -Name VirtioFsSvc -ErrorAction SilentlyContinue)) {
        New-Service -Name VirtioFsSvc `

The provisioning script downloads install.ps1 from Chocolatey and runs it with Invoke-Expression, with no checksum, signature, or pinned version. It then installs a pre-release WinFSP package without pinning a specific version. A compromised upstream — or a TLS-interception path during first boot — yields Administrator code execution inside the guest. The PR is the PoC today, but the same script becomes the first-boot path for every Windows guest once pkg/cidata calls into it.

Fix:

• Bundle the WinFSP MSI onto the cidata ISO. Install with msiexec /i ... /quiet.
• If bundling is impossible, download a pinned WinFSP release URL and verify its SHA-256 against a hash compiled into the generator before invoking msiexec. Drop --pre.

import (
	_ "embed"
	"bytes"
	"encoding/base64"
	"fmt"
	"html/template"
	"strings"
)

//go:embed template.xml
var rawTemplate string

html/template applies HTML-context-aware escaping driven by a state machine that does not match XML. For ASCII input the output is correct enough. As soon as a hostname, username, or extra command contains a quote, ampersand, or angle bracket, the escaping drifts — " for ", ' for ', HTML comment handling rather than XML — in ways that downstream XSD validators on some Windows builds reject.

The fields that reach element text (Username, Hostname, ExtraCommands, SSHPublicKeys) are already constrained by validation upstream, so the practical risk today is small. The structural defect is that the package depends on html/template's contextual rules to produce a non-HTML format.

Fix: switch to text/template. Escape XML element text and attribute values explicitly with encoding/xml.EscapeString for the free-form fields.

-"html/template"
+"text/template"

               publicKeyToken="31bf3856ad364e35"
               language="neutral" versionScope="nonSxS"
               xmlns:wcm="http://schemas.microsoft.com/WMIConfig/2002/State">
      <DriverPaths>
        <PathAndCredentials wcm:action="add" wcm:keyValue="1">
          <Path>E:\viostor\2k25\amd64</Path>
        </PathAndCredentials>
        <PathAndCredentials wcm:action="add" wcm:keyValue="2">
          <Path>E:\NetKVM\2k25\amd64</Path>
        </PathAndCredentials>
        <PathAndCredentials wcm:action="add" wcm:keyValue="3">
          <Path>E:\viofs\2k25\amd64</Path>
        </PathAndCredentials>
        <PathAndCredentials wcm:action="add" wcm:keyValue="4">
          <Path>E:\Balloon\2k25\amd64</Path>
        </PathAndCredentials>
        <PathAndCredentials wcm:action="add" wcm:keyValue="5">
          <Path>E:\viorng\2k25\amd64</Path>
        </PathAndCredentials>
      </DriverPaths>
    </component>

    <component name="Microsoft-Windows-International-Core-WinPE"
               processorArchitecture="amd64"

-device ide-cd,bus=ide.1,drive=cdrom0 \   # Windows ISO
-device ide-cd,bus=ide.2,drive=cdrom1 \   # virtio ISO  → guest assigns E:
-device ide-cd,bus=ide.3,drive=cdrom2 \   # cidata ISO  → guest assigns F:

The answer file assumes the virtio driver ISO lands on E: and cidata on F:. Windows assigns drive letters in attach order, but the install ISO often takes the first letter, and any additional disks shift the assignments. The PoC works because run.sh controls attach order exactly. Wiring this into pkg/qemu/qemu.go (the PR's "Next Steps") and adding a user's disk.additionalDisks shifts the letters; installation then hangs at winload.exe because the driver paths no longer resolve.

Fix:

• In windowsPE, replace hard-coded E:\... paths with %CDROM%\... or scan all CD-ROMs via the wcm:keyValue mechanism.
• Set a known volume label (cidata) in BuildISO and resolve the script location at first logon via wmic logicaldisk where VolumeName='cidata' get DeviceID.

    $sshDir  = 'C:\ProgramData\ssh'
    $keyFile = Join-Path $sshDir 'administrators_authorized_keys'
    if (-not (Test-Path $sshDir)) {
        New-Item -ItemType Directory -Path $sshDir -Force | Out-Null
    }
    Set-Content -Path $keyFile -Value $SSHKey -Encoding UTF8 -Force
    icacls $keyFile /inheritance:r                | Out-Null
    icacls $keyFile /grant 'SYSTEM:(F)'           | Out-Null
    icacls $keyFile /grant 'Administrators:(F)'   | Out-Null
}

On Windows PowerShell 5.1, -Encoding UTF8 writes a UTF-8 BOM. The Windows OpenSSH authorized_keys parser treats the BOM as part of the first key's algorithm string, so the key never matches and SSH authentication silently fails. The PoC reports "all steps completed successfully" because the Set-Content itself succeeds, but ssh from the host hangs at password prompt.

Fix: SSH keys are pure ASCII, so encode as ASCII (or UTF8NoBOM on PS 7+, or [IO.File]::WriteAllText with a no-BOM encoding).

-Set-Content -Path $keyFile -Value $SSHKey -Encoding UTF8 -Force
+Set-Content -Path $keyFile -Value $SSHKey -Encoding Ascii -Force

Note: if I3's recommended fix lands (drop the -SSHKey parameter and copy authorized_keys from the cidata ISO), apply the same no-BOM rule to whichever code emits the file.

run_qemu() {
    info "Launching QEMU — Windows installation will begin automatically"
    warn "First boot (OS install) takes 15-30 min depending on your machine"

    qemu-system-x86_64 \
        -name guest=win2k25,debug-threads=on \
        -machine pc-q35-noble,usb=off,vmport=off,dump-guest-core=off,memory-backend=pc.ram,hpet=off,acpi=on \
        -accel kvm \
        -cpu host,migratable=on,hv-time=on,hv-relaxed=on,hv-vapic=on,hv-spinlocks=0x1fff \
        -m size=4096 \
        -object memory-backend-memfd,id=pc.ram,share=true,size=4096M \
        -overcommit mem-lock=off \
        -smp 4,sockets=2,cores=2,threads=1 \

pc-q35-noble is an Ubuntu Noble downstream alias; upstream QEMU on Fedora/Arch/macOS does not register it. -accel kvm is Linux-only. Anyone running the PoC on macOS or a non-Ubuntu Linux distribution hits a QEMU startup error before the install begins. The PR description claims macOS testing is in scope for the production wiring, so the PoC harness should at least start there.

Fix: use the upstream q35 machine type and allow the accelerator to fall back.

--machine pc-q35-noble,usb=off,vmport=off,dump-guest-core=off,memory-backend=pc.ram,hpet=off,acpi=on \
--accel kvm \
+-machine q35,usb=off,vmport=off,dump-guest-core=off,memory-backend=pc.ram,hpet=off,acpi=on \
+-accel kvm -accel hvf -accel tcg \

func (c *Config) validate() error {
	switch {
	case c.Hostname == "":
		return fmt.Errorf("autounattend: Hostname is required")
	case len(c.Hostname) > 15:
		return fmt.Errorf("autounattend: Hostname %q exceeds 15-char NetBIOS limit", c.Hostname)
	case c.Password == "":
		return fmt.Errorf("autounattend: Password is required")
	case len(c.SSHPublicKeys) == 0:
		return fmt.Errorf("autounattend: at least one SSH public key is required")
	case c.WindowsEditionIndex < 1:

len() on a Go string returns the byte count. A caller that bypasses SanitiseHostname and passes "日本サーバ" (15 UTF-8 bytes, 5 runes) passes validation. The template embeds those bytes into <ComputerName>, and Sysprep rejects the name at first boot — NetBIOS names are ASCII-only.

Fix: reject any character outside [A-Za-z0-9-] in validate(), or document that Hostname must be the output of SanitiseHostname and remove the byte-length check from validate so it is the single source of truth.

    sed "s|REPLACE_WITH_YOUR_SSH_PUBLIC_KEY|${ssh_key}|g" \
        "$SCRIPT_DIR/autounattend.xml" > "$TMP_DIR/autounattend.xml"

    cp "$SCRIPT_DIR/first_logon.ps1" "$TMP_DIR/first_logon.ps1"

    "$ISO_TOOL" -output "$CIDATA_ISO" -joliet -rock -volid cidata "$TMP_DIR" \
        2>&1 | grep -v '^$' || true

    [[ -s "$CIDATA_ISO" ]] || fatal "ISO tool produced an empty file"
    info "win-cidata.iso ready ($(du -sh "$CIDATA_ISO" | cut -f1))"
}

create_disk() {
    info "Creating 40 GB qcow2 disk"

The || true after the pipe suppresses every non-zero exit, including a real failure from the ISO tool. The size check only catches an empty output; a tool that exits 1 after writing a partial non-empty file passes the check, and the script proceeds into a confusing Windows install failure 15 minutes later.

Fix: capture the output and check the exit code separately.

local iso_output
if ! iso_output=$("$ISO_TOOL" -output "$CIDATA_ISO" -joliet -rock -volid cidata "$TMP_DIR" 2>&1); then
    printf '%s\n' "$iso_output" | grep -v '^$' || true
    rm -f "$CIDATA_ISO"
    fatal "ISO tool failed"
fi

    qemu-img create -f qcow2 "$QCOW2" 40G -q
}

start_virtiofsd() {
    info "Starting virtiofsd"
    virtiofsd \
        --shared-dir="$SCRIPT_DIR" \
        --socket-path="$SCRIPT_DIR/virtiofsd.sock" \
        --sandbox none &
    echo $! > "$VIRTIOFSD_PID_FILE"
    sleep 1
    [[ -S "$SCRIPT_DIR/virtiofsd.sock" ]] || fatal "virtiofsd socket not created"
}

run_qemu() {
    info "Launching QEMU — Windows installation will begin automatically"

The EXIT trap at run.sh:30 clears the socket on normal exit, but kill -9 or a host crash bypasses traps. The next run finds virtiofsd.sock on disk, virtiofsd refuses to bind, and the script aborts with no path to recovery short of manual cleanup.

        kill "$(cat "$VIRTIOFSD_PID_FILE")" 2>/dev/null || true
        rm -f "$VIRTIOFSD_PID_FILE"
    fi
    rm -f "$SCRIPT_DIR/virtiofsd.sock"
}
trap cleanup EXIT

check_prereqs() {
    local missing=()
    for cmd in qemu-system-x86_64 qemu-img virtiofsd; do
        command -v "$cmd" &>/dev/null || missing+=("$cmd")

Fix: delete the socket file immediately before starting the daemon.

 start_virtiofsd() {
     info "Starting virtiofsd"
+    rm -f "$SCRIPT_DIR/virtiofsd.sock"
     virtiofsd \

	switch {
	case c.Hostname == "":
		return fmt.Errorf("autounattend: Hostname is required")
	case len(c.Hostname) > 15:
		return fmt.Errorf("autounattend: Hostname %q exceeds 15-char NetBIOS limit", c.Hostname)
	case c.Password == "":
		return fmt.Errorf("autounattend: Password is required")
	case len(c.SSHPublicKeys) == 0:
		return fmt.Errorf("autounattend: at least one SSH public key is required")
	case c.WindowsEditionIndex < 1:
		return fmt.Errorf("autounattend: WindowsEditionIndex must be >= 1")
	}

Windows Server 2025 enforces the default local-account password policy: length ≥ 8, three of four character classes (lower/upper/digit/symbol), and no username substring. A caller that supplies "abc" passes validation here, the XML renders, and Setup fails after the user-creation step. The install drops into OOBE with no message back to the Lima host; the diagnosis is buried in Windows Setup logs.

Fix: validate against the default policy in validate() so the caller gets a clear error from Generate instead of a stalled install.

case !meetsWindowsComplexity(c.Password):
    return fmt.Errorf("autounattend: Password does not meet Windows complexity requirements (>=8 chars, three of: lower/upper/digit/symbol)")