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

Digital code signatures for process, file and dll events #733

Merged
merged 12 commits into from
Feb 17, 2020
Merged

Conversation

rw-access
Copy link
Contributor

Closes #681

I'm not 100% sold on anything yet. I think there may be room to break out this x.509 information into a separate field set. Ideally in a way that's still intuitive enough in naming. This information is pretty sparsely populated by most endpoint solutions. It's typically just signature status -- valid y/n, trusted y/n, etc. as well as the name of the signer (subject name).

@rw-access rw-access added the endpoint Relevant to elastic endpoint security label Jan 24, 2020
@rw-access rw-access requested a review from webmat January 24, 2020 21:39
@webmat
Copy link
Contributor

webmat commented Feb 4, 2020

In order to review this, I'd like to see some data examples from the Mac and Windows platforms.

I tried viewing some on both, and I'd like to confirm how they map with what you're proposing here. Perhaps I'm way off track, please correct me if that's the case :-)

Mac OS

checking the code signature for Safari.app:

$ codesign -dv --verbose=4 /Applications/Firefox.app
Executable=/Applications/Safari.app/Contents/MacOS/Safari
Identifier=com.apple.Safari
Format=app bundle with Mach-O thin (x86_64)
CodeDirectory v=20100 size=321 flags=0x2000(library-validation) hashes=3+5 location=embedded
VersionPlatform=1
VersionMin=658944
VersionSDK=658944
Hash type=sha256 size=32
CandidateCDHash sha256=96d97014be33f78e4b7ccec056f34512c21462d3
Hash choices=sha256
Page size=4096
CDHash=96d97014be33f78e4b7ccec056f34512c21462d3
Signature size=4547
Authority=Software Signing
Authority=Apple Code Signing Certification Authority
Authority=Apple Root CA
Info.plist entries=41
TeamIdentifier=not set
Sealed Resources version=2 rules=13 files=2227
Internal requirements count=1 size=64

In this case, the best candidate I see for code_signature.subject_name is one of the "Authority" entries. .valid and .exists are straightforward to populate as well.

Not sure about .trusted nor .status. Do you expect these to be available on the Mac?

Windows

checking the code signature for the IE executable:

IE signature on Windows

And:

PS C:\Users\vagrant> Get-AuthenticodeSignature -FilePath "C:\Program Files\Internet Explorer\iexplore.exe"


    Directory: C:\Program Files\Internet Explorer


SignerCertificate                         Status                                 Path
-----------------                         ------                                 ----
108E2BA23632620C427C570B6D9DB51AC31387FE  Valid                                  iexplore.exe

In the Windows case, I don't see a signer name. Unless that would be "Copyright" (doesn't sound right to me)?

  • .valid and .exists seem straightforward to populate just like the Mac
  • How is .trusted populated?
  • And I suppose "Status" from "Get-AuthenticodeSignature" would populate .status?
    • I get "UnknownError" if I check the signature on an unsigned file.

@rw-access
Copy link
Contributor Author

re: trusted/status -- those don't exist in the file, but have to be evaluated by the client. for example, the Elastic Endpoint calls several Windows APIs to verify that a signature is cryptographically correct, and then there are additional steps to validate the trust level of the certificate. we get a message as a result "trusted", "noSignature", "untrustedRoot", "badDigest", etc depending on the error message. we typically check for trusted or noSignature and signer. having valid and exists as their own fields simplifies a lot of the rule logic

@rw-access
Copy link
Contributor Author

rw-access commented Feb 4, 2020

For the PowerShell command there should be a lot more fields that aren't being shown.

That screenshot is actually PE data, not signer information. You'd have to click on the "Digital Signatures" tab. That would show this:
image

Sigcheck is the most analogous tool to codesign for windows. That output looks like this, which also mixes in some PE metadata:
image

Here's its output with a broken signature:
image

@webmat
Copy link
Contributor

webmat commented Feb 5, 2020

Thanks for the pointers. Pretty awesome, how I skipped right over the "Digital Signatures" tab, there 😂 🤦‍♂

@elasticmachine, run elasticsearch-ci/docs

@webmat
Copy link
Contributor

webmat commented Feb 5, 2020

I just realized that Python interprets the YAML true, then outputs it as "True", which is not actually considered a valid boolean by Elasticsearch 🤦‍♂

Can you remove the examples for now? This is the only place in ECS where we give examples for boolean fields. So props for being thorough here, but we'll fix that small issue separately :-)

docs/field-details.asciidoc Outdated Show resolved Hide resolved
@webmat
Copy link
Contributor

webmat commented Feb 6, 2020

@elasticmachine, run elasticsearch-ci/docs

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

Thanks for adjusting the boolean examples. Looks better now.

I have a few adjustments I think are needed, and a potential can of worms again 😬

Unless we address the can of worms, this is pretty close to being ready, though :-) On top of each comment below as todo's

  • don't forget to merge master to get rid of the conflict on the CSV file

Thanks Ross!

expected:
- file
- process
# - dll
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 merge this PR before DLL as well, and add this reuse of code_signature via the DLL PR (#679) as well.

description: >
Boolean to capture if a signature is present.

This should only populated if the signature was checked.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should only populated if the signature was checked.

Why? Isn't this overloading the definition of code_signature.exists? Whether a signature exists is orthogonal to whether its validity has been assessed, IMO.

The ternary approach would make more sense on code_signature.valid, though:

  • haven't checked validity: code_signature.valid is absent
  • checked validity, good: code_signature.valid=true
  • checked validity, bad: code_signature.valid=false

Or am I missing something?

If you agree, we should adjust the definitions of valid and exists accordingly.

- name: code_signature
title: Code Signature
group: 2
description: These fields contain information about binary code signatures.
Copy link
Contributor

Choose a reason for hiding this comment

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

What about a shorter short definition, and a more fleshed out full definition?

Suggested change
description: These fields contain information about binary code signatures.
description: These fields contain information about code signatures for various forms of executable code, such as executables, libraries or scripts.
short: Information about binary code signatures.

And the above begs the question: will we be wanting to track signatures other than code signatures? If so, do we want to discuss whether this field set should be named signature instead? Happy to close that can of worms immediately, if you think it's an unlikely hypothetical ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i thought about that, and the big downside I came up with is that the word "signature" is often used like "rule", so I didn't want to potentially clobber up a namespace that may be used.

I definitely want something shorter than code_signature, because process.code_signature.valid is lengthy

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 we should stick with code_signature, then. Are you good with that?

schemas/code_signature.yml Outdated Show resolved Hide resolved
schemas/code_signature.yml Outdated Show resolved Hide resolved
schemas/code_signature.yml Show resolved Hide resolved
description: >
Boolean to capture if a signature is present.

This should only populated if the signature was checked.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This should only populated if the signature was checked.
This should only be populated if the signature was checked.

schemas/code_signature.yml Outdated Show resolved Hide resolved
@webmat
Copy link
Contributor

webmat commented Feb 10, 2020

I spotted two more small details, which should be fixed. Once fixed, and if you're good with sticking with the name code_signature for this field set, I think we'll be ready to merge

@rw-access
Copy link
Contributor Author

rw-access commented Feb 10, 2020

I had another thought... Would it make sense to just include this within pe as pe.signature? Maybe I'm just trying to avoid the lengthy code_signature

@webmat
Copy link
Contributor

webmat commented Feb 11, 2020

@rw-access Well PE is named really Windows-specific. And code signatures are expected on Mac OS as well.

So I'd rather keep them separate.

@rw-access rw-access changed the title Digital code signatures for process, file, dll and driver events Digital code signatures for process, file and dll events Feb 12, 2020
Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

Alright, looking great! Thanks @rw-access

@webmat webmat merged commit 007f604 into master Feb 17, 2020
webmat pushed a commit to webmat/ecs that referenced this pull request Mar 4, 2020
- code_signature (elastic#733)
- second entry for elastic#739 in the schema section, mentioning the addition of `process.parent.hash`

Also adjusted the wording of elastic#731 and elastic#747.
@rw-access rw-access deleted the code-signature branch March 24, 2020 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
endpoint Relevant to elastic endpoint security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code signature field set
2 participants