Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution] Prevent non-customizable fields from updating for Prebuilt rule types #195318

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export const getPrebuiltRuleMock = (rewrites?: Partial<PrebuiltRuleAsset>): Preb
language: 'kuery',
rule_id: 'rule-1',
version: 1,
author: [],
...rewrites,
} as PrebuiltRuleAsset);

Expand Down Expand Up @@ -51,6 +52,7 @@ export const getPrebuiltThreatMatchRuleMock = (): PrebuiltRuleAsset => ({
language: 'kuery',
rule_id: 'rule-1',
version: 1,
author: [],
threat_query: '*:*',
threat_index: ['list-index'],
threat_mapping: [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,27 @@ describe('DetectionRulesClient.patchRule', () => {
expect(rulesClient.create).not.toHaveBeenCalled();
});

it('throws an error if rule has external rule source and non-customizable fields are changed', async () => {
// Mock the existing rule
const existingRule = {
...getRulesSchemaMock(),
rule_source: { type: 'external', is_customized: true },
};
(getRuleByRuleId as jest.Mock).mockResolvedValueOnce(existingRule);

// Mock the rule update
const rulePatch = getCreateRulesSchemaMock('query-rule-id');
rulePatch.license = 'new license';

// Mock the rule returned after update; not used for this test directly but
// needed so that the patchRule method does not throw
rulesClient.update.mockResolvedValue(getRuleMock(getQueryRuleParams()));

await expect(detectionRulesClient.patchRule({ rulePatch })).rejects.toThrow(
'Cannot update "license" field for prebuilt rules'
);
});

describe('actions', () => {
it("updates the rule's actions if provided", async () => {
// Mock the existing rule
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -498,5 +498,26 @@ describe('DetectionRulesClient.updateRule', () => {
})
);
});

it('throws an error if rule has external rule source and non-customizable fields are changed', async () => {
// Mock the existing rule
const existingRule = {
...getRulesSchemaMock(),
rule_source: { type: 'external', is_customized: true },
};

(getRuleByRuleId as jest.Mock).mockResolvedValueOnce(existingRule);

// Mock the rule update
const ruleUpdate = { ...getCreateRulesSchemaMock(), author: ['new user'] };

// Mock the rule returned after update; not used for this test directly but
// needed so that the patchRule method does not throw
rulesClient.update.mockResolvedValue(getRuleMock(getQueryRuleParams()));

await expect(detectionRulesClient.updateRule({ ruleUpdate })).rejects.toThrow(
'Cannot update "author" field for prebuilt rules'
);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import type { MlAuthz } from '../../../../../machine_learning/authz';
import type { IPrebuiltRuleAssetsClient } from '../../../../prebuilt_rules/logic/rule_assets/prebuilt_rule_assets_client';
import { applyRulePatch } from '../mergers/apply_rule_patch';
import { getIdError } from '../../../utils/utils';
import { validateNonCustomizablePatchFields } from '../../../utils/validate';
import { convertAlertingRuleToRuleResponse } from '../converters/convert_alerting_rule_to_rule_response';
import { convertRuleResponseToAlertingRule } from '../converters/convert_rule_response_to_alerting_rule';
import { ClientError, toggleRuleEnabledOnUpdate, validateMlAuth } from '../utils';
Expand Down Expand Up @@ -51,6 +52,8 @@ export const patchRule = async ({

await validateMlAuth(mlAuthz, rulePatch.type ?? existingRule.type);

validateNonCustomizablePatchFields(rulePatch, existingRule);

const patchedRule = await applyRulePatch({
prebuiltRuleAssetClient,
existingRule,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import type { RuleResponse } from '../../../../../../../common/api/detection_eng
import type { MlAuthz } from '../../../../../machine_learning/authz';
import { applyRuleUpdate } from '../mergers/apply_rule_update';
import { getIdError } from '../../../utils/utils';
import { validateNonCustomizableUpdateFields } from '../../../utils/validate';
import { convertRuleResponseToAlertingRule } from '../converters/convert_rule_response_to_alerting_rule';

import { ClientError, toggleRuleEnabledOnUpdate, validateMlAuth } from '../utils';
Expand Down Expand Up @@ -50,6 +51,8 @@ export const updateRule = async ({
throw new ClientError(error.message, error.statusCode);
}

validateNonCustomizableUpdateFields(ruleUpdate, existingRule);

const ruleWithUpdates = await applyRuleUpdate({
prebuiltRuleAssetClient,
existingRule,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
RuleResponse,
type RuleResponseAction,
type RuleUpdateProps,
type RulePatchProps,
} from '../../../../../common/api/detection_engine';
import {
RESPONSE_ACTION_API_COMMAND_TO_CONSOLE_COMMAND_MAP,
Expand All @@ -25,6 +26,7 @@ import { CustomHttpRequestError } from '../../../../utils/custom_http_request_er
import { hasValidRuleType, type RuleAlertType, type RuleParams } from '../../rule_schema';
import { type BulkError, createBulkErrorObject } from '../../routes/utils';
import { internalRuleToAPIResponse } from '../logic/detection_rules_client/converters/internal_rule_to_api_response';
import { ClientError } from '../logic/detection_rules_client/utils';

export const transformValidateBulkError = (
ruleId: string,
Expand Down Expand Up @@ -117,3 +119,31 @@ function rulePayloadContainsResponseActions(rule: RuleCreateProps | RuleUpdatePr
function ruleObjectContainsResponseActions(rule?: RuleAlertType) {
return rule != null && 'params' in rule && 'responseActions' in rule?.params;
}

export const validateNonCustomizableUpdateFields = (
ruleUpdate: RuleUpdateProps,
existingRule: RuleResponse
) => {
// We don't allow non-customizable fields to be changed for prebuilt rules
if (existingRule.rule_source && existingRule.rule_source.type === 'external') {
if (!isEqual(ruleUpdate.author, existingRule.author)) {
throw new ClientError(`Cannot update "author" field for prebuilt rules`, 400);
} else if (ruleUpdate.license !== existingRule.license) {
throw new ClientError(`Cannot update "license" field for prebuilt rules`, 400);
}
}
};

export const validateNonCustomizablePatchFields = (
rulePatch: RulePatchProps,
existingRule: RuleResponse
) => {
// We don't allow non-customizable fields to be changed for prebuilt rules
if (existingRule.rule_source && existingRule.rule_source.type === 'external') {
if (rulePatch.author && !isEqual(rulePatch.author, existingRule.author)) {
throw new ClientError(`Cannot update "author" field for prebuilt rules`, 400);
} else if (rulePatch.license != null && rulePatch.license !== existingRule.license) {
throw new ClientError(`Cannot update "license" field for prebuilt rules`, 400);
}
}
};
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ import {
removeServerGeneratedPropertiesIncludingRuleId,
getSimpleRuleOutputWithoutRuleId,
updateUsername,
createHistoricalPrebuiltRuleAssetSavedObjects,
installPrebuiltRules,
createRuleAssetSavedObject,
} from '../../../utils';
import {
createAlertsIndex,
Expand Down Expand Up @@ -238,6 +241,25 @@ export default ({ getService }: FtrProviderContext) => {
});
});

it('throws an error if rule has external rule source and non-customizable fields are changed', async () => {
// Install base prebuilt detection rule
await createHistoricalPrebuiltRuleAssetSavedObjects(es, [
createRuleAssetSavedObject({ rule_id: 'rule-1', author: ['elastic'] }),
]);
await installPrebuiltRules(es, supertest);

const { body } = await securitySolutionApi
.patchRule({
body: {
rule_id: 'rule-1',
author: ['new user'],
},
})
.expect(400);

expect(body.message).toEqual('Cannot update "author" field for prebuilt rules');
});

describe('max signals', () => {
afterEach(async () => {
await deleteAllRules(supertest, log);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ import {
getSimpleRuleOutputWithoutRuleId,
removeServerGeneratedPropertiesIncludingRuleId,
updateUsername,
createHistoricalPrebuiltRuleAssetSavedObjects,
installPrebuiltRules,
createRuleAssetSavedObject,
} from '../../../utils';
import {
createAlertsIndex,
Expand Down Expand Up @@ -347,6 +350,41 @@ export default ({ getService }: FtrProviderContext) => {
},
]);
});

it('throws an error if rule has external rule source and non-customizable fields are changed', async () => {
// Install base prebuilt detection rule
await createHistoricalPrebuiltRuleAssetSavedObjects(es, [
createRuleAssetSavedObject({ rule_id: 'rule-1', author: ['elastic'] }),
createRuleAssetSavedObject({ rule_id: 'rule-2', license: 'basic' }),
]);
await installPrebuiltRules(es, supertest);

const { body } = await securitySolutionApi
.bulkPatchRules({
body: [
{ rule_id: 'rule-1', author: ['new user'] },
{ rule_id: 'rule-2', license: 'new license' },
],
})
.expect(200);

expect([body[0], body[1]]).toEqual([
{
error: {
message: 'Cannot update "author" field for prebuilt rules',
status_code: 400,
},
rule_id: 'rule-1',
},
{
error: {
message: 'Cannot update "license" field for prebuilt rules',
status_code: 400,
},
rule_id: 'rule-2',
},
]);
});
});
});
};
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ import {
getSimpleMlRuleUpdate,
getSimpleRule,
updateUsername,
createHistoricalPrebuiltRuleAssetSavedObjects,
installPrebuiltRules,
createRuleAssetSavedObject,
} from '../../../utils';
import {
createAlertsIndex,
Expand Down Expand Up @@ -309,6 +312,33 @@ export default ({ getService }: FtrProviderContext) => {
expect(updatedRuleResponse).toMatchObject(expectedRule);
});
});

it('throws an error if rule has external rule source and non-customizable fields are changed', async () => {
// Install base prebuilt detection rule
await createHistoricalPrebuiltRuleAssetSavedObjects(es, [
createRuleAssetSavedObject({ rule_id: 'rule-1', license: 'elastic' }),
]);
await installPrebuiltRules(es, supertest);

const { body: existingRule } = await securitySolutionApi
.readRule({
query: { rule_id: 'rule-1' },
})
.expect(200);

const { body } = await securitySolutionApi
.updateRule({
body: getCustomQueryRuleParams({
...existingRule,
rule_id: 'rule-1',
id: undefined,
license: 'new license',
}),
})
.expect(400);

expect(body.message).toEqual('Cannot update "license" field for prebuilt rules');
});
});
});
};
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ import {
getSimpleRuleUpdate,
getSimpleRule,
updateUsername,
createHistoricalPrebuiltRuleAssetSavedObjects,
installPrebuiltRules,
createRuleAssetSavedObject,
} from '../../../utils';
import {
createAlertsIndex,
Expand Down Expand Up @@ -370,6 +373,30 @@ export default ({ getService }: FtrProviderContext) => {
},
]);
});

it('throws an error if rule has external rule source and non-customizable fields are changed', async () => {
// Install base prebuilt detection rule
await createHistoricalPrebuiltRuleAssetSavedObjects(es, [
createRuleAssetSavedObject({ rule_id: 'rule-1', author: ['elastic'] }),
]);
await installPrebuiltRules(es, supertest);

const { body } = await securitySolutionApi
.bulkUpdateRules({
body: [getCustomQueryRuleParams({ rule_id: 'rule-1', author: ['new user'] })],
})
.expect(200);

expect([body[0]]).toEqual([
{
error: {
message: 'Cannot update "author" field for prebuilt rules',
status_code: 400,
},
rule_id: 'rule-1',
},
]);
});
});
});
};
Loading