Skip to content

Don't fail in CSPGlobalCheck if CSP is invalid#68

Merged
phosphore merged 1 commit into
doyensec:masterfrom
baltpeter:csp-error-handling
Jul 8, 2020
Merged

Don't fail in CSPGlobalCheck if CSP is invalid#68
phosphore merged 1 commit into
doyensec:masterfrom
baltpeter:csp-error-handling

Conversation

@baltpeter

Copy link
Copy Markdown
Contributor

Just a quick fix: When the CSPGlobalCheck encountered an invalid CSP, this resulted in an unhandled promise rejection. With this PR, the error is caught and execution can continue.


Two examples of where different instances of this bug occurred:

  • https://github.com/deltachat/deltachat-desktop

    (node:42242) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'split' of undefined
        at csp.CspParser.parse (/home/benni/coding/uni/electronegativity/node_modules/@doyensec/csp-evaluator/csp_evaluator.js:596:110)
        at new csp.CspParser (/home/benni/coding/uni/electronegativity/node_modules/@doyensec/csp-evaluator/csp_evaluator.js:595:566)
        at CSPGlobalCheck.perform (/home/benni/coding/uni/electronegativity/dist/finder/checks/GlobalChecks/CSPGlobalCheck.js:65:26)
        at GlobalChecks.getResults (/home/benni/coding/uni/electronegativity/dist/finder/globalchecks.js:119:32)
        at run (/home/benni/coding/uni/electronegativity/dist/runner.js:217:32)
    (Use `node --trace-warnings ...` to show where the warning was created)
    (node:42242) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch   block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag   `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 2)
    (node:42242) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the   Node.js process with a non-zero exit code.
    
  • https://github.com/MyCryptoHQ/MyCrypto

    (node:42440) UnhandledPromiseRejectionWarning: URIError: URI malformed
        at decodeURI (<anonymous>)
        at Function.goog.Uri.decodeOrEmpty_ (/home/benni/coding/uni/electronegativity/node_modules/@doyensec/csp-evaluator/csp_evaluator.js:431:436)
        at goog.Uri.setDomain (/home/benni/coding/uni/electronegativity/node_modules/@doyensec/csp-evaluator/csp_evaluator.js:423:345)
        at new goog.Uri (/home/benni/coding/uni/electronegativity/node_modules/@doyensec/csp-evaluator/csp_evaluator.js:417:141)
        at /home/benni/coding/uni/electronegativity/node_modules/@doyensec/csp-evaluator/csp_evaluator.js:588:231
        at Object.csp.utils.applyCheckFunktionToDirectives (/home/benni/coding/uni/electronegativity/node_modules/@doyensec/csp-evaluator/csp_evaluator.js:558:175)
        at csp.securityChecks.checkIpSource (/home/benni/coding/uni/electronegativity/node_modules/@doyensec/csp-evaluator/csp_evaluator.js:588:65)
        at csp.CspEvaluator.evaluate (/home/benni/coding/uni/electronegativity/node_modules/@doyensec/csp-evaluator/csp_evaluator.js:595:404)
        at CSPGlobalCheck.perform (/home/benni/coding/uni/electronegativity/dist/finder/checks/GlobalChecks/CSPGlobalCheck.js:67:34)
        at GlobalChecks.getResults (/home/benni/coding/uni/electronegativity/dist/finder/globalchecks.js:119:32)
        at run (/home/benni/coding/uni/electronegativity/dist/runner.js:217:32)
    (Use `node --trace-warnings ...` to show where the warning was created)
    (node:42440) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch   block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag   `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 2)
    (node:42440) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the   Node.js process with a non-zero exit code.
    

@phosphore

Copy link
Copy Markdown
Contributor

I'm wondering if we should issue a proper CSP_CHECK finding having a low confidence score in these cases instead of just returning a warning. I trust the library to be quite solid and if it fails it's probably because the CSP is malformed and not working correctly (and that's the case for Deltachat https://github.com/deltachat/deltachat-desktop/blob/fd5b31318571b138bb07a6136c387675c873a346/src/main/index.ts#L165), but it's also true that in some sources it could include variables used for later building the CSP directives (as in MyCrypto https://github.com/MyCryptoHQ/MyCrypto/blob/b1e00aff66b5593c829c17533cb7523df4e6ad20/src/index.html#L6).
@ikkisoft what's your take on this?

@ikkisoft

ikkisoft commented Jul 6, 2020

Copy link
Copy Markdown
Contributor

In my experience, most of the CSP policies are hardcoded so the dynamically built case is rather a corner case.

I think it would be best to issue a finding for such situation - since a syntax error might cause the CSP to never be used. By leaving confidence low and maybe marking as "manual review", we can cover the corner cases.

@baltpeter baltpeter force-pushed the csp-error-handling branch from ed1f2c6 to 86fb1ed Compare July 7, 2020 19:49
@baltpeter

Copy link
Copy Markdown
Contributor Author

I've changed the patch to issue a finding instead.

@phosphore

Copy link
Copy Markdown
Contributor

LGTM!

@phosphore phosphore merged commit 6c9b089 into doyensec:master Jul 8, 2020
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.

3 participants