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

feat(crowdsec): Update module (v2.1.1) #687

Merged
merged 3 commits into from
Aug 24, 2024

Conversation

julienloizelet
Copy link
Contributor

@julienloizelet julienloizelet commented Aug 22, 2024

Added

  • Add attribute and tag for reputation, mitre techniques and cves
  • Add config to enable/disable tag creation for IP attributes

Notes/Questions

We're in the process of improving the crowdsec module so that it has more options and the template reflects the new fields.

We have a questions concerning MISP-Objects
in particular ours: see here

  1. We've noticed that the ip-range property has an associated misp-attribute "ip-src", which is incorrect and can cause problems if we try to query Crowdsec CTI on it. We've fixed it in this PR, but let us know if it breaks anything.

  2. In this PR, we've passed in a custom "crowdsec_template" object during MISP object instantiation, so we can be sure that a change to the MISP object won't break the module.
    We imagine that this isn't necessarily good practice, and that you'd prefer us to update the MISP/misp-objects repo.
    If we update this repo, is there a risk that someone will change it in the future and break our CrowdSec module without an alert being raised?
    Finally, should we make a PR on the https://github.com/MISP/misp-objects repo first, and modify this PR (by removing the "crowdsec_template" custom object) once the repo misp-object PR has been merged?

Thanks

@adulau
Copy link
Member

adulau commented Aug 22, 2024

Thank you very much for the update. Here is some notes:

  • ip-src can be a CIDR block, if you want to use range in MISP. I saw that you replace it with text which is fine too but you won't benefit from the advanced correlation in MISP if you have an IP matching a CIDR block for example. If the CrowdSec ip-range, it would be nice to split then the source in different ip-src if those are IP addresses are not contiguous. If it's an actual CIDR block, then the ip-src should be fine.
  • Can you add back the documentation fields in moduleinfo? This is the source information for generating the documentation.
  • I would recommend to have the object in the https://github.com/MISP/misp-objects repository. You can indeed do a PR on the repo and then after we will update it in this repo. It's also nice for other tools that would like to use your object too or for example misp-stix library.

If you have more question don't hesitate.

@julienloizelet
Copy link
Contributor Author

@adulau : Thanks for your answer.

I'll convert this PR to a draft, if you don't mind, while I make the PR on the misp-objects repo, then make the corresponding changes here.

@julienloizelet julienloizelet marked this pull request as draft August 22, 2024 05:57
@julienloizelet
Copy link
Contributor Author

@adulau : After discussion with the CrowdSec team, I will keep the "ip-src" as ip-range misp-attributes.

But I have then a new question:

As the CrowdSec plugin is configured to enrich "ip-src" ( mispattributes = {"input": ["ip-dst", "ip-src"], "format": "misp_standard"} ), the MISP UI will let an user try to enrich an ip range with CrowdSec, and this will lead to an error (because the CrowdSec CTI endpoint does not accept ip range as value).

Thus, I will modify the code to bypass such ip range enrichment.

I could just failed silently but I wonder if there is a way to warn the user ?

To explain the problem a little better, here are a few screenshots.

Once an IP has been successfully enriched, we will have a CrowdSec object with a lot of attributes, and especially a "ip-src" IP range attribute. In the UI, an user will see "*" to enrich it:

range-enrich

If he clicks on the "*" on the right, he will see:

crowdsec-enrich-proposal

And it will failed with an error:

crowdsec-enrich-failed

This behavior is OK for me.

But the next one is more problematic:

On the left menu, we can see "Enrich Event" :

enrich-event-from-left-menu

And then:

enrich-event-from-left-menu-popup

Here, if the user click on Enrich, it will end with a blank screen ( and a PHP warning/error depending on the PHP display error config, I assume):

enrich-event-from-left-menu-failed-bad

I guess MISP is trying here to enrich multiple attributes at the same time, and the error thrown by the ip range one is not handled properly.

If we make the modification to not enrich an ip-range during the "handler" method, is there a way, to tell the user that something went wrong ? Or should we just bypass the enrichment silently ?

Thanks !

@julienloizelet
Copy link
Contributor Author

@adulau : I figured out that the blank screen I had :

enrich-event-from-left-menu-failed-bad

was due to the "DEBUG=1" setting in my docker .env file. As it should be always 0 in production, all is fine.

Finally, if I detect that a value to enrich is not a valid IP, I will return an error:

import ipaddress
  
...
...

try:
    ipaddress.ip_address(ip)
except ValueError:
    return {
        "error": f"Invalid IP: {ip}"
    }

Thanks

@adulau
Copy link
Member

adulau commented Aug 22, 2024

Thanks a lot for the feedback. For the issue when the DEBUG is enabled. I shared it with my colleague for the MISP core part. For the other issue, you were faster than me ;-)

Do you want to add some specific tests for crowdsec? is there a way to test the API without an API key?

@julienloizelet
Copy link
Contributor Author

julienloizelet commented Aug 23, 2024

Do you want to add some specific tests for crowdsec? is there a way to test the API without an API key?

API key is mandatory.

So, I think it will be impossible to add relevant tests in the /tests/test_expansions.py

We added some basic unit tests in our development repo by mocking the API response, but I guess the /tests/test_expansions.py test is more about integration test.

Thanks

@julienloizelet
Copy link
Contributor Author

julienloizelet commented Aug 23, 2024

@adulau : thanks for the merge.

Do you know when this will be rolled out to the MISP ecosystem?

I mean: I'm running a MISP instance using the MISP docker repository and the misp-modules container still have an old version of the crowdsec-ip-context definition.

Should I wait for something or edit some docker env variable ?

Thanks again

[EDIT]: I guess I have to wait for a bump from the pymisp library. for the submodule to point to the new commit.

@adulau
Copy link
Member

adulau commented Aug 23, 2024

You can already use it. I did an update of the submodule on 2.4 and develop in MISP. If you update your MISP and update the object via the UI, you'll get the latest misp object template. And it will be available in the next release of PyMISP.

@adulau
Copy link
Member

adulau commented Aug 23, 2024

If you are fine with the module, I think it can be already merged.

@julienloizelet
Copy link
Contributor Author

You can already use it. I did an update of the submodule on 2.4 and develop in MISP. If you update your MISP and update the object via the UI, you'll get the latest misp object template. And it will be available in the next release of PyMISP.

I've updated my misp-core container using your last commit (docker env CORE_COMMIT=f559d4c1f2e7a67a6d213ff1f824ec78e92ef69a )

And I can see in the UI that I have the last template (version 4 with reputation attribute):

Capture d’écran du 2024-08-23 16-24-12

But when I try to enrich, there is an error and I can see in the misp-modules container this is because the process does not use the new template :

The template (crowdsec-ip-context) doesn't have the object_relation (reputation) you're trying to add. If you are creating a new event to push to MISP, please review your code so it matches the template.
2024-08-23 07:23:03,557 - misp-modules - ERROR - Something went wrong when processing query request

And indeed, when I browse files in the misp-modules container, I can see than the content of the /usr/local/lib/python3.12/site-packages/pymisp/data/misp-objects/objects/crowdsec-ip-context/definition.json is still the old one.

I assume the python code in the misp-modules container comes from some pymisp install (which does not have the last object).

All works great if I modify manually this file and update the content with the last version.

In any case, I'm going to update the code here to move forward.

Thanks

@julienloizelet julienloizelet changed the title feat(crowdsec): Update module (v2.1.0) feat(crowdsec): Update module (v2.1.1) Aug 23, 2024
@julienloizelet julienloizelet marked this pull request as ready for review August 23, 2024 08:06
@julienloizelet
Copy link
Contributor Author

@adulau : I've updated the code and set this PR in "Ready for review" state.

As said above, I was not able to test on my docker environment without editing manually the definition.json file of the crowdsec-ip-context object (in the misp-modules docker container). I don't know if there are users that use the docker stack but I suppose they will have the same issue.

If you think all is well, let's merge. We can also wait for a pymisp update, I don't know which is the best option.

Thanks

@adulau adulau merged commit a01aa15 into MISP:main Aug 24, 2024
6 checks passed
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.

2 participants