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

[System] Fix AccessList & AccessMask processing in security data_stream #2156

Merged
merged 1 commit into from Nov 19, 2021

Conversation

leehinman
Copy link
Contributor

What does this PR do?

Bug fix for processing AccessList and AccessMask in System security data_stream.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
    - [ ] If I'm introducing a new feature, I have modified the Kibana version constraint in my package's manifest.yml file to point to the latest Elastic stack release (e.g. ^7.13.0).

How to test this PR locally

elastic-package test

@elasticmachine
Copy link

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

@elasticmachine
Copy link

elasticmachine commented Nov 11, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Duration: 14 min 17 sec

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@andrewkroh
Copy link
Member

Bug fix for processing AccessList and AccessMask in System security data_stream.

Could you elaborate on what problem this solves or error messages this corrects.

"16901": "Remote Access"
"16902": "Subscribe"
"16903": "Publish"
AccessMaskDescriptions:
Copy link
Member

Choose a reason for hiding this comment

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

What's the source of these values? I recommend adding a comment in case we need to refresh the list in the future.

ArrayList results = new ArrayList();
Long accessMask = Long.decode(ctx.winlog.event_data.AccessMask);
for (entry in params.AccessMaskDescriptions.entrySet()) {
Long accessFlag = Long.decode(entry.getKey());
Copy link
Member

Choose a reason for hiding this comment

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

Ideally it would store the key as a long in order to avoid having to repeatedly parse each string to a number. But IIRC there is a bug in that hex values are always strings anyways (elastic/elasticsearch#66555). So you would have to convert the values to decimal to make that work. Not sure it's worth the loss in clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have similar logic in a few other places. I think I'll leave it as is for now, and we can change them all to Longs. We should be able to keep clarity with some well placed comments.

- According to MS documentation and AccessList contains a space
  separated list of access masks and AccessMask contains an integer.
- Old code treated AccessMask as if it was a space separated list
  of access masks, this was causing script errors.
- Fix code to treat AccessList as space separated list of access masks
- Add new code to parse AccessMask correctly
@leehinman leehinman merged commit c4b104f into elastic:master Nov 19, 2021
@leehinman leehinman deleted the windows_4663 branch November 19, 2021 22:53
eyalkraft pushed a commit to build-security/integrations that referenced this pull request Mar 30, 2022
…ic#2156)

- According to MS documentation and AccessList contains a space
  separated list of access masks and AccessMask contains an integer.
- Old code treated AccessMask as if it was a space separated list
  of access masks, this was causing script errors.
- Fix code to treat AccessList as space separated list of access masks
- Add new code to parse AccessMask correctly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants