From 32a20dbfe3d6956488d957b626cb184ceff4d881 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Tue, 6 Oct 2015 11:10:44 +0600 Subject: [PATCH] FindSqlInjection: support of long/double parameters in indirectly called methods; isSafeValue is called for correct location when SQL query is not the last parameter. --- .../cs/findbugs/detect/FindSqlInjection.java | 32 +++++++++++++++++-- .../src/java/sfBugsNew/Feature314.java | 25 +++++++++++++-- 2 files changed, 52 insertions(+), 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 index 37c8132855..e45b77a363 100644 --- a/findbugs/src/java/edu/umd/cs/findbugs/detect/FindSqlInjection.java +++ b/findbugs/src/java/edu/umd/cs/findbugs/detect/FindSqlInjection.java @@ -47,6 +47,7 @@ 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; @@ -54,12 +55,16 @@ 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 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 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 index 066faa1cf8..bc01142ce2 100644 --- 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(); + } + } } }