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

Update main.py #229

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update main.py #229

wants to merge 1 commit into from

Conversation

eoinmiller-r7
Copy link

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/3664

Describe changes:
suricata-update will only process contents of filenames that end in ".rules". However when using things like ThreatQ's exports of IDS signatures, there is no filename in the output. Please see the url value of the below source YAML.

Enabled source config:

checksum: false 
source: rapid7/open
url: https://<HOSTNAME>/api/export/<REDACTED-HASH>/?token=<REDACTED-TOKEN>

After running suricata-update, it will retrieve the ThreatQ feed of rules successfully and place them in the cache dir, specifically "d8431e4d1a8e4054452d35cdea90127a-". Since the filename is null, there is nothing after the MD5 and hyphen:

root@suricata:~# ls -laFh /var/lib/suricata/update/cache/
total 2.6M
drwxr-x--- 3 root root 4.0K Apr 24 00:26 ./
drwxr-xr-x 4 root root 4.0K Nov 20 23:58 ../
-rw-r--r-- 1 root root 2.5M Apr 24 00:27 1168f1cf2d4676c8d507bbb6ea3b2078-emerging.rules.tar.gz
-rw-r--r-- 1 root root 7.5K Apr 24 01:11 d8431e4d1a8e4054452d35cdea90127a-
-rw-r--r-- 1 root root 5.5K Apr 23 18:20 index.yaml
drwxr-xr-x 3 root root 4.0K Nov 20 22:49 tmp/

In suricata/update/main.py, we found that commenting out the last two lines in this snippet will cause the byte string value in the dict named files will be processed and added to the output appropriately:

...
for filename in sorted(files):
        if not filename.endswith(".rules"):    
            continue
...

Alternatively, it also may be possible to specify the output filename to be used in the source YAML config.

@eoinmiller-r7 eoinmiller-r7 requested a review from jasonish as a code owner April 24, 2020 01:41
@@ -1028,8 +1028,6 @@ def _main():

rules = []
for filename in sorted(files):
if not filename.endswith(".rules"):
continue
Copy link
Member

Choose a reason for hiding this comment

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

Hi @eoinmiller-r7 !
Thanks for your first contribution to the project. :)
I think I understand your issue but maybe dropping this can create issues for cases like filemd5 and datasets. We do not want all files to be parsed as rules.
But, it would be nice to hear @jasonish 's opinion on this.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should fake out the filename if none is present here.. When we get here: https://github.com/OISF/suricata-update/blob/master/suricata/update/main.py#L222, we know we are dealing with a single, unarchived rule file. We could give it a dummy name here, the source name, or perhaps the md5, this will allow parsing to continue, plus give the cache a better filename.

We could add a forced filename option to the source, but should first handle the case where that does not exist as its not something we want to require.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants