Skip to content

Conversation

@t1mlange
Copy link
Contributor

No description provided.

@StevenArzt
Copy link
Member

This merge request changes the semantis of taint wrappers. At the moment, we do not models that simply retain taint. If a callee removes a taint, that is explicitly modeled using a clear flow. I even remember code that drops identity summaries, i.e., summary flows with equal from and to. Therefore, I'm quite reluctant to merge such a fundamental change without very careful consideration and evaluation.

@t1mlange
Copy link
Contributor Author

This merge request changes the semantis of taint wrappers. At the moment, we do not models that simply retain taint. If a callee removes a taint, that is explicitly modeled using a clear flow. I even remember code that drops identity summaries, i.e., summary flows with equal from and to. Therefore, I'm quite reluctant to merge such a fundamental change without very careful consideration and evaluation.

First, I thought the classSupported boolean would ensure that when we have summaries for the class, but not for the method itself that the taint is kept.

if (classSupported.value)
return Collections.singleton(taintedAbs);
else {
reportMissingSummary(callee, stmt, taintedAbs);
return fallbackWrapper != null ? fallbackWrapper.getTaintsForMethod(stmt, d1, taintedAbs) : null;
}

Looking at the summary resolving, classSupported is only set when there is a method match. IMO line 58 is dead code here but what should be executed if there is a class summary but no method summary.

if (calleeClass != null)
isClassSupported = getSummaries(methodSig, classSummaries, calleeClass);
// If we haven't found any summaries, we look at the class from the declared
// type at the call site
if (declaredClass != null && !isClassSupported)
isClassSupported = getSummaries(methodSig, classSummaries, declaredClass);
// If we still don't have anything, we must try the hierarchy. Since this
// best-effort approach can be fairly imprecise, it is our last resort.
if (!isClassSupported && calleeClass != null)
isClassSupported = getSummariesHierarchy(methodSig, classSummaries, calleeClass);
if (declaredClass != null && !isClassSupported)
isClassSupported = getSummariesHierarchy(methodSig, classSummaries, declaredClass);
if (isClassSupported) {
if (classSummaries.isEmpty())
return SummaryResponse.EMPTY_BUT_SUPPORTED;
return new SummaryResponse(classSummaries, isClassSupported);

I have pushed some changes that should keep the current semantics of identity on not explicitly mentioned methods in summarized classes while allowing to omit the identity on unrelated classes.

@StevenArzt StevenArzt merged commit 782ff5b into secure-software-engineering:develop Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants