interfaces.php: remove unsafe unserialize from /tmp; add safe read/write and strict permissions

[SORencber]

Summary

interfaces.php read "/tmp/regdomain.cache" and "/tmp/.interfaces.apply" via unserialize() without security checks. Since /tmp is world‑writable, this allows potential third‑party file injection.
This PR adds a safe deserialization helper, enforces atomic writes with strict permissions, and rejects unsafe inputs.

Key changes

New helper opnsense_safe_tmp_unserialize($path):
Ownership check: accept only files owned by the php‑fpm user (www).
Size limit: reject files larger than 1 MiB (OOB/DoS protection).
allowed_classes=false to disable class instantiation during unserialize.
Returns only arrays; otherwise returns null.

Updated reads:

"/tmp/regdomain.cache" and "/tmp/.interfaces.apply" now read via opnsense_safe_tmp_unserialize(...).

Updated writes:

Write to a temporary file with LOCK_EX → chmod 0600 → atomic rename().
Both regdomain cache and apply list now follow the same atomic/permissions model.

Why this is needed

/tmp is world‑writable (1777). The sticky bit restricts deletion, not file creation.
unserialize() is unsafe for untrusted input (risk of PHP Object Injection/DoS).
This PR adds minimal but effective barriers that meaningfully reduce risk; a full follow‑up is proposed below.

Security impact

Reduces Object Injection and DoS vectors:
Only www‑owned files are accepted.
Oversized files are rejected.
Class instantiation during deserialization is disabled.
Malicious/invalid files are safely ignored.
Backward compatibility
Valid flows are unchanged; only unsafe/malformed inputs are rejected (previously could warn or behave unpredictably).

Signed-off-by: Serhat Ömer Rençber [email protected]

[AdSchellevis]

these legacy files are bound to be removed at some point, I don't expect we want to add more glue in the meantime with a risk of regressions (unless there there is an active exploit option using the gui).

[fichtner]

I see the point but also have the suspicion this regresses more than it should by nature of defensive approach. The locking in the static PHP pages is almost nonexistent and MVC conversion is a good way forward for multiple reasons, however, it’s also one of the most critical and overloaded pages that still exist in the project.

I wonder what we did for gateway_watcher.php and if that approach is safe and can be adapted to provide a safer way of introducing locking and sanity checking as a general concept?

Cheers,
Franco

[SORencber]

Hi Franco,
Agreed on the regression risk and that the static page has little locking. To align with gateway_watcher and keep this safe and low‑risk, I’ll trim this PR to the smallest possible change:
Minimal change
Replace unserialize calls with unserialize(..., ['allowed_classes' => false]) only.
Adopt gateway_watcher-style atomic writes: write to .next, then rename(), no extra locks/permissions.
No helper, no ownership checks, no LOCK_EX/0600 changes.
If deserialization doesn’t yield an array, treat as empty and log a notice (diagnostics only).

Thanks!

[fichtner]

That sounds like a good direction. Although gateway_watcher is a monolithic process we can do write and move, but use a temporary name instead of a fixed name. This will be more apparent in interfaces code where multiple processes could write the file.

[SORencber]

Hi Franco, Ad,
I’ve trimmed the PR to the smallest safe change as discussed:
Replace unserialize(...) with allowed_classes=false and guard non-array results.
Switch writes to a unique temp name + rename() (no LOCK_EX/0600, no helper).
Behaviour otherwise unchanged; aligns with the gateway_watcher write-then-move idea without a fixed temp name.

If further trimming is preferred, I can adjust accordingly. Thanks!

[fichtner]

Thanks!

one more remark/change request is about tempnam() function use which is more or less the idiom we’re using in such cases, e.g.

core/src/etc/inc/system.inc

Line 241 in f4f67d0

$tempfile = tempnam('/tmp', 'resolv.conf');

[SORencber]

Switched to tempnam('/tmp', ...) for temporary files as suggested; still using write-then-rename with unique names. No other behavior changes. best regards.

[fichtner]

Ok, we will discuss on Monday how to proceed, but for me this looks reasonable (minus easily fixable code style issues you don’t need tu worry about).

Cheers,
Franco

[SORencber]

Applied tempnam()+rename() idiom, inlined allowed_classes=false, and cleaned up temp file writes into explicit steps. No functional changes intended. Thanks!

[SORencber]

Hi @fichtner , given this page’s legacy footprint and concurrency concerns, a robust solution should be an MVC migration. I’d like to propose a focused discussion on:
Scope and responsibilities split: apply queue, regdomain cache, interface reconfigure/reset paths.
Storage and consistency: move transient state under a scoped runtime dir (e.g. /var/run/opnsense/interfaces/), JSON format, centralized atomic write helpers.
Concurrency model: single-writer via configd queued actions; predictable recovery on reboot/partial writes.
Backward compatibility: read-compat for one release; deprecate legacy /tmp usage afterwards.
Test plan: PPP/VLAN/LAGG/BRIDGE/VIP flows, apply/rollback, reboot recovery with .next handling, and diagnostics.
If agreeable, I can draft a short RFC/issue with a concrete migration plan and phased PRs so we keep risk low and reviewable. Best Regards.

[fichtner]

Scoping this is complicated for historic reasons and wide ranging impact of „interface“ modeling. I think the prerequisites to this are PPP and wireless conversion to MVC/API. Not sure if all the others are done now? The regdomain issue is likely only related to ancient wireless code for example. In such singular instances and also because we aim for privilege separation (which is currently optional) all such instances of writing management data should be abstracted through configd backend actions. This is regardless of MVC conversion which would require this too.

[SORencber]

Thanks @fichtner — makes sense.
To move safely from the current mitigation toward a maintainable structure,
I’d propose the following phased approach:

Phase 0 —

Planning / RFC: Map responsibilities (apply queue, regdomain cache, reconfigure/reset), define concurrency model (single-writer via configd), and open two tracking issues (configd abstraction, PPP/wireless MVC scope).

Phase 1 —

configd abstraction (low regression): Add backend actions for transient interface data (apply.json, regdomain.json), introduce atomic write helpers, and replace direct FS writes in PHP with configd calls.

Phase 2 —

PPP/Wireless MVC/API (prerequisite): Introduce MVC controllers and API endpoints, gradually replacing legacy handlers and direct file access.

Phase 3 —

Interfaces MVC migration: Add new API controllers (ApplyController, etc.), switch UI logic to API calls, maintain full UI parity, and phase out legacy /tmp paths. Each phase would be small, reviewable, and backwards-compatible during transition.

Best Regards.

[AdSchellevis]

In practice migration is the easiest when there's less code to skim through, for which reason I'm still not a huge fan of adding a lot of failsafe code when there's no clear problem to be fixed.

It would make sense to try to move calls to configd if we could make then sustainable for future steps, but as this item is not on our roadmap at the moment (https://opnsense.org/roadmap/), I still believe we have to choose our battles wisely. Time spend on legacy pages can not be spend elsewhere.

Very minor additions (like adding 'allowed_classes' => false) are obviously fine to add, as these are easy to test and less likely to regress.

[fichtner]

@SORencber I wrote 215b723 for the occasion. This helps to simplify the code here. I don't see much value in checking bytes written by file_put_contents() as it should land an error in the log when unmuted (no @) and writing zero bytes can be a valid use case (truncating the file for another process).

[fichtner]

wouldn't mute file_get_contents() as we know the file exists in 99.9% if the cases since we checked for it :)

for unserialize() I'm more or less leaning the same way as it has never caused harm before that I could think of

[fichtner]

Suggested change

	$parsed_xml = @unserialize(@file_get_contents('/tmp/regdomain.cache'), ['allowed_classes' => false]);
	if (!is_array($parsed_xml)) {
	$parsed_xml = array();
	}
	$parsed_xml = unserialize(file_get_contents('/tmp/regdomain.cache'), ['allowed_classes' => false]) ?? [];

if it's not an array beforehand it should still error later IMO

[fichtner]

Suggested change

	$tmpFile = tempnam('/tmp', 'regdomain.cache.');
	if ($tmpFile !== false) {
	$bytesWritten = file_put_contents($tmpFile, serialize($parsed_xml));
	if ($bytesWritten !== false) {
	rename($tmpFile, '/tmp/regdomain.cache');
	}
	}
	file_safe('/tmp/regdomain.cache', serialize($parsed_xml));

etc

[SORencber]

util.inc: add optional hardening to file_safe() while keeping defaults

• Add opt-in options: durable (fsync), base_dir, reject_symlink,
  require_regular, force_path/force_data, source_base_dir, max_bytes,
  owner/group, tmp_in_target/tmp_dir
• Default behaviour unchanged; existing call sites unaffected
• Improves safety for callers that need path hardening and durable writes

Refs: 215b723 (introduce file_safe helper), discussion in PR #9259

function file_safe($file, $text_or_source = '', $mode = 0644, array $options = [])
{
    /*
     * Safe file write helper with optional hardening.
     *
     * Default behaviour matches the original minimal helper:
     * - create temp file
     * - if $text_or_source is an existing file path, copy it; otherwise write contents
     * - chmod() and atomic rename() to $file
     *
     * Options (all optional, default off):
     * - durable (bool): use fflush()+fsync() for stronger persistence guarantees
     * - base_dir (string): enforce that target directory resolves under this base
     * - reject_symlink (bool): reject when $file is a symbolic link
     * - require_regular (bool): when $file exists, require it to be a regular file
     * - force_path (bool): treat $text_or_source strictly as a path to copy
     * - force_data (bool): treat $text_or_source strictly as data to write
     * - source_base_dir (string): if copying from a path, enforce it resides under this base
     * - max_bytes (int): if copying from a path, reject when source size exceeds this limit
     * - owner (int|string): chown() on the temp file before rename
     * - group (int|string): chgrp() on the temp file before rename
     * - tmp_in_target (bool): create temp file in target directory when possible
     * - tmp_dir (string): fallback temp directory (default '/tmp')
     */

    $durable = !empty($options['durable']);
    $baseDir = isset($options['base_dir']) ? $options['base_dir'] : null;
    $rejectSymlink = !empty($options['reject_symlink']);
    $requireRegular = !empty($options['require_regular']);
    $forcePath = !empty($options['force_path']);
    $forceData = !empty($options['force_data']);
    $sourceBaseDir = isset($options['source_base_dir']) ? $options['source_base_dir'] : null;
    $maxBytes = isset($options['max_bytes']) ? (int)$options['max_bytes'] : null;
    $owner = array_key_exists('owner', $options) ? $options['owner'] : null;
    $group = array_key_exists('group', $options) ? $options['group'] : null;
    $tmpInTarget = !empty($options['tmp_in_target']);
    $tmpDir = isset($options['tmp_dir']) ? $options['tmp_dir'] : '/tmp';

    /* basic path validation */
    if (!is_string($file) || $file === '' || $file[0] !== DIRECTORY_SEPARATOR) {
        return false;
    }

    $targetDir = dirname($file);
    $targetBaseName = basename($file);

    $realTargetDir = realpath($targetDir);
    if ($realTargetDir === false) {
        return false;
    }

    if ($baseDir !== null) {
        $realBaseDir = realpath($baseDir);
        if ($realBaseDir === false) {
            return false;
        }
        if (strncmp($realTargetDir . DIRECTORY_SEPARATOR, $realBaseDir . DIRECTORY_SEPARATOR, strlen($realBaseDir) + 1) !== 0) {
            return false;
        }
    }

    if ($rejectSymlink && is_link($file)) {
        return false;
    }

    if ($requireRegular && file_exists($file)) {
        $st = @lstat($file);
        if (!is_array($st)) {
            return false;
        }
        /* ensure target is a regular file if it exists */
        if (($st['mode'] & 0170000) !== 0100000) {
            return false;
        }
    }

    /* choose temp directory */
    $tempDir = $tmpInTarget ? $realTargetDir : $tmpDir;
    if (!is_dir($tempDir) || !is_writable($tempDir)) {
        /* fallback to target directory if specified temp dir is not usable */
        $tempDir = $realTargetDir;
    }

    $temp = @tempnam($tempDir, str_replace('/', '_', $file) . '-');
    if ($temp === false) {
        return false;
    }

    /* write data or copy from source path */
    $treatAsPath = ($forcePath || (!$forceData && is_string($text_or_source) && file_exists($text_or_source)));

    if ($treatAsPath) {
        $sourcePath = $text_or_source;

        if ($sourceBaseDir !== null) {
            $realSourceBase = realpath($sourceBaseDir);
            $realSource = realpath($sourcePath);
            if ($realSourceBase === false || $realSource === false ||
                strncmp(dirname($realSource) . DIRECTORY_SEPARATOR, $realSourceBase . DIRECTORY_SEPARATOR, strlen($realSourceBase) + 1) !== 0) {
                @unlink($temp);
                return false;
            }
        }

        if ($maxBytes !== null) {
            $size = @filesize($sourcePath);
            if ($size !== false && $size > $maxBytes) {
                @unlink($temp);
                return false;
            }
        }

        if (!@copy($sourcePath, $temp)) {
            @unlink($temp);
            return false;
        }
    } elseif (is_string($text_or_source) && strlen($text_or_source)) {
        $fp = @fopen($temp, 'wb');
        if ($fp === false) {
            @unlink($temp);
            return false;
        }
        if (@fwrite($fp, $text_or_source) === false) {
            fclose($fp);
            @unlink($temp);
            return false;
        }
        if (!@fflush($fp)) {
            fclose($fp);
            @unlink($temp);
            return false;
        }
        if ($durable && function_exists('fsync') && !@fsync($fp)) {
            fclose($fp);
            @unlink($temp);
            return false;
        }
        fclose($fp);
    } else {
        /* empty content: truncate to zero; tempnam already created the file */
        if ($durable && function_exists('fsync')) {
            $fp = @fopen($temp, 'ab');
            if ($fp !== false) {
                @fflush($fp);
                @fsync($fp);
                fclose($fp);
            }
        }
    }

    /* apply permissions/ownership before rename */
    @chmod($temp, $mode);
    if ($owner !== null) {
        @chown($temp, $owner);
    }
    if ($group !== null) {
        @chgrp($temp, $group);
    }

    if (!@rename($temp, $file)) {
        @unlink($temp);
        return false;
    }

    if ($durable && function_exists('fsync')) {
        $fp2 = @fopen($file, 'rb');
        if ($fp2 !== false) {
            @fsync($fp2);
            fclose($fp2);
        }
    }

    return true;
}

[fichtner]

Errr, we can talk about improving file_safe() but I don't want to bloat it as it becomes less and less interesting for quick and easy file creation. From experience most bad things never happen or they happen in spades but then the system has other issues.

As for rolling back changes and file creations it doesn't really matter in the grand scheme of things and it also makes debugging harder. Ideally a lot of logging would help, but further bloat the code. Fsync can also impact performance on larger files and I don't know of many instances where it was very strictly necessary (one exception is the update procedure where worst case hundreds of thousands of files are created which may make one unavailable for a split second causing an error here or there.

Let's keep the scope of the PR as it was and do a separate PR for file_safe()?

[SORencber]

Thanks for the feedback. I agree to keep this PR focused. I’ll revert/remove any file_safe() changes from this PR and push an update. I’ll open a separate issue/PR to discuss file_safe() improvements (e.g., optional fsync for critical paths, clearer error surfacing) with some measurements, keeping overhead minimal. If you have specific constraints or a preferred direction, happy to follow.