KeePass is a free, open source, light-weight and easy-to-use password manager that allows users to manage their passwords across multiple accounts and applications in a secure way. It supports a password database that is locked with a single master key. KeePassDroid is a port of this password manager for Android. It is a popular password manager on Android Play Store with over 36,000 users giving a four star rating.

We ran iCR on KeePassDroid version 2.5.8. This is a slightly older version. The current version of KeePassDroid is 2.5.14. Version 2.5.8 has 249 Java files with a little over 30,000 lines of code.

iCR scan took about two minutes and it produced 175 fix suggestions. In this blog, we will describe some of the findings.

Checking Erroneous Return Value:

Not checking the return value of a method is a common problem covered by CWE 252. It states that two common programmer assumptions are that a method can never fail and it does not matter if the method call fails. If an attacker can force the function to fail or otherwise return a value that is not expected, then the subsequent program logic could lead to a vulnerability, because the software is not in a state that the programmer assumes.

iCR identified four places in the code where the developers did not choose to check the return values. All four cases involved checking the return value of the read method of an InputStream. This method returns -1 when the end of the stream is reached, but in these four cases, there is not check for erroneous return value.

A couple of such instances were in the LEDataInputStream.java file which contains LEDataInputStream class which extends InputStream. This class redefines a number of read methods. In one case, in the readUShort method, the code written is:

public static int readUShort(InputStream is) throws IOException {
                  byte[] buf = new byte[2];
                  is.read(buf, 0, 2);
                  return readUShort(buf, 0);
          }

In this case, is.read will populate the buffer bug, but in error cases, the buffer may be corrupted. There is no check introduced here. iCR introduces the following code stub that the developer has to complete to introduce the correct fix:

--- ...source/keepassdroid/app/src/main/java/com/keepassdroid/stream
/LEDataInputStream.java
+++ ...source/keepassdroid/app/src/main/java/com/keepassdroid/stream
/LEDataInputStream.java
@@ -130,7 +130,10 @@
        public static int readUShort(InputStream is) throws IOException {
                  byte[] buf = new byte[2];

-                 is.read(buf, 0, 2);
+                 int readResult = is.read(buf, 0, 2);
+               // OR Warning: Handle Error Code
+               if (readResult == -1) {
+               }

                  return readUShort(buf, 0);
          }

Interestingly such checks are not introduced in readUShort and readInt methods in this class. But in the same class, readBytes method contains a check on the error return value (-1). So, such error value checking should be introduced in the other classes as well.

Missing Call to Super Method:

Overriding methods should reference the method in the parent class. Sometimes this is not needed. However, if the same method is overridden many times in code and most of the cases call the super method, then the few methods that do not call the super method should be checked to determine the reason why they do not follow the others. If the omission was by an error, then there is a bug. If the omission was intentional, then perhaps there should be a comment justifying the reason to make the code better.

In KeePassDroid, there were fourteen classes that extend the OnFinish class defined in the code in the com.keepassdroid.database.edit package. Thirteen of them override and implement the run method. One does not implement the run method, but it has two subclasses both implementing the run method. So, there are fifteen implementations of the run method in total. Out of this, nine methods call the super.run() method in the code. Eight of them call this before exiting, one call it at the start of the run method implementation. That leaves six classes that do not call super.run(). iCR identifies this as a potential problem and flags each of the methods that do not call super.

Note, we have not gone through the business logic to identify if these methods actually require a call to the super.run() method.

 

Improper Control of Document Type Definition:

XML Document Type Definition (DTD) should be disabled to prevent information disclosure via XXE (XML eXternal Entity) injection attacks. This is covered by CWE 611 (Improper Restriction of XML External Entity Reference) and CWE 827 (Improper Control of Document Type Definition). Also OWASP A4-XML eXternal Entities cover this case.

In this case, the reference to a Document Type Definition (DTD) is not restricted to the intended control sphere. From CWE 827, “This might allow attackers to reference arbitrary DTDs, possibly causing the software to expose files, consume excessive system resources, or execute arbitrary http requests on behalf of the attacker.”

iCR found this in one place, in the loadXmlKeyFile method inside PwDatabaseV4.java file. It suggest the following fix:

--- ...source/keepassdroid/app/src/main/java/com/keepassdroid/database
/PwDatabaseV4.java
+++ ...source/keepassdroid/app/src/main/java/com/keepassdroid/database
/PwDatabaseV4.java
@@ -227,6 +227,8 @@
  protected byte[] loadXmlKeyFile(InputStream keyInputStream) {
     try {
         DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
+        dbf.setAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
+        dbf.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
         DocumentBuilder db = dbf.newDocumentBuilder();
         Document doc = db.parse(keyInputStream);

This will explicitly disable the DTD access.

 

Restrict Access to Broadcast Sender:

This is an issue specific for Android applications. While sending broadcast messages in Android, broadcast permission should be specified so that only receivers with proper permission can receive it. Without this, the application uses an implicit intent. CWE 927 covers this issue and explains it this way: “Since an implicit intent does not specify a particular application to receive the data, any application can process the intent by using an Intent Filter for that intent. This can allow untrusted applications to obtain sensitive data.”

iCR identified two places in KeePassDroid application that does not use implicit intent properly. One is in the onCreate() method implementation in the EntryActivity class (inside EntryActivity.java) where an intent filter is created and registered to a receiver without specifying any broadcast permission. The other case is in the in the onCreate() method implementation in the App class (inside App.java). In both of these cases, iCR generated a warning so that developers can review the case.