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 @@ -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,11 @@ export const patchRule = async ({

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

// We don't allow non-customizable fields to be changed for prebuilt rules
if (existingRule.rule_source && existingRule.rule_source.type === 'external') {
dplumlee marked this conversation as resolved.
Show resolved Hide resolved
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,11 @@ export const updateRule = async ({
throw new ClientError(error.message, error.statusCode);
}

// We don't allow non-customizable fields to be changed for prebuilt rules
if (existingRule.rule_source && existingRule.rule_source.type === 'external') {
dplumlee marked this conversation as resolved.
Show resolved Hide resolved
validateNonCustomizableUpdateFields(ruleUpdate, existingRule);
}

const ruleWithUpdates = await applyRuleUpdate({
prebuiltRuleAssetClient,
existingRule,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import type {
EsqlRule,
NewTermsRule,
QueryRule,
RulePatchProps,
} from '../../../../../common/api/detection_engine';
import {
type ResponseAction,
Expand All @@ -38,6 +39,7 @@ import {
} 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 @@ -128,3 +130,25 @@ function ruleObjectContainsResponseActions(
): rule is Rule<UnifiedQueryRuleParams | EsqlRuleParams | EqlRuleParams | NewTermsRuleParams> {
return rule != null && 'params' in rule && 'responseActions' in rule?.params;
}

export const validateNonCustomizableUpdateFields = (
ruleUpdate: RuleUpdateProps,
existingRule: RuleResponse
) => {
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);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm update logic here, we want to throw on unsetting fields here too, correct? This is the only difference between the update and patch validation methods in this file

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's correct. No changes should be allowed to the fields, including unsetting them.

};

export const validateNonCustomizablePatchFields = (
rulePatch: RulePatchProps,
existingRule: RuleResponse
) => {
if (rulePatch.author && !isEqual(rulePatch.author, existingRule.author)) {
throw new ClientError(`Cannot update "author" field for prebuilt rules`, 400);
} else if (rulePatch.license && rulePatch.license !== existingRule.license) {
dplumlee marked this conversation as resolved.
Show resolved Hide resolved
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',
},
]);
});
});
});
};