MC_OVERRIDABLE_METHOD_CALL_IN_READ_OBJECT false positive in 4.8.4

[kohlschuetter]

With 4.8.4 (and latest 4.8.5-SNAPSHOT), I'm getting two false-positive errors:

[ERROR] Medium: Overridable method equals is called from readObject. [org.newsclub.net.unix.AFSocketAddress] At AFSocketAddress.java:[line 906] MC_OVERRIDABLE_METHOD_CALL_IN_READ_OBJECT
[ERROR] Medium: Overridable method readUTF is called from readObject. [org.newsclub.net.unix.AFSocketAddress] At AFSocketAddress.java:[line 905] MC_OVERRIDABLE_METHOD_CALL_IN_READ_OBJECT

Method in question:

private void readObject(ObjectInputStream in) throws ClassNotFoundException, IOException {
    in.defaultReadObject();

    String af = in.readUTF(); // 905 HERE
    if ("undefined".equals(af)) { // 906 HERE
      this.addressFamily = null;
    } else {
      this.addressFamily = Objects.requireNonNull(AFAddressFamily.getAddressFamily(af),
          "address family");
    }
  }

In 905, it's clearly a false-positive because the object has been passed as the ObjectInputStream.
In 906, it's clearly a false-positive because we're operating on a string constant.

I think generally, these errors should not occur because they don't work on this (the instance where readObject is called) but on some other object.

I understand MC_OVERRIDABLE_METHOD_CALL_IN_READ_OBJECT has been newly introduced in 4.8.4.

[kohlschuetter]

For what it's worth, I'm using spotbugs-maven-plugin 4.8.3.1 (or 4.8.4.0) with spotbugs 4.8.5-SNAPSHOT.
I couldn't reproduce this via spotbugs-tests, so here are the steps to reproduce:

1. Clone junixsocket from https://github.com/kohlschutter/junixsocket/tree/813f25a01fe35958b2ac2282d8f7f371f1471af0
2. Clone kohlschutter-parent from https://github.com/kohlschutter/kohlschutter-parent/tree/212cf152f4ca1ece6c417c475a8c5b6254538edc
3. Make sure both projects are siblings in the same directory
4. Build/check junixsocket-common (should succeed):

cd junixsocket/junixsocket-common
mvn clean compile spotbugs:check

5. Modify kohlschutter-parent/pom.xml: uncomment the <dependencies> section to include a specific spotbugs version:

                <plugin>
                    <groupId>com.github.spotbugs</groupId>
                    <artifactId>spotbugs-maven-plugin</artifactId>
                    <version>${spotbugs.plugin.version}</version>
                    <dependencies>
                        <dependency>
                            <groupId>com.github.spotbugs</groupId>
                            <artifactId>spotbugs</artifactId>
                            <version>4.8.4</version><!-- or 4.8.5-SNAPSHOT -->
                        </dependency>
                    </dependencies>
                </plugin>

6. Re-run check from step 4 above (which will now fail)

[dsubelman]

I'm having the same issue with readInt(). Both methods, readUTF() and readInt(), are not final. However, for example, defaultReadObject() is also not final, but it doesn't trigger any bug alert.
Am I missing something?

[gtoison]

Hello @isuckatcs, maybe this changed with #2895, could you please have a look a this issue ?

[isuckatcs]

However, for example, defaultReadObject() is also not final, but it doesn't trigger any bug alert.

Here the error is not triggered, because defaultReadObject() is an exception that can be called.

See SER09-J:

Exceptions
SER09-J-EX0: The readObject() method may invoke the overridable methods defaultReadObject() and readFields() in class java.io.ObjectInputStream [SCG 2009].

AFAIK the way the check is written should prevent these errors to be reported, but after thinking about it further these might actually be correct to report. SER09-J says the following:

Invoking overridable methods from the readObject() method can provide the overriding method with access to the object's state before it is fully initialized.

When the overridden method is invoked, the stack frame looks like this:

- readObject()
  - overriddenMethod()

This means, that if the caller of overriddenMethod() starts walking the stack, it can actually have access to the object by fishing for this in the stack frame of readObject(). This is something that is easily possible to do in C++, see my example on godbolt. I don't see why this wouldn't be possible in Java too using some trick.

@JuditKnoll any thought on this?

[dsubelman]

Thanks for the complete explanation. You are correct that both defaultReadObject() and readFields() don't report any errors. However, with readFields() and the get(String name, Object val) method it does report an error for the get method:

ObjectInputStream.GetField fieldValues = ois.readFields();
String nameField = (String) fieldValues.get("name", "null"); // Error reported here

Is that the expected behavior?

[isuckatcs]

@dsubelman I double checked your example, and according to the current rules of the check, the get() call should be reported.

private boolean shouldIgnoreCallInReadObject(XMethod method) {
    // SER09-J-EX0: The readObject() method may invoke the overridable methods defaultReadObject() and readFields()
    // in class java.io.ObjectInputStream
    boolean inOIS = "java.io.ObjectInputStream".equals(method.getClassName());
    String methodName = method.getName();
    return inOIS && ("defaultReadObject".equals(methodName) || "readFields".equals(methodName));
}

if (!shouldIgnoreCallInReadObject(method) && !method.isPrivate() && !method.isFinal()) {
    bugAccumulator.accumulateBug(
            new BugInstance(this, "MC_OVERRIDABLE_METHOD_CALL_IN_READ_OBJECT", NORMAL_PRIORITY)
                    .addClass(this)
                    .addMethod(caller)
                    .addString(method.getName()), sourceLine);
    return false;
}

These rules correspong to the wording of SER09-J, according to which

The readObject() method must not call any overridable methods. [...] The readObject() method may invoke the overridable methods defaultReadObject() and readFields() in class java.io.ObjectInputStream.

The rule says any overridable methods, not just the ones invoked on this.

Initially I remembered that the check is written so that it only checks calls on this, but that limitation is only there for indirect calls (See this discussion for more details). I would only add that by the time, I was answering this, I didn't think about walking the stack from the overridden method. With that in mind, I think that limitation shouldn't be there and the call mentioned in the discussion should be reported too, assuming that the C++ example I gave can be translated to Java too.

[dsubelman]

@isuckatcs understood, thanks again for the complete explanation.

[kohlschuetter]

The rule says any overridable methods, not just the ones invoked on this.

I'm not quite following this logic.
The example in SER09-J clearly describes what they mean — an overridable method of the instance in question, not just any "method overridable in theory".

I mean, how else would you be able to call in.readUTF()? We don't control the very subclass of ObjectInputStream that gets passed to readObject, nor are we able to subclass it further.

Yes, you may have a scenario where calling some other object's method indirectly calls an overridable method of our instance, but I'm pretty sure you won't be able reliably detect these cases while allowing all others, so why bother going beyond the exact, simple scenario given in SER09-J?

Coming back to my original problem, could you please, if I'm mistaken, suggest how I should rewrite the following method so spotbugs won't fail?

private void readObject(ObjectInputStream in) throws ClassNotFoundException, IOException {
    in.defaultReadObject();

    String af = in.readUTF(); // 905 HERE
    if ("undefined".equals(af)) { // 906 HERE
      this.addressFamily = null;
    } else {
      this.addressFamily = Objects.requireNonNull(AFAddressFamily.getAddressFamily(af),
          "address family");
    }
  }

[isuckatcs]

I'm not quite following this logic.
The example in SER09-J clearly describes what they mean — an overridable method of the instance in question, not just any "method overridable in theory".

You are correct that the example only mentions the overridable method of the instance, but in that context the exception wouldn't make sense I think.

SER09-J-EX0: The readObject() method may invoke the overridable methods defaultReadObject() and readFields() in class java.io.ObjectInputStream

If we only care about overridable methods invoked on the instance, why does it have to be explicitly stated, that calling 2 overridable methods and only these 2 on java.io.ObjectInputStream is allowed? The input stream is not the instance, so if we only care about methods invoked on the instance, we wouldn't care about these methods anyway, right?

For a moment I came to think about what if the exception refers to the readObject() inside the class. E.g.:

public class ObjectInputStream
    extends InputStream implements ObjectInput, ObjectStreamConstants
{
    ...
    private final Object readObject(Class<?> type)
        throws IOException, ClassNotFoundException
    {
        ...
    }

    public final Object readObject()
        throws IOException, ClassNotFoundException {
        return readObject(Object.class);
    }
    ...
}

However I think it wouldn't make sense in this context, because these methods cannot be overridden and are also shipped with the JDK you use, so you have no way to modify them. So it probably doesn't even make sense to report errors for them, as you have no way to deal with the errors.

Yes, you may have a scenario where calling some other object's method indirectly calls an overridable method of our instance, but I'm pretty sure you won't be able reliably detect these cases while allowing all others, so why bother going beyond the exact, simple scenario given in SER09-J?

This logic was implemented in #1716, I only extended the check and this feature was already supported.

Coming back to my original problem, could you please, if I'm mistaken, suggest how I should rewrite the following method so spotbugs won't fail?

If I understand it correctly, this method is supposed to deserialize the object. During serialization you convert the object to a byte stream and store it on the disk. Conversely, when you deserialize, you read that exact bytestream and convert it back to the object, which shouldn't require any additional logic.

What I see in your method is that you first created a different kind of representation of the object, which you stored on disk and now you read that different representation and try to parse it into the object. I think this transformation wasn't considered, when SER09-J was written, unless I understand something wrong.

[kohlschuetter]

For a moment I came to think about what if the exception refers to the readObject() inside the class. E.g.:
...
However I think it wouldn't make sense in this context, because these methods cannot be overridden and are also shipped with the JDK you use, so you have no way to modify them. So it probably doesn't even make sense to report errors for them, as you have no way to deal with the errors.

The readObject in the rule refers to Object.readObject(ObjectInputStream), not ObjectInputStream.readObject. Moreover, while ObjectInputStream.readObject is declared final, it can actually indeed be overridden, look for enableOverride:

    public final Object readObject()
        throws IOException, ClassNotFoundException {
        return readObject(Object.class);
    }

    private final Object readObject(Class<?> type)
        throws IOException, ClassNotFoundException
    {
        if (enableOverride) {
            return readObjectOverride();
        }
...
    protected Object readObjectOverride()
        throws IOException, ClassNotFoundException
    {
        return null;
    }

But that is beside the point.

Importantly, after reviewing SER09-J and the cited SCG 2009 guidelines, I feel that the exception SER09-J-EX0

SER09-J-EX0: The readObject() method may invoke the overridable methods defaultReadObject() and readFields() in class java.io.ObjectInputStream [SCG 2009].

is not quoting the cited guidelines correctly.

"Guideline 7-4 / OBJECT-4: Prevent constructors from calling methods that can be overridden" says:

Prevent constructors from calling methods that can be overridden
Constructors that call overridable methods give attackers a reference to this (the object being constructed) before the object has been fully initialized. Likewise, clone, readObject, or readObjectNoData methods that call overridable methods may do the same. The readObject methods will usually call java.io.ObjectInputStream.defaultReadObject, which is an overridable method.

So the Sun guidelines claim that calling defaultReadObject is actually bad because it supposedly gives attackers an opportunity to access this since it can be overridden by a subclass of ObjectInputStream.

Why would we therefore make an exception for it???

Now, in "Guideline 8-3 / SERIAL-3: View deserialization the same as object construction", they indeed explain an alternative approach, which doesn't require defaultReadObject, namely using in.readFields() instead:

    private void readObject(
        java.io.ObjectInputStream in
    ) throws java.io.IOException, ClassNotFoundException {
        java.io.ObjectInputStream.GetField fields =
            in.readFields();
        this.data = ((byte[])fields.get("data")).clone();
    }

While this may indeed be a "stricter" solution, this is not always desirable because one would have to manually assign all fields (which exactly defaultReadObject is supposed to simplify, and which is called when there is no custom readObject(ObjectInputStream) method).

So, should we then only allow readFields but not defaultReadObject?

No.

If you look at ObjectInputStream.defaultReadObject() you will see that in actually no way will any subclass have access to our instance's this, especially because the method does not take our this as a parameter!

In fact, ObjectInputStream only has access to this internally via SerialCallbackContext.getObj, which is not exposed to subclasses at all!

How will an attacker make use of an ObjectInputStream subclass then? It doesn't make sense!

Therefore, it also doesn't make sense for spotbugs to consider ObjectInputStream.defaultReadObject() "safer" than ObjectInputStream.readInt(), ObjectInputStream.readObject(), etc., especially when it actually internally does access this whereas readInt(), readObject(), etc., don't?

Is in.defaultReadObject() perhaps in anyway less safe than using in.readFields()? Well, it allows an attacker to supply an arbitrary set of field-value pairs, but that's all. These objects won't magically get access to "this". Lastly, any decent readObject(ObjectInputStream in) implementation, even ones using in.readFields() should verify the loaded values anyway.

As I've just shown, ObjectInputStream.defaultReadObject should not be regarded safer, but just as safe as any other method in ObjectInputStream, and MC_OVERRIDABLE_METHOD_CALL_IN_READ_OBJECT should not complain about their use.

So, we've now established that the goal of these guidelines were to:

1. prevent access to an incompletely initialized "this" to an attacker who somehow supplies a malicious ObjectInputStream.
2. prevent access to an incompletely initialized "this" to a subclass that gets a method call via a non-final class method from within readObject.

We've ruled out possibility 1: there is no way an ObjectInputStream subclass could access this prematurely (other than via reflection or JNI trickery, at which point the security model is broken).

Possibility 2 is indeed a legitimate use case for MC_OVERRIDABLE_METHOD_CALL_IN_READ_OBJECT , but it is not being used in my example code. It should therefore not trigger an error.

[isuckatcs]

there is no way an ObjectInputStream subclass could access this prematurely (other than via reflection or JNI trickery, [...])

When you use JNI, you declare the method in Java and implement it in C++ AFAIK. The C++ method has access to this, so can't it also fish for curContext inside the object pointed by this?

From Java, I don't see any straightforward way to get access to this either.

I agree that defaultReadObject() can indeed lead to the same problem, that this check wants to protect against, so making it an exception doesn't make sense, but for the other methods I think the error is reported correctly if the JNI or reflection tricks you refered to do work.

On the other hand, limiting the check for overridable methods invoked on this contradicts the cert. @JuditKnoll what is the prefere way to deal with this? Should we just limit the detection as proposed?

[kohlschuetter]

With JNI, the Java security model is entirely defeated (you can access private members, raw memory, etc.). There are separate warnings that cover this scenario already. Reflection access has been historically similarly dangerous, and is easier to contain since Java 9.

Serialization is a typical attack vector when it comes to providing malicious data, but, in my view, the scenario where providing a custom ObjectInputStream subclass may lead to a problem that can only be triggered via JNI is a very hypothetical problem. If there is an attacker with JNI access, they can just initialize the class themselves and assign members at will.

Given that SER09-J is already considered a low P4 / Low Severity problem, and given the history of spotbugs previously not hitting this issue, I feel that these new warnings (which at least for the string comparison in line 906 of my example is a clear false positive) are not helpful.

[gtoison]

The scenario where a native library is used with a custom ObjectInputStream seems very far-fetched to me too.
The assumption is that the attacker might control the data in the input stream, but for that he doesn't really need to override methods in ObjectInputStream.

IMO the rule is for attacks where the code is something like:

if (isAdmin(in.readXyz()) {
     this.admin = true;
} else {
     ...
}

An attacker might get an object where the private admin field is set to true by overriding isAdmin()

[kohlschuetter]

@gtoison Correct, and that (possibility 2) is why I think having this warning in the general case is important, rather than to silence it altogether. I do see some paranoid use case for the override-ObjectInputStream scenario; in case that is desired to be checked by spotbugs, such warning should then be separate, and not be enabled by default.