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

added sql injection rules for node express #8

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Newbi3Fix3r
Copy link
Contributor

No description provided.

metavariable: $SQL
patterns:
- pattern-regex: .*\b(?i)(SELECT|CREATE|DROP|INSERT|DELETE|UPDATE|ALTER)\b.*
fix: $CONN.escape($REQ.body.$DATA)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why only body parameters? I guess it can be req.params, req.cookies, req.headers, req.query or req.body

- metavariable-pattern:
metavariable: $SQL
patterns:
- pattern-regex: .*\b(?i)(SELECT|CREATE|DROP|INSERT|DELETE|UPDATE|ALTER)\b.*
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't it enough that that we use "$CONN.query($SQL,...)"? why do we need to validate it starts with an SQL command? Just want to save the performance of executing another regex and it seems it doesn't add much value?

Potential SQL Injection identified. Consider using the connection.escape method built in node-mysql
patterns:
- pattern-inside: |
$MYSQL=require('mysql')
Copy link
Collaborator

Choose a reason for hiding this comment

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

there are other require/import styles (can look at cookie attribute rules for reference) that can be used in JS/TS. We should probably include all styles or just not include the "require" in the rule.

rules:
- id: nodejs-mysql-body-inection
languages:
- javascript
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should probably also use this rule for typescript as well?

- pattern-inside: $APP.options(..., function $FUNC($REQ, $RES) {...})
- pattern-regex: \+.*?\+
- pattern-not-regex: \+.*?escape.*?\+
- pattern: $REQ.body.$DATA
Copy link
Collaborator

Choose a reason for hiding this comment

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

why only body parameters? I guess it can be req.params, req.cookies, req.headers, req.query or req.body

- pattern-inside: $APP.head(..., function $FUNC($REQ, $RES) {...})
- pattern-inside: $APP.delete(..., function $FUNC($REQ, $RES) {...})
- pattern-inside: $APP.options(..., function $FUNC($REQ, $RES) {...})
- pattern-regex: \+.*?\+
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about only a single plus from the right or from the left

- pattern-inside: $APP.head(..., function $FUNC($REQ, $RES) {...})
- pattern-inside: $APP.delete(..., function $FUNC($REQ, $RES) {...})
- pattern-inside: $APP.options(..., function $FUNC($REQ, $RES) {...})
- pattern-regex: \+.*?\+
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it can work with a non regex rule for performance. something like: pattern: ... + $REQ.body.$DATA + ...

- pattern-inside: $APP.head(..., function $FUNC($REQ, $RES) {...})
- pattern-inside: $APP.delete(..., function $FUNC($REQ, $RES) {...})
- pattern-inside: $APP.options(..., function $FUNC($REQ, $RES) {...})
- pattern-regex: \+.*?\+
Copy link
Collaborator

Choose a reason for hiding this comment

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

it is more complete to have also a variant like this:

  • pattern-inside: |
    $STR = ... + $REQ.body.$DATA + ...;
    ...
    -pattern: ... + $STR + ...

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