GHSA-WCMJ-X466-56MM: Symlink-Safe Provider Cache Extraction Review

Vulnerability

GHSA-WCMJ-X466-56MM describes an arbitrary file write condition during provider extraction in OpenTofu. The vulnerable flow accepted a preexisting target path and used os.Stat(targetDir), which follows symlinks, before deciding whether an existing package could be reused. In the affected scenario, an attacker who can pre-seed the provider cache/workspace path with a UNIX symbolic link can redirect extraction or overwrite behavior outside the intended workspace/cache root.

The core issue is not package authenticity but filesystem object trust. The installer treated the presence of an existing path as a normal reuse case and then compared hashes, but the initial path inspection followed symlinks. That makes the target path attacker-influenced at the filesystem layer. The advisory summary and patch series indicate the impact is arbitrary file write outside the workspace when provider extraction encounters a malicious symlink at the install destination.

// vulnerable behavior excerpt
if _, err := os.Stat(targetDir); err == nil {
    fileHash, fileErr := PackageHashV1(meta.Location)
    if targetHash == fileHash && fileErr == nil && targetErr == nil {
        // Package is properly installed
    }
}

Relevant sources: PR #4082, PR #4087, PR #4088, PR #4089, and the GitHub Security Advisory.

Patch

The patch hardens the preexisting-target logic in internal/getproviders/package_location_local_archive.go by replacing os.Stat with os.Lstat. This is the key security change because Lstat inspects the directory entry itself rather than the symlink target. The new logic explicitly treats any existing directory entry at targetDir as something to validate before proceeding.

// patched behavior excerpt
if info, err := os.Lstat(targetDir); err == nil {
    log.Printf("[TRACE] There's already a directory entry at %s, so we'll check if it matches our expectations", targetDir)
    isEmptyDir := info.IsDir() && targetHash == emptyPackageHashV1
    if !isEmptyDir {
        fileHash, fileErr := PackageHashV1(meta.Location)
        var err error
        if fileErr != nil {
            err = fmt.Errorf("failed to calculate checksum for temporary copy of provider package at %s: %s", meta.Location.String(), fileErr)
        } else if targetErr != nil {
            err = fmt.Errorf("failed to calculate checksum for existing cached provider package at %s: %s", targetDir, targetErr)
        } else if targetHash != fileHash {
            err = fmt.Errorf("existing cached package at %s does not match the content of the downloaded package; does it contain local modifications?", targetDir)
        }
        if err != nil {
            tracing.SetSpanError(span, err)
            return authResult, err
        }
    }
}

Two supporting changes matter:

• An emptyPackageHashV1 constant was added in internal/getproviders/hash.go, with a corresponding test in internal/getproviders/hash_test.go. This enables a narrow compatibility exception for a directly precreated empty directory.
• End-to-end tests were expanded in internal/command/e2etest/providers_tamper_test.go, including coverage for a conflicting preexisting directory and, per the test name shown in the diff, a symlink case. The changelog also documents the new fail-closed behavior for conflicting cache entries in PR #4088 and PR #4089.

Technically, the patch changes the trust model from overwrite-or-reuse to validate-or-fail. A direct empty directory is allowed, but a symlink to an empty directory is explicitly disallowed by the info.IsDir() check on the Lstat result.

Review

Pros

• The switch from os.Stat to os.Lstat is the right primitive for a symlink-following bug. It addresses the filesystem root cause rather than only checking content after dereference.
• The new logic is fail-closed for preexisting non-matching entries. Instead of silently clobbering, it returns a clear error: existing cached package ... does not match the content of the downloaded package.
• The special-case allowance is narrow. Only a direct empty directory is accepted, and the code comment explicitly excludes a symlink to an empty directory.
• The patch preserves legitimate cache reuse by comparing package hashes when an entry already exists, reducing regression risk for normal plugin cache workflows.
• Added tests around empty-directory handling and cache tampering improve confidence that the new behavior is intentional and stable across future refactors.

Cons

• The provided snippets show validation at the target directory entry, but they do not demonstrate broader path-walk hardening for parent directories. If any parent component in the extraction path can be attacker-controlled symlinks, that would need separate review outside the shown diff.
• The patch appears scoped to local archive installation logic in package_location_local_archive.go. If other extraction paths or installers exist, equivalent symlink-safe checks must also be present there to ensure uniform protection.
• The empty-directory exception relies on the hash of the existing target resolving to the canonical empty package hash. That is reasonable, but it introduces a behavioral branch that should remain well-tested because filesystem races or platform-specific directory semantics can complicate emptiness checks.
• The snippets do not show explicit anti-TOCTOU protections between Lstat, hashing, and subsequent extraction. The main exploit class here is mitigated by refusing symlink entries up front, but race resistance is not fully assessable from the supplied material.

Verdict

Root-cause.

This patch directly addresses the vulnerable trust boundary by stopping symlink-following at the initial existence check and by refusing conflicting preexisting entries instead of overwriting them. The use of os.Lstat is the decisive fix for the advisory's symlink redirection mechanism, while checksum comparison and explicit erroring harden the installer against unsafe reuse. Based on the supplied diffs and advisory context, this is a source-grounded root-cause fix for the reported arbitrary file write vector in the provider cache target path, with the caveat that full repository-wide assurance would still require confirming similar extraction code paths are equally hardened.

References: https://github.com/opentofu/opentofu/pull/4082, https://github.com/opentofu/opentofu/pull/4087, https://github.com/opentofu/opentofu/pull/4088, https://github.com/opentofu/opentofu/pull/4089, https://github.com/advisories/GHSA-WCMJ-X466-56MM.