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

JS/TS: insecure Helmet middleware (new query) #16540

Merged

Conversation

aegilops
Copy link
Contributor

@aegilops aegilops commented May 21, 2024

The Helmet πŸͺ– middleware is used to set security-related HTTP headers in Express applications. This query finds instances where the middleware is configured with important ⚠️ security features disabled 🚫.

I've included a new data extension, specific to this query, called requiredHelmetSecuritySetting, which users can set to a string of extra features that should not be disabled, beyond the defaults in the query. The extension is specific to this query, rather than general purpose; I don't know of other similar queries for other frameworks, so designing something that would work well in other contexts would be speculative.

I'd like to discuss πŸ—£οΈ what numeric severity πŸ”’ to give this, since it's a configuration and not a direct vulnerability on its own.

Copy link
Contributor

QHelp previews:

javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp

errors/warnings:

/home/runner/work/codeql/codeql/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp:9:17: element "ul" not allowed here; expected the element end-tag, text or element "a", "b", "code", "em", "i", "img", "strong", "sub", "sup" or "tt"
/home/runner/work/codeql/codeql/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp:22:17: element "ul" not allowed here; expected the element end-tag, text or element "a", "b", "code", "em", "i", "img", "strong", "sub", "sup" or "tt"
/home/runner/work/codeql/codeql/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp:31:47: found attribute "class", but no attributes allowed here
/home/runner/work/codeql/codeql/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp:42:47: found attribute "class", but no attributes allowed here
/home/runner/work/codeql/codeql/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp:48:47: element "code" not allowed here; expected the element end-tag or text
/home/runner/work/codeql/codeql/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp:48:47: found attribute "class", but no attributes allowed here
/home/runner/work/codeql/codeql/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp:60:12: element "p" not allowed here; expected the element end-tag, text or element "a", "b", "code", "em", "i", "img", "strong", "sub", "sup" or "tt"
/home/runner/work/codeql/codeql/javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp:63:7: The element type "code" must be terminated by the matching end-tag "</code>".
A fatal error occurred: 1 qhelp files could not be processed.

Copy link
Contributor

github-actions bot commented May 21, 2024

QHelp previews:

javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp

Insecure configuration of Helmet security middleware

Helmet is a collection of middleware functions for securing Express apps. It sets various HTTP headers to guard against common web vulnerabilities. This query detects Helmet misconfigurations that can lead to security vulnerabilities, specifically:

  • Disabling frame protection
  • Disabling Content Security Policy
    Content Security Policy (CSP) helps spot and prevent injection attacks such as Cross-Site Scripting (XSS). Removing frame protections exposes an application to attacks such as clickjacking, where an attacker can trick a user into clicking on a button or link on a targeted page when they intended to click on the page carrying out the attack.

Recommendation

To help mitigate these vulnerabilities, ensure that the following Helmet functions are not disabled, and are configured appropriately to your application:

  • frameguard
  • contentSecurityPolicy

Example

The following code snippet demonstrates Helmet configured in an insecure manner:


            const helmet = require('helmet');
            app.use(helmet({
                frameguard: false,
                contentSecurityPolicy: false
            }));
        

In this example, the defaults are used, which enables frame protection and a default Content Security Policy.


            app.use(helmet());
        

You can also enable a custom Content Security Policy by passing an object to the contentSecurityPolicy key. For example, taken from the Helmet docs:


            app.use(
                helmet({
                    contentSecurityPolicy: {
                        directives: {
                            "script-src": ["'self'", "example.com"],
                            "style-src": null,
                        },
                    },
                })
            );
        

References

Copy link
Contributor

github-actions bot commented Jun 7, 2024

QHelp previews:

javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp

Insecure configuration of Helmet security middleware

Helmet is a collection of middleware functions for securing Express apps. It sets various HTTP headers to guard against common web vulnerabilities. This query detects Helmet misconfigurations that can lead to security vulnerabilities, specifically:

  • Disabling frame protection
  • Disabling Content Security Policy
    Content Security Policy (CSP) helps spot and prevent injection attacks such as Cross-Site Scripting (XSS). Removing frame protections exposes an application to attacks such as clickjacking, where an attacker can trick a user into clicking on a button or link on a targeted page when they intended to click on the page carrying out the attack.

Users of the query can extend the set of required Helmet features by adding additional checks for them, using CodeQL data extensions in a CodeQL model pack. See `CUSTOMIZING.md` in the query source for more information.

Recommendation

To help mitigate these vulnerabilities, ensure that the following Helmet functions are not disabled, and are configured appropriately to your application:

  • frameguard
  • contentSecurityPolicy

Example

The following code snippet demonstrates Helmet configured in an insecure manner:

const helmet = require('helmet');

app.use(helmet({
    frameguard: false,
    contentSecurityPolicy: false
}));

In this example, the defaults are used, which enables frame protection and a default Content Security Policy.

app.use(helmet());

You can also enable a custom Content Security Policy by passing an object to the contentSecurityPolicy key. For example, taken from the Helmet docs:

app.use(
    helmet({
        contentSecurityPolicy: {
            directives: {
                "script-src": ["'self'", "example.com"],
                "style-src": null,
            },
        },
    })
);

References

@aegilops aegilops marked this pull request as ready for review June 7, 2024 14:56
@aegilops aegilops requested a review from a team as a code owner June 7, 2024 14:56
@aegilops
Copy link
Contributor Author

aegilops commented Jun 7, 2024

I'm not sure what to do about the change note failure. Do I need to document this change in a change note, and if so, where please?

@erik-krogh
Copy link
Contributor

erik-krogh commented Jun 7, 2024

Do I need to document this change in a change note, and if so, where please?

Yes.
javascript/ql/src/change-notes/yyyy-mm-dd-name-of-your-node.md.

See this doc on change-notes for what to put there: https://github.com/github/codeql/blob/main/docs/change-notes.md

Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

Some comments for now.

javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp Outdated Show resolved Hide resolved
javascript/ql/src/Security/CWE-693/InsecureHelmet.ql Outdated Show resolved Hide resolved
javascript/ql/src/Security/CWE-693/InsecureHelmet.ql Outdated Show resolved Hide resolved
@aegilops
Copy link
Contributor Author

aegilops commented Jun 19, 2024

I've got those updates done for you @erik-krogh, thanks so much for the review so far πŸ™

  • added a change note
  • switched to modern dataflow libraries you suggested
  • used a default data extension, since using an external predicate always requires external data
  • added a CUSTOMIZING.md to explain how to customize the query with data extensions
  • fixed the inconsistent naming
  • updated the severity to 7.0, as we discussed
  • moved the examples to separate files outside the .qhelp file
  • updated the name to js/... (instead of javascript/...), which I think is the convention

Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

You need to update the expected output of the test, and I think we could use some more references in the QHelp.

But the rest looks good πŸ‘

Copy link
Contributor

github-actions bot commented Jul 1, 2024

QHelp previews:

javascript/ql/src/Security/CWE-693/InsecureHelmet.qhelp

Insecure configuration of Helmet security middleware

Helmet is a collection of middleware functions for securing Express apps. It sets various HTTP headers to guard against common web vulnerabilities. This query detects Helmet misconfigurations that can lead to security vulnerabilities, specifically:

  • Disabling frame protection
  • Disabling Content Security Policy
    Content Security Policy (CSP) helps spot and prevent injection attacks such as Cross-Site Scripting (XSS). Removing frame protections exposes an application to attacks such as clickjacking, where an attacker can trick a user into clicking on a button or link on a targeted page when they intended to click on the page carrying out the attack.

Users of the query can extend the set of required Helmet features by adding additional checks for them, using CodeQL data extensions in a CodeQL model pack. See CUSTOMIZING.md in the query source for more information.

Recommendation

To help mitigate these vulnerabilities, ensure that the following Helmet functions are not disabled, and are configured appropriately to your application:

  • frameguard
  • contentSecurityPolicy

Example

The following code snippet demonstrates Helmet configured in an insecure manner:

const helmet = require('helmet');

app.use(helmet({
    frameguard: false,
    contentSecurityPolicy: false
}));

In this example, the defaults are used, which enables frame protection and a default Content Security Policy.

app.use(helmet());

You can also enable a custom Content Security Policy by passing an object to the contentSecurityPolicy key. For example, taken from the Helmet docs:

app.use(
    helmet({
        contentSecurityPolicy: {
            directives: {
                "script-src": ["'self'", "example.com"],
                "style-src": null,
            },
        },
    })
);

References

@aegilops
Copy link
Contributor Author

aegilops commented Jul 8, 2024

πŸ‘‹ I fixed the expected result, added more references, and fixed a bit of markup in the query help.

I just updated my branch with main and pushed my changes, so I hope this will be a clean run and OK to merge.

Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

Nice, this looks good to me πŸ‘

One below comment about dataExtensions:. I don't know what the right answer is 🀷

I did some evaluations to check that the results/performance look OK.
(nightly, default (internal links)).

Performance was great πŸ‘
And the evaluations found a single new TP, which looked like the below, so that's nice.

app.use(helmet({
  contentSecurityPolicy: false, // @TODO implement

@aegilops
Copy link
Contributor Author

One future enhancement would be to look for places where a config is defined that flows to the app use.

I'm not going to do that right now, since I don't know if it is done in real use, but it's a possibility. I wanted to keep the initial implementation simple and performant, and to work for cases I saw in real code.

There may also be other variations in how the middleware is activated, which I can look out for in future.

@erik-krogh erik-krogh merged commit de9370a into github:main Jul 11, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants