Skip to content

Commit 8b79989

Browse files
PhilippSalvisbergPhilippSalvisberg
authored
Merge pull request #30 from Trivadis/bugfix/issue-23-9501-false-negative
Bugfix/issue 23 9501 false negative
2 parents 0075c45 + 4702df5 commit 8b79989

File tree

2 files changed

+104
-5
lines changed

2 files changed

+104
-5
lines changed

‎src/main/java/com/trivadis/tvdcc/validators/SQLInjection.xtend‎

Lines changed: 47 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,18 +21,21 @@ import com.trivadis.oracle.plsql.plsql.Body
2121
import com.trivadis.oracle.plsql.plsql.ConstructorDeclaration
2222
import com.trivadis.oracle.plsql.plsql.CreateFunction
2323
import com.trivadis.oracle.plsql.plsql.CreateProcedure
24+
import com.trivadis.oracle.plsql.plsql.DeclareSection
2425
import com.trivadis.oracle.plsql.plsql.ExecuteImmediateStatement
2526
import com.trivadis.oracle.plsql.plsql.FuncDeclInType
2627
import com.trivadis.oracle.plsql.plsql.FunctionDefinition
2728
import com.trivadis.oracle.plsql.plsql.FunctionOrParenthesisParameter
2829
import com.trivadis.oracle.plsql.plsql.OpenForStatement
2930
import com.trivadis.oracle.plsql.plsql.ParameterDeclaration
31+
import com.trivadis.oracle.plsql.plsql.PlsqlBlock
3032
import com.trivadis.oracle.plsql.plsql.ProcDeclInType
3133
import com.trivadis.oracle.plsql.plsql.ProcedureCallOrAssignmentStatement
3234
import com.trivadis.oracle.plsql.plsql.ProcedureDefinition
3335
import com.trivadis.oracle.plsql.plsql.SimpleExpressionNameValue
3436
import com.trivadis.oracle.plsql.plsql.SimpleExpressionStringValue
3537
import com.trivadis.oracle.plsql.plsql.UserDefinedType
38+
import com.trivadis.oracle.plsql.plsql.VariableDeclaration
3639
import com.trivadis.oracle.plsql.validation.PLSQLCopGuideline
3740
import com.trivadis.oracle.plsql.validation.PLSQLCopValidator
3841
import com.trivadis.oracle.plsql.validation.PLSQLJavaValidator
@@ -213,8 +216,11 @@ class SQLInjection extends PLSQLJavaValidator implements PLSQLCopValidator {
213216
214217
215218
def isAsserted(SimpleExpressionNameValue n) {
216-
val body = EcoreUtil2.getContainerOfType(n, Body)
217-
val usages = EcoreUtil2.getAllContentsOfType(body, SimpleExpressionNameValue).filter[it.value.equalsIgnoreCase(n.value)]
219+
var EObject obj = EcoreUtil2.getContainerOfType(n, Body)
220+
if (obj === null) {
221+
obj = EcoreUtil2.getContainerOfType(n, DeclareSection)
222+
}
223+
val usages = EcoreUtil2.getAllContentsOfType(obj, SimpleExpressionNameValue).filter[it.value.equalsIgnoreCase(n.value)]
218224
for (usage : usages) {
219225
val name = usage.qualifiedFunctionName
220226
for (assertPackage : ASSERT_PACKAGES) {
@@ -262,6 +268,32 @@ class SQLInjection extends PLSQLJavaValidator implements PLSQLCopValidator {
262268
}
263269
}
264270
271+
def getDeclareSection(Body body) {
272+
val parent = body.eContainer
273+
var DeclareSection declareSection;
274+
if (parent instanceof CreateFunction) {
275+
declareSection = parent.declareSection
276+
} else if (parent instanceof CreateProcedure) {
277+
declareSection = parent.declareSection
278+
} else if (parent instanceof FuncDeclInType) {
279+
declareSection = parent.declareSection
280+
} else if (parent instanceof ProcDeclInType) {
281+
declareSection = parent.declareSection
282+
} else if (parent instanceof ConstructorDeclaration) {
283+
declareSection = parent.declareSection
284+
} else if (parent instanceof PlsqlBlock) {
285+
declareSection = parent.declareSection
286+
} else if (parent instanceof FunctionDefinition) {
287+
declareSection = parent.declareSection
288+
} else if (parent instanceof ProcedureDefinition) {
289+
declareSection = parent.declareSection
290+
} else {
291+
// CreatePackageBody, CreateTrigger, CreateTypeBody
292+
declareSection = null;
293+
}
294+
return declareSection;
295+
}
296+
265297
def HashMap<String, SimpleExpressionNameValue> getSimpleExpressinNamesFromAssignments(SimpleExpressionNameValue n) {
266298
val expressions = new HashMap<String, SimpleExpressionNameValue>
267299
val body = EcoreUtil2.getContainerOfType(n, Body)
@@ -281,8 +313,16 @@ class SQLInjection extends PLSQLJavaValidator implements PLSQLCopValidator {
281313
}
282314
}
283315
}
284-
if (expressions.size == 0) {
285-
expressions.put(n.value.toLowerCase, n);
316+
val declareSection = body.declareSection
317+
if (declareSection !== null) {
318+
val variable = EcoreUtil2.getAllContentsOfType(declareSection, VariableDeclaration).findFirst [
319+
it.variable.value.equalsIgnoreCase(n.value) && it.getDefault() !== null
320+
]
321+
if (variable !== null) {
322+
for (name : getRelevantSimplExpressionNameValues(variable.getDefault())) {
323+
expressions.put(name.value.toLowerCase, name)
324+
}
325+
}
286326
}
287327
return expressions;
288328
}
@@ -292,6 +332,9 @@ class SQLInjection extends PLSQLJavaValidator implements PLSQLCopValidator {
292332
if (obj !== null) {
293333
if (obj instanceof SimpleExpressionNameValue) {
294334
expressions.putAll(obj.simpleExpressinNamesFromAssignments)
335+
if (expressions.size == 0) {
336+
expressions.put(obj.value.toLowerCase, obj);
337+
}
295338
} else {
296339
for (name : getRelevantSimplExpressionNameValues(obj)) {
297340
expressions.put(name.value.toLowerCase, name)

‎src/test/java/com/trivadis/tvdcc/validators/tests/SQLInjectionTest.xtend‎

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -415,6 +415,62 @@ class SQLInjectionTest extends AbstractValidatorTest {
415415
Assert.assertEquals(7, issues.get(0).lineNumber)
416416
Assert.assertEquals(7, issues.get(1).lineNumber)
417417
}
418+
419+
@Test
420+
def void issue23_assign_parameter_in_declare_section_nok() {
421+
val stmt = '''
422+
CREATE OR REPLACE PROCEDURE get_record (
423+
user_name IN VARCHAR2,
424+
service_type IN VARCHAR2,
425+
rec OUT VARCHAR2
426+
) IS
427+
l_user_name VARCHAR2(4000) := user_name;
428+
l_service_type VARCHAR2(4000) := service_type;
429+
query VARCHAR2(4000);
430+
BEGIN
431+
-- Following SELECT statement is vulnerable to modification
432+
-- because it uses concatenation to build WHERE clause.
433+
query := q'[SELECT value FROM secret_records WHERE user_name=']'
434+
|| l_user_name
435+
|| q'[' AND service_type=']'
436+
|| l_service_type
437+
|| q'[']';
438+
DBMS_OUTPUT.PUT_LINE('Query: ' || query);
439+
EXECUTE IMMEDIATE query INTO rec;
440+
DBMS_OUTPUT.PUT_LINE('Rec: ' || rec);
441+
END;
442+
'''
443+
val issues = stmt.issues
444+
Assert.assertEquals(2, issues.size)
445+
}
446+
447+
@Test
448+
def void issue23_assign_parameter_in_declare_section_ok() {
449+
val stmt = '''
450+
CREATE OR REPLACE PROCEDURE get_record (
451+
user_name IN VARCHAR2,
452+
service_type IN VARCHAR2,
453+
rec OUT VARCHAR2
454+
) IS
455+
l_user_name VARCHAR2(4000) := sys.dbms_assert.enquote_name(user_name);
456+
l_service_type VARCHAR2(4000) := sys.dbms_assert.enquote_name(service_type);
457+
query VARCHAR2(4000);
458+
BEGIN
459+
-- Following SELECT statement is vulnerable to modification
460+
-- because it uses concatenation to build WHERE clause.
461+
query := q'[SELECT value FROM secret_records WHERE user_name=']'
462+
|| l_user_name
463+
|| q'[' AND service_type=']'
464+
|| l_service_type
465+
|| q'[']';
466+
DBMS_OUTPUT.PUT_LINE('Query: ' || query);
467+
EXECUTE IMMEDIATE query INTO rec;
468+
DBMS_OUTPUT.PUT_LINE('Rec: ' || rec);
469+
END;
470+
'''
471+
val issues = stmt.issues
472+
Assert.assertEquals(0, issues.size)
473+
}
418474

419475
@Test
420476
def void issue24_using_parameter_without_expression_in_execute_immediate() {
@@ -429,5 +485,5 @@ class SQLInjectionTest extends AbstractValidatorTest {
429485
val issues = stmt.issues
430486
Assert.assertEquals(1, issues.size)
431487
}
432-
488+
433489
}

0 commit comments

Comments
 (0)