Skip to content
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
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
1 change: 1 addition & 0 deletions images/chromium-headful/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ RUN set -eux; \
wget ca-certificates python2 supervisor xclip xdotool \
pulseaudio dbus-x11 xserver-xorg-video-dummy \
libcairo2 libxcb1 libxrandr2 libxv1 libopus0 libvpx7 \
x11-xserver-utils \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need this for xrandr

gstreamer1.0-plugins-base gstreamer1.0-plugins-good \
gstreamer1.0-plugins-bad gstreamer1.0-plugins-ugly \
gstreamer1.0-pulseaudio gstreamer1.0-omx; \
Expand Down
24 changes: 23 additions & 1 deletion images/chromium-headful/run-docker.sh
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,29 @@ CHROMIUM_FLAGS="${CHROMIUM_FLAGS:-$CHROMIUM_FLAGS_DEFAULT}"
rm -rf .tmp/chromium
mkdir -p .tmp/chromium
FLAGS_FILE="$(pwd)/.tmp/chromium/flags"
echo "$CHROMIUM_FLAGS" > "$FLAGS_FILE"

# Convert space-separated flags to JSON array format, handling quoted strings
# Use eval to properly parse quoted strings (respects shell quoting)
if [ -n "$CHROMIUM_FLAGS" ]; then
eval "FLAGS_ARRAY=($CHROMIUM_FLAGS)"
else
FLAGS_ARRAY=()
fi

FLAGS_JSON='{"flags":['
FIRST=true
for flag in "${FLAGS_ARRAY[@]}"; do
if [ -n "$flag" ]; then
if [ "$FIRST" = true ]; then
FLAGS_JSON+="\"$flag\""
FIRST=false
else
FLAGS_JSON+=",\"$flag\""
fi
fi
done
FLAGS_JSON+=']}'
echo "$FLAGS_JSON" > "$FLAGS_FILE"
Copy link

Choose a reason for hiding this comment

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

Bug: Flag Parsing Vulnerability Causes JSON Malformation

The script's logic for converting CHROMIUM_FLAGS into a JSON array doesn't properly handle flag values containing spaces or JSON special characters. This can result in malformed JSON and introduces a shell injection vulnerability.

Fix in Cursor Fix in Web

Copy link

Choose a reason for hiding this comment

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

Bug: Chromium Flag Parsing and JSON Formatting Errors

The new logic for converting Chromium flags to JSON has two issues. First, IFS=' ' read -ra incorrectly splits flags that contain spaces within quotes (e.g., --user-agent="My Custom Agent"). Second, if $CHROMIUM_FLAGS is empty or contains only spaces, the resulting JSON is malformed as {"flags":]} instead of {"flags":[]}.

Additional Locations (1)

Fix in Cursor Fix in Web

Copy link

Choose a reason for hiding this comment

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

Bug: Shell Code Injection via User-Controlled Flags

The eval "FLAGS_ARRAY=($CHROMIUM_FLAGS)" command in both scripts introduces a command injection vulnerability. Since CHROMIUM_FLAGS can be user-controlled, malicious input could lead to arbitrary shell code execution.

Additional Locations (1)

Fix in Cursor Fix in Web

Copy link
Contributor Author

Choose a reason for hiding this comment

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

again, not user facing and this is fine

Copy link

Choose a reason for hiding this comment

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

Bug: JSON Flag Parsing Fails on Special Characters

The JSON generation for Chromium flags doesn't escape special characters within flag values. If a flag contains double quotes, backslashes, or other JSON special characters, the resulting JSON will be invalid. This could cause the downstream flag parsing system to fail or misinterpret flags.

Additional Locations (1)

Fix in Cursor Fix in Web


echo "flags file: $FLAGS_FILE"
cat "$FLAGS_FILE"
Expand Down
27 changes: 26 additions & 1 deletion images/chromium-headful/run-unikernel.sh
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,32 @@ CHROMIUM_FLAGS="${CHROMIUM_FLAGS:-$chromium_flags_default}"
rm -rf .tmp/chromium
mkdir -p .tmp/chromium
FLAGS_DIR=".tmp/chromium"
echo "$CHROMIUM_FLAGS" > "$FLAGS_DIR/flags"

# Convert space-separated flags to JSON array format, handling quoted strings
# Use eval to properly parse quoted strings (respects shell quoting)
if [ -n "$CHROMIUM_FLAGS" ]; then
eval "FLAGS_ARRAY=($CHROMIUM_FLAGS)"
else
FLAGS_ARRAY=()
fi
Copy link

Choose a reason for hiding this comment

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

Bug: Environment Variable Injection Vulnerability

The use of eval "FLAGS_ARRAY=($CHROMIUM_FLAGS)" in both scripts introduces a command injection vulnerability. Since CHROMIUM_FLAGS can be set via environment variables, malicious input could lead to arbitrary shell code execution.

Additional Locations (1)

Fix in Cursor Fix in Web

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is fine. It is not for production use case

Copy link

Choose a reason for hiding this comment

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

Bug: Shell Injection via CHROMIUM_FLAGS

The eval command in run-unikernel.sh and run-docker.sh parses CHROMIUM_FLAGS. This creates a security vulnerability, allowing arbitrary command injection if CHROMIUM_FLAGS contains malicious shell code, as it can be controlled by user input or environment variables.

Additional Locations (1)

Fix in Cursor Fix in Web


FLAGS_JSON='{"flags":['
FIRST=true
for flag in "${FLAGS_ARRAY[@]}"; do
if [ -n "$flag" ]; then
if [ "$FIRST" = true ]; then
FLAGS_JSON+="\"$flag\""
FIRST=false
else
FLAGS_JSON+=",\"$flag\""
fi
fi
Copy link

Choose a reason for hiding this comment

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

Bug: Shell Command Injection via eval

The eval command used to parse CHROMIUM_FLAGS in both run-unikernel.sh and run-docker.sh introduces a command injection vulnerability. If CHROMIUM_FLAGS contains malicious shell commands, eval will execute them, potentially leading to arbitrary code execution.

Additional Locations (1)

Fix in Cursor Fix in Web

done
FLAGS_JSON+=']}'
echo "$FLAGS_JSON" > "$FLAGS_DIR/flags"
Copy link

Choose a reason for hiding this comment

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

Bug: JSON Parsing Fails on Quoted Flags

The new JSON flag conversion logic in run-unikernel.sh incorrectly parses CHROMIUM_FLAGS. It fails to properly handle flags containing spaces (like quoted arguments) and does not escape JSON special characters (quotes, backslashes). This results in malformed JSON, preventing Chromium from starting and creating a shell injection vulnerability.

Fix in Cursor Fix in Web


echo "flags file: $FLAGS_DIR/flags"
cat "$FLAGS_DIR/flags"

# Re-create the volume from scratch every run
kraft cloud volume rm "$volume_name" || true
Expand Down
6 changes: 5 additions & 1 deletion images/chromium-headful/xorg.conf
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ Section "Monitor"

# 1920x1080 @ 60.00 Hz (GTF) hsync: 67.08 kHz; pclk: 172.80 MHz
Modeline "1920x1080_60.00" 172.80 1920 2040 2248 2576 1080 1081 1084 1118 -HSync +Vsync
# 1920x1200 @ 60.00 Hz (GTF) hsync: 74.52 kHz; pclk: 193.25 MHz
Modeline "1920x1200_60.00" 193.25 1920 2056 2256 2592 1200 1203 1209 1242 -HSync +Vsync
# 1440x900 @ 60.00 Hz (GTF) hsync: 55.92 kHz; pclk: 106.29 MHz
Modeline "1440x900_60.00" 106.29 1440 1520 1672 1904 900 901 904 932 -HSync +Vsync
# 1280x720 @ 60.00 Hz (GTF) hsync: 44.76 kHz; pclk: 74.48 MHz
Modeline "1280x720_60.00" 74.48 1280 1336 1472 1664 720 721 724 746 -HSync +Vsync
# 1152x648 @ 60.00 Hz (GTF) hsync: 40.26 kHz; pclk: 59.91 MHz
Expand Down Expand Up @@ -105,7 +109,7 @@ Section "Screen"
SubSection "Display"
Viewport 0 0
Depth 24
Modes "1920x1080_60.00" "1280x720_60.00" "1152x648_60.00" "1024x576_60.00" "960x720_60.00" "800x600_60.00" "2560x1440_30.00" "1920x1080_30.00" "1368x768_30.00" "1280x720_30.00" "1152x648_30.00" "1024x576_30.00" "960x720_30.00" "800x600_30.00" "3840x2160_25.00" "2560x1440_25.00" "1920x1080_25.00" "1600x900_25.00" "1368x768_25.00" "800x1600_30.00" "800x1600_25.00"
Modes "1920x1080_60.00" "1920x1200_60.00" "1440x900_60.00" "1280x720_60.00" "1152x648_60.00" "1024x576_60.00" "960x720_60.00" "800x600_60.00" "2560x1440_30.00" "1920x1080_30.00" "1368x768_30.00" "1280x720_30.00" "1152x648_30.00" "1024x576_30.00" "960x720_30.00" "800x600_30.00" "3840x2160_25.00" "2560x1440_25.00" "1920x1080_25.00" "1600x900_25.00" "1368x768_25.00" "800x1600_30.00" "800x1600_25.00"
EndSubSection
EndSection

Expand Down
18 changes: 16 additions & 2 deletions server/cmd/api/api/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

"github.com/onkernel/kernel-images/server/lib/devtoolsproxy"
"github.com/onkernel/kernel-images/server/lib/logger"
"github.com/onkernel/kernel-images/server/lib/nekoclient"
oapi "github.com/onkernel/kernel-images/server/lib/oapi"
"github.com/onkernel/kernel-images/server/lib/recorder"
"github.com/onkernel/kernel-images/server/lib/scaletozero"
Expand All @@ -29,10 +30,12 @@ type ApiService struct {
procMu sync.RWMutex
procs map[string]*processHandle

// Neko authenticated client
nekoAuthClient *nekoclient.AuthClient

// DevTools upstream manager (Chromium supervisord log tailer)
upstreamMgr *devtoolsproxy.UpstreamManager

stz scaletozero.Controller
stz scaletozero.Controller
}

var _ oapi.StrictServerInterface = (*ApiService)(nil)
Expand All @@ -47,6 +50,16 @@ func New(recordManager recorder.RecordManager, factory recorder.FFmpegRecorderFa
return nil, fmt.Errorf("upstreamMgr cannot be nil")
}

// Initialize Neko authenticated client
adminPassword := os.Getenv("NEKO_ADMIN_PASSWORD")
if adminPassword == "" {
adminPassword = "admin" // Default from neko.yaml
}
nekoAuthClient, err := nekoclient.NewAuthClient("http://127.0.0.1:8080", "admin", adminPassword)
if err != nil {
return nil, fmt.Errorf("failed to create neko auth client: %w", err)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

why do we prefer to initial this here instead of passing it in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no reasons lol

return &ApiService{
recordManager: recordManager,
factory: factory,
Expand All @@ -55,6 +68,7 @@ func New(recordManager recorder.RecordManager, factory recorder.FFmpegRecorderFa
procs: make(map[string]*processHandle),
upstreamMgr: upstreamMgr,
stz: stz,
nekoAuthClient: nekoAuthClient,
}, nil
}

Expand Down
Loading
Loading