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

CipherWithNoIntegrityDetector throws exception on algorithm-only cipher lookups #24

Closed
ThrawnCA opened this issue Dec 22, 2014 · 7 comments
Assignees
Labels
Milestone

Comments

@ThrawnCA
Copy link

javax.crypto.Cipher.getInstance permits lookups using only the algorithm name; however, CipherWithNoIntegrityDetector expects a slash to be present in the argument, leading to an ArrayIndexOutOfBoundsException at line 94 when code like the following is used:

Cipher cipher = Cipher.getInstance("RSA");

 [java]   Exception analyzing TokenEncryptor using detector com.h3xstream.findsecbugs.crypto.CipherWithNoIntegrityDetector
 [java]     java.lang.ArrayIndexOutOfBoundsException: 1
 [java]       At com.h3xstream.findsecbugs.crypto.CipherWithNoIntegrityDetector.sawOpcode(CipherWithNoIntegrityDetector.java:94)
 [java]       At edu.umd.cs.findbugs.visitclass.DismantleBytecode.visit(DismantleBytecode.java:883)
 [java]       At edu.umd.cs.findbugs.visitclass.BetterVisitor.visitCode(BetterVisitor.java:218)
 [java]       At edu.umd.cs.findbugs.visitclass.PreorderVisitor.visitCode(PreorderVisitor.java:229)
 [java]       At edu.umd.cs.findbugs.bcel.OpcodeStackDetector.visitCode(OpcodeStackDetector.java:63)
 [java]       At org.apache.bcel.classfile.Code.accept(Code.java:135)
 [java]       At edu.umd.cs.findbugs.visitclass.PreorderVisitor.doVisitMethod(PreorderVisitor.java:301)
 [java]       At edu.umd.cs.findbugs.visitclass.PreorderVisitor.visitJavaClass(PreorderVisitor.java:389)
 [java]       At org.apache.bcel.classfile.JavaClass.accept(JavaClass.java:215)
 [java]       At edu.umd.cs.findbugs.BytecodeScanningDetector.visitClassContext(BytecodeScanningDetector.java:38)
 [java]       At edu.umd.cs.findbugs.DetectorToDetector2Adapter.visitClass(DetectorToDetector2Adapter.java:76)
 [java]       At edu.umd.cs.findbugs.FindBugs2.analyzeApplication(FindBugs2.java:1089)
 [java]       At edu.umd.cs.findbugs.FindBugs2.execute(FindBugs2.java:283)
 [java]       At edu.umd.cs.findbugs.FindBugs.runMain(FindBugs.java:402)
 [java]       At edu.umd.cs.findbugs.FindBugs2.main(FindBugs2.java:1200)
@h3xstream h3xstream self-assigned this Dec 22, 2014
@h3xstream h3xstream added the bug label Dec 22, 2014
h3xstream added a commit that referenced this issue Dec 22, 2014
…uate by CipherWithNoIntegrityDetector. #24

Tested with EcbModeDetectorTest
@h3xstream
Copy link
Member

It should be fix. You can test it with the latest version or wait a week or two for the new release.

@ThrawnCA
Copy link
Author

ThrawnCA commented Jan 9, 2015

New version fixes it, thanks.

According to the comments, though, RSA/ECB/* ciphers are OK, and yet this detector rejects them?

@h3xstream
Copy link
Member

This means there is a bug and a test case is missing.
I'll look at it.

@h3xstream h3xstream reopened this Jan 9, 2015
@ThrawnCA
Copy link
Author

Our affected code is a Guice module that looks like this:

public class CryptographyModule implements Module {

    private static final String JCE_PROVIDER_BOUNCY_CASTLE = "BC";

    @Retention(RetentionPolicy.RUNTIME)
    @BindingAnnotation
    public @interface Rsa {
        String CIPHER = "RSA/NONE/OAEPWithSHA256AndMGF1Padding";
    }

    public void configure(Binder binder) {
        checkBouncyCastleIsInstalled(binder);

        binder.bind(EncryptionStrategy.class).annotatedWith(Rsa.class).to(RsaEncryptionStrategy.class)
                .in(Scopes.SINGLETON);
    }

    public void checkBouncyCastleIsInstalled(Binder binder) {
        try {
            Cipher.getInstance(Rsa.CIPHER, JCE_PROVIDER_BOUNCY_CASTLE);
        } catch (GeneralSecurityException e) {
            binder.addError("cannot find the required cryptography extensions, is bouncy castle installed?", e);
        }
    }
}

Changing the mode to ECB didn't help (and ECB isn't really a recommended mode anyway; what does the 'NONE' mode do?).

@h3xstream h3xstream added this to the version-1.3.1 milestone Jan 17, 2015
@ThrawnCA
Copy link
Author

I would be happy to help with this, but I'm not entirely clear on which RSA ciphers are in fact safe.

I notice, however, that the 'no padding' bug description suggests that 'RSA/ECB/OAEPWithMD5AndMGF1Padding' is a good choice.

h3xstream added a commit that referenced this issue Jan 23, 2015
@h3xstream
Copy link
Member

The detector that trigger ECB mode usage target Symetric cipher only. The RSA/ECB/* is a false positive for this detector.

The only weak RSA cipher that I am aware is the RSA/None/NoPadding which is cover by another detector.

The fix
The missing test case have proven once more that it is easy to do placebo fix.
I have create a test case with a sample identical to your code.

@ThrawnCA
Copy link
Author

ThrawnCA commented Aug 5, 2015

We're still seeing this bug detector fire on our code, along with the EcbModeDetector.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants