RCE via unsafe evaluation of user-provided filter expressions

Before fix PoC (executed in a browser context such as Grafana's UI): // Vulnerable path demonstration (pseudocode - would be in frontend code path handling table filters) function vulnerableEvaluate(expr) { // insecure path: constructs a Function from user-provided expression and executes it const fn = new Function('$', "'use strict'; return " + expr + ";"); return fn(undefined); } // Attack: supply a filter that executes an IIFE when evaluated vulnerableEvaluate("(function(){ window.__grafana_pwn = true; })()"); console.log(window.__grafana_pwn); // would log true in a vulnerable UI After fix, the same input would be handled by the safe expression parser and would not execute arbitrary code. A safe test would show no side effects and no value being computed from the expression.

diff --git a/packages/grafana-ui/src/components/Table/TableNG/Filter/FilterList.test.tsx b/packages/grafana-ui/src/components/Table/TableNG/Filter/FilterList.test.tsx index 94cdbfe37c124..46b1df00884a8 100644 --- a/packages/grafana-ui/src/components/Table/TableNG/Filter/FilterList.test.tsx +++ b/packages/grafana-ui/src/components/Table/TableNG/Filter/FilterList.test.tsx @@ -37,7 +37,7 @@ describe('FilterList', () => { expect(screen.getByRole('checkbox', { name: 'cherry' })).toBeInTheDocument(); }); - it('only shows options whose label matches the search string', () => { + it('filters options by label regex match', () => { const options: SelectableValue[] = [ { value: 'apple', label: 'apple' }, { value: 'apricot', label: 'apricot' }, @@ -74,7 +74,7 @@ describe('FilterList', () => { expect(screen.queryByRole('checkbox', { name: 'apple' })).not.toBeInTheDocument(); }); - it('shows "No values" label when no options match the search', () => { + it('shows "No values" when no options match', () => { const options: SelectableValue[] = [{ value: 'apple', label: 'apple' }]; render( <FilterList options={options} values={[]} onChange={jest.fn()} searchFilter="zzz" operator={containsOp()} /> @@ -83,131 +83,100 @@ describe('FilterList', () => { }); }); - describe('numeric comparison operators', () => { - it('EQUALS shows only the item with the matching numeric value', () => { - const options = numericOptions([10, 20, 30]); + describe('comparison operators (smoke tests — logic covered in filterExpression.test.ts)', () => { + it.each([ + [FilterOperator.EQUALS, [20]], + [FilterOperator.NOT_EQUALS, [10, 30]], + [FilterOperator.GREATER, [30]], + [FilterOperator.GREATER_OR_EQUAL, [20, 30]], + [FilterOperator.LESS, [10]], + [FilterOperator.LESS_OR_EQUAL, [10, 20]], + ] as Array<[FilterOperator, number[]]>)('%s shows only matching options', (operator, visible) => { + const all = [10, 20, 30]; render( <FilterList - options={options} - values={[]} - onChange={jest.fn()} - searchFilter="20" - operator={op(FilterOperator.EQUALS)} - /> - ); - expect(screen.queryByRole('checkbox', { name: '10' })).not.toBeInTheDocument(); - expect(screen.getByRole('checkbox', { name: '20' })).toBeInTheDocument(); - expect(screen.queryByRole('checkbox', { name: '30' })).not.toBeInTheDocument(); - }); - - it('NOT_EQUALS excludes the item with the matching value', () => { - const options = numericOptions([10, 20, 30]); - render( - <FilterList - options={options} - values={[]} - onChange={jest.fn()} - searchFilter="20" - operator={op(FilterOperator.NOT_EQUALS)} - /> - ); - expect(screen.getByRole('checkbox', { name: '10' })).toBeInTheDocument(); - expect(screen.queryByRole('checkbox', { name: '20' })).not.toBeInTheDocument(); - expect(screen.getByRole('checkbox', { name: '30' })).toBeInTheDocument(); - }); - - it('GREATER shows only values strictly greater than the threshold', () => { - const options = numericOptions([10, 20, 30]); - render( - <FilterList - options={options} + options={numericOptions(all)} values={[]} onChange={jest.fn()} searchFilter="20" - operator={op(FilterOperator.GREATER)} + operator={op(operator)} /> ); - expect(screen.queryByRole('checkbox', { name: '10' })).not.toBeInTheDocument(); - expect(screen.queryByRole('checkbox', { name: '20' })).not.toBeInTheDocument(); - expect(screen.getByRole('checkbox', { name: '30' })).toBeInTheDocument(); + for (const n of all) { + const el = screen.queryByRole('checkbox', { name: String(n) }); + visible.includes(n) ? expect(el).toBeInTheDocument() : expect(el).not.toBeInTheDocument(); + } }); - it('GREATER_OR_EQUAL shows values >= the threshold', () => { - const options = numericOptions([10, 20, 30]); + it('excludes options with undefined value', () => { + const options: SelectableValue[] = [{ label: 'no-value' }, { value: '20', label: '20' }]; render( <FilterList options={options} values={[]} onChange={jest.fn()} searchFilter="20" - operator={op(FilterOperator.GREATER_OR_EQUAL)} + operator={op(FilterOperator.EQUALS)} /> ); - expect(screen.queryByRole('checkbox', { name: '10' })).not.toBeInTheDocument(); + expect(screen.queryByRole('checkbox', { name: 'no-value' })).not.toBeInTheDocument(); expect(screen.getByRole('checkbox', { name: '20' })).toBeInTheDocument(); - expect(screen.getByRole('checkbox', { name: '30' })).toBeInTheDocument(); }); + }); - it('LESS shows only values strictly less than the threshold', () => { - const options = numericOptions([10, 20, 30]); + describe('EXPRESSION operator (smoke tests — logic covered in filterExpression.test.ts)', () => { + it('filters options using a compound && expression', () => { render( <FilterList - options={options} + options={numericOptions([5, 15, 25])} values={[]} onChange={jest.fn()} - searchFilter="20" - operator={op(FilterOperator.LESS)} + searchFilter="$ > 10 && $ < 20" + operator={op(FilterOperator.EXPRESSION)} /> ); - expect(screen.getByRole('checkbox', { name: '10' })).toBeInTheDocument(); - expect(screen.queryByRole('checkbox', { name: '20' })).not.toBeInTheDocument(); - expect(screen.queryByRole('checkbox', { name: '30' })).not.toBeInTheDocument(); + expect(screen.queryByRole('checkbox', { name: '5' })).not.toBeInTheDocument(); + expect(screen.getByRole('checkbox', { name: '15' })).toBeInTheDocument(); + expect(screen.queryByRole('checkbox', { name: '25' })).not.toBeInTheDocument(); }); - it('LESS_OR_EQUAL shows values <= the threshold', () => { - const options = numericOptions([10, 20, 30]); + it('shows "No values" for an unparseable expression', () => { render( <FilterList - options={options} + options={numericOptions([10, 20])} values={[]} onChange={jest.fn()} - searchFilter="20" - operator={op(FilterOperator.LESS_OR_EQUAL)} + searchFilter="$ ===" + operator={op(FilterOperator.EXPRESSION)} /> ); - expect(screen.getByRole('checkbox', { name: '10' })).toBeInTheDocument(); - expect(screen.getByRole('checkbox', { name: '20' })).toBeInTheDocument(); - expect(screen.queryByRole('checkbox', { name: '30' })).not.toBeInTheDocument(); + expect(screen.getByText('No values')).toBeInTheDocument(); }); - }); - describe('EXPRESSION operator', () => { - it('evaluates a JS expression where $ is the column value', () => { - const options = numericOptions([5, 15, 25]); + it('shows "No values" and does not fall back to regex when expression is invalid', () => { render( <FilterList - options={options} + options={[ + { value: '1', label: 'foo' }, + { value: '2', label: 'foobar' }, + ]} values={[]} onChange={jest.fn()} - searchFilter="$ > 10 && $ < 20" + searchFilter="foo" operator={op(FilterOperator.EXPRESSION)} /> ); - expect(screen.queryByRole('checkbox', { name: '5' })).not.toBeInTheDocument(); - expect(screen.getByRole('checkbox', { name: '15' })).toBeInTheDocument(); - expect(screen.queryByRole('checkbox', { name: '25' })).not.toBeInTheDocument(); + expect(screen.getByText('No values')).toBeInTheDocument(); }); - it('shows "No values" when the expression throws a syntax error', () => { - const options = numericOptions([10, 20]); - // "$ ===" is a valid regex (won't throw new RegExp) but an invalid JS expression - // ("return $ ===;" throws SyntaxError in the Function constructor). + it('returns false when option.value is undefined', () => { + const options: SelectableValue[] = [{ label: 'no-value' }]; render( <FilterList options={options} values={[]} onChange={jest.fn()} - searchFilter="$ ===" + searchFilter="$ > 0" operator={op(FilterOperator.EXPRESSION)} /> ); @@ -251,7 +220,7 @@ describe('FilterList', () => { expect(onChange.mock.calls[0][0]).toEqual([]); }); - it('only removes the currently visible items when deselecting, preserving hidden selections', async () => { + it('preserves hidden selections when deselecting visible items', async () => { const user = userEvent.setup(); const options: SelectableValue[] = [ { value: 'apple', label: 'apple' }, @@ -259,7 +228,6 @@ describe('FilterList', () => { { value: 'banana', label: 'banana' }, ]; const onChange = jest.fn(); - // All three are selected, but search filters to only "ap*" items render( <FilterList options={options} values={options} onChange={onChange} searchFilter="ap" operator={containsOp()} /> ); @@ -267,7 +235,6 @@ describe('FilterList', () => { const selectAll = screen.getByTestId(selectors.components.Panels.Visualization.TableNG.Filters.SelectAll); await user.click(selectAll.querySelector('input')!); - // banana should be preserved because it was not visible const remaining: SelectableValue[] = onChange.mock.calls[0][0]; expect(remaining.map((v) => v.value)).toEqual(['banana']); }); diff --git a/packages/grafana-ui/src/components/Table/TableNG/Filter/FilterList.tsx b/packages/grafana-ui/src/components/Table/TableNG/Filter/FilterList.tsx index afdbf36754c66..f924b1d5f81e0 100644 --- a/packages/grafana-ui/src/components/Table/TableNG/Filter/FilterList.tsx +++ b/packages/grafana-ui/src/components/Table/TableNG/Filter/FilterList.tsx @@ -4,7 +4,7 @@ import { useCallback, useMemo } from 'react'; import * as React from 'react'; import { FixedSizeList as List, type ListChildComponentProps } from 'react-window'; -import { type GrafanaTheme2, formattedValueToString, getValueFormat, type SelectableValue } from '@grafana/data'; +import { type GrafanaTheme2, type SelectableValue } from '@grafana/data'; import { selectors } from '@grafana/e2e-selectors'; import { t, Trans } from '@grafana/i18n'; @@ -12,6 +12,7 @@ import { useStyles2, useTheme2 } from '../../../../themes/ThemeContext'; import { Checkbox } from '../../../Forms/Checkbox'; import { Label } from '../../../Forms/Label'; import { Stack } from '../../../Layout/Stack/Stack'; +import { comparableValue, parseExpression } from '../../filterExpression'; import { FilterOperator } from '../types'; interface Props { @@ -26,78 +27,30 @@ interface Props { const ITEM_HEIGHT = 32; const MIN_HEIGHT = ITEM_HEIGHT * 4.5; // split an item in the middle to imply there are more items to scroll -const comparableValue = (value: string): string | number | Date | boolean => { - value = value.trim().replace(/\\/g, ''); - - // Does it look like a Date (Starting with pattern YYYY-MM-DD* or YYYY/MM/DD*)? - if (/^(\d{4}-\d{2}-\d{2}|\d{4}\/\d{2}\/\d{2})/.test(value)) { - const date = new Date(value); - if (!isNaN(date.getTime())) { - const fmt = getValueFormat('dateTimeAsIso'); - return formattedValueToString(fmt(date.getTime())); - } - } - // Does it look like a Number? - const num = parseFloat(value); - if (!isNaN(num)) { - return num; - } - // Does it look like a Bool? - const lvalue = value.toLowerCase(); - if (lvalue === 'true' || lvalue === 'false') { - return lvalue === 'true'; - } - // Anything else - return value; -}; - export const FilterList = ({ options, values, caseSensitive, onChange, searchFilter, operator }: Props) => { const regex = useMemo(() => new RegExp(searchFilter, caseSensitive ? undefined : 'i'), [searchFilter, caseSensitive]); + const predicate = useMemo(() => { + if (!searchFilter || operator.value === FilterOperator.CONTAINS) { + return null; + } + if (operator.value === FilterOperator.EXPRESSION) { + return parseExpression(searchFilter) ?? (() => false); + } + return parseExpression(`$ ${operator.value} ${searchFilter}`) ?? (() => false); + }, [searchFilter, operator]); + const items = useMemo( () => options.filter((option) => { - if (!searchFilter || operator.value === FilterOperator.CONTAINS) { - if (option.label === undefined) { - return false; - } - return regex.test(option.label); - } else if (operator.value === FilterOperator.EXPRESSION) { - if (option.value === undefined) { - return false; - } - try { - const xpr = searchFilter.replace(/\\/g, ''); - const fnc = new Function('$', `'use strict'; return ${xpr};`); - const val = comparableValue(option.value); - return fnc(val); - } catch (_) {} - return false; - } else { - if (option.value === undefined) { - return false; - } - - const value1 = comparableValue(option.value); - const value2 = comparableValue(searchFilter); - - switch (operator.value) { - case '=': - return value1 === value2; - case '!=': - return value1 !== value2; - case '>': - return value1 > value2; - case '>=': - return value1 >= value2; - case '<': - return value1 < value2; - case '<=': - return value1 <= value2; - } + if (predicate === null) { + return option.label !== undefined && regex.test(option.label); + } + if (option.value === undefined) { return false; } + return predicate(comparableValue(option.value)); }), - [options, regex, operator, searchFilter] + [options, regex, predicate] ); const selectedItems = useMemo(() => items.filter((item) => values.includes(item)), [items, values]); diff --git a/packages/grafana-ui/src/components/Table/TableRT/FilterList.test.tsx b/packages/grafana-ui/src/components/Table/TableRT/FilterList.test.tsx new file mode 100644 index 0000000000000..699d11d5960f5 --- /dev/null +++ b/packages/grafana-ui/src/components/Table/TableRT/FilterList.test.tsx @@ -0,0 +1,274 @@ +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +import { type SelectableValue } from ' ... [truncated]