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

Humble #63

Closed
wants to merge 3 commits into from
Closed

Humble #63

wants to merge 3 commits into from

Conversation

cyberamt
Copy link
Contributor

This pull request introduces a new scanning modul to artemis called humble.

humble is an open source HTTP-Header security scanner (repo)

I have strongly oriented myself to the implementation of dnsreaper. karton_humble.py has the class Humble which is responsible for the call and additionally the function process_json_data which parses the output.

The parser tries to parse missing security headers so that artemis can display them as interesting findings. If necessary, the parser needs a little refinement so that depricated headers and the like are correctly recognized as "interesting".

Please review the changes and let me know if there are any concerns or suggestions for improvement.

@cyberamt
Copy link
Contributor Author

Closes #61

@kazet kazet self-requested a review January 8, 2024 10:48
)

# strip the first 74 and the last characters from the output to get the location and filename of the output file
filename = data.decode("ascii", errors="ignore")[74:-1]
Copy link
Member

Choose a reason for hiding this comment

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

the "74" is too magic a number, what about using .removeprefix() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The number was also created by magic :) Unfortunately to find the file path correctly some of the statements have to be cut, but .removeprefix() looks like a good solution for this, I'll adjust it accordingly 👍

@kazet
Copy link
Member

kazet commented Jan 8, 2024

Hello,

thanks for the contribution and sorry for the delay in review! As Humble looks to be a nice tool and is MIT-licensed, what would you think about sending this pull request to the core repository so that the module will be enabled by default?

def get_email_template_fragments() -> List[ReportEmailTemplateFragment]:
return [
ReportEmailTemplateFragment.from_file(
str(Path(__file__).parents[0] / "template_missing_security_header.jinja2"), priority=10
Copy link
Member

Choose a reason for hiding this comment

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

IMO the priority is a little too high, I would suggest 2 (as it's the case with missing SSL redirect), 10 is for critical vulnerabilities (e.g. SQL injection or RCE)

category = key_parts[1].capitalize()

# Check if the key is not in the excluded categories and there are relevant values
if (key_parts[1].lower() != "info" and
Copy link
Member

Choose a reason for hiding this comment

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

you may use the category variable here


identity = "humble"
filters = [
{"type": TaskType.DOMAIN.value},
Copy link
Member

Choose a reason for hiding this comment

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

if instead of DOMAIN tasks you listen on SERVICE tasks with service=HTTP (as e.g. bruter does: https://github.com/CERT-Polska/Artemis/blob/main/artemis/modules/bruter.py#L82) you will catch all HTTP services on all ports, including non-standard ones (if that's your intention)

@kazet
Copy link
Member

kazet commented Jan 8, 2024

After running the module, a redundand comma appears:
image

@cyberamt
Copy link
Contributor Author

cyberamt commented Jan 8, 2024

Hey, thanks for the many comments, as you can see I need to get a bit more into the specifics of artemis, I'll incorporate your suggestions (domain -> service / priority = 2) and then make a new pull request for the main repository. If you want you can reject this pull request and close the issue.

@kazet
Copy link
Member

kazet commented Jan 8, 2024

as you can see I need to get a bit more into the specifics of artemis

it's a good PR, I think it's close to being merged ;)

Do you want to be mentioned as a contributor in the main project's README? If yes, do you want to be mentioned as cyberamt or in other way?

@kazet kazet closed this Jan 8, 2024
@cyberamt cyberamt deleted the humble branch February 14, 2024 07:37
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