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][Event Filters] Warning callout for incomplete code signature entries #193749

Merged
merged 14 commits into from
Oct 4, 2024

Conversation

parkiino
Copy link
Contributor

@parkiino parkiino commented Sep 23, 2024

Summary

Navigate to Security Solution > Manage > Event Filters > Add Event Filter

  • Warning callout shown when code signature field is incomplete (i.e. process.code_signature.subject_name w/o process.code_signature.trusted or vice versa)
  • For mac operating systems, process.code_signature.team_id is also accepted as an equivalent to subject_name
  • Warning callout is also shown for nested entries for this code signature field: process.Ext.code_signature
  • Unit Tests

Paired PR for endpoint exceptions: #198245

Screenshots

image
image

MAC
image

NESTED
image

Followup prs: need to address user being allowed to choose the nested field: process.Ext.code_signature for a non-nested entry, need to address what happens when a user chooses false instead of true for the trusted field option

@parkiino parkiino marked this pull request as ready for review September 23, 2024 17:22
@parkiino parkiino requested review from a team as code owners September 23, 2024 17:22
@parkiino parkiino added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.16.0 labels Sep 23, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-defend-workflows (Team:Defend Workflows)

Comment on lines 1070 to 1089
items[0]?.entries.forEach((e) => {
if (e.type === 'nested' && e.field === 'process.Ext.code_signature') {
e.entries.forEach((n) => {
if (n.field === 'subject_name') {
nestedName = true;
} else if (n.field === 'trusted') {
nestedTrusted = true;
}
});
} else if (
e.field === 'process.code_signature.subject_name' ||
(os.includes('macos') && e.field === 'process.code_signature.team_id')
) {
name = true;
} else if (e.field === 'process.code_signature.trusted') {
trusted = true;
}
});
return name !== trusted || nestedName !== nestedTrusted;
};
Copy link
Contributor

@szwarckonrad szwarckonrad Sep 24, 2024

Choose a reason for hiding this comment

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

I’d recommend simplifying the code to avoid nested iterations. Consider using a single for loop with early returns. Here’s a rough pseudocode example

for (e of entries) {
    if (e.type === 'nested' && e.field === 'process.Ext.code_signature) {
        const includesNestedName = e.entries.some(entry => entry.field === 'subject_name')
        const includesNestedTrusted = e.entries.some(entry => entry.field === 'trusted')
        if (includesNestedName !== includesNestedTrusted) {
             return true // Mismatch found, no need to continue
     } else {
           if (e.field === 'process.code_signature.subject_name' ||
           (os.includes('macos') && e.field === 'process.code_signature.team_id')
    ) {
      name = true;
    } if (e.field === 'process.code_signature.trusted') {
      trusted = true;
    }
   if (name !== trusted) {
       return true; // Mismatch found in outer entries
   }
return false; // No mismatch
}

Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid ? and checks for os you can go with
const { os_types = ['windows'], entries = [] } = items[0] || {};
if items[0] is undefined then it will default to os_types = ['windows'] and empty entries, otherwise it will be overwritten with what items[0].os_types and .entries carries.

Copy link
Member

@ashokaditya ashokaditya Oct 1, 2024

Choose a reason for hiding this comment

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

I'm wondering if either code block (including the suggestion) here is going to make sense in 3 months? Can we break it down into smaller chunks of contextual logic? Something like

isNestedTrustedEntry();
isNestedNameEntry();
isNameEntry();
isTrustedEntry();

and use that to compose a boolean statement?

If you do decide to break them into chunks, perhaps it would be a good idea to write unit tests for those so that we understand what that logic does.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea @ashokaditya!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I tried to incorporate some of your logic for the single for loop with early returns. I could only do it for the nested condition however because the name and trusted fields need to be checked in all the entries.

size="s"
data-test-subj="partialCodeSignatureCallout"
>
<p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I believe you can pass the tagName="p" prop to <FormattedMessage />, and it will render the message inside a

tag.

Copy link
Contributor

@vitaliidm vitaliidm left a comment

Choose a reason for hiding this comment

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

should this logic apply to other code_signature fields like:

Screenshot 2024-09-25 at 15 33 05 Screenshot 2024-09-25 at 15 33 38

])
).toBeFalsy();
});
it('returns true if the entry has code signature subject name but not trusted field or vice versa', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to split into 2 tests

])
).toBeTruthy();
});
it('returns false if the entry has both code signature subject, or team id for mac, name and trusted field or vice versa', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's split it into 2 cases for better maintainability and readability

Comment on lines 1070 to 1089
items[0]?.entries.forEach((e) => {
if (e.type === 'nested' && e.field === 'process.Ext.code_signature') {
e.entries.forEach((n) => {
if (n.field === 'subject_name') {
nestedName = true;
} else if (n.field === 'trusted') {
nestedTrusted = true;
}
});
} else if (
e.field === 'process.code_signature.subject_name' ||
(os.includes('macos') && e.field === 'process.code_signature.team_id')
) {
name = true;
} else if (e.field === 'process.code_signature.trusted') {
trusted = true;
}
});
return name !== trusted || nestedName !== nestedTrusted;
};
Copy link
Member

@ashokaditya ashokaditya Oct 1, 2024

Choose a reason for hiding this comment

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

I'm wondering if either code block (including the suggestion) here is going to make sense in 3 months? Can we break it down into smaller chunks of contextual logic? Something like

isNestedTrustedEntry();
isNestedNameEntry();
isNameEntry();
isTrustedEntry();

and use that to compose a boolean statement?

If you do decide to break them into chunks, perhaps it would be a good idea to write unit tests for those so that we understand what that logic does.

@parkiino parkiino requested a review from vitaliidm October 2, 2024 15:25
Copy link
Contributor

@szwarckonrad szwarckonrad left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for incorporating my suggestions!

Copy link
Contributor

@vitaliidm vitaliidm left a comment

Choose a reason for hiding this comment

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

Thank you for addressing comments on tests.

One question, I still have is regarding other signature fields - #193749 (review)

@parkiino
Copy link
Contributor Author

parkiino commented Oct 3, 2024

should this logic apply to other code_signature fields like:

Hi @vitaliidm so technically that field is a nested only field, which we do check for incomplete code signatures if it's in a nested condition entry. I guess it's a bug that it is shown as an option for non-nested condition entries, but fixing that would be for a different ticket.

@parkiino parkiino enabled auto-merge (squash) October 4, 2024 04:39
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 5920 5921 +1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/securitysolution-exception-list-components 95 96 +1
@kbn/securitysolution-list-utils 161 162 +1
total +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 20.5MB 20.6MB +5.7KB
Unknown metric groups

API count

id before after diff
@kbn/securitysolution-exception-list-components 106 107 +1
@kbn/securitysolution-list-utils 209 211 +2
total +3

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@parkiino parkiino merged commit 61c9137 into elastic:main Oct 4, 2024
40 checks passed
@parkiino parkiino deleted the task/code-signature-warning branch October 4, 2024 16:02
@animehart animehart added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) and removed backport:skip This commit does not require backporting labels Oct 7, 2024
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11211353751

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 7, 2024
…e signature entries (elastic#193749)

## Summary
Navigate to Security Solution > Manage > Event Filters > Add Event
Filter

- [x] Warning callout shown when code signature field is incomplete
(i.e. `process.code_signature.subject_name` w/o
`process.code_signature.trusted` or vice versa)
- [x] For mac operating systems, `process.code_signature.team_id` is
also accepted as an equivalent to `subject_name`
- [x] Warning callout is also shown for nested entries for this code
signature field: `process.Ext.code_signature`
- [x] Unit Tests

# Screenshots

![image](https://github.com/user-attachments/assets/e77cffa7-8b60-4441-9319-aa9964224bb9)

![image](https://github.com/user-attachments/assets/6ec7c6a1-28e8-4f8e-a6aa-3e65b1e0ba1b)

MAC

![image](https://github.com/user-attachments/assets/86354b92-d7e3-44f1-8719-d9791dcaf9cd)

NESTED

![image](https://github.com/user-attachments/assets/1392d7b2-0b63-40b8-95be-8a5bfa2e0af1)

Followup prs: need to address user being allowed to choose the nested
field: `process.Ext.code_signature` for a non-nested entry, need to
address what happens when a user chooses `false` instead of true for the
`trusted` field option

---------

Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit 61c9137)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 7, 2024
…te code signature entries (#193749) (#195184)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Security Solution][Event Filters] Warning callout for incomplete
code signature entries
(#193749)](#193749)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Candace
Park","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-04T06:26:39Z","message":"[Security
Solution][Event Filters] Warning callout for incomplete code signature
entries (#193749)\n\n## Summary\r\nNavigate to Security Solution >
Manage > Event Filters > Add Event\r\nFilter\r\n\r\n- [x] Warning
callout shown when code signature field is incomplete\r\n(i.e.
`process.code_signature.subject_name`
w/o\r\n`process.code_signature.trusted` or vice versa)\r\n- [x] For mac
operating systems, `process.code_signature.team_id` is\r\nalso accepted
as an equivalent to `subject_name`\r\n- [x] Warning callout is also
shown for nested entries for this code\r\nsignature field:
`process.Ext.code_signature`\r\n- [x] Unit Tests\r\n\r\n#
Screenshots\r\n\r\n![image](https://github.com/user-attachments/assets/e77cffa7-8b60-4441-9319-aa9964224bb9)\r\n\r\n![image](https://github.com/user-attachments/assets/6ec7c6a1-28e8-4f8e-a6aa-3e65b1e0ba1b)\r\n\r\nMAC\r\n\r\n![image](https://github.com/user-attachments/assets/86354b92-d7e3-44f1-8719-d9791dcaf9cd)\r\n\r\nNESTED\r\n\r\n![image](https://github.com/user-attachments/assets/1392d7b2-0b63-40b8-95be-8a5bfa2e0af1)\r\n\r\nFollowup
prs: need to address user being allowed to choose the nested\r\nfield:
`process.Ext.code_signature` for a non-nested entry, need to\r\naddress
what happens when a user chooses `false` instead of true for
the\r\n`trusted` field option\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"61c9137a1eeb1548e1878110194abc173fe64724","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:Defend
Workflows","backport:prev-minor","v8.16.0"],"title":"[Security
Solution][Event Filters] Warning callout for incomplete code signature
entries","number":193749,"url":"https://github.com/elastic/kibana/pull/193749","mergeCommit":{"message":"[Security
Solution][Event Filters] Warning callout for incomplete code signature
entries (#193749)\n\n## Summary\r\nNavigate to Security Solution >
Manage > Event Filters > Add Event\r\nFilter\r\n\r\n- [x] Warning
callout shown when code signature field is incomplete\r\n(i.e.
`process.code_signature.subject_name`
w/o\r\n`process.code_signature.trusted` or vice versa)\r\n- [x] For mac
operating systems, `process.code_signature.team_id` is\r\nalso accepted
as an equivalent to `subject_name`\r\n- [x] Warning callout is also
shown for nested entries for this code\r\nsignature field:
`process.Ext.code_signature`\r\n- [x] Unit Tests\r\n\r\n#
Screenshots\r\n\r\n![image](https://github.com/user-attachments/assets/e77cffa7-8b60-4441-9319-aa9964224bb9)\r\n\r\n![image](https://github.com/user-attachments/assets/6ec7c6a1-28e8-4f8e-a6aa-3e65b1e0ba1b)\r\n\r\nMAC\r\n\r\n![image](https://github.com/user-attachments/assets/86354b92-d7e3-44f1-8719-d9791dcaf9cd)\r\n\r\nNESTED\r\n\r\n![image](https://github.com/user-attachments/assets/1392d7b2-0b63-40b8-95be-8a5bfa2e0af1)\r\n\r\nFollowup
prs: need to address user being allowed to choose the nested\r\nfield:
`process.Ext.code_signature` for a non-nested entry, need to\r\naddress
what happens when a user chooses `false` instead of true for
the\r\n`trusted` field option\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"61c9137a1eeb1548e1878110194abc173fe64724"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193749","number":193749,"mergeCommit":{"message":"[Security
Solution][Event Filters] Warning callout for incomplete code signature
entries (#193749)\n\n## Summary\r\nNavigate to Security Solution >
Manage > Event Filters > Add Event\r\nFilter\r\n\r\n- [x] Warning
callout shown when code signature field is incomplete\r\n(i.e.
`process.code_signature.subject_name`
w/o\r\n`process.code_signature.trusted` or vice versa)\r\n- [x] For mac
operating systems, `process.code_signature.team_id` is\r\nalso accepted
as an equivalent to `subject_name`\r\n- [x] Warning callout is also
shown for nested entries for this code\r\nsignature field:
`process.Ext.code_signature`\r\n- [x] Unit Tests\r\n\r\n#
Screenshots\r\n\r\n![image](https://github.com/user-attachments/assets/e77cffa7-8b60-4441-9319-aa9964224bb9)\r\n\r\n![image](https://github.com/user-attachments/assets/6ec7c6a1-28e8-4f8e-a6aa-3e65b1e0ba1b)\r\n\r\nMAC\r\n\r\n![image](https://github.com/user-attachments/assets/86354b92-d7e3-44f1-8719-d9791dcaf9cd)\r\n\r\nNESTED\r\n\r\n![image](https://github.com/user-attachments/assets/1392d7b2-0b63-40b8-95be-8a5bfa2e0af1)\r\n\r\nFollowup
prs: need to address user being allowed to choose the nested\r\nfield:
`process.Ext.code_signature` for a non-nested entry, need to\r\naddress
what happens when a user chooses `false` instead of true for
the\r\n`trusted` field option\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"61c9137a1eeb1548e1878110194abc173fe64724"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Candace Park <[email protected]>
tiansivive pushed a commit to tiansivive/kibana that referenced this pull request Oct 7, 2024
…e signature entries (elastic#193749)

## Summary
Navigate to Security Solution > Manage > Event Filters > Add Event
Filter

- [x] Warning callout shown when code signature field is incomplete
(i.e. `process.code_signature.subject_name` w/o
`process.code_signature.trusted` or vice versa)
- [x] For mac operating systems, `process.code_signature.team_id` is
also accepted as an equivalent to `subject_name`
- [x] Warning callout is also shown for nested entries for this code
signature field: `process.Ext.code_signature`
- [x] Unit Tests

# Screenshots

![image](https://github.com/user-attachments/assets/e77cffa7-8b60-4441-9319-aa9964224bb9)

![image](https://github.com/user-attachments/assets/6ec7c6a1-28e8-4f8e-a6aa-3e65b1e0ba1b)

MAC

![image](https://github.com/user-attachments/assets/86354b92-d7e3-44f1-8719-d9791dcaf9cd)

NESTED

![image](https://github.com/user-attachments/assets/1392d7b2-0b63-40b8-95be-8a5bfa2e0af1)

Followup prs: need to address user being allowed to choose the nested
field: `process.Ext.code_signature` for a non-nested entry, need to
address what happens when a user chooses `false` instead of true for the
`trusted` field option

---------

Co-authored-by: kibanamachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants