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(tool_arsenal): incorrect regex version numbers #1086

Merged
merged 6 commits into from
Dec 8, 2023

Conversation

AnonymousWP
Copy link
Contributor

@AnonymousWP AnonymousWP commented Nov 27, 2023

Tried to fix sublist3r, OneForAll, crlfuzz and Hakrawler by spending quite some time and effort on it, but no luck. Hardest was OneForAll, because that's the only one that has a command for outputting the version. The other two don't.

Copy link
Contributor

👋 Hi @AnonymousWP,
Thank you for sending this pull request.
Please make sure you have followed our contribution guidelines.
We will review this PR as soon as possible. Thank you for your patience.

@AnonymousWP
Copy link
Contributor Author

Regarding this commit (feat(external_tools): add lookup command theHarvester):

I finally found a way to make it work directly via the container itself (see image), but once I try this via the YAML file, it doesn't work. I tried multiple combinations, but it's still not working. It complains about that it's an invalid lookup command ({"status":false,"message":"Invalid version lookup command."}). I'd like some help, but at least we're much further now than before. I think it has to do with the logic of how regex is interpreted by reNgine, but can't find the code that's responsible for version_lookup_command.

image

@psyray
Copy link
Contributor

psyray commented Nov 28, 2023

Remember I already work on this on one of your PR
You must upgrade the db field size or the command will be truncated
#882 (comment)

@AnonymousWP
Copy link
Contributor Author

Remember I already work on this on one of your PR You must upgrade the db field size or the command will be truncated #882 (comment)

Yeah I know, I've seen that, but that PR is already merged, so this branch should contain your db field size fix as well.

@psyray
Copy link
Contributor

psyray commented Nov 29, 2023

Yeah I know, I've seen that, but that PR is already merged, so this branch should contain your db field size fix as well.

Have you check it ?
Confirm that the db field has the correct size and the command was not truncated.
You could use pgAdmin 4 remote with ssh tunnel or psql command in db container

@AnonymousWP
Copy link
Contributor Author

Yeah I know, I've seen that, but that PR is already merged, so this branch should contain your db field size fix as well.

Have you check it ? Confirm that the db field has the correct size and the command was not truncated. You could use pgAdmin 4 remote with ssh tunnel or psql command in db container

I haven't checked it in the database itself, but I assume that 1. when your PR is merged and 2. I brought down the containers, re-built and then started them again, the database should contain the change. Right?

@AnonymousWP
Copy link
Contributor Author

AnonymousWP commented Dec 6, 2023

I tried fixing https://github.com/dwisiswant0/crlfuzz, but no luck unfortunately. The issue is within the update command, but attempted several approaches. Anyway, now I look into crlfuzz, it's not working anyway, see: dwisiswant0/crlfuzz#44. So might as well drop it?

So unless you guys want to help me fixing the ones I was struggling with (see initial post), I think this PR can be marked as ready. Let me know @yogeshojha @psyray.

@psyray
Copy link
Contributor

psyray commented Dec 6, 2023

I haven't checked it in the database itself, but I assume that 1. when your PR is merged and 2. I brought down the containers, re-built and then started them again, the database should contain the change. Right?

Should, but maybe it's not.
You have to be sure.

@psyray
Copy link
Contributor

psyray commented Dec 6, 2023

Regarding this commit (feat(external_tools): add lookup command theHarvester):

I finally found a way to make it work directly via the container itself (see image), but once I try this via the YAML file, it doesn't work. I tried multiple combinations, but it's still not working. It complains about that it's an invalid lookup command ({"status":false,"message":"Invalid version lookup command."}). I'd like some help, but at least we're much further now than before. I think it has to do with the logic of how regex is interpreted by reNgine, but can't find the code that's responsible for version_lookup_command.

It doesn't work because you should cd into the harvester install folder
#882 (comment)

My command in this comment works
cd /usr/src/github/theHarvester/theHarvester/lib/;python3 -c 'from version import version; print(version())'

@psyray
Copy link
Contributor

psyray commented Dec 6, 2023

OK I've tried some tests for theHarvester, and while command given above works in the shell, it does not work in reNgine.
Problem comes from the subprocess.Popen function in the run_command method

_, stdout = run_command(tool.version_lookup_command)

popen = subprocess.Popen(

My thought is that we need to create a python script like this, for example in web/version-check/theHarvester.py

import sys;
sys.path.append("/usr/src/github/theHarvester/theHarvester/lib/");
from version import version;
print(version())

And call this script from the version lookup command
python3 /usr/src/app/version-check/theHarvester.py

Works in shell
image

And in reNgine
image

@yogeshojha
Copy link
Owner

theHarvester version lookup is fixed.
image

@yogeshojha
Copy link
Owner

@psyray it is unrelated to run_command

The version lookup command executes from rengine-web-1 container, and we do not have all requirements for theHarvester installed in rengine-web-1, it is rather in rengine-celery-1, so you end up getting this error.

image

Instead of creating a new file/api to just check theHarvester version, I guess we can cat and grep the lib/version.py

@yogeshojha
Copy link
Owner

Fix version lookup for oneforall

image

image

@yogeshojha
Copy link
Owner

@AnonymousWP unless we have something else to fix here, please remove this as draft.

All the tool versions looks good to me! tested and verified.

@AnonymousWP AnonymousWP marked this pull request as ready for review December 8, 2023 11:30
@AnonymousWP
Copy link
Contributor Author

I haven't checked it in the database itself, but I assume that 1. when your PR is merged and 2. I brought down the containers, re-built and then started them again, the database should contain the change. Right?

Should, but maybe it's not. You have to be sure.

Well, Yogesh just said he tested it, so seems like it does work.

@psyray @yogeshojha Thanks for the help and suggestions. ❤️ Marked it as ready for review.

@yogeshojha yogeshojha merged commit fd5a5e5 into master Dec 8, 2023
@AnonymousWP AnonymousWP deleted the fixes-for-tool-arsenal branch December 8, 2023 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants