RDKEMW-15199:[SECVULN] Command injection via /opt/.telemetry/dca_temp…

[dharshini555]

…_file.conf

[Copilot]

Pull request overview

Addresses a reported command-injection vector by adding validation/sanitization for XConf-provided top_log.txt marker search strings before accepting them into the profile.

Changes:

• Added <ctype.h> dependency and introduced sanitize_string() helper.
• Added validation check for TopMarker (top_log.txt) ref/search string in addParameter().

[Copilot]

sanitize_string() unconditionally frees the input buffer. This makes ownership unclear and is easy to misuse (e.g., if a non-heap string is ever passed) and prevents callers from using a sanitized result. Prefer a pure validation function that does not free/modify the caller's memory, or return a newly allocated sanitized string and let the caller manage free().

Suggested change

	static int sanitize_string(char *str)
	{
	char *src = str, *dst = str;
	while (*src)
	{
	if (isalnum(*src) || *src == '.' || *src == '-' || *src == '_')
	{
	*dst++ = *src;
	}
	src++;
	}
	*dst = '\0'; // Null-terminate
	if(strncmp(src, dst, strlen(src)) == 0)
	{
	free(str);
	return 0;
	}
	else
	{
	T2Error("Invalid search string configuration. Sanitizing it\n");
	free(str);
	return -1;
	}
	static int sanitize_string(const char *str)
	{
	if (str == NULL)
	{
	return -1;
	}
	while (*str)
	{
	if (!(isalnum((unsigned char)*str) || *str == '.' || *str == '-' || *str == '_'))
	{
	T2Error("Invalid search string configuration\n");
	return -1;
	}
	str++;
	}
	return 0;

[Copilot]

sanitize_string(strdup(ref)) can crash on OOM because strdup(ref) may return NULL and sanitize_string() dereferences its argument. Also, since you only need to validate characters, avoid allocating/freeing altogether by validating ref directly (or fail early if ref is empty/invalid) before storing it in tMarker->searchString.

[Copilot]

New security behavior (rejecting invalid top_log.txt search strings) should be covered by unit tests to prevent regressions. There are existing gtests for XConf parsing (e.g., source/test/t2parser/t2parserxconfTest.cpp), but none assert that a profile with an invalid content/search string is rejected.

[Copilot]

isalnum(*src) is undefined for negative char values. Cast to unsigned char (e.g., isalnum((unsigned char)*src)) before passing to <ctype.h> macros to avoid UB on platforms where char is signed.

Suggested change

	if (isalnum(*src) || *src == '.' || *src == '-' || *src == '_')
	if (isalnum((unsigned char)*src) || *src == '.' || *src == '-' || *src == '_')

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.

[Copilot]

sanitize_string dereferences str unconditionally (e.g., while (*src)), but it can be called with NULL if strdup(ref) fails (or if a future caller passes NULL). This would crash on low-memory conditions. Consider changing the helper to accept const char* and validate without allocating, or at minimum check the strdup result before calling and handle NULL inside sanitize_string.

Suggested change

	char *src = str, *dst = str;
	int invalid_char_found = 0;
	char *src = NULL;
	char *dst = NULL;
	int invalid_char_found = 0;
	if (str == NULL)
	{
	T2Error("sanitize_string received NULL input\n");
	return -1;
	}
	src = str;
	dst = str;

[Copilot]

sanitize_string frees the input buffer on both success and failure. That makes ownership unclear and easy to misuse (e.g., passing a non-heap pointer would become an invalid free), and it also means the in-place sanitization work is discarded immediately. Recommend removing free(str) from this helper and either (a) make it a pure validator (int is_valid_search_string(const char*)) or (b) return the sanitized string to the caller so the sanitized value is what gets stored/used.

Suggested change

	free(str);
	return 0;
	}
	else
	{
	T2Error("Invalid search string configuration. Sanitizing it\n");
	free(str);
	return 0;
	}
	else
	{
	T2Error("Invalid search string configuration. Sanitizing it\n");

[Copilot]

The allowed character set includes '-'. Since topMarker->searchString is later interpolated into shell commands (e.g., pidof %s and busybox ps | grep %s ... in source/dcautil/dcaproc.c), allowing values that start with - can lead to option-injection behavior (e.g., grep -v, pidof -s) even if metacharacters are filtered. Consider rejecting strings that begin with - (or tightening the allowed set) and/or ensuring downstream commands use -- / proper quoting to force argument interpretation.

Suggested change

	if (isalnum((unsigned char)*src) || *src == '.' || *src == '-' || *src == '_')
	if (isalnum((unsigned char)*src) || *src == '.' || *src == '_'
	|| (*src == '-' && dst != str))

[Copilot]

At the call site, sanitize_string(strdup(ref)) allocates and then immediately discards the buffer; this adds an unnecessary allocation on the hot path and still doesn't guard against strdup returning NULL. Prefer validating ref directly without allocating, or store a sanitized copy and use that as tMarker->searchString.

[Copilot]

This change introduces new security-relevant behavior (rejecting/skipping top_log.txt markers when ref contains disallowed characters), but there is no unit test coverage ensuring an invalid content (e.g., containing ; or whitespace) is rejected and does not get added to topMarkerList. Add a test case to prevent regressions for the command-injection fix.