Skip to content

Commit a6ea3e8

Browse files
h3xstreamh3xstream
committed
Merge pull request #53 from formanek/master
ECB and no integrity detection + tests
2 parents 6190db7 + 8fec379 commit a6ea3e8

File tree

3 files changed

+100
-43
lines changed

3 files changed

+100
-43
lines changed

‎plugin/src/main/java/com/h3xstream/findsecbugs/crypto/CipherWithNoIntegrityDetector.java‎

Lines changed: 35 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,12 @@
2323
import edu.umd.cs.findbugs.OpcodeStack;
2424
import edu.umd.cs.findbugs.Priorities;
2525
import edu.umd.cs.findbugs.bcel.OpcodeStackDetector;
26+
import java.util.regex.Pattern;
2627
import org.apache.bcel.Constants;
2728

28-
import java.util.Arrays;
29-
import java.util.List;
30-
3129
/**
32-
* <p>
3330
* This detector mark cipher usage that doesn't provide integrity.
31+
* <p>
3432
* The identification will be made base on the mode use.
3533
* </p>
3634
* <p>
@@ -53,53 +51,55 @@
5351
* </ul>
5452
* </p>
5553
* Ref: <a href="http://en.wikipedia.org/wiki/Authenticated_encryption">Wikipedia: Authenticated encryption</a>
54+
* Ref for the list of potential ciphers:
55+
* http://docs.oracle.com/javase/7/docs/technotes/guides/security/StandardNames.html#impl
56+
* Note: Not all ECB mode are vulnerable. see RSA/ECB/*
5657
*/
5758
public class CipherWithNoIntegrityDetector extends OpcodeStackDetector {
5859

5960
private static final String ECB_MODE_TYPE = "ECB_MODE";
6061
private static final String PADDING_ORACLE_TYPE = "PADDING_ORACLE";
6162
private static final String CIPHER_INTEGRITY_TYPE = "CIPHER_INTEGRITY";
6263

63-
private static final boolean DEBUG = false;
64-
private static final List<String> SECURE_CIPHER_MODES = Arrays.asList("CCM","CWC","OCB","EAX","GCM");
64+
private static final Pattern AUTHENTICATED_CIPHER_MODES = Pattern.compile(".*/(CCM|CWC|OCB|EAX|GCM)/.*");
65+
private static final Pattern INSECURE_ECB_MODES = Pattern.compile("(AES|DES(ede)?)(/ECB/.*)?");
6566

66-
private BugReporter bugReporter;
67+
private final BugReporter bugReporter;
6768

6869
public CipherWithNoIntegrityDetector(BugReporter bugReporter) {
6970
this.bugReporter = bugReporter;
7071
}
7172

7273
@Override
7374
public void sawOpcode(int seen) {
74-
if (seen == Constants.INVOKESTATIC && getClassConstantOperand().equals("javax/crypto/Cipher") &&
75-
getNameConstantOperand().equals("getInstance")) {
76-
OpcodeStack.Item item = stack.getStackItem(stack.getStackDepth() - 1); //The first argument is last
77-
if (StackUtils.isConstantString(item)) {
78-
String cipherValue = (String) item.getConstant();
79-
if (DEBUG) System.out.println(cipherValue);
80-
81-
//Ref for the list of potential ciphers : http://docs.oracle.com/javase/7/docs/technotes/guides/security/StandardNames.html#impl
82-
//Note: Not all ECB mode are vulnerable. see RSA/ECB/*
83-
if (cipherValue.contains("AES/ECB/") || cipherValue.contains("DES/ECB/") || cipherValue.contains("DESede/ECB/")) {
84-
bugReporter.reportBug(new BugInstance(this, ECB_MODE_TYPE, Priorities.HIGH_PRIORITY) //
85-
.addClass(this).addMethod(this).addSourceLine(this));
86-
}
87-
88-
if (cipherValue.contains("/CBC/PKCS5Padding")) {
89-
bugReporter.reportBug(new BugInstance(this, PADDING_ORACLE_TYPE, Priorities.HIGH_PRIORITY) //
90-
.addClass(this).addMethod(this).addSourceLine(this));
91-
}
92-
93-
String[] cipherDefinition = cipherValue.split("/");
94-
if(cipherDefinition.length > 1 && !cipherValue.startsWith("RSA/")) { //Some cipher will not have mode specified (ie: "RSA" .. issue GitHub #24)
95-
String cipherMode = cipherDefinition[1];
75+
if ((seen != Constants.INVOKESTATIC
76+
|| !getClassConstantOperand().equals("javax/crypto/Cipher"))
77+
|| !getNameConstantOperand().equals("getInstance")) {
78+
return;
79+
}
80+
OpcodeStack.Item item = stack.getStackItem(stack.getStackDepth() - 1);
81+
String cipherValue;
82+
if (StackUtils.isConstantString(item)) {
83+
cipherValue = (String) item.getConstant();
84+
} else {
85+
return;
86+
}
87+
if (INSECURE_ECB_MODES.matcher(cipherValue).matches()) {
88+
reportBug(ECB_MODE_TYPE);
89+
}
90+
if (cipherValue.contains("/CBC/PKCS5Padding")) {
91+
reportBug(PADDING_ORACLE_TYPE);
92+
}
9693

97-
if (!SECURE_CIPHER_MODES.contains(cipherMode)) { //Not a secure mode!
98-
bugReporter.reportBug(new BugInstance(this, CIPHER_INTEGRITY_TYPE, Priorities.HIGH_PRIORITY) //
99-
.addClass(this).addMethod(this).addSourceLine(this));
100-
}
101-
}
102-
}
94+
//Some cipher will not have mode specified (ie: "RSA" .. issue GitHub #24)
95+
if (!AUTHENTICATED_CIPHER_MODES.matcher(cipherValue).matches()
96+
&& !cipherValue.startsWith("RSA")) {
97+
reportBug(CIPHER_INTEGRITY_TYPE);
10398
}
10499
}
100+
101+
private void reportBug(String type) {
102+
bugReporter.reportBug(new BugInstance(this, type, Priorities.HIGH_PRIORITY)
103+
.addClass(this).addMethod(this).addSourceLine(this));
104+
}
105105
}

‎plugin/src/test/java/com/h3xstream/findsecbugs/crypto/CipherWithNoIntegrityDetectorTest.java‎

Lines changed: 48 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,33 +19,73 @@
1919

2020
import com.h3xstream.findbugs.test.BaseDetectorTest;
2121
import com.h3xstream.findbugs.test.EasyBugReporter;
22-
import org.testng.annotations.Test;
23-
24-
import static org.mockito.Mockito.never;
22+
import java.util.Arrays;
23+
import java.util.List;
2524
import static org.mockito.Mockito.spy;
25+
import static org.mockito.Mockito.times;
2626
import static org.mockito.Mockito.verify;
27+
import org.testng.annotations.Test;
2728

2829
public class CipherWithNoIntegrityDetectorTest extends BaseDetectorTest {
2930

3031
/**
31-
* Test to confirm the fix of https://github.com/h3xstream/find-sec-bugs/issues/24
32+
* General tests plus test to confirm the fix of
33+
* https://github.com/h3xstream/find-sec-bugs/issues/24
34+
*
3235
* @throws Exception
3336
*/
3437
@Test
3538
public void detectCustomDigest() throws Exception {
3639
//Locate test code
3740
String[] files = {
38-
getClassFilePath("testcode/crypto/CipherNoIntegrityBugFixRsa")
41+
getClassFilePath("testcode/crypto/CipherNoIntegrity"),
42+
getClassFilePath("testcode/crypto/CipherNoIntegrityBugFixRsa")
3943
};
4044

4145
//Run the analysis
4246
EasyBugReporter reporter = spy(new EasyBugReporter());
4347
analyze(files, reporter);
4448

45-
verify(reporter,never()).doReportBug(
49+
List<Integer> linesECB = Arrays.asList(9, 11);
50+
for (Integer line : linesECB) {
51+
verify(reporter).doReportBug(
52+
bugDefinition()
53+
.bugType("ECB_MODE")
54+
.inClass("CipherNoIntegrity").atLine(line)
55+
.build()
56+
);
57+
}
58+
verify(reporter, times(linesECB.size())).doReportBug(
59+
bugDefinition()
60+
.bugType("ECB_MODE")
61+
.build()
62+
);
63+
64+
List<Integer> linesNoIntegrity = Arrays.asList(9, 10, 11, 12);
65+
for (Integer line : linesNoIntegrity) {
66+
verify(reporter).doReportBug(
67+
bugDefinition()
68+
.bugType("CIPHER_INTEGRITY")
69+
.inClass("CipherNoIntegrity").atLine(line)
70+
.build()
71+
);
72+
}
73+
verify(reporter, times(linesNoIntegrity.size())).doReportBug(
74+
bugDefinition()
75+
.bugType("CIPHER_INTEGRITY")
76+
.build()
77+
);
78+
79+
verify(reporter).doReportBug(
80+
bugDefinition()
81+
.bugType("PADDING_ORACLE")
82+
.inClass("CipherNoIntegrity").atLine(12)
83+
.build()
84+
);
85+
verify(reporter, times(1)).doReportBug(
4686
bugDefinition()
47-
.bugType("ECB_MODE_TYPE")
48-
.build()
87+
.bugType("PADDING_ORACLE")
88+
.build()
4989
);
5090
}
5191
}

‎plugin/src/test/java/testcode/crypto/CipherNoIntegrity.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
package testcode.crypto;
2+
3+
import javax.crypto.Cipher;
4+
5+
public class CipherNoIntegrity {
6+
7+
public static void main(String[] args) throws Exception {
8+
Cipher.getInstance("AES/GCM/..."); // ok
9+
Cipher.getInstance("AES"); // ECB and no integrity
10+
Cipher.getInstance("DES/CTR/NoPadding"); // no integrity
11+
Cipher.getInstance("DESede/ECB/PKCS5Padding"); // ECB and no integrity
12+
Cipher.getInstance("AES/CBC/PKCS5Padding"); // oracle and no integrity
13+
Cipher.getInstance("RSA"); // ok
14+
Cipher.getInstance("RSA/ECB/PKCS1Padding"); // ok
15+
Cipher.getInstance(args[0]); // ok
16+
}
17+
}

0 commit comments

Comments
 (0)