CryptoAnalysis icon indicating copy to clipboard operation
CryptoAnalysis copied to clipboard

Fix check on negated alternatives

Open marvinvo opened this issue 3 years ago • 4 comments

How does the check work?

CrySL required preds will be stored as CrySLConstraints. The ConstraintSolvers will convert required CrySLConstraints to AlternativeReqPredicates, see here https://github.com/CROSSINGTUD/CryptoAnalysis/blob/35d09163f97b6919a4359fcaa0e846af95c1fed1/CryptoAnalysis/src/main/java/crypto/constraints/ConstraintSolver.java#L111 All required preds, also the AlternativeReqPredicates, will be used to check wether a AnalysisSeedWithSpecification satisfies its required predicates or not in the checkPredicates() method. https://github.com/CROSSINGTUD/CryptoAnalysis/blob/35d09163f97b6919a4359fcaa0e846af95c1fed1/CryptoAnalysis/src/main/java/crypto/analysis/AnalysisSeedWithSpecification.java#L377

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.

marvinvo avatar Feb 23 '22 09:02 marvinvo

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;			
	}
}

marvinvo avatar Feb 23 '22 10:02 marvinvo

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

Also, could you please look into https://github.com/CROSSINGTUD/CryptoAnalysis/pull/265 and it's liked issue. It this a duplicate?

AnakinRaW avatar Mar 01 '22 14:03 AnakinRaW

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: https://github.com/CROSSINGTUD/CryptoAnalysis/blob/35d09163f97b6919a4359fcaa0e846af95c1fed1/CryptoAnalysis/src/main/java/crypto/predicates/PredicateHandler.java#L233 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..

marvinvo avatar Mar 02 '22 13:03 marvinvo

scheduled for 2.8

AnakinRaW avatar Oct 19 '22 12:10 AnakinRaW

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

smeyer198 avatar Jan 15 '24 15:01 smeyer198