dashpay / dash-wallet

Dash Wallet for Android
https://dash.org
181 stars 169 forks source link

Cryptographic APIs misuses #898

Open misterAnderson90 opened 2 years ago

misterAnderson90 commented 2 years ago

I'm a PhD student interested in finding security vulnerabilities in open source projects.

We found a total of 180 warnings (indicating potential vulnerabilities) when running the CogniCrypt static analyzer (*) on dash-wallet (or its library dependencies). We documented each one of these issues in private gists for the sake of confidentiality (non-disclosure).

Can you please let us know whether we can share these gists with you? We are eager to evaluate the perception of developers (e.g. severity of these warnings) and improve dash-wallet's security, and the quality of the reports of static analysis tools. (*) https://github.com/CROSSINGTUD/CryptoAnalysis

HashEngineering commented 2 years ago

This tool does look interesting. For me it gives stack overflow errors or out of memory errors.

misterAnderson90 commented 2 years ago

Hello @HashEngineering,

Depending on the program size it requires a lot of memory to complete the analysis. At Dash-wallet, I needed 30 GB of ram. Are you interested in receiving these warnings reports? If yes, how can I share them with you?

HashEngineering commented 2 years ago

I changed the RAM parameters and was able to run the report. It will take some time to go through the findings.

I suppose that the rules in the system are looking for insecure code.

One example is this:

Findings in Java Class: org.bitcoinj.script.Script

         in Method: void executeScript(org.bitcoinj.core.Transaction,long,org.bitcoinj.script.Script,java.util.LinkedList,java.util.Set)
                ConstraintError violating CrySL rule for java.security.MessageDigest (on Object #4679457ec2309e70f0138dea44d0aac0793412778b328d5792e0d2a285939f76)
                        First parameter (with value "SHA-1") should be any of {SHA-256, SHA-384, SHA-512, SHA-512/256, BLAKE2B-256, BLAKE2B-384, BLAKE2B-512, BLAKE2S-256, KECCAK-256, KECCAK-384, KECCAK-512, WHIRLPOOL}
                        at statement: $r18 = staticinvoke <java.security.MessageDigest: java.security.MessageDigest getInstance(java.lang.String)>(varReplacer58484)

Which if we get the source in dashj - we find this (originally is taken from the Bitcoin standard).

                case OP_SHA1:
                    if (stack.size() < 1)
                        throw new ScriptException(ScriptError.SCRIPT_ERR_INVALID_STACK_OPERATION, "Attempted OP_SHA1 on an empty stack");
                    try {
                        stack.add(MessageDigest.getInstance("SHA-1").digest(stack.pollLast()));
                    } catch (NoSuchAlgorithmException e) {
                        throw new RuntimeException(e);  // Cannot happen.
                    }
                    break;

The OP_SHA1 is an opcode for the Dash script language. Most likely it is no longer used. I suppose we could work on getting this opcode made obsolete or deprecated. But that is outside the scope of my work which is to implement the Dash standard in Java.

Other items are in dependencies and further investigation would be required to determine if an update of such dependencies would eliminate the warning.