Skip to content

fix: bat security #658

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Yundi339 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: bat security #658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 14 additions & 9 deletions src_assets/windows/misc/gamepad/install-gamepad.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,18 +48,23 @@ for /f "tokens=3" %%a in ('reg query "HKCU\Software\Microsoft\Windows\CurrentVer
rem get browser_download_url from asset 0 of https://api.github.com/repos/nefarius/vigembus/releases/latest
set latest_release_url=https://api.github.com/repos/nefarius/vigembus/releases/latest

rem Use curl to get the api response, and find the browser_download_url.
rem `--connect-timeout 10 --max-time 20` ensures we don't hang for minutes if
rem GitHub or the local network is unreachable during install.
for /F "tokens=* USEBACKQ" %%F in (`curl -s --connect-timeout 10 --max-time 20 !proxy! -L %latest_release_url% ^| findstr browser_download_url`) do (
set browser_download_url=%%F
rem Step 1: download release metadata via curl (preserves the existing proxy
rem support through !proxy!). Saving to disk avoids piping a multi-megabyte
rem JSON payload through cmd.exe's narrow `for /F` token buffer.
set "release_json=%temp_dir%\vigembus_release.json"
curl -f -s -L --connect-timeout 10 --max-time 20 !proxy! -o "%release_json%" "%latest_release_url%"
if errorlevel 1 (
echo ERROR: Could not fetch ViGEmBus release metadata.
exit /b 1
)

rem Strip quotes
set browser_download_url=%browser_download_url:"=%
rem Step 2: parse the JSON via PowerShell ConvertFrom-Json instead of fragile
rem findstr + substring stripping. The previous approach silently produced an
rem invalid URL if the JSON layout shifted, or if any asset name contained
rem characters that confused `set` parsing.
for /F "tokens=* USEBACKQ delims=" %%F in (`powershell -NoProfile -Command "try { $r = Get-Content -LiteralPath '%release_json%' -Raw ^| ConvertFrom-Json; $a = $r.assets ^| Where-Object { $_.name -like '*.exe' } ^| Select-Object -First 1; if ($a -and $a.browser_download_url) { $a.browser_download_url } } catch { }"`) do set "browser_download_url=%%F"
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# 只读验证：定位是否仍存在将批处理变量内联到 PowerShell 单引号字符串的模式
rg -n -C2 --iglob "*.bat" "Get-Content -LiteralPath '%[A-Za-z0-9_]+%'"

Repository: AlkaidLab/foundation-sunshine

Length of output: 860

🏁 Script executed:

cat -n src_assets/windows/misc/gamepad/install-gamepad.bat

Repository: AlkaidLab/foundation-sunshine

Length of output: 4892

避免在 PowerShell 命令字符串中直接嵌入批处理路径变量。

Line 65 将 %release_json% 直接拼接到 PowerShell -Command 参数的单引号字符串内。当该路径包含单引号字符时（如通过网络路径或特殊环境），会破坏 PowerShell 字符串边界，导致解析失败甚至命令注入风险。建议改用环境变量传参方案，由 PowerShell 本地解析环境变量，避免 cmd 层面的嵌入歧义。

建议修改 
 set "release_json=%temp_dir%\vigembus_release.json"
 curl -f -s -L --connect-timeout 10 --max-time 20 !proxy! -o "%release_json%" "%latest_release_url%"
 if errorlevel 1 (
   echo ERROR: Could not fetch ViGEmBus release metadata.
   exit /b 1
 )
 
 rem Step 2: parse the JSON via PowerShell ConvertFrom-Json instead of fragile
 rem findstr + substring stripping. The previous approach silently produced an
 rem invalid URL if the JSON layout shifted, or if any asset name contained
 rem characters that confused `set` parsing.
+set "RELEASE_JSON=%release_json%"
-for /F "tokens=* USEBACKQ delims=" %%F in (`powershell -NoProfile -Command "try { $r = Get-Content -LiteralPath '%release_json%' -Raw ^| ConvertFrom-Json; $a = $r.assets ^| Where-Object { $_.name -like '*.exe' } ^| Select-Object -First 1; if ($a -and $a.browser_download_url) { $a.browser_download_url } } catch { }"`) do set "browser_download_url=%%F"
+for /F "tokens=* USEBACKQ delims=" %%F in (`powershell -NoProfile -Command "try { $r = Get-Content -LiteralPath $env:RELEASE_JSON -Raw ^| ConvertFrom-Json; $a = $r.assets ^| Where-Object { $_.name -like '*.exe' } ^| Select-Object -First 1; if ($a -and $a.browser_download_url) { $a.browser_download_url } } catch { }"`) do set "browser_download_url=%%F"
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src_assets/windows/misc/gamepad/install-gamepad.bat` at line 65, The for /F
line embeds the batch variable %release_json% directly into the PowerShell
-Command string, which breaks when the path contains quotes and is a
security/quoting risk; instead export the path into an environment variable
(e.g., set "RELEASE_JSON=%release_json%") and then invoke PowerShell reading
$env:RELEASE_JSON inside the -Command so PowerShell parses the path natively and
you avoid injecting cmd quoting into the command; update the for /F invocation
that sets browser_download_url and the surrounding logic to use the environment
variable (and remove direct %release_json% usage) so Get-Content uses
$env:RELEASE_JSON safely.


rem Remove the browser_download_url key
set browser_download_url=%browser_download_url:browser_download_url: =%
del /q "%release_json%" >nul 2>&1

if "%browser_download_url%"=="" (
echo ERROR: Could not resolve ViGEmBus download URL.
Expand Down
16 changes: 11 additions & 5 deletions src_assets/windows/misc/migration/migrate-config.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,12 @@ if exist "%OLD_DIR%\covers\" (
if not exist "%NEW_DIR%\covers\" (
move "%OLD_DIR%\covers" "%NEW_DIR%\"

rem Fix apps.json image path values that point at the old covers directory
powershell -c "(Get-Content '%NEW_DIR%\apps.json').replace('.\/covers\/', '.\/config\/covers\/') | Set-Content '%NEW_DIR%\apps.json'"
rem Fix apps.json image path values that point at the old covers directory.
rem Pass the path via environment to PowerShell and use -LiteralPath to avoid
rem PowerShell code injection if the install path contains characters like ' or $.
set "MIGRATE_APPS_JSON=%NEW_DIR%\apps.json"
powershell -NoProfile -Command "$p = $env:MIGRATE_APPS_JSON; (Get-Content -LiteralPath $p).Replace('.\/covers\/', '.\/config\/covers\/') | Set-Content -LiteralPath $p"
set "MIGRATE_APPS_JSON="
Comment on lines +58 to +63
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

缺少 apps.json 存在性保护会触发无意义报错

Line 62 在 apps.json 不存在但 covers 存在时仍会执行替换逻辑，导致 PowerShell 报错。建议先判断文件是否存在再执行替换。

🔧 建议修改 
-        set "MIGRATE_APPS_JSON=%NEW_DIR%\apps.json"
-        powershell -NoProfile -Command "$p = $env:MIGRATE_APPS_JSON; (Get-Content -LiteralPath $p).Replace('.\/covers\/', '.\/config\/covers\/') | Set-Content -LiteralPath $p"
-        set "MIGRATE_APPS_JSON="
+        if exist "%NEW_DIR%\apps.json" (
+            set "MIGRATE_APPS_JSON=%NEW_DIR%\apps.json"
+            powershell -NoProfile -Command "$p = $env:MIGRATE_APPS_JSON; (Get-Content -LiteralPath $p).Replace('.\/covers\/', '.\/config\/covers\/') | Set-Content -LiteralPath $p"
+            set "MIGRATE_APPS_JSON="
+        )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
rem Fix apps.json image path values that point at the old covers directory.
rem Pass the path via environment to PowerShell and use -LiteralPath to avoid
rem PowerShell code injection if the install path contains characters like ' or $.
set "MIGRATE_APPS_JSON=%NEW_DIR%\apps.json"
powershell -NoProfile -Command "$p = $env:MIGRATE_APPS_JSON; (Get-Content -LiteralPath $p).Replace('.\/covers\/', '.\/config\/covers\/') | Set-Content -LiteralPath $p"
set "MIGRATE_APPS_JSON="
rem Fix apps.json image path values that point at the old covers directory.
rem Pass the path via environment to PowerShell and use -LiteralPath to avoid
rem PowerShell code injection if the install path contains characters like ' or $.
if exist "%NEW_DIR%\apps.json" (
set "MIGRATE_APPS_JSON=%NEW_DIR%\apps.json"
powershell -NoProfile -Command "$p = $env:MIGRATE_APPS_JSON; (Get-Content -LiteralPath $p).Replace('.\/covers\/', '.\/config\/covers\/') | Set-Content -LiteralPath $p"
set "MIGRATE_APPS_JSON="
)
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src_assets/windows/misc/migration/migrate-config.bat` around lines 58 - 63,
The batch currently runs the PowerShell replacement unconditionally which causes
errors when apps.json is missing; change migrate-config.bat so that after
setting MIGRATE_APPS_JSON it checks for file existence and only runs the
PowerShell Replace when the file exists (e.g. use a conditional check like if
exist "%MIGRATE_APPS_JSON%" ... or inside PowerShell use Test-Path -LiteralPath
$p before performing the (Get-Content ...).Replace(... ) | Set-Content
-LiteralPath $p); keep the existing use of MIGRATE_APPS_JSON and the
-LiteralPath usage and still clear MIGRATE_APPS_JSON afterwards.

)
)

Expand All @@ -69,6 +73,8 @@ if exist "%NEW_DIR%\apps.json" (
powershell -ExecutionPolicy Bypass -File "%~dp0migrate-images.ps1" "%NEW_DIR%"
)

rem Remove log files
del "%OLD_DIR%\*.txt"
del "%OLD_DIR%\*.log"
rem Remove legacy Sunshine log files left at the install root by older versions.
rem Restrict to known patterns instead of all *.txt / *.log to avoid clobbering
rem user-placed files (e.g. notes, third-party README's) in the install dir.
if exist "%OLD_DIR%\sunshine.log" del /q "%OLD_DIR%\sunshine.log" >nul 2>&1
for %%F in ("%OLD_DIR%\sunshine.log.*") do if exist "%%~fF" del /q "%%~fF" >nul 2>&1
21 changes: 18 additions & 3 deletions src_assets/windows/misc/vsink/install-vsink.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,25 @@ if %errorLevel% equ 0 (
:: Set variables
set "installer=VBCABLE_Driver_Pack43.zip"
set "download_url=https://download.vb-audio.com/Download_CABLE/VBCABLE_Driver_Pack43.zip"
set "temp_dir=%TEMP%\vb_cable_install"

:: Create temp directory
if not exist "%temp_dir%" mkdir "%temp_dir%"
:: Use an unpredictable temp directory under the admin user's %TEMP% to defeat
:: any pre-positioned binary at a guessable path. %TEMP% under an elevated
:: shell is the admin's own profile (not world-writable), but using a fresh
:: random directory eliminates the residual TOCTOU window between mkdir and
:: VBCABLE_Setup_x64.exe launch.
for /F "usebackq delims=" %%R in (`powershell -NoProfile -Command "[guid]::NewGuid().ToString('N')"`) do set "RAND_ID=%%R"
if "%RAND_ID%"=="" set "RAND_ID=%RANDOM%%RANDOM%%RANDOM%"
set "temp_dir=%TEMP%\sunshine-vbcable-%RAND_ID%"

:: Create temp directory (start clean; refuse to proceed if pre-existing path
:: cannot be removed, in case an attacker pre-created a hardlink/junction).
if exist "%temp_dir%" rd /s /q "%temp_dir%"
if exist "%temp_dir%" (
echo ERROR: Could not prepare temp directory: %temp_dir%
pause
exit /b 1
)
mkdir "%temp_dir%"

:: Download installer
echo Downloading VB-Cable driver...
Expand Down
Loading