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

BED-4965 - Owns/Owner Rework #993

Merged
merged 39 commits into from
Feb 6, 2025
Merged

BED-4965 - Owns/Owner Rework #993

merged 39 commits into from
Feb 6, 2025

Conversation

Mayyhem
Copy link
Contributor

@Mayyhem Mayyhem commented Dec 2, 2024

Description

This change was made to account for cases where the OWNER RIGHTS SID (S-1-3-4) is explicitly granted permissions on AD objects, which in some cases renders implicit owner rights non-abusable.

  • Display Owner SID in entity panel
  • Create OwnsLimitedRights, OwnsRaw, WriteOwnerLimitedRights, and WriteOwnerRaw edges during ingest
  • Create Owns and WriteOwner edges during post-processing

Please refer to https://specterops.atlassian.net/wiki/spaces/BE/pages/750157858/Owns+WriteOwner for details.

Motivation and Context

This PR addresses: https://specterops.atlassian.net/browse/BED-4965

How Has This Been Tested?

Integration tests were written for post-processed edges. Ingest edges were tested manually.

A test environment including test cases for OWNER RIGHTS permissions was configured using a PowerShell script included in the linked Confluence page. After generating test data for two domains, one where implicit owner rights were blocked and another where they were not, tests included:

... and confirming that expected ingest and post-processed edges were created or removed, where appropriate.

Also ran and passed:

  • just test
  • View > Testing > Go (all tests)
     
    This change does not account for inheritance hashes for ACEs but will require an update after that initiative is complete.

Screenshots (optional):

Types of changes

  • Chore (a change that does not modify the application functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Database Migrations

Checklist:

cmd/api/src/daemons/datapipe/convertors.go Show resolved Hide resolved
packages/go/analysis/ad/owns.go Outdated Show resolved Hide resolved
packages/go/analysis/ad/owns.go Outdated Show resolved Hide resolved
packages/go/analysis/ad/owns.go Outdated Show resolved Hide resolved
packages/go/analysis/ad/owns.go Outdated Show resolved Hide resolved
packages/go/analysis/ad/owns.go Outdated Show resolved Hide resolved
packages/go/analysis/ad/owns.go Outdated Show resolved Hide resolved
packages/go/analysis/ad/owns.go Outdated Show resolved Hide resolved
Copy link
Contributor

@zinic zinic left a comment

Choose a reason for hiding this comment

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

Added a commit to the changeset that highlights a few requested fixes via FIXME comments. Additionally we'd ask that the following changes also be made:

Please adhere to the standard outlined here for your conditional statements: https://specterops.atlassian.net/wiki/spaces/DEVRES/pages/1507363/Golang+Coding+and+Formatting+Standards#GolangCodingandFormattingStandards-InlineallInitializerStatements

rvazarkar and others added 23 commits February 6, 2025 13:18
* Added inheritance condition for WriteOwner abuse, added comments to Owns/WriteOwner logic

* Saving changes to post Owns/WriteOwner
… computer object ownership and WriteOwner permissions
@zinic zinic merged commit 09f9a54 into main Feb 6, 2025
8 checks passed
@zinic zinic deleted the BED-4965 branch February 6, 2025 22:07
@github-actions github-actions bot locked and limited conversation to collaborators Feb 6, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants