Update CSP module #442
Update CSP module #442
Conversation
OussamaBeng
force-pushed
the
upgrade-csp-module
branch
from
3be4cd8 to
5d06507
Compare
| for policy_name in CSP_CHECK_LISTS: | ||
| 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) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
adding more information about the CSP directives and the unsafe values to avoid
OussamaBeng
force-pushed
the
upgrade-csp-module
branch
from
5d06507 to
c1d178f
Compare
| await self.add_vuln_low( | ||
| category=NAME, | ||
| request=request, | ||
| info=info, | ||
| wstg=WSTG_CODE, | ||
| response=response | ||
| ) |
There was a problem hiding this comment.
Do we always add this vuln ? Even if all the CSP are correctly configured ?
There was a problem hiding this comment.
Indeed, looks like an indentation issue
|
|
||
| # 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]: |
There was a problem hiding this comment.
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 valuesAlso policy_values should be translated to a string joined by commas when displayed.
|
|
||
| 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" |
There was a problem hiding this comment.
Beware, you are setting info to an empty string just bellow
| @@ -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: | |||
There was a problem hiding this comment.
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
adding more information about the CSP directives and the unsafe values to avoid