FindSqlInjection: support of long/double parameters in indirectly called

methods; isSafeValue is called for correct location when SQL query is not the last parameter.
findbugsproject · Oct 6, 2015 · 32a20db · 32a20db
1 parent 370808a
commit 32a20db
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 5 deletions.
diff --git a/findbugs/src/java/edu/umd/cs/findbugs/detect/FindSqlInjection.java b/findbugs/src/java/edu/umd/cs/findbugs/detect/FindSqlInjection.java
@@ -47,19 +47,24 @@
 import edu.umd.cs.findbugs.BugReporter;
 import edu.umd.cs.findbugs.Detector;
 import edu.umd.cs.findbugs.SourceLineAnnotation;
+import edu.umd.cs.findbugs.ba.AnalysisContext;
 import edu.umd.cs.findbugs.ba.BasicBlock;
 import edu.umd.cs.findbugs.ba.CFG;
 import edu.umd.cs.findbugs.ba.CFGBuilderException;
 import edu.umd.cs.findbugs.ba.ClassContext;
 import edu.umd.cs.findbugs.ba.DataflowAnalysisException;
 import edu.umd.cs.findbugs.ba.EdgeTypes;
 import edu.umd.cs.findbugs.ba.Location;
+import edu.umd.cs.findbugs.ba.SignatureParser;
 import edu.umd.cs.findbugs.ba.constant.Constant;
 import edu.umd.cs.findbugs.ba.constant.ConstantDataflow;
 import edu.umd.cs.findbugs.ba.constant.ConstantFrame;
 import edu.umd.cs.findbugs.ba.type.TopType;
 import edu.umd.cs.findbugs.ba.type.TypeDataflow;
 import edu.umd.cs.findbugs.ba.type.TypeFrame;
+import edu.umd.cs.findbugs.ba.vna.ValueNumber;
+import edu.umd.cs.findbugs.ba.vna.ValueNumberDataflow;
+import edu.umd.cs.findbugs.ba.vna.ValueNumberFrame;
 import edu.umd.cs.findbugs.classfile.CheckedAnalysisException;
 import edu.umd.cs.findbugs.classfile.Global;
 import edu.umd.cs.findbugs.classfile.MethodDescriptor;
@@ -496,6 +501,7 @@ private void analyzeMethod(ClassContext classContext, Method method) throws Data
         StringAppendState stringAppendState = getStringAppendState(cfg, cpg);
 
         ConstantDataflow dataflow = classContext.getConstantDataflow(method);
+        ValueNumberDataflow vnd = classContext.getValueNumberDataflow(method);
         for (Iterator<Location> i = cfg.locationIterator(); i.hasNext();) {
             Location location = i.next();
             Instruction ins = location.getHandle().getInstruction();
@@ -522,15 +528,16 @@ private void analyzeMethod(ClassContext classContext, Method method) throws Data
                 }
             }
             ConstantFrame frame = dataflow.getFactAtLocation(location);
-            int numArguments = frame.getNumArguments(invoke, cpg);
-            Constant value = frame.getStackValue(numArguments - 1 - paramNumber);
+            SignatureParser parser = new SignatureParser(invoke.getSignature(cpg));
+            Constant value = frame.getArgument(invoke, cpg, paramNumber, parser);
+            ValueNumber vn = vnd.getFactAtLocation(location).getArgument(invoke, cpg, paramNumber, parser);
 
             if (!value.isConstantString()) {
                 // TODO: verify it's the same string represented by
                 // stringAppendState
                 // FIXME: will false positive on const/static strings
                 // returns by methods
-                Location prev = getPreviousLocation(cfg, location, true);
+                Location prev = getValueNumberCreationLocation(vnd, vn);
                 if (prev == null || !isSafeValue(prev, cpg)) {
                     BugInstance bug = generateBugInstance(javaClass, methodGen, location.getHandle(), stringAppendState,
                             executeMethod);
@@ -544,6 +551,25 @@ private void analyzeMethod(ClassContext classContext, Method method) throws Data
         bugAccumulator.reportAccumulatedBugs();
     }
 
+    private Location getValueNumberCreationLocation(ValueNumberDataflow vnd, ValueNumber vn) {
+        ConstantPoolGen cpg = vnd.getCFG().getMethodGen().getConstantPool();
+        for(Iterator<Location> it = vnd.getCFG().locationIterator(); it.hasNext(); ) {
+            Location loc = it.next();
+            if(loc.getHandle().getInstruction().produceStack(cpg) != 1) {
+                continue;
+            }
+            try {
+                ValueNumberFrame vnf = vnd.getFactAfterLocation(loc);
+                if(vnf.getTopValue().equals(vn)) {
+                    return loc;
+                }
+            } catch (DataflowAnalysisException e) {
+                AnalysisContext.logError("While analyzing "+vnd.getCFG().getMethodGen()+" at "+loc, e);
+            }
+        }
+        return null;
+    }
+
     @Override
     public void report() {
     }

diff --git a/findbugsTestCases/src/java/sfBugsNew/Feature314.java b/findbugsTestCases/src/java/sfBugsNew/Feature314.java
@@ -1,7 +1,11 @@
 package sfBugsNew;
 
-import java.io.*;
-import java.sql.*;
+import java.io.FileNotFoundException;
+import java.io.FileOutputStream;
+import java.sql.Connection;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+import java.sql.Statement;
 
 import edu.umd.cs.findbugs.annotations.ExpectWarning;
 import edu.umd.cs.findbugs.annotations.NoWarning;
@@ -71,11 +75,28 @@ public boolean test(Connection c, String code) throws SQLException {
         return Sql.hasResult(c, "SELECT 1 FROM myTable WHERE code='"+code+"'");
     }
 
+    @ExpectWarning("SQL_NONCONSTANT_STRING_PASSED_TO_EXECUTE")
+    public boolean testSqlLong(Connection c, String code) throws SQLException {
+        return Sql.hasResult(c, 1L, "SELECT 1 FROM myTable WHERE code='"+code+"'", 2L, "blahblah");
+    }
+
+    @NoWarning("SQL_NONCONSTANT_STRING_PASSED_TO_EXECUTE")
+    public boolean testSqlLongOk(Connection c, String code) throws SQLException {
+        return Sql.hasResult(c, 1L, "SELECT COUNT(*) FROM myTable", 2L, "Code: "+code);
+    }
+
     public static class Sql {
         public static boolean hasResult(Connection c, String query) throws SQLException {
             try (Statement st = c.createStatement(); ResultSet rs = st.executeQuery(query)) {
                 return rs.next();
             }
         }
+
+        public static boolean hasResult(Connection c, long l1, String query, long l2, String somethingElse) throws SQLException {
+            System.out.println(somethingElse+": "+l1+":"+l2);
+            try (Statement st = c.createStatement(); ResultSet rs = st.executeQuery(query)) {
+                return rs.next();
+            }
+        }
     }
 }