Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix check on negated alternatives #372

Closed
wants to merge 2 commits into from

Conversation

marvinvo
Copy link
Contributor

How does the check work?

CrySL required preds will be stored as CrySLConstraints. The ConstraintSolvers will convert required CrySLConstraints to AlternativeReqPredicates, see here

requiredPredicates.add(collectAlternativePredicates((CrySLConstraint) cons, null));

All required preds, also the AlternativeReqPredicates, will be used to check wether a AnalysisSeedWithSpecification satisfies its required predicates or not in the checkPredicates() method.
for (ISLConstraint con : constraintSolver.getRequiredPredicates()) {

What is the issue?

Checking on an AlternativeReqPredicate with all alternative predicates negated - keep in mind that they are connected with a logical or - is done by returning false (false = predicates does not satisfy constraints) if only one negated alternative predicate is ensured by the seed.
That is wrong, because it should actually satisfy its constraints, if at least one negated alternative predicate is not ensured, since they are connected with a logical or.

This needs to be testet first

Current tests does not cover this code, so someone should write tests before merging.

Since they are negated predicates and connected with an logical or, the alternative pred should be satisfied as long as not all predicates are ensured.
@marvinvo marvinvo requested a review from schlichtig February 23, 2022 09:36
@marvinvo
Copy link
Contributor Author

It's a wrong fix I did there, should be more like:


if (negatives.size() == alternatives.size()) {
	if (alternatives.parallelStream().allMatch(e -> e is in list of ensured predicates) {
	    return false;			
	}
}

@AnakinRaW
Copy link
Collaborator

AnakinRaW commented Mar 1, 2022

@marvinvo
Could you please write some tests?
Negative and positive tests please!

Also, could you please look into #265 and it's liked issue. It this a duplicate?

This also needed a more concrete assertion with signatur "errorCount(int expectedCount)"
@marvinvo
Copy link
Contributor Author

marvinvo commented Mar 2, 2022

Here a little update:
I created a rule as follows:

SPEC java.security.SecureRandom

OBJECTS 
	byte[] seed;
	byte[] next;
	
EVENTS
	c1: SecureRandom();
	Cons := c1;

	nB: nextBytes(next);
	Ends := nB;

ORDER
	Cons, Ends*
 	
REQUIRES	
	!test[next] || !test2[next];
	
ENSURES
	test[next] after Ends;

The assertion code

SecureRandom sr = new SecureRandom();
byte[] genSeed = new byte[32];
sr.nextBytes(genSeed); // this ensured genSeed as test
Assertions.hasEnsuredPredicate(genSeed);
SecureRandom sr2 = new SecureRandom();
sr2.nextBytes(genSeed); 
// genSeed should be either !test or !test2
// since test2 was nowhere ensured, the result should throw no errors
Assertions.errorCount(0);

does not throw any errors, which is weird in my eyes. So I started debugging and it turns out, that the error is catched by an if statement at this line:

if (!rule.getPredicates().parallelStream().anyMatch(e -> missingPred.getPred().getPredName().equals(e.getPredName()) && missingPred.getPred().getParameters().get(0).equals(e.getParameters().get(0)))) {

Is there a reason why it should throw now errors if a predicate is missed but could be ensured by the rule?

I'm still convinced that this is an issue, but it may need a larger test with at least two rules to get over this if statement..

@AnakinRaW
Copy link
Collaborator

scheduled for 2.8

smeyer198 added a commit to marvinvo/CryptoAnalysis that referenced this pull request Jan 15, 2024
@smeyer198
Copy link
Contributor

A fix and tests have been added in #376. Therefore, this PR is not required anymore

@smeyer198 smeyer198 closed this Jan 15, 2024
@smeyer198 smeyer198 deleted the fix_alternative_pred_check branch January 15, 2024 15:21
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.

3 participants