SQL Oddities

[craigfrancis]

I don't think these are exploitable, so no need to rush.

In daemon.go, batchScheduleNewVersions(), there is a database query that includes a comment with the version name, but it has been concatenated directly into the SQL string:

https://github.com/SjonHortensius/phpshell/blob/dee4a4ab55c94429c3379c4bf5d8e1dbb679bb49/daemon.go#L554-L556

The name comes from the version table, which allows 24 characters.

---

The version table is populated via VersionUpdate.php, which worries me in the sense that it's basically using string concatenation... but the only untrusted value is the name from php.net/releases, where I'd prefer it to be checked with something like:

    if (preg_match('/^([0-9]+)\.([0-9]+)\.([0-9]+)$/', $name, $matches)) {
        [$x, $vMajor, $vMinor, $vRelease] = $matches;
    } else {
        fprintf(STDERR, "[%s] name cannot be parsed\n", $name);
        continue;
    }

---

I'd also note that there are a couple of SQL queries that aren't using a literal-string (psalm/phpStan) for the $query, and should really be providing these variables separately, via $parameters.

https://github.com/SjonHortensius/phpshell/blob/dee4a4ab55c94429c3379c4bf5d8e1dbb679bb49/library/PhpShell/Action/Cli/VersionUpdate.php#L107-L113

https://github.com/SjonHortensius/phpshell/blob/dee4a4ab55c94429c3379c4bf5d8e1dbb679bb49/library/PhpShell/Action/Cli/VersionUpdate.php#L153

[SjonHortensius]

Thanks, I agree. Thankfully these were in debug/backend code but they shouldn't be there. Fixed

[craigfrancis]

Thanks for updating; and yes, if it had been something to worry about I'd have emailed you directly/privately.