Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add several new observable types #1326

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

MiDoCisco
Copy link
Contributor

@MiDoCisco MiDoCisco commented Jan 27, 2025

Related Issue: N/A

Description of changes:

We are populating the observables with values which are notable for correlation or display to the users. There are several values, which we think make good observables, but are not marked as such. This PR tries to add them.

These are the details:

  1. Added email.subject, email.uid and message_uid as an Observable types - type_id: 39, type_id: 40 and 45.
  2. Added process_entity.uid as an Observable type - type_id: 41
  3. Added file_path_t as an Observable type - type_id: 42 and marked fields as this type
    • lineage dictionary attribute
    • affected_package.path object attribute
    • file.path object attribute
    • image.path object attribute
    • kernel.path object attribute
    • malware.path object attribute
    • process_entity.path object attribute
  4. Added extensions/windows/reg_key_path_t as an Observable type - type_id: 43 and marked fields as this type
    • reg_key.path object attribute
    • reg_value.path object attribute
  5. Added reg_value.name as an Observable type - type_id: 44

I have updated CHANGELOG.md with the same granularity of the information it already uses, but it's not a "single-line" description. Should I reduce the detail to make it single line?

Note On Compatibility

There is a failing compatibility validator, as I changed some fields from string_t to the new file_path_t and reg_key_path_t - as this seem to be the only way to mark several different fields as same observable. Since both old and new types are strings (the new one just adds semantics) I don't see how this could break type compatibility (e.g. in generated classes). But I am happy to learn about any concerns and also whether there is a better (more compatible) way to mark multiple fields with the same observable type.

@floydtree floydtree added the v1.5.0 Items to be considered for OCSF v1.5.0 label Jan 27, 2025
@MiDoCisco MiDoCisco force-pushed the add-new-observable-types branch 2 times, most recently from 9ca7d3e to 14abf4d Compare February 4, 2025 11:05
---------

Signed-off-by: Michal Dobisek <mdobisek@cisco.com>
---------

Signed-off-by: Michal Dobisek <mdobisek@cisco.com>
---------

Signed-off-by: Michal Dobisek <mdobisek@cisco.com>
---------

Signed-off-by: Michal Dobisek <mdobisek@cisco.com>
---------

Signed-off-by: Michal Dobisek <mdobisek@cisco.com>
@MiDoCisco MiDoCisco force-pushed the add-new-observable-types branch from 14abf4d to 30632a1 Compare February 12, 2025 11:43
@MiDoCisco
Copy link
Contributor Author

The compatibility validation complains here, because some attributes changed from string_t to a new custom type which is basically string, but with observable type set. That's unavoidable outcome of the change this PR aims to do.
However, as far as I am aware, this should not cause any compatibility issue, in e.g. generated classes, because the value was string and still is string.

I am not sure what is the correct handling of such case - should some label be added to indicate the breakage is expected?

  [ERROR] Type of object malware.path changed from string_t to file_path_t
  [ERROR] Type of object file.path changed from string_t to file_path_t
  [ERROR] Type of object process.lineage changed from string_t to file_path_t
  [ERROR] Type of object win/reg_key.path changed from string_t to reg_key_path_t
  [ERROR] Type of object win/reg_value.path changed from string_t to reg_key_path_t
  [ERROR] Type of object image.path changed from string_t to file_path_t
  [ERROR] Type of object affected_package.path changed from string_t to file_path_t
  [ERROR] Type of object kernel.path changed from string_t to file_path_t

@floydtree
Copy link
Contributor

@MiDoCisco This was a topic of discussion in the yesterday's weekly community call. While I agree, since we aren't changing the base/physical type and only introducing a new logical type, it technically should not be a breaking change. However, @hmadison had a few concerns (I'll let him expand on those here), that we need to deliberate over before we finalize framework's strategy about such changes going forward.

Now, coming to the specific outcome you are looking for in this PR, we can solve it without introducing a new logical type - You just need to add observable definition in the core dictionary.json, where we define path. e.g. -

"path": {
      "observable": 42, // the added line
      "caption": "Path",
      "description": "The path that pertains to the event or object. See specific usage.",
      "type": "string_t"
    },

By doing this, all instances of path attribute will be considered an observable.

@MiDoCisco
Copy link
Contributor Author

Thanks @floydtree for the explanation and proposal. I have initially planned to make the path in dictionary.json an observable, but when I looked up it's usages and it could mean many things:

  • filesystem path
  • url path
  • registry path

and it seemed to me that it would be wrong to define all of them as single observable. But if this worry would not be shared by others, I am happy to do as you suggest.

@hmadison
Copy link
Contributor

However, @hmadison had a few concerns (I'll let him expand on those here), that we need to deliberate over before we finalize framework's strategy about such changes going forward.

My general concern is that if we are going from a string_t to something narrow and specialized like uuid_t, we will need to account for the impact that will have on storage. While we (mostly) treat types in OCSF as aliases on top of primitive values, there are situations like with uuid_t, or ip_t where constraining the type would open up more storage savings or search optimizations. For those two, it's possible to represent each as an integer (of varying size) which, at scale, adds up to a large storage savings.

While in the case of path_t it's not very clear to me that there's some storage or search optimization that can be performed. I would feel better if there was a "more documented" way to go and express these kinds of type changes.

@MiDoCisco
Copy link
Contributor Author

While in the case of path_t it's not very clear to me that there's some storage or search optimization that can be performed. I would feel better if there was a "more documented" way to go and express these kinds of type changes.

Thanks @hmadison so if I understand correctly, you are more after opening discussion/defining guidelines than requesting specific action for this PR (like avoid the new types in favour of marking generic path as observable)? Those actions might eventually arise from that discussion.

@pagbabian-splunk
Copy link
Contributor

We are looking at this PR in particular, and this one data type promotion in particular. It's very clear that promoting any base type to some new data type will be hard to corral and likely be breaking.

Even for this one, file_path_t which is not just any path, but just a file path, we could find that we weren't very strict with our attributes that are supposed to be file paths (i.e. are there any that don't have path in their name?). We'd have to to go through and make sure, although not the worst thing in the world, given they can be promoted when we encounter them since we would say it's not breaking, but only for file path.

Hence, we can't generally change the validator for string_t promotions. We would have to special case only string_t to file_path_t. I'm not sure about it, vs. how important it is for all paths, not just specific ones for specific attributes, we should address.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
v1.5.0 Items to be considered for OCSF v1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants