SpotBugs (4.8.3) Analysis for app (spotbugsGplayDebug)

SpotBugs Analysis generated at: Wed, 20 Mar 2024 10:43:42 +0000

Package Code Size Bugs High Prio Bugs Medium Prio Bugs Low Prio Bugs Exp. Bugs Ratio
Overall (87 packages), (1700 classes) 61589 597 42 555
com.nextcloud.android.sso 369 12 3 9
com.nextcloud.android.sso.aidl 271 3 3
com.nextcloud.client.account 226 7 7
com.nextcloud.client.database.migrations 484 1 1
com.owncloud.android 616 4 4
com.owncloud.android.authentication 1044 9 9
com.owncloud.android.datamodel 4095 45 1 44
com.owncloud.android.datastorage 135 1 1
com.owncloud.android.datastorage.providers 147 3 3
com.owncloud.android.db 569 5 5
com.owncloud.android.files 561 5 5
com.owncloud.android.files.services 150 1 1
com.owncloud.android.operations 2831 71 19 52
com.owncloud.android.providers 1181 11 11
com.owncloud.android.services 492 13 1 12
com.owncloud.android.syncadapter 321 4 4
com.owncloud.android.ui 417 8 8
com.owncloud.android.ui.activities 319 1 1
com.owncloud.android.ui.activities.data.activities 103 2 1 1
com.owncloud.android.ui.activity 7469 79 2 77
com.owncloud.android.ui.adapter 3595 48 48
com.owncloud.android.ui.asynctasks 658 35 1 34
com.owncloud.android.ui.dialog 1312 26 1 25
com.owncloud.android.ui.dialog.parcel 198 1 1
com.owncloud.android.ui.fragment 3989 72 3 69
com.owncloud.android.ui.fragment.contactsbackup 386 10 10
com.owncloud.android.ui.fragment.util 88 4 4
com.owncloud.android.ui.helpers 648 14 14
com.owncloud.android.ui.preview 2202 51 51
com.owncloud.android.utils 3129 47 8 39
com.owncloud.android.utils.theme 159 4 4
PSC / PSC_PRESIZE_COLLECTIONS

This method allocates a collection using the default constructor even though it is known a priori (or at least can be reasonably guessed) how many items are going to be placed in the collection, and thus needlessly causes intermediate reallocations of the collection.

You can use the constructor that takes an initial size and that will be much better, but due to the loadFactor of Maps and Sets, even this will not be a correct estimate.

If you are using Guava, use its methods that allocate maps and sets with a predetermined size, to get the best chance for no reallocations, such as:

If not, a good estimate would be the expectedSize / {LOADING_FACTOR} which by default is 0.75

PMB / PMB_POSSIBLE_MEMORY_BLOAT

This class defines static fields that are Collections, StringBuffers, or StringBuilders that do not appear to have any way to clear or reduce their size. That is, a collection is defined and has method calls like
{add(), append(), offer(), put(), ...}
with no method calls to removal methods like
{clear(), delete(), pop(), remove(), ...}
This means that the collection in question can only ever increase in size, which is a potential cause of memory bloat.

If this collection is a list, set or otherwise of static things (e.g. a List<String> for month names), consider adding all of the elements in a static initializer, which can only be called once:


private static List<String> monthNames = new ArrayList<String>();
static {
    monthNames.add("January");
    monthNames.add("February");
    monthNames.add("March");
    ...
}

MUI / MUI_CALLING_SIZE_ON_SUBCONTAINER

This method calls size on the keySet(), entrySet() or values() collections of a Map. These sub collections will have the same size as the base Map and so it is just simpler to call size on that Map. Calling size() on one of these sub collections will causes unnecessary allocations to occur.

SIC / SIC_INNER_SHOULD_BE_STATIC

This class is an inner class, but does not use its embedded reference to the object which created it.  This reference makes the instances of the class larger, and may keep the reference to the creator object alive longer than necessary.  If possible, the class should be made static.

CLI / CLI_CONSTANT_LIST_INDEX

This method accesses an array or list using a constant integer index. Often, this is a typo where a loop variable is intended to be used. If however, specific list indices mean different specific things, then perhaps replacing the list with a first-class object with meaningful accessors would make the code less brittle.

OC / OC_OVERZEALOUS_CASTING

This method casts the right hand side of an expression to a class that is more specific than the variable on the left hand side of the assignment. The cast only has to be as specific as the variable that is on the left. Using a more specific type on the right hand side just increases cohesion.

WOC / WOC_WRITE_ONLY_COLLECTION_FIELD

This class creates and initializes a collection as a field but then never accesses this collection to gain information or fetch items from the collection. It is likely that this collection is left over from a past effort, and can be removed.

SECHPP / HTTP_PARAMETER_POLLUTION

Concatenating unvalidated user input into a URL can allow an attacker to override the value of a request parameter. Attacker may be able to override existing parameter values, inject a new parameter or exploit variables out of a direct reach. HTTP Parameter Pollution (HPP) attacks consist of injecting encoded query string delimiters into other existing parameters. If a web application does not properly sanitize the user input, a malicious user may compromise the logic of the application to perform either client-side or server-side attacks.
In the following example the programmer has not considered the possibility that an attacker could provide a parameter lang such as en&user_id=1, which would enable him to change the user_id at will.

Vulnerable Code:

String input = request.getParameter("lang");
GetMethod get = new GetMethod("http://www.host.com/viewDetails");
get.setQueryString("lang=" + input + "&user_id=" + userId);
get.execute();

Solution:
You can either encode user input before placing it in HTTP parameters or use the UriBuilder class from Apache HttpClient.

URIBuilder uriBuilder = new URIBuilder("http://www.host.com/viewDetails");
uriBuilder.addParameter("lang", input);
uriBuilder.addParameter("user_id", userId);

HttpGet httpget = new HttpGet(uriBuilder.build().toString()); //OK


References
CAPEC-460: HTTP Parameter Pollution (HPP)

NP / NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE

The return value from a method is dereferenced without a null check, and the return value of that method is one that should generally be checked for null. This may lead to a NullPointerException when the code is executed.

LII / LII_LIST_INDEXED_ITERATING

This method uses an integer-based for loop to iterate over a java.util.List, by calling List.get(i) each time through the loop. The integer is not used for other reasons. It is better to use an Iterator instead, as depending on List implementation, iterators can perform better, and they also allow for exchanging of other collection types without issue.

ENMI / ENMI_NULL_ENUM_VALUE

This method sets the value of an enum reference to null. An enum should never have a null value. If there is a state where you do not know what the value of an enum should be, than that should be one of the proper enum value. So add a MyEnum.UNKNOWN or such. This keeps the logic of switch statements, etc, much simpler.

REC / REC_CATCH_EXCEPTION

This method uses a try-catch block that catches Exception objects, but Exception is not thrown within the try block, and RuntimeException is not explicitly caught. It is a common bug pattern to say try { ... } catch (Exception e) { something } as a shorthand for catching a number of types of exception each of whose catch blocks is identical, but this construct also accidentally catches RuntimeException as well, masking potential bugs.

A better approach is to either explicitly catch the specific exceptions that are thrown, or to explicitly catch RuntimeException exception, rethrow it, and then catch all non-Runtime Exceptions, as shown below:

try {
    ...
} catch (RuntimeException e) {
    throw e;
} catch (Exception e) {
    ... deal with all non-runtime exceptions ...
}
MS / MS_EXPOSE_REP

A public static method returns a reference to a mutable object or an array that is part of the static state of the class. Any code that calls this method can freely modify the underlying array. One fix is to return a copy of the array.

SCII / SCII_SPOILED_CHILD_INTERFACE_IMPLEMENTOR

This class declares that it implements an interface, but does so by relying on methods supplied by superclasses, even though those superclasses know nothing about the interface in question. If you wish to have the child not implement all the methods of the interface, it would probably be better to declare the superclass as implementing the interface, and if that class does not provide all the methods, then declare that superclass abstract.

NAB / NAB_NEEDLESS_BOOLEAN_CONSTANT_CONVERSION

This method assigns a Boxed boolean constant to a primitive boolean variable, or assigns a primitive boolean constant to a Boxed boolean variable. Use the correct constant for the variable desired. Use


boolean b = true;
boolean b = false;
or

Boolean b = Boolean.TRUE;
Boolean b = Boolean.FALSE;

Be aware that this boxing happens automatically when you might not expect it. For example,


Map statusMap = ...

public Boolean someMethod() {
    statusMap.put("foo", true);  //the "true" here is boxed
    return false;  //the "false" here is boxed
}
has two cases of this needless autoboxing. This can be made more efficient by simply substituting in the constant values:

Map statusMap = ...

public Boolean someMethod() {
    statusMap.put("foo", Boolean.TRUE);
    return Boolean.FALSE;
}

RCN / RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE

A value is checked here to see whether it is null, but this value cannot be null because it was previously dereferenced and if it were null a null pointer exception would have occurred at the earlier dereference. Essentially, this code and the previous dereference disagree as to whether this value is allowed to be null. Either the check is redundant or the previous dereference is erroneous.

PL / PL_PARALLEL_LISTS

This class appears to maintain two or more lists or arrays whose contents are related in a parallel way. That is, you have something like:


List<String> words = new ArrayList<String>();
List<Integer> wordCounts = new ArrayList<String>();
where the elements of the list at index 0 are related, the elements at index 1 are related and so on.

Consider creating a separate class to hold all the related pieces of information, and adding instances of this class to just one list or array, or if just two values, use a Map to associate one value with the other like:


private class WordAndCount{public String word;  public int count}

List<WordAndCount> wordsAndCounts = new ArrayList<WordAndCount>();
//or, for just two elements
Map wordCounts = new HashMap();

OCP / OCP_OVERLY_CONCRETE_PARAMETER

This method uses concrete classes for parameters when only methods defined in an implemented interface or superclass are used. Consider increasing the abstraction of the interface to make low impact changes easier to accomplish in the future.

Take the following example:


private void appendToList(ArrayList<String> list) {
    if (list.size() < 100) {
        list.add("Foo");
    }
}
The parameter list is currently defined as an ArrayList, which is a concrete implementation of the List interface. Specifying ArrayList is unnecessary here, because we aren't using any ArrayList-specific methods (like ensureCapacity() or trimToSize()). Instead of using the concrete definition, it is better to do something like:

private void appendToList(List<String> list) {
    ...
If the design ever changes, e.g. a LinkedList is used instead, this code won't have to change.

IDEs tend to have tools to help generalize parameters. For example, in Eclipse, the refactoring tool Generalize Declared Type helps find an appropriate level of concreteness.

MOM / MOM_MISLEADING_OVERLOAD_MODEL

This class 'overloads' the same method with both instance and static versions. As the use of these two models is different, it will be confusing to the users of these methods.

CFS / CFS_CONFUSING_FUNCTION_SEMANTICS

This method appears to modify a parameter, and then return this parameter as the method's return value. This will be confusing to callers of this method, as it won't be apparent that the 'original' passed-in parameter will be changed as well. If the purpose of this method is to change the parameter, it would be more clear to change the method to have a void return value. If a return type is required due to interface or superclass contract, perhaps a clone of the parameter should be made.

SACM / SACM_STATIC_ARRAY_CREATED_IN_METHOD

This method creates an array initialized by constants. Each time this method is called this array will be recreated. It would be more performant to define the array as a static field of the class instead.

AFBR / AFBR_ABNORMAL_FINALLY_BLOCK_RETURN

This method returns or throws exceptions from a finally block. This will mask real program logic in the try block, and short-circuit normal method termination.

Se / SE_COMPARATOR_SHOULD_BE_SERIALIZABLE

This class implements the Comparator interface. You should consider whether or not it should also implement the Serializable interface. If a comparator is used to construct an ordered collection such as a TreeMap, then the TreeMap will be serializable only if the comparator is also serializable. As most comparators have little or no state, making them serializable is generally easy and good defensive programming.

SF / SF_SWITCH_NO_DEFAULT

This method contains a switch statement where default case is missing. Usually you need to provide a default case.

Because the analysis only looks at the generated bytecode, this warning can be incorrect triggered if the default case is at the end of the switch statement and the switch statement doesn't contain break statements for other cases.

UCF / UCF_USELESS_CONTROL_FLOW

This method contains a useless control flow statement, where control flow continues onto the same place regardless of whether or not the branch is taken. For example, this is caused by having an empty statement block for an if statement:

if (argv.length == 0) {
    // TODO: handle this case
}
SEO / SEO_SUBOPTIMAL_EXPRESSION_ORDER

This method builds a conditional expression, for example, in an if or while statement, where the expressions contain both simple local variable comparisons and comparisons on method calls. The expression orders these so that the method calls come before the simple local variable comparisons. This causes method calls to be executed in conditions when they do not need to be, and thus potentially causes a lot of code to be executed for nothing. By ordering the expressions so that the simple conditions containing local variable conditions are first, you eliminate this waste. This assumes that the method calls do not have side effects. If the methods do have side effects, it is probably a better idea to pull these calls out of the condition and execute them first, assigning a value to a local variable. In this way you give a hint that the call may have side effects.

Example:


if ((calculateHaltingProbability() > 0) && shouldCalcHalting) { }
would be better as

if (shouldCalcHalting && (calculateHaltingProbability() > 0) { }

DRE / DRE_DECLARED_RUNTIME_EXCEPTION

This method declares a RuntimeException derived class in its throws clause. This may indicate a misunderstanding as to how unchecked exceptions are handled. If it is felt that a RuntimeException is so prevalent that it should be declared for documentation purposes, using javadoc's @throws clause is a better idea, as it doesn't needless pollute the method signature,

BED / BED_HIERARCHICAL_EXCEPTION_DECLARATION

This method declares that it throws an exception that is the child of another exception that is also declared to be thrown. Given that the parent exception is declared, there is no need for the child exception to also be declared; it just adds confusion.

MS / EI_EXPOSE_STATIC_REP2

This code stores a reference to an externally mutable object into a static field. If unchecked changes to the mutable object would compromise security or other important properties, you will need to do something different. Storing a copy of the object is better approach in many situations.

SECBROAD / ANDROID_BROADCAST

Broadcast intents can be listened by any application with the appropriate permission. It is suggested to avoid transmitting sensitive information when possible.

Code at risk:

Intent i = new Intent();
i.setAction("com.insecure.action.UserConnected");
i.putExtra("username", user);
i.putExtra("email", email);
i.putExtra("session", newSessionId);

this.sendBroadcast(v1);


Solution (if possible):

Intent i = new Intent();
i.setAction("com.secure.action.UserConnected");

sendBroadcast(v1);


Configuration (receiver)[1] Source: StackOverflow:

<manifest ...>

    <!-- Permission declaration -->
    <permission android:name="my.app.PERMISSION" />

    <receiver
        android:name="my.app.BroadcastReceiver"
        android:permission="my.app.PERMISSION"> <!-- Permission enforcement -->
        <intent-filter>
            <action android:name="com.secure.action.UserConnected" />
        </intent-filter>
    </receiver>

    ...
</manifest>

Configuration (sender)[1] Source: StackOverflow:

<manifest>
    <!-- We declare we own the permission to send broadcast to the above receiver -->
    <uses-permission android:name="my.app.PERMISSION"/>

    <!-- With the following configuration, both the sender and the receiver apps need to be signed by the same developer certificate. -->
    <permission android:name="my.app.PERMISSION" android:protectionLevel="signature"/>
</manifest>


References
CERT: DRD03-J. Do not broadcast sensitive information using an implicit intent
Android Official Doc: BroadcastReceiver (Security)
Android Official Doc: Receiver configuration (see android:permission)
[1] StackOverflow: How to set permissions in broadcast sender and receiver in android
CWE-925: Improper Verification of Intent by Broadcast Receiver
CWE-927: Use of Implicit Intent for Sensitive Communication

UTWR / UTWR_USE_TRY_WITH_RESOURCES
[

This method allocates and uses an auto closeable resource. However, it manually closes the resource in a finally block. While this is correct management, it doesn't rely on the idiomatic way available to JDK 7 and above, allows for possible subtle problems, and complicates the reading of code by developers expecting the use of try-with-resources.

Switch to using try with resources, as:

    		    try (InputStream is = getAStream()) {
    		        useTheStream(is);
    		    }
    		
SBSC / SBSC_USE_STRINGBUFFER_CONCATENATION

The method seems to be building a String using concatenation in a loop. In each iteration, the String is converted to a StringBuffer/StringBuilder, appended to, and converted back to a String. This can lead to a cost quadratic in the number of iterations, as the growing string is recopied in each iteration.

Better performance can be obtained by using a StringBuffer (or StringBuilder in Java 5) explicitly.

For example:

// This is bad
String s = "";
for (int i = 0; i < field.length; ++i) {
    s = s + field[i];
}

// This is better
StringBuffer buf = new StringBuffer();
for (int i = 0; i < field.length; ++i) {
    buf.append(field[i]);
}
String s = buf.toString();
PCOA / PCOA_PARTIALLY_CONSTRUCTED_OBJECT_ACCESS

This constructor makes a call to a non-final method. Since this method can be overridden, a subclass' implementation will be executing against an object that has not been initialized at the subclass level. You should mark all methods called from the constructor as final to avoid this problem.

SEC / SEC_SIDE_EFFECT_CONSTRUCTOR

This method creates an object but does not assign this object to any variable or field. This implies that the class operates through side effects in the constructor, which is a bad pattern to use, as it adds unnecessary coupling. Consider pulling the side effect out of the constructor, into a separate method, or into the calling method.

SECUHE / UNSAFE_HASH_EQUALS

An attacker might be able to detect the value of the secret hash due to the exposure of comparison timing. When the functions Arrays.equals() or String.equals() are called, they will exit earlier if fewer bytes are matched.

Vulnerable Code:

String actualHash = ...

if(userInput.equals(actualHash)) {
    ...
}

Solution:

String actualHash = ...

if(MessageDigest.isEqual(userInput.getBytes(),actualHash.getBytes())) {
    ...
}


References
CWE-203: Information Exposure Through DiscrepancyKey

SECOBDES / OBJECT_DESERIALIZATION

Object deserialization of untrusted data can lead to remote code execution, if there is a class in classpath that allows the trigger of malicious operation.

Libraries developers tend to fix class that provided potential malicious trigger. There are still classes that are known to trigger Denial of Service[1].

Deserialization is a sensible operation that has a great history of vulnerabilities. The web application might become vulnerable as soon as a new vulnerability is found in the Java Virtual Machine[2] [3].

Code at risk:

public UserData deserializeObject(InputStream receivedFile) throws IOException, ClassNotFoundException {

    try (ObjectInputStream in = new ObjectInputStream(receivedFile)) {
        return (UserData) in.readObject();
    }
}

Solutions:

Avoid deserializing object provided by remote users.


References
CWE-502: Deserialization of Untrusted Data
Deserialization of untrusted data
Serialization and Deserialization
A tool for generating payloads that exploit unsafe Java object deserialization
[1] Example of Denial of Service using the class java.util.HashSet
[2] OpenJDK: Deserialization issue in ObjectInputStream.readSerialData() (CVE-2015-2590)
[3] Rapid7: Sun Java Calendar Deserialization Privilege Escalation (CVE-2008-5353)

STT / STT_STRING_PARSING_A_FIELD

This method calls a parsing method (indexOf, lastIndexOf, startsWith, endsWith, substring, indexOf) on a String that is a field, or comes from a collection that is a field. This implies that the String in question is holding multiple parts of information inside the string, which would be more maintainable and type safe if that value was a true collection or a first class object with fields, rather than a String.

NP / NP_PARAMETER_MUST_BE_NONNULL_BUT_MARKED_AS_NULLABLE

This parameter is always used in a way that requires it to be non-null, but the parameter is explicitly annotated as being Nullable. Either the use of the parameter or the annotation is wrong.

BL / BL_BURYING_LOGIC

Looks for relatively large if blocks of code, where you unconditionally return from them, and then follow that with an unconditional return of a small block. This places the bulk of the logic to the right indentation-wise, making it more difficult to read than needed. It would be better to invert the logic of the if block, and immediately return, allowing the bulk of the logic to be move to the left for easier reading.

UCPM / UCPM_USE_CHARACTER_PARAMETERIZED_METHOD

This method passes a constant literal String of length 1 as a parameter to a method, when a similar method is exposed that takes a char. It is simpler and more expedient to handle one character, rather than a String.

Instead of making calls like:


String myString = ...
if (myString.indexOf("e") != -1) {
    int i = myString.lastIndexOf("e");
    System.out.println(myString + ":" + i);  //the Java compiler will use a StringBuilder internally here [builder.append(":")]
    ...
    return myString.replace("m","z");
}
Replace the single letter Strings with their char equivalents like so:

String myString = ...
if (myString.indexOf('e') != -1) {
    int i = myString.lastIndexOf('e');
    System.out.println(myString + ':' + i);  //the Java compiler will use a StringBuilder internally here [builder.append(':')]
    ...
    return myString.replace('m','z');
}

SS / SS_SHOULD_BE_STATIC

This class contains an instance final field that is initialized to a compile-time static value. Consider making the field static.

LSC / LSC_LITERAL_STRING_COMPARISON

This line is in the form of

String str = ...
str.equals("someOtherString");
//or
str.compareTo("someOtherString");

A NullPointerException may occur if the String variable str is null. If instead the code was restructured to

String str = ...
"someOtherString".equals(str);
//or
"someOtherString".compareTo(str);

that is, call equals() or compareTo() on the string literal, passing the variable as an argument, then this exception could never happen as both equals() and compareTo() check for null.

UAC / UAC_UNNECESSARY_API_CONVERSION_FILE_TO_PATH

This method creates a java.nio.file.Path object by first creating a java.io.File object, and then calling toPath() on it. It is simpler to just construct the Path object directly, say by using {@code Paths.get(String...)}.

EXS / EXS_EXCEPTION_SOFTENING_RETURN_FALSE

This method catches an exception and returns a boolean that represents whether an exception occurred or not. This throws away the value of exception handling and lets code ignore the resultant 'error code' return value. You should just throw the exception to the caller instead.

MUI / MUI_CONTAINSKEY_BEFORE_GET

This method checks for the presence of a key in a map using containsKey(), before attempting to fetch the value of the key using get(). This equates to doing two map lookups in a row. It is much simpler to just fetch the value with get, and checking for non null instead.

As an example, instead of using

    	    	Map myMap = getSomeMap();
    	    	if (myMap.containsKey("foo")) {
    	    		String value = myMap.get("foo");
    	    		....
    	    	}
    	    
convert this to
     	    	Map myMap = getSomeMap();
     	    	String value = myMap.get("foo");
    	    	if (value != null) {
    	    		....
    	    	}
    	    

The only caveat to this is that if you use a null value in a map to represent a third state for the key, then in this case using containsKey is 'correct'. This means an entry found in the map with a null value is taken differently than no entry at all. However, this is a very subtle programming paradigm, and likely to cause problems. If you wish to mark an entry as not being present, it is better to use a named 'sentinel' value to denote this, so instead of:

    	    	Map myMap = getSomeMap();
    	    	if (myMap.containsKey("foo")) {
    	    		String value = myMap.get("foo");
    	    		....
    	    	}
    	    
convert this to
     	    	Map myMap = getSomeMap();
     	    	String value = myMap.get("foo");
    	    	if (NOT_FOUND.equals(value)) {
    	    		....
    	    	}
    	    	where NOT_FOUND is some constant that denotes this special status. Of course you will need to find a special
    	    	sentinel value for each type you are using that isn't possible to have normally.
    	    

STT / STT_TOSTRING_STORED_IN_FIELD

This method calls the toString() method on an object and stores the value in a field. Doing this throws away the type safety of having the object defined by a Class. Using String makes it very easy to use the wrong type of value, and the compiler will not catch these mistakes. You should delay converting values to Strings for as long as possible, and thus not store them as fields.

NAB / NAB_NEEDLESS_BOXING_VALUEOF

This method passes a String to a wrapped primitive object's parse method, which in turn calls the valueOf method to convert to a boxed primitive. When it is desired to convert from a String to a boxed primitive object, it is simpler to use the BoxedPrimitive.valueOf(String) method.

Instead of something like:


Boolean bo = Boolean.valueOf(Boolean.parseBoolean("true"));
Float f = Float.valueOf(Float.parseFloat("1.234"));
Simply do:

Boolean bo = Boolean.valueOf("true");
Float f = Float.valueOf("1.234");

WMI / WMI_WRONG_MAP_ITERATOR

This method accesses the value of a Map entry, using a key that was retrieved from a keySet iterator. It is more efficient to use an iterator on the entrySet of the map, to avoid the Map.get(key) lookup.

RV / RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT

This code calls a method and ignores the return value. However, our analysis shows that the method (including its implementations in subclasses if any) does not produce any effect other than return value. Thus, this call can be removed.

We are trying to reduce the false positives as much as possible, but in some cases this warning might be wrong. Common false-positive cases include:

- The method is designed to be overridden and produce a side effect in other projects which are out of the scope of the analysis.

- The method is called to trigger the class loading which may have a side effect.

- The method is called just to get some exception.

If you feel that our assumption is incorrect, you can use a @CheckReturnValue annotation to instruct SpotBugs that ignoring the return value of this method is acceptable.

NP / NP_GUARANTEED_DEREF

There is a statement or branch that if executed guarantees that a value is null at this point, and that value that is guaranteed to be dereferenced (except on forward paths involving runtime exceptions).

Note that a check such as if (x == null) throw new NullPointerException(); is treated as a dereference of x.

SECXML / POTENTIAL_XML_INJECTION

Unsafe strings (e.g. from a user input) may contain XML tags. If such a string is inserted into an XML document then it may change its structure to a valid but semanticallly different document. To prevent this unsafe strings must b sanitized first.

Example:
An XML document may look like this:

⟨product⟩
    ⟨name⟩Cell Phone⟨/name⟩
    ⟨price⟩800⟨/price⟩
    ⟨amount⟩1⟨/amount⟩
⟨/product⟩

An attacker may put the following string to the field "amount":
1⟨/amount⟩⟨price⟩1⟨/price⟩⟨amount⟩1

If the XML parser works in a way that the second price overwrites the first one then the attacker may buy the cell phone for 1 singe dollar.

References
IDS16-J. Prevent XML Injection

FCBL / FCBL_FIELD_COULD_BE_LOCAL

This class defines fields that are used in a local only fashion, specifically private fields or protected fields in final classes that are accessed first in each method with a store vs. a load. This field could be replaced by one or more local variables.

RFI / RFI_SET_ACCESSIBLE

This method uses the reflective setAccessible method to alter the behavior of methods and fields in classes in ways that were not expected to be accessed by the author. Doing so circumvents the protections that the author provided through the class definition, and may expose your application unexpected side effects and problems. This functionality is deprecated in Java 9, and in Java 10 it is expected that this functionality won't work at all.

EXS / EXS_EXCEPTION_SOFTENING_NO_CONSTRAINTS

This method is not constrained by an interface or superclass, but converts a caught checked exception to an unchecked exception and throws it. It would be more appropriate just to throw the checked exception, adding the exception to the throws clause of the method.

RCN / RCN_REDUNDANT_NULLCHECK_OF_NULL_VALUE

This method contains a redundant check of a known null value against the constant null.

SECPTO / PATH_TRAVERSAL_OUT

A file is opened to write to its contents. The filename comes from an input parameter. If an unfiltered parameter is passed to this file API, files at an arbitrary filesystem location could be modified.

This rule identifies potential path traversal vulnerabilities. In many cases, the constructed file path cannot be controlled by the user. If that is the case, the reported instance is a false positive.


References
WASC-33: Path Traversal
OWASP: Path Traversal
CAPEC-126: Path Traversal
CWE-22: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')

MDM / MDM_STRING_BYTES_ENCODING

The behavior of the String(byte[] bytes) and String.getBytes() is undefined if the string cannot be encoded in the platform's default charset. Instead, use the String(byte[] bytes, String encoding) or String.getBytes(String encoding) constructor which accepts the string's encoding as an argument. Be sure to specify the encoding used for the user's locale.

As per the Java specifications, "UTF-8", "US-ASCII", "UTF-16" and "ISO-8859-1" will all be valid encoding charsets. If you aren't sure, try "UTF-8".

New in Java 1.7, you can specify an encoding from StandardCharsets, like StandardCharsets.UTF_8. These are generally preferrable because you don't have to deal with UnsupportedEncodingException.

SPP / SPP_PASSING_THIS_AS_PARM

This method calls an instance method passing the object that the method is called on as a parameter, such as foo.someMethod(foo); As you already have access to this object thru this, you don't need to pass it.

SPP / SPP_NULL_BEFORE_INSTANCEOF

This method checks a reference for null just before seeing if the reference is an instanceof some class. Since instanceof will return false for null references, the null check is not needed.

SUA / SUA_SUSPICIOUS_UNINITIALIZED_ARRAY

This method returns an array that was allocated but apparently not initialized. It is possible that the caller of this method will do the work of initializing this array, but that is not a common pattern, and it is assumed that this array has just been forgotten to be initialized.

Eq / EQ_COMPARETO_USE_OBJECT_EQUALS

This class defines a compareTo(...) method but inherits its equals() method from java.lang.Object. Generally, the value of compareTo should return zero if and only if equals returns true. If this is violated, weird and unpredictable failures will occur in classes such as PriorityQueue. In Java 5 the PriorityQueue.remove method uses the compareTo method, while in Java 6 it uses the equals method.

From the JavaDoc for the compareTo method in the Comparable interface:

It is strongly recommended, but not strictly required that (x.compareTo(y)==0) == (x.equals(y)). Generally speaking, any class that implements the Comparable interface and violates this condition should clearly indicate this fact. The recommended language is "Note: this class has a natural ordering that is inconsistent with equals."

CE / CE_CLASS_ENVY

This method makes extensive use of methods from another class over methods of its own class. Typically this means that the functionality that is accomplished by this method most likely belongs with the class that is being used so liberally. Consider refactoring this method to be contained in that class, and to accept all the parameters needed in the method signature.

DMI / DMI_RANDOM_USED_ONLY_ONCE

This code creates a java.util.Random object, uses it to generate one random number, and then discards the Random object. This produces mediocre quality random numbers and is inefficient. If possible, rewrite the code so that the Random object is created once and saved, and each time a new random number is required invoke a method on the existing Random object to obtain it.

If it is important that the generated Random numbers not be guessable, you must not create a new Random for each random number; the values are too easily guessable. You should strongly consider using a java.security.SecureRandom instead (and avoid allocating a new SecureRandom for each random number needed).

CC / CC_CYCLOMATIC_COMPLEXITY

This method has a high cyclomatic complexity figure, which represents the number of branch points. It is likely difficult to test, and is brittle to change. Consider refactoring this method into several to reduce the risk.

ITU / ITU_INAPPROPRIATE_TOSTRING_USE

This method calls algorithmic operations on a String that was returned from a toString() method. As these methods are for debugging/logging purposes, it shouldn't be the basis of core logic in your code.

IOI / IOI_USE_OF_FILE_STREAM_CONSTRUCTORS

This method creates and uses a java.io.FileInputStream or java.io.FileOutputStream object. Unfortunately both of these classes implement a finalize method, which means that objects created will likely hang around until a full garbage collection occurs, which will leave excessive garbage on the heap for longer, and potentially much longer than expected. Java 7 introduced two ways to create streams for reading and writing files that do not have this concern. You should consider switching from these above classes to InputStream is = java.nio.file.Files.newInputStream(myfile.toPath()); OutputStream os = java.nio.file.Files.newOutputStream(myfile.toPath());

SECMD5 / WEAK_MESSAGE_DIGEST_MD5

The algorithms MD2, MD4 and MD5 are not a recommended MessageDigest. PBKDF2 should be used to hash password for example.

"The security of the MD5 hash function is severely compromised. A collision attack exists that can find collisions within seconds on a computer with a 2.6 GHz Pentium 4 processor (complexity of 224.1).[1] Further, there is also a chosen-prefix collision attack that can produce a collision for two inputs with specified prefixes within hours, using off-the-shelf computing hardware (complexity 239).[2]"
- Wikipedia: MD5 - Security
"SHA-224, SHA-256, SHA-384, SHA-512, SHA-512/224, and SHA-512/256:
The use of these hash functions is acceptable for all hash function applications."
- NIST: Transitioning the Use of Cryptographic Algorithms and Key Lengths p.15
"The main idea of a PBKDF is to slow dictionary or brute force attacks on the passwords by increasing the time needed to test each password. An attacker with a list of likely passwords can evaluate the PBKDF using the known iteration counter and the salt. Since an attacker has to spend a significant amount of computing time for each try, it becomes harder to apply the dictionary or brute force attacks."
- NIST: Recommendation for Password-Based Key Derivation p.12

Vulnerable Code:

MessageDigest md5Digest = MessageDigest.getInstance("MD5");
    md5Digest.update(password.getBytes());
    byte[] hashValue = md5Digest.digest();

byte[] hashValue = DigestUtils.getMd5Digest().digest(password.getBytes());


Solution (Using bouncy castle):

public static byte[] getEncryptedPassword(String password, byte[] salt) throws NoSuchAlgorithmException, InvalidKeySpecException {
    PKCS5S2ParametersGenerator gen = new PKCS5S2ParametersGenerator(new SHA256Digest());
    gen.init(password.getBytes("UTF-8"), salt.getBytes(), 4096);
    return ((KeyParameter) gen.generateDerivedParameters(256)).getKey();
}

Solution (Java 8 and later):
public static byte[] getEncryptedPassword(String password, byte[] salt) throws NoSuchAlgorithmException, InvalidKeySpecException {
    KeySpec spec = new PBEKeySpec(password.toCharArray(), salt, 4096, 256 * 8);
    SecretKeyFactory f = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256");
    return f.generateSecret(spec).getEncoded();
}


References
[1] On Collisions for MD5: Master Thesis by M.M.J. Stevens
[2] Chosen-prefix collisions for MD5 and applications: Paper written by Marc Stevens
Wikipedia: MD5
NIST: Transitioning the Use of Cryptographic Algorithms and Key Lengths
NIST: Recommendation for Password-Based Key Derivation
Stackoverflow: Reliable implementation of PBKDF2-HMAC-SHA256 for Java
CWE-327: Use of a Broken or Risky Cryptographic Algorithm

Dm / DM_DEFAULT_ENCODING

Found a call to a method which will perform a byte to String (or String to byte) conversion, and will assume that the default platform encoding is suitable. This will cause the application behavior to vary between platforms. Use an alternative API and specify a charset name or Charset object explicitly.

SECSSSRFUC / URLCONNECTION_SSRF_FD

Server-Side Request Forgery occur when a web server executes a request to a user supplied destination parameter that is not validated. Such vulnerabilities could allow an attacker to access internal services or to launch attacks from your web server.

URLConnection can be used with file:// protocol or other protocols to access local filesystem and potentially other services.

Vulnerable Code:

new URL(String url).openConnection()
new URL(String url).openStream()
new URL(String url).getContent()

Solution/Countermeasures:


References
CWE-918: Server-Side Request Forgery (SSRF)
Understanding Server-Side Request Forgery
CWE-73: External Control of File Name or Path
Abusing jar:// downloads

CBC / CBC_CONTAINS_BASED_CONDITIONAL

This method uses an overly complex if expression made up of multiple conditions joined by OR, where the same local variable is compared to a static value. When the number of conditions grows, it is much cleaner to build a static set of the possible values, and use the contains method on that set. This will shorten the code, and make it more self documenting.

OBL / OBL_UNSATISFIED_OBLIGATION_EXCEPTION_EDGE

This method may fail to clean up (close, dispose of) a stream, database object, or other resource requiring an explicit cleanup operation.

In general, if a method opens a stream or other resource, the method should use a try/finally block to ensure that the stream or resource is cleaned up before the method returns.

This bug pattern is essentially the same as the OS_OPEN_STREAM and ODR_OPEN_DATABASE_RESOURCE bug patterns, but is based on a different (and hopefully better) static analysis technique. We are interested is getting feedback about the usefulness of this bug pattern. For sending feedback, check:

In particular, the false-positive suppression heuristics for this bug pattern have not been extensively tuned, so reports about false positives are helpful to us.

See Weimer and Necula, Finding and Preventing Run-Time Error Handling Mistakes (PDF), for a description of the analysis technique.

PA / PA_PUBLIC_PRIMITIVE_ATTRIBUTE

SEI CERT rule OBJ01-J requires that accessibility to fields must be limited. Otherwise, the values of the fields may be manipulated from outside the class, which may be unexpected or undesired behaviour. In general, requiring that no fields are allowed to be public is overkill and unrealistic. Even the rule mentions that final fields may be public. Besides final fields, there may be other usages for public fields: some public fields may serve as "flags" that affect the behavior of the class. Such flag fields are expected to be read by the current instance (or the current class, in case of static fields), but written by others. If a field is both written by the methods of the current instance (or the current class, in case of static fields) and from the outside, the code is suspicious. Consider making these fields private and provide appropriate setters, if necessary. Please note that constructors, initializers and finalizers are exceptions, if only they write the field inside the class, the field is not considered as written by the class itself.

UrF / URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD

This field is never read.  The field is public or protected, so perhaps it is intended to be used with classes not seen as part of the analysis. If not, consider removing it from the class.

PME / PME_POOR_MANS_ENUM

This field, although defined as a simple variable (int, String, etc), only has a set of constant values assigned to it. Thus it appears to be used like an enum value, and should probably be defined as such.

NP / NP_NULL_ON_SOME_PATH

There is a branch of statement that, if executed, guarantees that a null value will be dereferenced, which would generate a NullPointerException when the code is executed. Of course, the problem might be that the branch or statement is infeasible and that the null pointer exception cannot ever be executed; deciding that is beyond the ability of SpotBugs.

NP / NP_LOAD_OF_KNOWN_NULL_VALUE

The variable referenced at this point is known to be null due to an earlier check against null. Although this is valid, it might be a mistake (perhaps you intended to refer to a different variable, or perhaps the earlier check to see if the variable is null should have been a check to see if it was non-null).

JLM / JLM_JSR166_UTILCONCURRENT_MONITORENTER

This method performs synchronization on an object that is an instance of a class from the java.util.concurrent package (or its subclasses). Instances of these classes have their own concurrency control mechanisms that are orthogonal to the synchronization provided by the Java keyword synchronized. For example, synchronizing on an AtomicBoolean will not prevent other threads from modifying the AtomicBoolean.

Such code may be correct, but should be carefully reviewed and documented, and may confuse people who have to maintain the code at a later date.

DCN / DCN_NULLPOINTER_EXCEPTION

According to SEI Cert rule ERR08-J NullPointerException should not be caught. Handling NullPointerException is considered an inferior alternative to null-checking.

This non-compliant code catches a NullPointerException to see if an incoming parameter is null:


boolean hasSpace(String m) {
  try {
    String ms[] = m.split(" ");
    return names.length != 1;
  } catch (NullPointerException e) {
    return false;
  }
}

A compliant solution would use a null-check as in the following example:


boolean hasSpace(String m) {
    if (m == null) return false;
    String ms[] = m.split(" ");
    return names.length != 1;
}
LEST / LEST_LOST_EXCEPTION_STACK_TRACE

This method catches an exception, and throws a different exception, without incorporating the original exception. Doing so hides the original source of the exception, making debugging and fixing these problems difficult. It is better to use the constructor of this new exception that takes an original exception so that this detail can be passed along to the user. If this exception has no constructor that takes an initial cause parameter, use the initCause method to initialize it instead.


catch (IOException e) {
    throw new MySpecialException("Failed to open configuration", e);
}

CT / CT_CONSTRUCTOR_THROW

Classes that throw exceptions in their constructors are vulnerable to Finalizer attacks

A finalizer attack can be prevented, by declaring the class final, using an empty finalizer declared as final, or by a clever use of a private constructor.

See SEI CERT Rule OBJ-11 for more information.

PRMC / PRMC_POSSIBLY_REDUNDANT_METHOD_CALLS

This method makes two consecutive calls to the same method, using the same constant parameters, on the same instance, without any intervening changes to the objects. If this method does not make changes to the object, which it appears it doesn't, then making two calls is just a waste. These method calls could be combined by assigning the result into a temporary variable, and using the variable the second time.

MF / MF_CLASS_MASKS_FIELD

This class defines a field with the same name as a visible instance field in a superclass. This is confusing, and may indicate an error if methods update or access one of the fields when they wanted the other.

SPP / SPP_USE_ISEMPTY

This method calls the size() method on a collection and compares the result to zero to see if the collection is empty. For better code clarity, it is better to just use col.isEmpty() or !col.isEmpty().

SECCI / COMMAND_INJECTION

The highlighted API is used to execute a system command. If unfiltered input is passed to this API, it can lead to arbitrary command execution.


Vulnerable Code:

import java.lang.Runtime;

Runtime r = Runtime.getRuntime();
r.exec("/bin/sh -c some_tool" + input);

References
OWASP: Command Injection
OWASP: Top 10 2013-A1-Injection
CWE-78: Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection')

ITC / ITC_INHERITANCE_TYPE_CHECKING

This method uses the instanceof operator in a series of if/else statements to differentiate blocks of code based on type. If these types are related by inheritance, it is cleaner to just define a method in the base class, and use overridden methods in these classes.

UCC / UCC_UNRELATED_COLLECTION_CONTENTS

This method adds unrelated objects to a collection or array, requiring careful and brittle data access to that collection. Create a separate class with the properties needed, and add an instance of this class to the collection or array, if possible.

CU / CU_CLONE_USABILITY_OBJECT_RETURN

This class implements the Cloneable interface but defines its clone method to return an Object. Since most likely users of this method will need to cast it to the real type, this will be more painful than necessary. Just declare the return value to be the type of this class.

EXS / EXS_EXCEPTION_SOFTENING_NO_CHECKED

This method's exception signature is constrained by an interface or superclass not to throw any checked exceptions. Therefore a caught checked exception was converted to an unchecked exception and thrown. However, it appears that the class in question is owned by the same author as the constraining interface or superclass. Consider changing the signature of this method to include the checked exception.

IS / IS2_INCONSISTENT_SYNC

The fields of this class appear to be accessed inconsistently with respect to synchronization.  This bug report indicates that the bug pattern detector judged that

A typical bug matching this bug pattern is forgetting to synchronize one of the methods in a class that is intended to be thread-safe.

You can select the nodes labeled "Unsynchronized access" to show the code locations where the detector believed that a field was accessed without synchronization.

Note that there are various sources of inaccuracy in this detector; for example, the detector cannot statically detect all situations in which a lock is held.  Also, even when the detector is accurate in distinguishing locked vs. unlocked accesses, the code in question may still be correct.

RV / RV_RETURN_VALUE_IGNORED_BAD_PRACTICE

This method returns a value that is not checked. The return value should be checked since it can indicate an unusual or unexpected function execution. For example, the File.delete() method returns false if the file could not be successfully deleted (rather than throwing an Exception). If you don't check the result, you won't notice if the method invocation signals unexpected behavior by returning an atypical return value.

OS / OS_OPEN_STREAM

The method creates an IO stream object, does not assign it to any fields, pass it to other methods that might close it, or return it, and does not appear to close the stream on all paths out of the method.  This may result in a file descriptor leak.  It is generally a good idea to use a finally block to ensure that streams are closed.