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

fix: ffuf ANSI code processing preventing task to finish #1058

Merged
merged 2 commits into from
Nov 24, 2023

Conversation

ocervell
Copy link
Contributor

@ocervell ocervell commented Nov 21, 2023

Should

Needs to be tested for potential impact on other tasks (e.g: dalfox)

if not line:
break
line = line.strip()
ansi_escape = re.compile(r'\x1B(?:[@-Z\\-_]|\[[0-?]*[ -/]*[@-~])')

Check warning

Code scanning / CodeQL

Overly permissive regular expression range Medium

Suspicious character range that is equivalent to [@A-Z].
if not line:
break
line = line.strip()
ansi_escape = re.compile(r'\x1B(?:[@-Z\\-_]|\[[0-?]*[ -/]*[@-~])')

Check warning

Code scanning / CodeQL

Overly permissive regular expression range Medium

Suspicious character range that is equivalent to \[0-9:;<=>?\].
@yogeshojha
Copy link
Owner

I think its better to wait for 2.0.2 release until we test this out.

Excellent work @ocervell, will test this out tomorow.

@AnonymousWP
Copy link
Contributor

AnonymousWP commented Nov 21, 2023

I think its better to wait for 2.0.2 release until we test this out.

Excellent work @ocervell, will test this out tomorow.

It's a pretty severe bug though, and as this PR will fix it, I think it can be safely merged. From what I'm understanding, Psyray is testing this right now, so let's wait until tests are finished.

@yogeshojha
Copy link
Owner

yogeshojha commented Nov 21, 2023

Lets hope so!

Anyways @ocervell would be the best person to fix this, lets trust his work. since he had worked on the rework of celery tasks and I think he is aware of the ffuf issue.

I initially thought it had something to do with celery, if this PR fixes, well and good for all of us.

@psyray
Copy link
Contributor

psyray commented Nov 21, 2023

Lets hope so!

Anyways @ocervell would be the best person to fix this, lets trust his work. since he had worked on the rework of celery tasks and I think he is aware of the ffuf issue.

I initially thought it had something to do with celery, if this PR fixes, well and good for all of us.

I've talked with him, and yes he is aware of this issue.
I'll test it now

@psyray
Copy link
Contributor

psyray commented Nov 21, 2023

@ocervell
Currently testing.
FFUF does not hangs but it restarts again and again
I'll let it go to see if it stops

@psyray
Copy link
Contributor

psyray commented Nov 21, 2023

For the restart, it seems that it comes from an option that I've set in the scanEngine, so forget about it.

Anyway, the same problem happens, I've tested on one of my website.
In nginx accesslog, requests stopped at the end of the wordlist
But on the server FFUF task is always running, and no SUCCESS added to log
image

Tree view of the running processes
image

@psyray
Copy link
Contributor

psyray commented Nov 21, 2023

OK found the bug.
In the Tree capture, I saw that celery was launched in --loglevel=info, so I've modified this in --loglevel=debug
And BAM !
image

rengine-celery-1       | Traceback (most recent call last):
rengine-celery-1       |   File "/usr/src/app/reNgine/celery_custom_task.py", line 129, in __call__
rengine-celery-1       |     self.result = self.run(*args, **kwargs)
rengine-celery-1       |   File "/usr/src/app/reNgine/tasks.py", line 1682, in dir_file_fuzz
rengine-celery-1       |     endpoint, created = save_endpoint(url, crawl=False, ctx=ctx)
rengine-celery-1       |   File "/usr/src/app/reNgine/tasks.py", line 4518, in save_endpoint
rengine-celery-1       |     endpoint, created = EndPoint.objects.get_or_create(
rengine-celery-1       |   File "/usr/local/lib/python3.10/dist-packages/django/db/models/manager.py", line 85, in manager_method
rengine-celery-1       |     return getattr(self.get_queryset(), name)(*args, **kwargs)
rengine-celery-1       |   File "/usr/local/lib/python3.10/dist-packages/django/db/models/query.py", line 581, in get_or_create
rengine-celery-1       |     return self.get(**kwargs), False
rengine-celery-1       |   File "/usr/local/lib/python3.10/dist-packages/django/db/models/query.py", line 439, in get
rengine-celery-1       |     raise self.model.MultipleObjectsReturned(
rengine-celery-1       | startScan.models.EndPoint.MultipleObjectsReturned: get() returned more than one EndPoint -- it returned 2!

I try to fix this one.
I saw this error multiples times, tehre is a bug elsewhere that register two times the same endpoints.
Or the query is not accurate and retrieve 2 endpoints instead of one

@yogeshojha
Copy link
Owner

@psyray for me this solution works! At the same time, I believe our ffuf command is also wrong, I am checking the command once again, but ffuf starts and stops without any issues.

@yogeshojha
Copy link
Owner

Yes one of the issues is that -e extensions is expecting extensions to begin with ., example .php,.html rather than php,html

@yogeshojha
Copy link
Owner

Fuzz works fine to what I see as of now.

image

@AnonymousWP
Copy link
Contributor

Also check the security alerts and its potential risks. Take action on them (e.g. dismissing if false positive).

@AnonymousWP AnonymousWP mentioned this pull request Nov 22, 2023
1 task
@psyray
Copy link
Contributor

psyray commented Nov 22, 2023

Fuzz works fine to what I see as of now.

image

@yogeshojha
Check #1063
Bug is dependent to duplicate field in db, so it would not come in all case

@psyray
Copy link
Contributor

psyray commented Nov 23, 2023

@ocervell Do you think we should merge this modification ?
It's implemented in my dev branch, but I don't know if it really resolve this bug because the problem was more a get_or_create crash on duplicate records than an ANSI problem

What was the use case that cause problem for you that need this modification ?

@AnonymousWP
Copy link
Contributor

And please, take a look to the security advisories as I pointed out earlier.

@ocervell
Copy link
Contributor Author

ocervell commented Nov 23, 2023

I am doubting that #1063 is related only to ffuf, seems like an extra issue that could be triggered by any duplicate endpoint added to the db; for me this PR resolves the problem with ffuf not completing properly.

As for the security advisory, not sure how impactful it is ... If you have a better regex to replace ANSI codes feel free to change it.

@yogeshojha
Copy link
Owner

I agree with @ocervell

duplicate endpoint is coming from elsewhere. The function where we save endpoint has get_or_create, that should have never written duplicate endpoints. It is either coming from http crawler or manual entry.

@psyray
Copy link
Contributor

psyray commented Nov 23, 2023

I agree with @ocervell

duplicate endpoint is coming from elsewhere. The function where we save endpoint has get_or_create, that should have never written duplicate endpoints. It is either coming from http crawler or manual entry.

Duplicate endpoint seems to come from the missing target_domain_id and PR has been merged.
#1069

Now duplicate endpoint task should better do his job.

Maybe there's another source of duplicate.

So we need to test with a clean db and launch multiples scans, then check the db with this query

SELECT http_url, COUNT(http_url) as url_count, is_default FROM public."startScan_endpoint"
group by http_url, is_default
having COUNT(http_url) > 1
order by url_count desc

Or this python code

>>> from django.db.models import Count
>>> EndPoint.objects.values('http_url').annotate(Count('http_url')).order_by().filter(http_url__count__gt=1)
<QuerySet [{'http_url': 'http://testphp.vulnweb.com', 'http_url__count': 2}]>

To see if duplicates are coming again

@ocervell ocervell changed the title Fix ffuf ANSI code processing preventing task to finish fix: ffuf ANSI code processing preventing task to finish Nov 23, 2023
Copy link
Owner

@yogeshojha yogeshojha left a comment

Choose a reason for hiding this comment

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

Approved

@yogeshojha yogeshojha merged commit 0ded32c into yogeshojha:master Nov 24, 2023
4 of 5 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.

Version 2, "Directories & files fuzz - In progress" Never stop
4 participants