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

Skip SID translation for capability SIDs #48

Merged
merged 2 commits into from
Aug 19, 2024

Conversation

cosmo0920
Copy link
Member

@cosmo0920 cosmo0920 commented Aug 14, 2024

S-1-15-3- prefixed SIDs are used for indicating capabilities and not mapped for the actual users.
We need to skip SID translation to eliminate the needless trying to translate SIDs.

ref: https://learn.microsoft.com/en-us/windows-server/identity/ad-ds/manage/understand-security-identifiers#capability-sids

@cosmo0920 cosmo0920 requested review from ashie, kenhys and daipom August 14, 2024 13:48
@daipom
Copy link

daipom commented Aug 15, 2024

Thanks! I will see this tomorrow.

@daipom
Copy link

daipom commented Aug 16, 2024

Thanks for this fix!
It looks good to me from a coding perspective, but I have a question about the purpose.
I understand it as follows. Is it correct?

  • Some capability SIDs will cause ERROR_NONE_MAPPED.
  • Some capability SIDs can be translated, but we are not happy with it.
  • For performance improvements, all capability SIDs should be skipped.

@cosmo0920
Copy link
Member Author

cosmo0920 commented Aug 16, 2024

  • Some capability SIDs will cause ERROR_NONE_MAPPED.

All of the capability SIDs will cause ERROR_NONE-MAPPED.
They are stored in HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\SecurityManager\CapabilityClasses\AllCachedCapabilities.

image

In the screenshot, they have S-1-15-3- prefix.

A part of the performance perspectives, all of the capability SIDs should be skipped for SID translations.

Plus, Active Directory does not resolve the actual usernames with those SIDs.

@daipom
Copy link

daipom commented Aug 16, 2024

Thanks!

All of the capability SIDs will cause ERROR_NONE-MAPPED.

On my local Windows 10 Home, I can confirm some simple capability SIDs can be translated.
I use PsGetSid.

$ PsGetsid S-1-15-3-1

PsGetSid v1.45 - Translates SIDs to names and vice versa
Copyright (C) 1999-2016 Mark Russinovich
Sysinternals - www.sysinternals.com

Account for XXX:
Well Known Group: APPLICATION PACKAGE AUTHORITY\インターネット接続

I'm not sure, but it looks like S-1-15-3-1 ~ S-1-15-3-10 are well-known capability SIDs, and they have SID names.
These 10 SIDs may be the exceptions.

Sorry, I'm not familiar with it.
If we can ignore these, this fix will be fine!

ext/winevt/winevt_utils.cpp Outdated Show resolved Hide resolved
Copy link

@daipom daipom left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for this enhancement!

Should we release v0.11.1 with this fix?

@ashie ashie merged commit 5e1d6ee into master Aug 19, 2024
8 checks passed
@ashie ashie deleted the skip-sid-translation-for-capability-sids branch August 19, 2024 05:10
@cosmo0920
Copy link
Member Author

Should we release v0.11.1 with this fix?

Yes. Let's release a point release if y'all have a cycle. It's not urgent but we need to release at the some point of future.

@ashie
Copy link
Member

ashie commented Aug 19, 2024

Should we release v0.11.1 with this fix?

I'll release it.

@daipom
Copy link

daipom commented Aug 19, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants