This repository has been archived by the owner. It is now read-only.
feat(CSP): Use joi to validate CSP reports. #4746
Merged
Conversation
| @@ -151,6 +152,21 @@ function makeApp() { | |||
| // The error handler must be before any other error middleware | |||
| app.use(raven.ravenModule.middleware.express.errorHandler(raven.ravenMiddleware)); | |||
|
|
|||
| // log any joi validation errors | |||
| app.use((err, req, res, next) => { | |||
shane-tomlinson
Feb 20, 2017
Author
Member
@vladikoff, @jrgm - I wonder if the errors are not logged because this comes after the raven middleware above.
@vladikoff, @jrgm - I wonder if the errors are not logged because this comes after the raven middleware above.
vladikoff
Feb 23, 2017
Contributor
Yes that is why, they are useful locally, but prod catches them early...
Yes that is why, they are useful locally, but prod catches them early...
server/lib/routes/post-csp.js
Outdated
| const BODY_SCHEMA = { | ||
| 'csp-report': joi.object().keys({ | ||
| // CSP 2, 3 required | ||
| 'blocked-uri': URL_TYPE.allow('self').required(), |
vladikoff
Feb 23, 2017
Contributor
I tried this in Chrome and via an unsafe eval like:
$('body').html('<script>alert(1)</script>')
csp failed to submit:
fxa-content-server.server.main.ERROR: {"error":"\"blocked-uri\" must be a valid uri with a scheme matching the http|https pattern","op":"validation.error","path":"/_/csp-violation"}
Payload:
{"csp-report":{"document-uri":"http://127.0.0.1:3030/confirm","referrer":"","violated-directive":"script-src","effective-directive":"script-src","original-policy":"connect-src 'self' http://127.0.0.1:9000 http://127.0.0.1:9010 http://127.0.0.1:1111 http://127.0.0.1:1114; default-src 'self'; font-src 'self'; img-src 'self' data: https://secure.gravatar.com http://127.0.0.1:1112; media-src blob:; object-src 'none'; report-uri /_/csp-violation; script-src 'self'; style-src 'self' 'sha256-9n6ek6ecEYlqel7uDyKLy6fdGNo3vw/uScXSq9ooQlk='","disposition":"enforce","blocked-uri":"inline","line-number":83,"column-number":12,"source-file":"http://127.0.0.1:3030/bower_components/jquery/dist/jquery.js","status-code":200}}
I tried this in Chrome and via an unsafe eval like:
$('body').html('<script>alert(1)</script>')
csp failed to submit:
fxa-content-server.server.main.ERROR: {"error":"\"blocked-uri\" must be a valid uri with a scheme matching the http|https pattern","op":"validation.error","path":"/_/csp-violation"}
Payload:
{"csp-report":{"document-uri":"http://127.0.0.1:3030/confirm","referrer":"","violated-directive":"script-src","effective-directive":"script-src","original-policy":"connect-src 'self' http://127.0.0.1:9000 http://127.0.0.1:9010 http://127.0.0.1:1111 http://127.0.0.1:1114; default-src 'self'; font-src 'self'; img-src 'self' data: https://secure.gravatar.com http://127.0.0.1:1112; media-src blob:; object-src 'none'; report-uri /_/csp-violation; script-src 'self'; style-src 'self' 'sha256-9n6ek6ecEYlqel7uDyKLy6fdGNo3vw/uScXSq9ooQlk='","disposition":"enforce","blocked-uri":"inline","line-number":83,"column-number":12,"source-file":"http://127.0.0.1:3030/bower_components/jquery/dist/jquery.js","status-code":200}}
vladikoff
Feb 23, 2017
Contributor
I guess this can also be "blocked-uri":"inline", and not just self
I guess this can also be "blocked-uri":"inline", and not just self
shane-tomlinson
Feb 23, 2017
Author
Member
Great catch.
Great catch.
shane-tomlinson
force-pushed the
use-joi-to-validate-csp-body
branch
3 times, most recently
from
3786cb5
to
721ea37
added 4 commits
Feb 20, 2017
What is the problem? All of our other servers use joi to validate POST bodies. The POST-csp route used very loose inline code. How does this fix it? Use joi like everywhere else. Strictly validate CSP bodies.
shane-tomlinson
force-pushed the
use-joi-to-validate-csp-body
branch
from
721ea37
to
733b840
vladikoff
added a commit
that referenced
this pull request
Mar 22, 2017
What is the problem? The POST /metrics-errors body was not fully validated and cruft ended up being reported. How does this fix it? This carries on with #4746 and uses joi to validate the POST /metrics-errors endpoint data before processing. What does this depend on? This PR is built on #4746 and should be reviewed afterwards. Why is this PR opened on the private repo? One of the tests shows how to overwrite `slice` which could conceivably be used to do bad things to the server (see the referenced Bugzilla bug) Ref https://bugzilla.mozilla.org/show_bug.cgi?id=1320221
vladikoff
added a commit
that referenced
this pull request
Mar 22, 2017
What is the problem? The POST /metrics body was not fully validated and cruft sometimes ended up in our logs. How does this fix it? This carries on with #4746 and uses joi to validate the POST /metrics endpoint data before processing. What's next? server/lib/flow-event performs validation that can be removed. I didn't want to add more to this PR than necessary.
vladikoff
added a commit
that referenced
this pull request
Mar 22, 2017
What is the problem? The POST /metrics-errors body was not fully validated and cruft ended up being reported. How does this fix it? This carries on with #4746 and uses joi to validate the POST /metrics-errors endpoint data before processing. What does this depend on? This PR is built on #4746 and should be reviewed afterwards. Why is this PR opened on the private repo? One of the tests shows how to overwrite `slice` which could conceivably be used to do bad things to the server (see the referenced Bugzilla bug) Ref https://bugzilla.mozilla.org/show_bug.cgi?id=1320221
vladikoff
added a commit
that referenced
this pull request
Mar 22, 2017
What is the problem? The POST /metrics body was not fully validated and cruft sometimes ended up in our logs. How does this fix it? This carries on with #4746 and uses joi to validate the POST /metrics endpoint data before processing. What's next? server/lib/flow-event performs validation that can be removed. I didn't want to add more to this PR than necessary.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
What is the problem?
All of our other servers use joi to validate POST bodies. The POST-csp route
used very loose inline validation.
How does this fix it?
Use joi like everywhere else. Strictly validate CSP bodies.
@vladikoff, @jrgm - r?