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.

Sign up for GitHub

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 CSP module #442

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

Update CSP module #442

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 65 additions & 20 deletions wapitiCore/attack/mod_csp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,68 @@
from wapitiCore.attack.attack import Attack
from wapitiCore.net import Request
from wapitiCore.net.response import Response
from wapitiCore.net.csp_utils import csp_header_to_dict, CSP_CHECK_LISTS, check_policy_values
from wapitiCore.net.csp_utils import csp_header_to_dict, CSP_CHECK_LISTS
from wapitiCore.definitions.csp import NAME, WSTG_CODE
from wapitiCore.main.log import log_red

MSG_NO_CSP = "CSP is not set"
MSG_CSP_MISSING = "CSP attribute \"{0}\" is missing"
MSG_CSP_UNSAFE = "CSP \"{0}\" value is not safe"

INFO_UNSAFE_INLINE = "\"unsafe-inline\" in \"{0}\" directive allows the execution of unsafe in-page scripts and event\
handlers."
INFO_UNSAFE_EVAL = "\"unsafe-eval\" in \"{0}\" directive allows the execution of code injected into DOM APIs such as\
eval()."
INFO_DATA_HTTP_HTTPS = "value \"{0}\" URI in \"{1}\" allows the execution of unsafe scripts."
INFO_ALLOW_ALL = "\"{0}\" directive should not allow \"*\" as source"
INFO_UNSAFE_OBJECT_SRC = "unsafe values \"{0}\" other then \"none\" identified in \"object-src\""
INFO_UNSAFE_BASE_URI = "unsafe values \"{0}\" other then \"none\" and \"self\" identified in \"base-uri\""
INFO_UNDEFINED_DIRECTIVE = "directive \"{0}\" is not defined"

# This module check the basics recommendations of CSP
def check_policy(policy_name, csp_dict):
"""
This function return the unsafe values for each directive of the tested CSP
"""
info = ""

if policy_name not in csp_dict and "default-src" not in csp_dict:
log_red(INFO_UNDEFINED_DIRECTIVE.format(policy_name))
info += INFO_UNDEFINED_DIRECTIVE.format(policy_name) + "\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Beware, you are setting info to an empty string just bellow


# The HTTP CSP "default-src" directive serves as a fallback for the other CSP fetch directives.
# https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/default-src
policy_values = csp_dict.get(policy_name) or csp_dict["default-src"]
info = ""
# If the tested element is default-src or script-src, we must ensure that none of this unsafe values are present
if policy_name in ["default-src", "script-src"]:
for unsafe_value in CSP_CHECK_LISTS[policy_name]:
if unsafe_value in policy_values:
if unsafe_value == "unsafe-inline":
log_red(INFO_UNSAFE_INLINE.format(policy_name))
info += INFO_UNSAFE_INLINE.format(policy_name) + "\n"
elif unsafe_value in ("data:", "http:", "https:"):
log_red(INFO_DATA_HTTP_HTTPS.format(unsafe_value, policy_name))
info += INFO_DATA_HTTP_HTTPS.format(unsafe_value, policy_name) + "\n"
elif unsafe_value == "*":
log_red(INFO_ALLOW_ALL.format(policy_name))
info += INFO_ALLOW_ALL.format(policy_name) + "\n"
elif unsafe_value == "unsafe-eval":
log_red(INFO_UNSAFE_EVAL.format(policy_name))
info += INFO_UNSAFE_EVAL.format(policy_name) + "\n"

# If the tested element is none of the previous list, we must ensure that one of this safe values is present
else:
for safe_value in CSP_CHECK_LISTS[policy_name]:
Copy link
Contributor

Choose a reason for hiding this comment

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

That loop is wrong as it doesn't do the action described in comment.

For example when tested on github.com it displays

unsafe values "['self']" other then "none" and "self" identified in "base-uri"

while github has the policy base-uri 'self'

Instead the loop should iterate over policy_values THEN check if the value is inside CSP_CHECK_LISTS[policy_name]

However on case like base-uri yolo 'self' it will still display something while self is in the policy.

I think the best way would be to use sets like this:

if not set(policy_values) & set(CSP_CHECK_LISTS[policy_name]):
  // log policy values

Also policy_values should be translated to a string joined by commas when displayed.

if safe_value not in policy_values:
if policy_name == "object-src":
log_red(INFO_UNSAFE_OBJECT_SRC.format(policy_values))
info += INFO_UNSAFE_OBJECT_SRC.format(policy_values) + "\n"
elif policy_name == "base-uri":
log_red(INFO_UNSAFE_BASE_URI.format(policy_values))
info += INFO_UNSAFE_BASE_URI.format(policy_values) + "\n"

return info


class ModuleCsp(Attack):
"""Evaluate the security level of Content Security Policies of the web server."""
name = "csp"
Expand Down Expand Up @@ -65,21 +117,14 @@ async def attack(self, request: Request, response: Optional[Response] = None):
)
else:
csp_dict = csp_header_to_dict(response.headers["Content-Security-Policy"])

info = ""
for policy_name in CSP_CHECK_LISTS:
Copy link
Contributor

Choose a reason for hiding this comment

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

It is convenient to have all policy names to check in CSP_CHECK_LISTS at this line, however it is very misleading that some keys correspond to safe values and the other to unsafe values.

It would be nice to split CSP_CHECK_LISTS in two dictionaries to differentiate the cases and prevent an error to come.

CSP is not easy to get so the more explicit the code, the lower the risk of introducing bugs

result = check_policy_values(policy_name, csp_dict)

if result <= 0:
if result == -1:
info = MSG_CSP_MISSING.format(policy_name)
else: # result == 0
info = MSG_CSP_UNSAFE.format(policy_name)

log_red(info)
await self.add_vuln_low(
category=NAME,
request=request_to_root,
info=info,
wstg=WSTG_CODE,
response=response
)
info += check_policy(policy_name, csp_dict)
Comment on lines 121 to +122
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, we check if the most important rules are well set
Shouldn't we also check for mistyped/non-existant CSP by being more strict ? (notify the user of weird CSP like someone who would have typed deflaut-src)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At present, we only check four directives, and it is difficult to check this case out.
The current version of the module will state that "default-src" is not in the CSP for "deflaut-src" incorrectly typed, for example.
but adding this verification later could be interesting


await self.add_vuln_low(
category=NAME,
request=request,
info=info,
wstg=WSTG_CODE,
response=response
)
Comment on lines +124 to +130
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we always add this vuln ? Even if all the CSP are correctly configured ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, looks like an indentation issue