From 93f1fd443e78dbcb98e37947623fb5e2a3dd79d8 Mon Sep 17 00:00:00 2001 From: Tim Lange Date: Thu, 1 Jun 2023 16:05:01 +0200 Subject: [PATCH 1/2] Handover aliases early --- .../infoflow/problems/AliasProblem.java | 78 ++++++++++++------- .../jimple/infoflow/test/HeapTestCode.java | 57 ++++++++++++++ .../jimple/infoflow/test/junit/HeapTests.java | 57 +++++++++++++- 3 files changed, 158 insertions(+), 34 deletions(-) diff --git a/soot-infoflow/src/soot/jimple/infoflow/problems/AliasProblem.java b/soot-infoflow/src/soot/jimple/infoflow/problems/AliasProblem.java index e7df76a2f..bdf30ff2e 100644 --- a/soot-infoflow/src/soot/jimple/infoflow/problems/AliasProblem.java +++ b/soot-infoflow/src/soot/jimple/infoflow/problems/AliasProblem.java @@ -120,21 +120,6 @@ private Set computeAliases(final DefinitionStmt defStmt, Value left final Set res = new MutableTwoElementSet(); - // Check whether the left side of the assignment matches our - // current taint abstraction - final boolean leftSideMatches = Aliasing.baseMatches(leftValue, source); - if (!leftSideMatches) - res.add(source); - else { - // The left side is overwritten completely - - // If we have an assignment to the base local of the current - // taint, all taint propagations must be below that point, - // so this is the right point to turn around. - for (Unit u : interproceduralCFG().getPredsOf(defStmt)) - manager.getMainSolver().processEdge(new PathEdge(d1, u, source)); - } - // We only handle assignments and identity statements if (defStmt instanceof IdentityStmt) { res.add(source); @@ -143,6 +128,12 @@ private Set computeAliases(final DefinitionStmt defStmt, Value left if (!(defStmt instanceof AssignStmt)) return res; + // Check whether the left side of the assignment matches our + // current taint abstraction + final boolean leftSideMatches = Aliasing.baseMatches(leftValue, source); + if (!leftSideMatches) + res.add(source); + // Get the right side of the assignment final Value rightValue = BaseSelector.selectBase(defStmt.getRightOp(), false); @@ -240,21 +231,12 @@ else if (defStmt.getRightOp() instanceof LengthExpr) { newLeftAbs = checkAbstraction(source.deriveNewAbstraction(ap, defStmt)); } - if (newLeftAbs != null) { - // If we ran into a new abstraction that points to a - // primitive value, we can remove it - if (newLeftAbs.getAccessPath().getLastFieldType() instanceof PrimType) - return res; - - if (!newLeftAbs.getAccessPath().equals(source.getAccessPath())) { - // Propagate the new alias upwards - res.add(newLeftAbs); - - // Inject the new alias into the forward solver - for (Unit u : interproceduralCFG().getPredsOf(defStmt)) - manager.getMainSolver() - .processEdge(new PathEdge(d1, u, newLeftAbs)); - } + if (newLeftAbs != null && !newLeftAbs.getAccessPath().equals(source.getAccessPath())) { + // Only inject the new alias into the forward solver but never propagate it upwards + // because the alias was created at this program point and won't be valid above. + for (Unit u : interproceduralCFG().getPredsOf(defStmt)) + manager.getMainSolver() + .processEdge(new PathEdge(d1, u, newLeftAbs)); } } @@ -734,6 +716,42 @@ public Set computeTargets(Abstraction source, Abstraction d1, if (abs != null) { res.add(abs); registerActivationCallSite(callSite, callee, abs); + + // Check whether the call site created an alias by having two equal + // arguments, e.g. caller(o, o);. If yes, inject the other parameter + // back into the callee. + for (int argIndex = 0; !isReflectiveCallSite && argIndex < ie.getArgCount(); argIndex++) { + if (i != argIndex && originalCallArg == ie.getArg(argIndex)) { + AccessPath aliasAp = manager.getAccessPathFactory().copyWithNewValue( + source.getAccessPath(), paramLocals[argIndex], + source.getAccessPath().getBaseType(), + false); + Abstraction aliasAbs = checkAbstraction( + source.deriveNewAbstraction(aliasAp, (Stmt) exitStmt)); + + manager.getMainSolver() + .processEdge(new PathEdge<>(d1, exitStmt, aliasAbs)); + } + } + + // A foo(A a) { + // return a; + // } + // A b = foo(a); + // An alias is created using the returned value. If no assignment + // happen inside the method, also no handover is triggered. Thus, + // for this special case, we hand over the current taint and let the + // forward analysis find out whether the return value actually created + // an alias or not. + for (Unit u : manager.getICFG().getStartPointsOf(callee)) { + if (!(u instanceof ReturnStmt)) + continue; + + if (paramLocals[i] == ((ReturnStmt) u).getOp()) { + manager.getMainSolver().processEdge(new PathEdge<>(d1, exitStmt, source)); + break; + } + } } } } diff --git a/soot-infoflow/test/soot/jimple/infoflow/test/HeapTestCode.java b/soot-infoflow/test/soot/jimple/infoflow/test/HeapTestCode.java index d152e7805..1a4522afc 100644 --- a/soot-infoflow/test/soot/jimple/infoflow/test/HeapTestCode.java +++ b/soot-infoflow/test/soot/jimple/infoflow/test/HeapTestCode.java @@ -793,6 +793,24 @@ public void singleAliasTest() { cm.publish(b.b); } + public void negativeSingleAliasTest() { + A a = new A(); + A b = fakeAlias(a); + a.b = TelephonyManager.getDeviceId(); + ConnectionManager cm = new ConnectionManager(); + cm.publish(b.b); + } + + public A doNotFold() { + A a = new A(); + System.out.println("XXX"); + return a; + } + + private A fakeAlias(A a) { + return doNotFold(); + } + private int intData; private void setIntData() { @@ -1611,4 +1629,43 @@ public void activationStatementTest1() { cm.publish(specialName); } + public void callSiteCreatesAlias() { + String tainted = TelephonyManager.getDeviceId(); + + Book book1 = new Book(); + leakingCallee(tainted, new Book(), book1); + leakingCallee(tainted, book1, book1); + } + + void leakingCallee(String tainted, Book book1, Book book2) { + book1.name = tainted; + ConnectionManager cm = new ConnectionManager(); + cm.publish(book2.name); + } + + public Book alias; + public void lhsNotUpwardsInAliasFlow() { + alias = new Book(); + + Book book = new Book(); + Book alias2 = alias; + alias = book; // alias only aliases book downwards from this program point + book.name = TelephonyManager.getDeviceId(); + ConnectionManager cm = new ConnectionManager(); + cm.publish(alias2.name); + } + + public void identityStmtIsNotAGoodHandoverPoint() { + Book book = new Book(); + // No need to propagate book forward again + callee(book); + book.name = TelephonyManager.getDeviceId(); + + ConnectionManager cm = new ConnectionManager(); + cm.publish(book.name); + } + + void callee(Book b) { + System.out.println(b); + } } diff --git a/soot-infoflow/test/soot/jimple/infoflow/test/junit/HeapTests.java b/soot-infoflow/test/soot/jimple/infoflow/test/junit/HeapTests.java index 9eca38936..d76cd2f9b 100644 --- a/soot-infoflow/test/soot/jimple/infoflow/test/junit/HeapTests.java +++ b/soot-infoflow/test/soot/jimple/infoflow/test/junit/HeapTests.java @@ -20,10 +20,7 @@ import org.junit.Ignore; import org.junit.Test; -import soot.RefType; -import soot.Scene; -import soot.SootField; -import soot.SootMethod; +import soot.*; import soot.jimple.AssignStmt; import soot.jimple.DefinitionStmt; import soot.jimple.InstanceInvokeExpr; @@ -38,6 +35,7 @@ import soot.jimple.infoflow.data.SootMethodAndClass; import soot.jimple.infoflow.entryPointCreators.DefaultEntryPointCreator; import soot.jimple.infoflow.entryPointCreators.SequentialEntryPointCreator; +import soot.jimple.infoflow.handlers.TaintPropagationHandler; import soot.jimple.infoflow.results.InfoflowResults; import soot.jimple.infoflow.sourcesSinks.definitions.MethodSourceSinkDefinition; import soot.jimple.infoflow.sourcesSinks.manager.ISourceSinkManager; @@ -674,6 +672,19 @@ public void singleAliasTest() { checkInfoflow(infoflow, 1); } + @Test(timeout = 300000) + public void negativeSingleAliasTest() { + IInfoflow infoflow = initInfoflow(); + infoflow.getConfig().setInspectSources(false); + infoflow.getConfig().setInspectSinks(false); + infoflow.getConfig().setWriteOutputFiles(true); + + List epoints = new ArrayList(); + epoints.add(""); + infoflow.computeInfoflow(appPath, libPath, epoints, sources, sinks); + negativeCheckInfoflow(infoflow); + } + @Test(timeout = 300000) public void intAliasTest() { IInfoflow infoflow = initInfoflow(); @@ -1269,4 +1280,42 @@ public void activationStatementTest1() { negativeCheckInfoflow(infoflow); } + + @Test(timeout = 300000) + public void callSiteCreatesAlias() { + IInfoflow infoflow = initInfoflow(); + List epoints = new ArrayList(); + epoints.add(""); + infoflow.computeInfoflow(appPath, libPath, epoints, sources, sinks); + checkInfoflow(infoflow, 1); + } + + @Test(timeout = 300000) + public void lhsNotUpwardsInAliasFlow() { + IInfoflow infoflow = initInfoflow(); + List epoints = new ArrayList(); + epoints.add(""); + infoflow.computeInfoflow(appPath, libPath, epoints, sources, sinks); + negativeCheckInfoflow(infoflow); + } + + @Test(timeout = 300000) + public void identityStmtIsNotAGoodHandoverPoint() { + IInfoflow infoflow = initInfoflow(); + List epoints = new ArrayList(); + infoflow.setTaintPropagationHandler(new TaintPropagationHandler() { + @Override + public void notifyFlowIn(Unit stmt, Abstraction taint, InfoflowManager manager, FlowFunctionType type) { + Assert.assertTrue(taint.isAbstractionActive()); + } + + @Override + public Set notifyFlowOut(Unit stmt, Abstraction d1, Abstraction incoming, Set outgoing, InfoflowManager manager, FlowFunctionType type) { + return outgoing; + } + }); + epoints.add(""); + infoflow.computeInfoflow(appPath, libPath, epoints, sources, sinks); + checkInfoflow(infoflow, 1); + } } From f108b202898205f9141bd35008b7d5800259fe71 Mon Sep 17 00:00:00 2001 From: Tim Lange Date: Thu, 1 Jun 2023 17:16:04 +0200 Subject: [PATCH 2/2] Fix new testcase in backward direction --- .../problems/BackwardsAliasProblem.java | 60 +++++++++---------- .../jimple/infoflow/test/junit/HeapTests.java | 1 - 2 files changed, 30 insertions(+), 31 deletions(-) diff --git a/soot-infoflow/src/soot/jimple/infoflow/problems/BackwardsAliasProblem.java b/soot-infoflow/src/soot/jimple/infoflow/problems/BackwardsAliasProblem.java index 38385e5bc..53ded3a95 100644 --- a/soot-infoflow/src/soot/jimple/infoflow/problems/BackwardsAliasProblem.java +++ b/soot-infoflow/src/soot/jimple/infoflow/problems/BackwardsAliasProblem.java @@ -15,22 +15,7 @@ import soot.Type; import soot.Unit; import soot.Value; -import soot.jimple.ArrayRef; -import soot.jimple.AssignStmt; -import soot.jimple.BinopExpr; -import soot.jimple.CastExpr; -import soot.jimple.DefinitionStmt; -import soot.jimple.FieldRef; -import soot.jimple.IdentityStmt; -import soot.jimple.InstanceFieldRef; -import soot.jimple.InstanceInvokeExpr; -import soot.jimple.InstanceOfExpr; -import soot.jimple.InvokeExpr; -import soot.jimple.NewArrayExpr; -import soot.jimple.ReturnStmt; -import soot.jimple.StaticFieldRef; -import soot.jimple.Stmt; -import soot.jimple.UnopExpr; +import soot.jimple.*; import soot.jimple.infoflow.InfoflowConfiguration; import soot.jimple.infoflow.InfoflowManager; import soot.jimple.infoflow.aliasing.Aliasing; @@ -135,42 +120,57 @@ private Set computeAliases(final DefinitionStmt defStmt, Abstractio AccessPath ap = source.getAccessPath(); Value sourceBase = ap.getPlainValue(); + Type rightType = rightOp.getType(); boolean handoverLeftValue = false; + boolean cutSubfield = false; boolean leftSideOverwritten = false; if (leftOp instanceof StaticFieldRef) { if (manager.getConfig() .getStaticFieldTrackingMode() != InfoflowConfiguration.StaticFieldTrackingMode.None && ap.firstFieldMatches(((StaticFieldRef) leftOp).getField())) { handoverLeftValue = true; + cutSubfield = true; } } else if (leftOp instanceof InstanceFieldRef) { InstanceFieldRef instRef = (InstanceFieldRef) leftOp; // base matches if (instRef.getBase() == sourceBase) { - // field matches - if (ap.firstFieldMatches(instRef.getField())) { - handoverLeftValue = true; - } - // whole object matches - else if (ap.getTaintSubFields() && ap.getFragmentCount() == 0) { - handoverLeftValue = true; - } - // due to cut down access path we can not know better - else if (source.dependsOnCutAP() || isCircularType(leftVal)) { + AccessPath mappedAp = Aliasing.getReferencedAPBase(ap, + new SootField[] { instRef.getField() }, manager); + if (mappedAp != null) { handoverLeftValue = true; + cutSubfield = true; + if (!mappedAp.equals(ap)) + ap = mappedAp; } } } else if (leftVal == sourceBase) { // Either the alias is overwritten here or a write to an array element - handoverLeftValue = leftOp instanceof ArrayRef; + handoverLeftValue = leftOp instanceof ArrayRef + && ap.getArrayTaintType() != AccessPath.ArrayTaintType.Length; leftSideOverwritten = !handoverLeftValue; } if (handoverLeftValue) { - // We found a missed path upwards - // inject same stmt in infoflow solver - handOver(d1, srcUnit, source); + Abstraction newAbs = null; + if (rightVal instanceof Constant) { + if (manager.getConfig().getImplicitFlowMode().trackControlFlowDependencies()) { + newAbs = source.deriveConditionalUpdate(assignStmt); + for (Unit pred : manager.getICFG().getPredsOf(srcUnit)) + handOver(d1, pred, newAbs); + } + } else { + AccessPath newAp = manager.getAccessPathFactory().copyWithNewValue(ap, rightOp, rightType, cutSubfield); + newAbs = source.deriveNewAbstraction(newAp, assignStmt); + } + + if (newAbs != null && !newAbs.equals(source)) { + // We found a missed path upwards + // inject same stmt in infoflow solver + for (Unit pred : manager.getICFG().getPredsOf(srcUnit)) + handOver(d1, pred, newAbs); + } } if (leftSideOverwritten) diff --git a/soot-infoflow/test/soot/jimple/infoflow/test/junit/HeapTests.java b/soot-infoflow/test/soot/jimple/infoflow/test/junit/HeapTests.java index d76cd2f9b..5723c8cda 100644 --- a/soot-infoflow/test/soot/jimple/infoflow/test/junit/HeapTests.java +++ b/soot-infoflow/test/soot/jimple/infoflow/test/junit/HeapTests.java @@ -677,7 +677,6 @@ public void negativeSingleAliasTest() { IInfoflow infoflow = initInfoflow(); infoflow.getConfig().setInspectSources(false); infoflow.getConfig().setInspectSinks(false); - infoflow.getConfig().setWriteOutputFiles(true); List epoints = new ArrayList(); epoints.add("");