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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
const express = require("express");
const app = express();
const mysql = require('mysql');

const connection = mysql.createConnection({
host : 'localhost',
user : 'username',
password : 'password',
database : 'databasename'
});

connection.connect((err) => {
if(err) throw err;
console.log('Connected to MySQL Server!');
});

//this is vulnerable code
app.get("/insecure",(req,res) => {
connection.query("SELECT * FROM studesnt_records WHERE sid = '"+ req.body.sid +"' AND name = '"+ req.body.name +"'", (error, results) => {
if (error) throw error;
});
});

//this is safer code || Use prepared statements where/when possible
app.get("/safer",(req,res) => {
connection.query("SELECT * FROM student_records WHERE sid = '"+ connection.escape(req.body.sid) +"' AND name = '"+ connection.escape(req.body.name) +"'", (error, results) => {
if (error) throw error;
});
});

//this is safe code
app.get("/safe",(req,res) => {
connection.query("SELECT * FROM health_records WHERE dob = ? AND name = ?",[req.body.dob, req.body.name], (error, results) => {
if(error) throw err;
console.log('The data from users table are: \n', rows);
connection.end();
});
});


//this is unexploitable code
app.get("/safe",(req,res) => {
connection.query("SELECT * FROM student_records WHERE sid = '123465' AND name = 'Dan'", (error, results) => {
if (error) throw error;
});
});


app.listen(3000, () => {
console.log('Server is running at port 3000');
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
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?

message: |
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.

...
$CONN = $MYSQL.createConnection(...)
...
- pattern: |
$CONN.query($SQL,...)
- pattern-either:
- pattern-inside: function ... ($REQ, $RES) {...}
- pattern-inside: function ... ($REQ, $RES, $NEXT) {...}
- pattern-inside: $APP.get(..., function $FUNC($REQ, $RES) {...})
- pattern-inside: $APP.post(..., function $FUNC($REQ, $RES) {...})
- pattern-inside: $APP.put(..., function $FUNC($REQ, $RES) {...})
- 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

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 + ...

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 + ...

- 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

- 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?

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

severity: ERROR