-
Notifications
You must be signed in to change notification settings - Fork 7
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
message: | | ||
Potential SQL Injection identified. Consider using the connection.escape method built in node-mysql | ||
patterns: | ||
- pattern-inside: | | ||
$MYSQL=require('mysql') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: \+.*?\+ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 + ... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is more complete to have also a variant like this:
|
||
- pattern-not-regex: \+.*?escape.*?\+ | ||
- pattern: $REQ.body.$DATA | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
There was a problem hiding this comment.
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?