carverauto/serviceradar

Fork 0

SRQL injection via unescaped string concatenation in GenericFilterBuilder.BuildFilters #688

New issue

Closed

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

mfreeman451 commented

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

Owner

Imported from GitHub.

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

Summary

Context: The MCP (Model Context Protocol) server provides tools for LLMs to query ServiceRadar data by constructing SRQL queries from user-supplied JSON parameters.
Bug: The GenericFilterBuilder.BuildFilters function in pkg/mcp/builder.go constructs SRQL query filter strings by directly concatenating user input without escaping special characters, enabling SRQL injection attacks.
Actual vs. expected: User-supplied values are inserted directly into SRQL WHERE clauses using fmt.Sprintf("%s = '%s'", sqlField, strValue) without escaping single quotes or other SQL metacharacters, whereas they should be properly escaped or use parameterized queries.
Impact: An attacker with access to the MCP server can execute arbitrary SRQL queries, potentially bypassing authorization checks, exfiltrating sensitive data, or causing denial of service by crafting malicious filter parameters.

Code with bug

// BuildFilters builds filters for a generic entity using field mappings
func (g *GenericFilterBuilder) BuildFilters(args json.RawMessage) (
	additionalFilters []string, responseFilters map[string]interface{}, err error) {
	var rawArgs map[string]interface{}

	if err = json.Unmarshal(args, &rawArgs); err != nil {
		return nil, nil, fmt.Errorf("invalid filter arguments: %w", err)
	}

	responseFilters = make(map[string]interface{})

	// Process field mappings
	for jsonField, sqlField := range g.FieldMappings {
		if value, exists := rawArgs[jsonField]; exists && value != nil {
			if strValue, ok := value.(string); ok && strValue != "" {
				additionalFilters = append(additionalFilters, fmt.Sprintf("%s = '%s'", sqlField, strValue)) // <-- BUG 🔴 strValue is not escaped
			}
		}
	}

	// Build response filters
	for _, field := range g.ResponseFields {
		if value, exists := rawArgs[field]; exists {
			responseFilters[field] = value
		}
	}

	return additionalFilters, responseFilters, nil
}

This vulnerable pattern appears throughout the MCP package in multiple locations:

pkg/mcp/builder.go:169 - GenericFilterBuilder (primary vulnerability)
pkg/mcp/builder.go:68 - BuildTimeRangeFilter with time.Format (less critical but still vulnerable)
pkg/mcp/tools_devices.go:84 - Device ID filter construction
pkg/mcp/tools_events.go:101 - Poller ID filter construction
pkg/mcp/tools_logs.go:105 - Poller ID filter construction
pkg/mcp/tools_sweeps.go:98 - Poller ID filter construction
pkg/mcp/tools_sweeps.go:155 - Poller ID filter construction
pkg/mcp/server.go:809 - Device ID filter construction

Evidence

Example

Consider a legitimate use case where an LLM queries events from a specific poller:

Benign request:

{
  "name": "events.getEvents",
  "arguments": {
    "event_type": "network_alert"
  }
}

This produces the filter: event_type = 'network_alert'

Attack scenario:

An attacker crafts a malicious request:

{
  "name": "events.getEvents",
  "arguments": {
    "event_type": "test' OR '1'='1"
  }
}

This produces the injected filter: event_type = 'test' OR '1'='1'

The resulting SRQL query becomes:

SHOW events WHERE (event_type = 'test' OR '1'='1') ORDER BY _tp_time DESC

The '1'='1' condition is always true, causing the query to bypass the intended filter and return all events regardless of type.

More severe attack with comment injection:

{
  "name": "sweeps.getRecentSweeps",
  "arguments": {
    "poller_id": "test' --"
  }
}

This produces: poller_id = 'test' --'

The -- comments out the rest of the query, potentially removing security checks or LIMIT clauses.

Failing test

Test script

/*
 * Copyright 2025 Carver Automation Corporation.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package mcp

import (
	"encoding/json"
	"testing"

	"github.com/stretchr/testify/assert"
)

func TestGenericFilterBuilder_BuildFilters_SQLInjection(t *testing.T) {
	// Create a GenericFilterBuilder with a field mapping
	builder := &GenericFilterBuilder{
		FieldMappings: map[string]string{
			"poller_id": "poller_id",
			"network":   "network",
		},
		ResponseFields: []string{"poller_id", "network"},
	}

	// Test case 1: Benign input
	t.Run("benign input", func(t *testing.T) {
		args := map[string]interface{}{
			"poller_id": "test123",
		}
		argsJSON, _ := json.Marshal(args)

		filters, _, err := builder.BuildFilters(argsJSON)
		assert.NoError(t, err)
		assert.Len(t, filters, 1)
		assert.Equal(t, "poller_id = 'test123'", filters[0])
	})

	// Test case 2: SQL injection attempt with single quote
	t.Run("SQL injection with single quote", func(t *testing.T) {
		args := map[string]interface{}{
			"poller_id": "test' OR '1'='1",
		}
		argsJSON, _ := json.Marshal(args)

		filters, _, err := builder.BuildFilters(argsJSON)
		assert.NoError(t, err)
		assert.Len(t, filters, 1)

		// This demonstrates the vulnerability - the single quote is not escaped
		expectedVulnerable := "poller_id = 'test' OR '1'='1'"
		assert.Equal(t, expectedVulnerable, filters[0])

		// If this were secure, it should escape the quote like:
		// "poller_id = 'test'' OR ''1''=''1'"
	})

	// Test case 3: SQL injection with comment
	t.Run("SQL injection with comment", func(t *testing.T) {
		args := map[string]interface{}{
			"poller_id": "test' --",
		}
		argsJSON, _ := json.Marshal(args)

		filters, _, err := builder.BuildFilters(argsJSON)
		assert.NoError(t, err)
		assert.Len(t, filters, 1)

		// The injected comment is not escaped
		expectedVulnerable := "poller_id = 'test' --'"
		assert.Equal(t, expectedVulnerable, filters[0])
	})

	// Test case 4: SQL injection with UNION SELECT
	t.Run("SQL injection with UNION", func(t *testing.T) {
		args := map[string]interface{}{
			"poller_id": "test' UNION SELECT password FROM users --",
		}
		argsJSON, _ := json.Marshal(args)

		filters, _, err := builder.BuildFilters(argsJSON)
		assert.NoError(t, err)
		assert.Len(t, filters, 1)

		// The UNION injection is not prevented
		expectedVulnerable := "poller_id = 'test' UNION SELECT password FROM users --'"
		assert.Equal(t, expectedVulnerable, filters[0])
	})
}

func TestBuildFilteredQuery_SQLInjection(t *testing.T) {
	// Test that BuildFilteredQuery propagates the injection when using GenericFilterBuilder
	t.Run("SQL injection propagates through BuildFilteredQuery", func(t *testing.T) {
		params := FilterQueryParams{
			Filter: "device_id = 'abc' OR '1'='1",
			Limit:  10,
		}

		query := BuildFilteredQuery("devices", params)

		// The injected filter is passed directly into the query
		assert.Contains(t, query, "device_id = 'abc' OR '1'='1")

		// The final query would be exploitable:
		// "SHOW devices WHERE (device_id = 'abc' OR '1'='1') ORDER BY _tp_time DESC LIMIT 10"
	})
}

Test output

=== RUN   TestGenericFilterBuilder_BuildFilters_SQLInjection
=== RUN   TestGenericFilterBuilder_BuildFilters_SQLInjection/benign_input
=== RUN   TestGenericFilterBuilder_BuildFilters_SQLInjection/SQL_injection_with_single_quote
=== RUN   TestGenericFilterBuilder_BuildFilters_SQLInjection/SQL_injection_with_comment
=== RUN   TestGenericFilterBuilder_BuildFilters_SQLInjection/SQL_injection_with_UNION
--- PASS: TestGenericFilterBuilder_BuildFilters_SQLInjection (0.00s)
    --- PASS: TestGenericFilterBuilder_BuildFilters_SQLInjection/benign_input (0.00s)
    --- PASS: TestGenericFilterBuilder_BuildFilters_SQLInjection/SQL_injection_with_single_quote (0.00s)
    --- PASS: TestGenericFilterBuilder_BuildFilters_SQLInjection/SQL_injection_with_comment (0.00s)
    --- PASS: TestGenericFilterBuilder_BuildFilters_SQLInjection/SQL_injection_with_UNION (0.00s)
PASS
ok  	github.com/carverauto/serviceradar/pkg/mcp	0.005s

The test passes, demonstrating that the code successfully produces the vulnerable query strings. This is proof that the injection vulnerability exists.

Inconsistency with own spec

Reference spec

From pkg/mcp/README.md:

## Security

- All tools require authentication when auth service is configured
- SRQL injection protection through parameterized queries
- Input validation on all parameters
- Secure token verification through ServiceRadar's auth system

Current code

// From pkg/mcp/builder.go:169
additionalFilters = append(additionalFilters, fmt.Sprintf("%s = '%s'", sqlField, strValue))

Contradiction

The README explicitly claims "SRQL injection protection through parameterized queries", but the implementation uses string concatenation with fmt.Sprintf to build queries. There are no parameterized queries being used anywhere in the pkg/mcp/builder.go file or related tool files. This is a direct contradiction between the documented security guarantee and the actual implementation.

Full context

The MCP server is a component that exposes ServiceRadar's data through the Model Context Protocol, enabling LLMs and AI agents to query the system. It provides tools like events.getEvents, devices.getDevices, logs.getLogs, and sweeps.getResults that accept JSON parameters and convert them into SRQL (ServiceRadar Query Language) queries.

The vulnerable code path works as follows:

Entry Point: An MCP client (typically an LLM) calls a tool like events.getEvents with JSON arguments containing user-controlled parameters like event_type, poller_id, or network.
Builder Invocation: The tool handler (e.g., registerGetEventsTool in pkg/mcp/tools_events.go) creates a GenericFilterBuilder with field mappings and calls BuildGenericFilterToolWithBuilder.
Query Construction: The BuildGenericFilterTool function calls the builder's BuildFilters method, which uses fmt.Sprintf to directly embed user input into SRQL filter strings without escaping.
Query Execution: The constructed filter is combined with other filters using CombineFilters and passed to BuildFilteredQuery, which produces a complete SRQL query string like SHOW events WHERE (event_type = 'test' OR '1'='1') ORDER BY _tp_time DESC.
Database Execution: The query is sent to m.executeSRQLQuery, which calls m.queryExecutor.ExecuteSRQLQuery. The query executor (the database service in pkg/db) receives the already-constructed malicious query string and executes it.

The vulnerability affects all entity-specific MCP tools that use the GenericFilterBuilder:

events.getEvents - Filters events by type, severity, and poller ID
sweeps.getResults - Filters network sweep results by poller ID and network range
Other tools that manually construct filters using the same vulnerable pattern

Since the MCP server is designed to be called by LLMs processing user queries, an attacker could craft prompts that cause the LLM to generate malicious MCP tool calls. Even with authentication, a legitimate authenticated user or compromised LLM could exploit this vulnerability to access unauthorized data or perform unauthorized queries.

External documentation

SRQL Language Reference

From docs/docs/srql-language-reference.md:

## Overview

ServiceRadar Query Language (SRQL) now uses a key:value syntax that is parsed and executed by the Rust-based SRQL service (`rust/srql`). The engine plans queries against our OCSF-aligned streaming schema defined in `pkg/db/migrations`, translates them to CNPG SQL via Diesel, and returns consistently shaped results.

### Key:Value Filters
- Basic comparisons use `field:value`, e.g. `hostname:%cam%` or `severity_id:2`.
- Values are case-sensitive unless the underlying column is normalized. Use quotes for values with spaces: `device.location:"Building A"`.

The SRQL documentation shows that the modern SRQL syntax uses field:value format, but the MCP builder constructs old-style SQL WHERE clauses like field = 'value'. This suggests the builder may be using a deprecated or parallel query construction method. Regardless, both formats are vulnerable to injection if values aren't properly escaped.

Why has this bug gone undetected?

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

Recent Feature: The MCP server appears to be a relatively new addition to the codebase (commit 6049244f with message "new mcp server -- untested"), meaning it hasn't been through extensive security review or penetration testing yet.
Trusted Input Assumption: The MCP server is designed to be called by LLMs, which might be considered "trusted" components in the architecture. Developers may have assumed that LLM-generated queries would not contain malicious payloads, overlooking the fact that LLMs process untrusted user input.
Authentication as Primary Defense: The system has authentication (authService) which may have given developers a false sense of security. Authentication prevents unauthorized access but doesn't prevent authenticated users (or compromised systems) from exploiting injection vulnerabilities.
Complex Attack Chain: Exploiting this vulnerability requires:
- Access to the MCP server (either direct API access or ability to influence LLM prompts)
- Knowledge of the SRQL query syntax and schema
- Crafting prompts that cause the LLM to generate specific malicious parameters
This complexity makes it less likely to be discovered through casual testing or normal operation.
No Input Validation: The code performs type checking (if strValue, ok := value.(string)) but no content validation or sanitization. The vulnerable pattern fmt.Sprintf("%s = '%s'", field, value) looks innocuous at first glance and might pass code review without careful security analysis.
Testing Gap: The existing test suite (pkg/mcp/server_test.go) only tests basic functionality with benign inputs. There are no security-focused tests that attempt injection attacks or validate input sanitization.
Documentation Misleading: The README claims "SRQL injection protection through parameterized queries" which may have given both developers and security reviewers false confidence that the issue was already addressed.
Query Language Abstraction: Since SRQL is a custom query language (not standard SQL), developers might not have immediately recognized the same SQL injection patterns that would be obvious in traditional SQL contexts.

Recommended fix

The recommended fix is to properly escape single quotes in user input before embedding them in SRQL query strings. At minimum, implement SQL-style quote escaping:

// escapeSRQLValue escapes single quotes in a string value for safe use in SRQL queries
func escapeSRQLValue(value string) string {
	return strings.ReplaceAll(value, "'", "''")
}

// BuildFilters builds filters for a generic entity using field mappings
func (g *GenericFilterBuilder) BuildFilters(args json.RawMessage) (
	additionalFilters []string, responseFilters map[string]interface{}, err error) {
	var rawArgs map[string]interface{}

	if err = json.Unmarshal(args, &rawArgs); err != nil {
		return nil, nil, fmt.Errorf("invalid filter arguments: %w", err)
	}

	responseFilters = make(map[string]interface{})

	// Process field mappings
	for jsonField, sqlField := range g.FieldMappings {
		if value, exists := rawArgs[jsonField]; exists && value != nil {
			if strValue, ok := value.(string); ok && strValue != "" {
				escapedValue := escapeSRQLValue(strValue) // <-- FIX 🟢
				additionalFilters = append(additionalFilters, fmt.Sprintf("%s = '%s'", sqlField, escapedValue))
			}
		}
	}

	// Build response filters
	for _, field := range g.ResponseFields {
		if value, exists := rawArgs[field]; exists {
			responseFilters[field] = value
		}
	}

	return additionalFilters, responseFilters, nil
}

However, a more comprehensive solution would be:

Use Parameterized Queries: If the SRQL execution engine supports parameterized queries (placeholders for values), refactor the query construction to use them instead of string concatenation.
Input Validation: Add validation to reject values containing suspicious patterns or restrict them to expected character sets (e.g., alphanumeric + specific allowed characters).
Apply Escaping Throughout: The same escaping function should be applied in all locations where user input is embedded in SRQL queries (pkg/mcp/tools_devices.go, tools_events.go, tools_logs.go, tools_sweeps.go, etc.).
Update Documentation: Correct the security claims in pkg/mcp/README.md to accurately reflect the implemented protections.
Security Testing: Add comprehensive security tests that attempt various injection patterns to ensure the fix is effective.

All occurrences of the vulnerable pattern throughout the pkg/mcp package should be fixed together:

pkg/mcp/builder.go:68-72 - BuildTimeRangeFilter (time format injection, lower severity)
pkg/mcp/tools_devices.go:84 - Device ID filter
pkg/mcp/tools_events.go:101 - Poller ID filter in alerts
pkg/mcp/tools_logs.go:105 - Poller ID filter in recent logs
pkg/mcp/tools_sweeps.go:98 - Poller ID filter in recent sweeps
pkg/mcp/tools_sweeps.go:155 - Poller ID filter in sweep summary
pkg/mcp/server.go:809 - Device ID filter in query devices
pkg/mcp/query_utils.go - Multiple instances of similar vulnerable patterns

All of these should be audited and fixed as part of a comprehensive remediation effort.

Imported from GitHub. Original GitHub issue: #2150 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/issues/2150 Original created: 2025-12-16T05:18:14Z --- # Summary - **Context**: The MCP (Model Context Protocol) server provides tools for LLMs to query ServiceRadar data by constructing SRQL queries from user-supplied JSON parameters. - **Bug**: The `GenericFilterBuilder.BuildFilters` function in `pkg/mcp/builder.go` constructs SRQL query filter strings by directly concatenating user input without escaping special characters, enabling SRQL injection attacks. - **Actual vs. expected**: User-supplied values are inserted directly into SRQL WHERE clauses using `fmt.Sprintf("%s = '%s'", sqlField, strValue)` without escaping single quotes or other SQL metacharacters, whereas they should be properly escaped or use parameterized queries. - **Impact**: An attacker with access to the MCP server can execute arbitrary SRQL queries, potentially bypassing authorization checks, exfiltrating sensitive data, or causing denial of service by crafting malicious filter parameters. # Code with bug ```go // BuildFilters builds filters for a generic entity using field mappings func (g *GenericFilterBuilder) BuildFilters(args json.RawMessage) ( additionalFilters []string, responseFilters map[string]interface{}, err error) { var rawArgs map[string]interface{} if err = json.Unmarshal(args, &rawArgs); err != nil { return nil, nil, fmt.Errorf("invalid filter arguments: %w", err) } responseFilters = make(map[string]interface{}) // Process field mappings for jsonField, sqlField := range g.FieldMappings { if value, exists := rawArgs[jsonField]; exists && value != nil { if strValue, ok := value.(string); ok && strValue != "" { additionalFilters = append(additionalFilters, fmt.Sprintf("%s = '%s'", sqlField, strValue)) // <-- BUG 🔴 strValue is not escaped } } } // Build response filters for _, field := range g.ResponseFields { if value, exists := rawArgs[field]; exists { responseFilters[field] = value } } return additionalFilters, responseFilters, nil } ``` This vulnerable pattern appears throughout the MCP package in multiple locations: - `pkg/mcp/builder.go:169` - GenericFilterBuilder (primary vulnerability) - `pkg/mcp/builder.go:68` - BuildTimeRangeFilter with time.Format (less critical but still vulnerable) - `pkg/mcp/tools_devices.go:84` - Device ID filter construction - `pkg/mcp/tools_events.go:101` - Poller ID filter construction - `pkg/mcp/tools_logs.go:105` - Poller ID filter construction - `pkg/mcp/tools_sweeps.go:98` - Poller ID filter construction - `pkg/mcp/tools_sweeps.go:155` - Poller ID filter construction - `pkg/mcp/server.go:809` - Device ID filter construction # Evidence ## Example Consider a legitimate use case where an LLM queries events from a specific poller: **Benign request:** ```json { "name": "events.getEvents", "arguments": { "event_type": "network_alert" } } ``` This produces the filter: `event_type = 'network_alert'` **Attack scenario:** An attacker crafts a malicious request: ```json { "name": "events.getEvents", "arguments": { "event_type": "test' OR '1'='1" } } ``` This produces the injected filter: `event_type = 'test' OR '1'='1'` The resulting SRQL query becomes: ``` SHOW events WHERE (event_type = 'test' OR '1'='1') ORDER BY _tp_time DESC ``` The `'1'='1'` condition is always true, causing the query to bypass the intended filter and return all events regardless of type. **More severe attack with comment injection:** ```json { "name": "sweeps.getRecentSweeps", "arguments": { "poller_id": "test' --" } } ``` This produces: `poller_id = 'test' --'` The `--` comments out the rest of the query, potentially removing security checks or LIMIT clauses. ## Failing test ### Test script ```go /* * Copyright 2025 Carver Automation Corporation. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at * * http://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. */ package mcp import ( "encoding/json" "testing" "github.com/stretchr/testify/assert" ) func TestGenericFilterBuilder_BuildFilters_SQLInjection(t *testing.T) { // Create a GenericFilterBuilder with a field mapping builder := &GenericFilterBuilder{ FieldMappings: map[string]string{ "poller_id": "poller_id", "network": "network", }, ResponseFields: []string{"poller_id", "network"}, } // Test case 1: Benign input t.Run("benign input", func(t *testing.T) { args := map[string]interface{}{ "poller_id": "test123", } argsJSON, _ := json.Marshal(args) filters, _, err := builder.BuildFilters(argsJSON) assert.NoError(t, err) assert.Len(t, filters, 1) assert.Equal(t, "poller_id = 'test123'", filters[0]) }) // Test case 2: SQL injection attempt with single quote t.Run("SQL injection with single quote", func(t *testing.T) { args := map[string]interface{}{ "poller_id": "test' OR '1'='1", } argsJSON, _ := json.Marshal(args) filters, _, err := builder.BuildFilters(argsJSON) assert.NoError(t, err) assert.Len(t, filters, 1) // This demonstrates the vulnerability - the single quote is not escaped expectedVulnerable := "poller_id = 'test' OR '1'='1'" assert.Equal(t, expectedVulnerable, filters[0]) // If this were secure, it should escape the quote like: // "poller_id = 'test'' OR ''1''=''1'" }) // Test case 3: SQL injection with comment t.Run("SQL injection with comment", func(t *testing.T) { args := map[string]interface{}{ "poller_id": "test' --", } argsJSON, _ := json.Marshal(args) filters, _, err := builder.BuildFilters(argsJSON) assert.NoError(t, err) assert.Len(t, filters, 1) // The injected comment is not escaped expectedVulnerable := "poller_id = 'test' --'" assert.Equal(t, expectedVulnerable, filters[0]) }) // Test case 4: SQL injection with UNION SELECT t.Run("SQL injection with UNION", func(t *testing.T) { args := map[string]interface{}{ "poller_id": "test' UNION SELECT password FROM users --", } argsJSON, _ := json.Marshal(args) filters, _, err := builder.BuildFilters(argsJSON) assert.NoError(t, err) assert.Len(t, filters, 1) // The UNION injection is not prevented expectedVulnerable := "poller_id = 'test' UNION SELECT password FROM users --'" assert.Equal(t, expectedVulnerable, filters[0]) }) } func TestBuildFilteredQuery_SQLInjection(t *testing.T) { // Test that BuildFilteredQuery propagates the injection when using GenericFilterBuilder t.Run("SQL injection propagates through BuildFilteredQuery", func(t *testing.T) { params := FilterQueryParams{ Filter: "device_id = 'abc' OR '1'='1", Limit: 10, } query := BuildFilteredQuery("devices", params) // The injected filter is passed directly into the query assert.Contains(t, query, "device_id = 'abc' OR '1'='1") // The final query would be exploitable: // "SHOW devices WHERE (device_id = 'abc' OR '1'='1') ORDER BY _tp_time DESC LIMIT 10" }) } ``` ### Test output ``` === RUN TestGenericFilterBuilder_BuildFilters_SQLInjection === RUN TestGenericFilterBuilder_BuildFilters_SQLInjection/benign_input === RUN TestGenericFilterBuilder_BuildFilters_SQLInjection/SQL_injection_with_single_quote === RUN TestGenericFilterBuilder_BuildFilters_SQLInjection/SQL_injection_with_comment === RUN TestGenericFilterBuilder_BuildFilters_SQLInjection/SQL_injection_with_UNION --- PASS: TestGenericFilterBuilder_BuildFilters_SQLInjection (0.00s) --- PASS: TestGenericFilterBuilder_BuildFilters_SQLInjection/benign_input (0.00s) --- PASS: TestGenericFilterBuilder_BuildFilters_SQLInjection/SQL_injection_with_single_quote (0.00s) --- PASS: TestGenericFilterBuilder_BuildFilters_SQLInjection/SQL_injection_with_comment (0.00s) --- PASS: TestGenericFilterBuilder_BuildFilters_SQLInjection/SQL_injection_with_UNION (0.00s) PASS ok github.com/carverauto/serviceradar/pkg/mcp 0.005s ``` The test passes, demonstrating that the code successfully produces the vulnerable query strings. This is proof that the injection vulnerability exists. ## Inconsistency with own spec ### Reference spec From `pkg/mcp/README.md`: ```markdown ## Security - All tools require authentication when auth service is configured - SRQL injection protection through parameterized queries - Input validation on all parameters - Secure token verification through ServiceRadar's auth system ``` ### Current code ```go // From pkg/mcp/builder.go:169 additionalFilters = append(additionalFilters, fmt.Sprintf("%s = '%s'", sqlField, strValue)) ``` ### Contradiction The README explicitly claims "SRQL injection protection through parameterized queries", but the implementation uses string concatenation with `fmt.Sprintf` to build queries. There are no parameterized queries being used anywhere in the `pkg/mcp/builder.go` file or related tool files. This is a direct contradiction between the documented security guarantee and the actual implementation. # Full context The MCP server is a component that exposes ServiceRadar's data through the Model Context Protocol, enabling LLMs and AI agents to query the system. It provides tools like `events.getEvents`, `devices.getDevices`, `logs.getLogs`, and `sweeps.getResults` that accept JSON parameters and convert them into SRQL (ServiceRadar Query Language) queries. The vulnerable code path works as follows: 1. **Entry Point**: An MCP client (typically an LLM) calls a tool like `events.getEvents` with JSON arguments containing user-controlled parameters like `event_type`, `poller_id`, or `network`. 2. **Builder Invocation**: The tool handler (e.g., `registerGetEventsTool` in `pkg/mcp/tools_events.go`) creates a `GenericFilterBuilder` with field mappings and calls `BuildGenericFilterToolWithBuilder`. 3. **Query Construction**: The `BuildGenericFilterTool` function calls the builder's `BuildFilters` method, which uses `fmt.Sprintf` to directly embed user input into SRQL filter strings without escaping. 4. **Query Execution**: The constructed filter is combined with other filters using `CombineFilters` and passed to `BuildFilteredQuery`, which produces a complete SRQL query string like `SHOW events WHERE (event_type = 'test' OR '1'='1') ORDER BY _tp_time DESC`. 5. **Database Execution**: The query is sent to `m.executeSRQLQuery`, which calls `m.queryExecutor.ExecuteSRQLQuery`. The query executor (the database service in `pkg/db`) receives the already-constructed malicious query string and executes it. The vulnerability affects all entity-specific MCP tools that use the `GenericFilterBuilder`: - `events.getEvents` - Filters events by type, severity, and poller ID - `sweeps.getResults` - Filters network sweep results by poller ID and network range - Other tools that manually construct filters using the same vulnerable pattern Since the MCP server is designed to be called by LLMs processing user queries, an attacker could craft prompts that cause the LLM to generate malicious MCP tool calls. Even with authentication, a legitimate authenticated user or compromised LLM could exploit this vulnerability to access unauthorized data or perform unauthorized queries. ## External documentation ### SRQL Language Reference From `docs/docs/srql-language-reference.md`: ```markdown ## Overview ServiceRadar Query Language (SRQL) now uses a key:value syntax that is parsed and executed by the Rust-based SRQL service (`rust/srql`). The engine plans queries against our OCSF-aligned streaming schema defined in `pkg/db/migrations`, translates them to CNPG SQL via Diesel, and returns consistently shaped results. ### Key:Value Filters - Basic comparisons use `field:value`, e.g. `hostname:%cam%` or `severity_id:2`. - Values are case-sensitive unless the underlying column is normalized. Use quotes for values with spaces: `device.location:"Building A"`. ``` The SRQL documentation shows that the modern SRQL syntax uses `field:value` format, but the MCP builder constructs old-style SQL WHERE clauses like `field = 'value'`. This suggests the builder may be using a deprecated or parallel query construction method. Regardless, both formats are vulnerable to injection if values aren't properly escaped. # Why has this bug gone undetected? This SRQL injection vulnerability has likely gone undetected for several reasons: 1. **Recent Feature**: The MCP server appears to be a relatively new addition to the codebase (commit `6049244f` with message "new mcp server -- untested"), meaning it hasn't been through extensive security review or penetration testing yet. 2. **Trusted Input Assumption**: The MCP server is designed to be called by LLMs, which might be considered "trusted" components in the architecture. Developers may have assumed that LLM-generated queries would not contain malicious payloads, overlooking the fact that LLMs process untrusted user input. 3. **Authentication as Primary Defense**: The system has authentication (`authService`) which may have given developers a false sense of security. Authentication prevents unauthorized access but doesn't prevent authenticated users (or compromised systems) from exploiting injection vulnerabilities. 4. **Complex Attack Chain**: Exploiting this vulnerability requires: - Access to the MCP server (either direct API access or ability to influence LLM prompts) - Knowledge of the SRQL query syntax and schema - Crafting prompts that cause the LLM to generate specific malicious parameters This complexity makes it less likely to be discovered through casual testing or normal operation. 5. **No Input Validation**: The code performs type checking (`if strValue, ok := value.(string)`) but no content validation or sanitization. The vulnerable pattern `fmt.Sprintf("%s = '%s'", field, value)` looks innocuous at first glance and might pass code review without careful security analysis. 6. **Testing Gap**: The existing test suite (`pkg/mcp/server_test.go`) only tests basic functionality with benign inputs. There are no security-focused tests that attempt injection attacks or validate input sanitization. 7. **Documentation Misleading**: The README claims "SRQL injection protection through parameterized queries" which may have given both developers and security reviewers false confidence that the issue was already addressed. 8. **Query Language Abstraction**: Since SRQL is a custom query language (not standard SQL), developers might not have immediately recognized the same SQL injection patterns that would be obvious in traditional SQL contexts. # Recommended fix The recommended fix is to properly escape single quotes in user input before embedding them in SRQL query strings. At minimum, implement SQL-style quote escaping: ```go // escapeSRQLValue escapes single quotes in a string value for safe use in SRQL queries func escapeSRQLValue(value string) string { return strings.ReplaceAll(value, "'", "''") } // BuildFilters builds filters for a generic entity using field mappings func (g *GenericFilterBuilder) BuildFilters(args json.RawMessage) ( additionalFilters []string, responseFilters map[string]interface{}, err error) { var rawArgs map[string]interface{} if err = json.Unmarshal(args, &rawArgs); err != nil { return nil, nil, fmt.Errorf("invalid filter arguments: %w", err) } responseFilters = make(map[string]interface{}) // Process field mappings for jsonField, sqlField := range g.FieldMappings { if value, exists := rawArgs[jsonField]; exists && value != nil { if strValue, ok := value.(string); ok && strValue != "" { escapedValue := escapeSRQLValue(strValue) // <-- FIX 🟢 additionalFilters = append(additionalFilters, fmt.Sprintf("%s = '%s'", sqlField, escapedValue)) } } } // Build response filters for _, field := range g.ResponseFields { if value, exists := rawArgs[field]; exists { responseFilters[field] = value } } return additionalFilters, responseFilters, nil } ``` However, a more comprehensive solution would be: 1. **Use Parameterized Queries**: If the SRQL execution engine supports parameterized queries (placeholders for values), refactor the query construction to use them instead of string concatenation. 2. **Input Validation**: Add validation to reject values containing suspicious patterns or restrict them to expected character sets (e.g., alphanumeric + specific allowed characters). 3. **Apply Escaping Throughout**: The same escaping function should be applied in all locations where user input is embedded in SRQL queries (`pkg/mcp/tools_devices.go`, `tools_events.go`, `tools_logs.go`, `tools_sweeps.go`, etc.). 4. **Update Documentation**: Correct the security claims in `pkg/mcp/README.md` to accurately reflect the implemented protections. 5. **Security Testing**: Add comprehensive security tests that attempt various injection patterns to ensure the fix is effective. # Related bugs All occurrences of the vulnerable pattern throughout the `pkg/mcp` package should be fixed together: - `pkg/mcp/builder.go:68-72` - BuildTimeRangeFilter (time format injection, lower severity) - `pkg/mcp/tools_devices.go:84` - Device ID filter - `pkg/mcp/tools_events.go:101` - Poller ID filter in alerts - `pkg/mcp/tools_logs.go:105` - Poller ID filter in recent logs - `pkg/mcp/tools_sweeps.go:98` - Poller ID filter in recent sweeps - `pkg/mcp/tools_sweeps.go:155` - Poller ID filter in sweep summary - `pkg/mcp/server.go:809` - Device ID filter in query devices - `pkg/mcp/query_utils.go` - Multiple instances of similar vulnerable patterns All of these should be audited and fixed as part of a comprehensive remediation effort.

mfreeman451 commented

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

Author

Owner

Imported GitHub comment.

Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/issues/2150#issuecomment-3663454878
Original created: 2025-12-17T03:20:20Z

we are replacing the golang based MCP system with elixir/hermes_mcp, closing this as will not be implemented.

Imported GitHub comment. Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/issues/2150#issuecomment-3663454878 Original created: 2025-12-17T03:20:20Z --- we are replacing the golang based MCP system with elixir/hermes_mcp, closing this as will not be implemented.