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

Error when merging imported entities #6559

Closed
yassine-ouaamou opened this issue Apr 3, 2024 · 14 comments · Fixed by #6683
Closed

Error when merging imported entities #6559

yassine-ouaamou opened this issue Apr 3, 2024 · 14 comments · Fixed by #6683
Assignees
Labels
bug use for describing something not working as expected solved use to identify issue that has been solved (must be linked to the solving PR)

Comments

@yassine-ouaamou
Copy link
Member

Description

When importing a bundle with an intrusion set having an alias which is similar to an existing one. The name of imported Intrusion Set is ignored (instead of being merged and used as an alias) and the id is also lost (instead of being added to STIX IDs)

Environment

Testing

Reproducible Steps

Steps to create the smallest reproducible scenario:

  1. Create an intrusion set called "ccccccccccccccc"
  2. Import the following bundle using the API (import_bundle_from_file). For this, I used the following script: https://github.com/OpenCTI-Platform/client-python/blob/master/examples/import_stix2_file.py

json-import.json

Expected Output

Intrusion Set names, aliases and IDs should be merged.

Actual Output

The name of imported Intrusion Set is ignored (instead of being merged and used as an alias) and the id is also lost (instead of being added to STIX IDs).
MISSING_REFERENCE_ERROR because the bundle contains a relationship between the concerned IS and a malware and the STIX is not recognized (because it was not merged)

Additional information

Screenshots (optional)

@yassine-ouaamou yassine-ouaamou added bug use for describing something not working as expected needs triage use to identify issue needing triage from Filigran Product team labels Apr 3, 2024
@nino-filigran
Copy link

nino-filigran commented Apr 4, 2024

Since I don't know how directly call the API, I've made a test using the workbench, and I have some similar issues:
image

Additionally, this is what is found when uploading the file to the workbench:

  • 1 malware, called "malware test", with description "ok" and label "malware
  • 1 intrusion set called testtesttest with description "ok"
  • 1 relation malwaretest authored by testestest
  • 1 container being a report called test

I need help to reproduce @yassine-ouaamou

@nino-filigran nino-filigran added needs more info Intel needed about the use case and removed needs triage use to identify issue needing triage from Filigran Product team needs more info Intel needed about the use case labels Apr 4, 2024
@nino-filigran
Copy link

Could not reproduce given I do not have a dev nor python client installed.

@yassine-ouaamou
Copy link
Member Author

If anyone wants to work on this and needs help reproducing, ping me please

@SamuelHassine SamuelHassine added this to the Release 6.0.10 milestone Apr 4, 2024
@marieflorescontact marieflorescontact self-assigned this Apr 5, 2024
@SamuelHassine SamuelHassine modified the milestones: Release 6.0.10, Release 6.1.0 Apr 8, 2024
@marieflorescontact
Copy link
Member

marieflorescontact commented Apr 9, 2024

after setting the confidence to 100 in the bundle, the merge was done correctly and the relationships were created without error.
1 question remains:

  • the entity's standardID has been replaced by the standardId from the bundle, but in elastic x_opencti_stix_ids is empty. Shouldn't it contain the entity's initial standardID?

@marieflorescontact marieflorescontact added the needs more info Intel needed about the use case label Apr 10, 2024
@nino-filigran
Copy link

nino-filigran commented Apr 10, 2024

Ouput:

  • if entity has lower conf level than something existing in the platform: the existing entity in platform will not be upserted except for the fields that do not exists. The stix ID remains the same, but the stix ID of the entity that is coming from the upload is not stored.
  • if entity has same or higher onf level than something existing in the platform: the entity will be upserted. Also, the previous standard ID will be lost and replaced with a new one. If the entity in the file had a stix ID, this stix ID will be stored in x_opencti_stix_ids

The question is:

  • should we keep in x_opencti_stix_ids the previous standard stix ID in case of lower conf level for the uploaded entity order to not lose track of things?
  • if the standard id has been lost, should we also keep it in x_opencti_stix_ids?

@nino-filigran
Copy link

@richard-julien given this is rather a technical question that could you have a look and let us what you think?

@richard-julien
Copy link
Member

richard-julien commented Apr 11, 2024

stix_ids must be cumulated independently of the confidence level, there is clearly a problem here.

Blind change proposal in upsertElement function

  if (isNotEmptyField(basePatch.stix_id) || isNotEmptyField(basePatch.x_opencti_stix_ids)) {
    const idsToAdd = [...(basePatch.x_opencti_stix_ids || [])];
    if (isNotEmptyField(basePatch.stix_id) && basePatch.stix_id !== element.standard_id && !idsToAdd.includes(basePatch.stix_id)) {
      idsToAdd.push(basePatch.stix_id);
    }
    if (idsToAdd.length > 0) {
      updatePatch.x_opencti_stix_ids = idsToAdd;
    }
  }

A specific backend test must also be added.

@richard-julien richard-julien added critical use to identify critical bug to fix ASAP and removed needs more info Intel needed about the use case labels Apr 11, 2024
@SamuelHassine
Copy link
Member

@Kedae @richard-julien @marieflorescontact @nino-filigran: this is critical indeed.

@SamuelHassine SamuelHassine modified the milestone: Release 6.1.0 Apr 12, 2024
@SouadHadjiat SouadHadjiat self-assigned this Apr 12, 2024
@SouadHadjiat
Copy link
Member

stix_ids must be cumulated independently of the confidence level, there is clearly a problem here.

Blind change proposal in upsertElement function

  if (isNotEmptyField(basePatch.stix_id) || isNotEmptyField(basePatch.x_opencti_stix_ids)) {
    const idsToAdd = [...(basePatch.x_opencti_stix_ids || [])];
    if (isNotEmptyField(basePatch.stix_id) && basePatch.stix_id !== element.standard_id && !idsToAdd.includes(basePatch.stix_id)) {
      idsToAdd.push(basePatch.stix_id);
    }
    if (idsToAdd.length > 0) {
      updatePatch.x_opencti_stix_ids = idsToAdd;
    }
  }

A specific backend test must also be added.

The code has been changed last month with this commit @richard-julien 8733303#diff-ad0de0206f921a6f08856e3f7b7f4384921bbd62a64c290fb6b8610140784a64 and it was almost the same as your proposal.

@SamuelHassine
Copy link
Member

I've tested on my side and IDs seem to be cumulated properly:

Creation

mutation {
  intrusionSetAdd(
    input: {
      name: "Intrusion Set Test"
      confidence: 75
      stix_id: "intrusion-set--18dee3ab-80cf-4c03-a8ed-d838734fd774"
    }
  ) {
    id
    name
    confidence
    x_opencti_stix_ids
  }
}

Result:

{
  "data": {
    "intrusionSetAdd": {
      "id": "22b36bec-1682-4171-8edf-10665bebbfd4",
      "name": "Intrusion Set Test",
      "confidence": 75,
      "x_opencti_stix_ids": [
        "intrusion-set--18dee3ab-80cf-4c03-a8ed-d838734fd774"
      ]
    }
  }
}

Second creation with a lower confidence level

mutation {
  intrusionSetAdd(
    input: {
      name: "Intrusion Set Test"
      confidence: 10
      stix_id: "intrusion-set--1d7a9036-d1cb-4479-955c-209f9c7140f5"
    }
  ) {
    id
    name
    confidence
    x_opencti_stix_ids
  }
}

Result:

{
  "data": {
    "intrusionSetAdd": {
      "id": "22b36bec-1682-4171-8edf-10665bebbfd4",
      "name": "Intrusion Set Test",
      "confidence": 75,
      "x_opencti_stix_ids": [
        "intrusion-set--18dee3ab-80cf-4c03-a8ed-d838734fd774",
        "intrusion-set--1d7a9036-d1cb-4479-955c-209f9c7140f5"
      ]
    }
  }
}

So not really sure what we are talking about here.

@SamuelHassine
Copy link
Member

SamuelHassine commented Apr 12, 2024

Also, I did the same queries but using x_opencti_stix_ids in the ìnput instead of stix_id and it is also cumulated properly.

@SamuelHassine
Copy link
Member

Ok, after digging in a little more, I understand this is a edge case. Less critical than expected but still very important.

@SouadHadjiat
Copy link
Member

SouadHadjiat commented Apr 15, 2024

More details about the issue @SamuelHassine :

  • We have an entity A in the platform.
  • We ingest entity B that has A's name in its aliases but has a different name (& different standard id). B has a lower confidence level than A.
  • When trying to ingest B, we find A (because of the shared alias). We then upsert A with informations of B. But since A has a higher confidence level, we keep A as it is, only add missing information (aliases of B) but we don't change the standard id, and we don't save the standard id of B in A.x_opencti_stix_ids
  • When trying to ingest B's relationships from the bundle, since they have B standard id, we don't find it and we through a missing reference error.

We found the part of the code that needs to change, but this has been changed recently to avoid accumulating standard ids in x_opencti_stix_ids. Should we revert this change ? see this issue #6253

@SamuelHassine
Copy link
Member

@SouadHadjiat Let's discuss this tomorrow, we have to keep the behaviour as it is, we took a lot of time to made this change. But this is something else I think.

@Kedae Kedae removed the critical use to identify critical bug to fix ASAP label Apr 17, 2024
SamuelHassine added a commit that referenced this issue Apr 18, 2024
Co-authored-by: Samuel Hassine <samuel.hassine@filigran.io>
Co-authored-by: Souad Hadjiat <souad.hadjiat@filigran.io>
@SamuelHassine SamuelHassine added the solved use to identify issue that has been solved (must be linked to the solving PR) label Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug use for describing something not working as expected solved use to identify issue that has been solved (must be linked to the solving PR)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants