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

discussion: new (clear and separate) requirement for CSP reporting/logging? #1445

Closed
elarlang opened this issue Dec 13, 2022 · 35 comments
Closed
Assignees
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet 6) PR awaiting review next meeting Filter for leaders V50 Group issues related to Web Frontend _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@elarlang
Copy link
Collaborator

Potentially related requirement #1311

Should we have clear separate logging requirement for like (just to give the point, not the finalized wording): Verify that Content Security Policy violations are reported (by browser) and logged (on trusted service layer).

Category: V7.., Level: 2 (CSP ruleset itself with level 1)

@elarlang elarlang added 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet Community wanted We would like feedback from the community to guide our decision otherwise we will progress josh/elar V7 Temporary label for grouping logging related issues labels Dec 13, 2022
@elarlang elarlang self-assigned this Dec 13, 2022
@tghosth
Copy link
Collaborator

tghosth commented Dec 28, 2022

I think this should be a specific requirement as it requires a specific mechanism to catch these logs which are sent by the browser.

Agree with L2.

Does requirement belong with logging or with CSP? I would be inclined to keep it with CSP because it is a different mechanism to the regular logging anyway.

@tghosth tghosth added _5.0 - prep This needs to be addressed to prepare 5.0 4b Major-rework These issues need to be part of a full chapter rework and removed 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet labels Jul 10, 2023
@elarlang elarlang added the V50 Group issues related to Web Frontend label Dec 9, 2023
@tghosth tghosth removed Community wanted We would like feedback from the community to guide our decision otherwise we will progress josh/elar labels May 2, 2024
@tghosth
Copy link
Collaborator

tghosth commented May 2, 2024

I still think this should be a separate requirement as it is not a standard log. I would put it as L2 in 7.2 and say:

Verify that a CSP report logging server is in use and that the reports it collects are then logged as part of the application's standard logging mechanism.

What do you think @elarlang

@tghosth tghosth added 4) proposal for review Issue contains clear proposal for add/change something and removed 4b Major-rework These issues need to be part of a full chapter rework V50 Group issues related to Web Frontend labels May 2, 2024
@elarlang
Copy link
Collaborator Author

elarlang commented May 2, 2024

I think to focus should be different for the requirements. Something like:

Verify that Content Security Policy violations are reported and logged.

We don't need to talk about the server or standard logging mechanism (this is too vague), there are separate services for that and I think it works well that way.

As the Content Security Policy itself is mostly second-layer defense, I think this requirement as one more step is a level 3 requirement.

@tghosth
Copy link
Collaborator

tghosth commented May 2, 2024

Agree with L3

How about:

Verify that CSP violations are reported and logged as part of the application's standard logging mechanism.

@elarlang
Copy link
Collaborator Author

elarlang commented May 2, 2024

I already shared my opinion on the standard logging part

We don't need to talk about the server or standard logging mechanism (this is too vague), there are separate services for that and I think it works well that way.

What risk do you have when using an external service or what risk are you going to solve, when requiring it to be part of the standard logging mechanism? If I call an external service as my standard logging mechanism, am I ok?

@tghosth tghosth added the next meeting Filter for leaders label May 2, 2024
@tghosth
Copy link
Collaborator

tghosth commented May 2, 2024

Discussed with @elarlang that "as part of the application's standard logging mechanism." is more of a recommendation anyway rather than a strict control so it isn't really necessary for here. I will open a PR for this.

Verify that Content Security Policy violations are reported and logged.

@set-reminder 5 mins @tghosth to open PR

Copy link

octo-reminder bot commented May 2, 2024

Reminder
Thursday, May 2, 2024 12:38 PM (GMT+02:00)

@tghosth to open PR

@tghosth tghosth removed the next meeting Filter for leaders label May 2, 2024
Copy link

octo-reminder bot commented May 2, 2024

🔔 @tghosth

@tghosth to open PR

@tghosth
Copy link
Collaborator

tghosth commented May 2, 2024

Waiting for #1947

@set-reminder 3 days Josh to address

Copy link

octo-reminder bot commented May 2, 2024

Reminder
Sunday, May 5, 2024 12:00 AM (GMT+02:00)

Josh to address

@tghosth tghosth added the 4a) Waiting for another This issue is waiting for another issue to be resolved label May 2, 2024
tghosth added a commit that referenced this issue May 2, 2024
@ryarmst
Copy link
Contributor

ryarmst commented May 6, 2024

Thanks @ScottHelme for the comment and Twitter discussion.

I will first add my overall thoughts on implementing violation reporting as I failed to in my initial comment: I do think reporting is a necessary component of implementing a CSP. IMO the greatest utility comes from implementing a non-breaking CSP, which should be a requirement any time a CSP is implemented.

The violations that occur in #2 are then the ones you want to be concerned about. If you have a genuine XSS attacks, or some other issue, that is being reported, I don't feel that depending on the CSP to 'fix' this is the correct approach. The CSP acts as a temporary measure, neutralising the attack in the browser, while you receive the reports, identify the source of the problem and then rectify it in the application itself. Without CSP reporting, this is not possible.

I don't think anyone has argued that CSP is a 'fix' to XSS, but a robust CSP can mitigate otherwise effective attacks in some scenarios. I would rather generalize it as a defense in depth mechanism rather than a "temporary measure" because there is no point at which a CSP ought to be removed when functioning.

I am skeptical of (but open to) the practical effectiveness of using reporting to identify application vulnerabilities through CSP violations. In any scenario where the attack surface is exposed to an attacker, they can craft payloads without sending reports. A successful attack (looking at XSS still) requires both an application vulnerability AND a CSP policy that will not block at least one viable exploitation of the vulnerability. Therefore, the most impactful vulnerabilities (the ones that can be exploited) will not generate reports. As @jmanico mentioned, an attacker also has the opportunity to pollute an exposed logging endpoint for misdirection or other purposes (though I have not heard of such an attack).

That said, I can think of a few possible scenarios where a report would be more likely to be triggered:

  1. An attacker attempting blind XSS against environments outside of their control.
  2. Non-targeted exploitation attempts and scanning.
  3. Incidental (unintentional) violations through user error or other means.

@ScottHelme also noted (Twitter) that he has seen many cases of reporting being the only identification vector of potential vulnerabilities. I have not seen any writeups or other evidence/data myself, but - based on his claim - I have changed my position in favour of reporting as a proactive security control. To be fair, as an appsec consultant, I mostly work with clients who either have not implemented a CSP or have implemented an overly permissive policy where basic attacks would not result in a violation, so I have very rarely seen situations where an organization would be positioned to make use of violation reports in this way.

Many people will often refer to a "CSP bypass", in which an attacker finds a way to get script into the page that is allowed to run. This isn't a CSP bypass, but is instead a problem with your CSP not being properly defined, so the reports then fall under scenario #1 above. If the script, or other action, is allowed per the CSP, everything is behaving as it should be.

I do not agree with this framing. I think there are enough scenarios where a robust CSP will not block certain vectors. For example, even use of the script-src nonce-... pattern can be defeated if the application is inappropriately embedding attacker-controlled content into a nonce-protected region.

I don't feel we can have a mechanism like CSP deployed without the feedback that violation reporting offers. CSP has the potential to break functionality on your site, and detect serious attacks.

I absolutely agree that reporting must be part of the CSP deployment process. I am still unsure of the practical efficacy for "detecting serious attacks" but I will defer to @ScottHelme's experience.

@elarlang
Copy link
Collaborator Author

elarlang commented May 6, 2024

It is defense in depth, it is useful.

Now the question still (#1445 (comment)) is, should we use it as a level 3 requirement or as a recommendation?

@tghosth
Copy link
Collaborator

tghosth commented May 6, 2024

At this point, I am leaning towards a relatively non-prescriptive L3 requirement. Whilst the definitions are not yet finalized, L3 is always a "stretch goal" and so I don't think this puts an unnecessary burden or risk on implementers.

Current wording being proposed:

# Description L1 L2 L3 CWE
7.2.7 [ADDED] Verify that Content Security Policy violation reports are logged.

@elarlang
Copy link
Collaborator Author

elarlang commented May 6, 2024

I prefer:

Verify that Content Security Policy violations are reported and logged.

For the chapter, I prefer "V50.2 Browser Security Mechanism Headers"

... and I'm also ok, when we cover it as a recommendation.

@tghosth
Copy link
Collaborator

tghosth commented May 6, 2024

I see this as logging and would prefer to keep it here, especially so as not to overload V50

@elarlang
Copy link
Collaborator Author

elarlang commented May 6, 2024

I know I opened the initial proposal for V7, but here has been a nice amount of feedback and arguments to rethink it all. I don't think we should focus on logging with this one.

Logging is the result of the configuration. If you don't have this in place, you don't have logging.

On the application side you need to make a configuration to have the logging somewhere. This somewhere can be an external service and out of the scope of the application.

We are not going to verify "logs exist on external service" but "Verify that CSP declaration has reporting defined".

Also, if you just require logging, it contains a hidden requirement to have the correct reporting declaration for CSP in place.

@tghosth
Copy link
Collaborator

tghosth commented May 7, 2024

Yeah but the hard part here isn't adding a bit of text to the CSP header, it is having the infra to collect the violation logs which is why I think it makes more sense here.

@elarlang
Copy link
Collaborator Author

elarlang commented May 7, 2024

The solution for receiving the reports can be whatever and I would not dictate it, it can be external service and out of scope, therefore.

@tghosth
Copy link
Collaborator

tghosth commented May 7, 2024

Yeah but it is a logging thing more than it is a browser header thing

@tghosth tghosth closed this as completed in ad3a759 May 7, 2024
@elarlang elarlang reopened this May 7, 2024
@OWASP OWASP deleted a comment from octo-reminder bot May 7, 2024
@tghosth tghosth added 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet V50 Group issues related to Web Frontend and removed 7) PR in non-master branch V7 Temporary label for grouping logging related issues labels May 7, 2024
@elarlang elarlang added the next meeting Filter for leaders label May 9, 2024
@tghosth
Copy link
Collaborator

tghosth commented May 9, 2024

I discussed this with Elar and he made two very good points

  1. The report logging mechanism almost certainly will be outside of the application and is therefore technically not in scope for ASVS as it isn't really up to the developer to maintain it.
  2. From a verification perspective, we are more likely to verify the HTTP headers rather than the existence of a logging server.

As such, I am comfortable moving this to V50.

I would suggest:

[ADDED] Verify that the Content Security Policy header specifies a location to report violations.

@tghosth tghosth removed the next meeting Filter for leaders label May 9, 2024
@elarlang
Copy link
Collaborator Author

elarlang commented May 9, 2024

ping @ryarmst @ScottHelme - any more comments on the latest proposal? If not, I'll go for PR.

@ryarmst
Copy link
Contributor

ryarmst commented May 9, 2024

@elarlang looks good to me!

@elarlang
Copy link
Collaborator Author

elarlang commented May 13, 2024

I made the PR, not that I used the header name (as all other requirements in the section do), not the policy name. Also modified 50.2.1.
f08863c

@elarlang elarlang added the next meeting Filter for leaders label May 13, 2024
@tghosth
Copy link
Collaborator

tghosth commented May 15, 2024

@elarlang do you want to remove this from 7.2.7 as well?

@elarlang
Copy link
Collaborator Author

@elarlang do you want to remove this from 7.2.7 as well?

Yes, thanks. I added it as a new requirement and did not recheck/remember, that it already got into the repo in another place.

tghosth pushed a commit that referenced this issue May 16, 2024
* new requirement for csp violations report

* 7.2.7 is now in 50.2.7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet 6) PR awaiting review next meeting Filter for leaders V50 Group issues related to Web Frontend _5.0 - prep This needs to be addressed to prepare 5.0
Projects
None yet
Development

No branches or pull requests

5 participants