Add serialVersionUID to serializable class

[rng70-or]

Motivation:

In file: AuthorizationStrategy.java, the class Unsecured is serializable but it does not contain any serialVersionUID field. The compiler generates one by default in such scenarios, but the generated id is dependent on compiler implementation and may cause unwanted problems during deserialization.

The Role of serialVersionUID:

The primary role of serialVersionUID is to provide version control during deserialization. When we deserialize an object, the JVM checks whether the serialVersionUID of the serialized data matches the serialVersionUID of the class in the current classpath. If they match, the deserialization proceeds without issues. However, if they do not match, program can encounter InvalidClassException.

As, serialVersionUID servers the purpose of version control of class during serialization-deserialization, without a serialVersionUID, we risk breaking backward compatibility when we make changes to our classes, which can lead to unexpected issues and errors during deserialization.

So, any change(addition or removal of any field) in class Unsecured in the future(if it needs to be change) will cause issue with all previous if no serialVersionUID is provided.

Sponsorship and Support:

This work is done by the security researchers from OpenRefactory and is supported by the Open Source Security Foundation (OpenSSF): Project Alpha-Omega. Alpha-Omega is a project partnering with open source software project maintainers to systematically find new, as-yet-undiscovered vulnerabilities in open source code - and get them fixed – to improve global software supply chain security.

The bug is found by running the Intelligent Code Repair (iCR) tool by OpenRefactory and then manually triaging the results.

[welcome]

Yay, your first pull request towards Jenkins core was created successfully! Thank you so much!

A contributor will provide feedback soon. Meanwhile, you can join the chats and community forums to connect with other Jenkins users, developers, and maintainers.

[NotMyFault]

The explanation lacks a practical issue, a proof of concept for the issue described, and lacks a proof of concept/real-world scenario to replicate that the changes proposed mitigate the issue described.

[rng70-or]

Let's take a Class for example

class Human implements Serializable {
    private final int age;
    private final String name;

    public Human(String name, int age) {
        this.name = name;
        this.age = age;
    }


    @Override
    public String toString() {
        return "Person [name=" + name + ", age=" + age + "]";
    }
}

and we have serialized one of the class instance

public class SerialVersionWithoutUIDBefore {
    public static void main(String[] args) {

        Serialize();

        Deserialize();
    }

    public static void Serialize()
    {
        Human person = new Human("Alice", 30);

        try (ObjectOutputStream outputStream = new ObjectOutputStream(new FileOutputStream("person.ser"))) {
            outputStream.writeObject(person);
            System.out.println("Object has been serialized.");
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

    public static void Deserialize()
    {
        try (ObjectInputStream inputStream = new ObjectInputStream(new FileInputStream("person.ser"))) {
            Human person2 = (Human) inputStream.readObject();
            System.out.println("\nObject has been deserialized.");
            System.out.println(person2);
        } catch (IOException | ClassNotFoundException e) {
            e.printStackTrace();
        }
    }

}

Here is the output of this normal execution process. Source Code

Now, if we change the Human class in any way, like

class Human implements Serializable {
    private final int age;
    private final String name;
    private final String address;

    public Human(String name, int age, String address) {
        this.name = name;
        this.age = age;
        this.address = address;
    }


    @Override
    public String toString() {
        return "Person [name=" + name + ", age=" + age +  ", address = " + address + "]";
    }
}

here, we have added another field address to the class. Now, if we want to desearialize the previously serialized object then it will throw InvalidClassException.

Here is the output after changing to class. Source Code

java.io.InvalidClassException: Human; local class incompatible: stream classdesc serialVersionUID = -6212638591031772587, local class serialVersionUID = 4801088500108260898

But what if we use serialVersionUID in the first place

class Human implements Serializable {
    private final int age;
    private final String name;
    private static final long serialVersionUID = 123456789L;

    public Human(String name, int age) {
        this.name = name;
        this.age = age;
    }


    @Override
    public String toString() {
        return "Person [name=" + name + ", age=" + age "]";
    }
}

the even we change the Class instance in any way, but instead of having a InvalidClassException it will properly be deserialized even the previous install of serialized object does not have any address instance

Object has been deserialized.
Person [name=Alice, age=30, address = null]

Here is the output of this normal execution process with serialVersionUID. Source Code

Thus it ensure the backward compatibility when we make changes to classes and it ensures that in near future if we change the class instance it will support the previous version of serialized objects.

[rng70-or]

Now, this same scenario can be applied for all real life scenario where serialVersionUID was not used. It can be dangerous for future when class instance got changed.

Same scenario can be applied for this project also, if it update the class signature without using serialVersionUID.

Hope, you understand why it is important to use serialVersionUID.

The previous scenario can be used as grandfather PoC.

[rng70-or]

Oh, okay. I will try to give you an real world scenario for sure.

[daniel-beck]

@NotMyFault There's previous PRs like #3613, #3897, is that what you're asking for?

[NotMyFault]

@NotMyFault There's previous PRs like #3613, #3897, is that what you're asking for?

Yeah basically, 3613 and 3897 address an issue with a clear description, why a serialVersionUID should be added, and how that mitigates the issue described.

This PR adds a serialVersionUID just for the sake of adding a serialVersionUID, lacking a real-world issue it's supposed to address, making me wonder about the rationale behind such a proposal, given the fact that the PR claims to be generated by static analysis tooling.

[rng70-or]

That is right. This PR is similar as #3897.

In this PR, they also have the same error as I have here

java.io.InvalidClassException: Human; local class incompatible: stream classdesc serialVersionUID = -6212638591031772587, local class serialVersionUID = 4801088500108260898

But I was one step behind that my testing was on a sample code written by me but theirs was based on a real-world scenario.

But, obviously this PR was not generated by a bot, but a real person.

Now, as you have a real-world scenario, would you consider this PR as not spam? It is ture that the bug was found by a static analysis tool but I validated this type of bug on my own code rather than on a real world scenario before submission and also I think there is no need to provide a real scenario as you already have here #3897

Dismissed

[NotMyFault]

I have removed the label and my assignment because I have no interest in reviewing this PR.

[rng70-or]

Thanks for your feedback. As, it is a simple valid fix, I'll leave it to you to fix(merge) or not.

[basil]

OK but there are 88 instances of this problem in core/src/main/java/ and this fixes one of them, reducing the number of instances of this problem from 88 to 87. While no contribution is too small, why not fix the other 87 since the fix is so simple?

[rng70-or]

for most of cases found, I have tried to solve this issue. change was made to 42 additional files excepts Test files. And yes, most of places where serialVersionUID was added in this code base was used 1L but there are some places where some random numbers were also used. So, I have tried to remove that inconsistency as well.

[rng70-or]

everything should be fine now. Restored all the previous instances of serialVersionUID with the previous value and added the missing instances of serialVersionUID initialized with 1L

[basil]

added the missing instances of serialVersionUID initialized with 1L

Still missing 77 instances of serialVersionUID AFAICT.

[rng70-or]

I have tried to add serialVersionUID instances as many serializable class possible. When I said serializable, I considered serializable by implements of Serializable and inheritance of first order parent which are serializable but not built in packages. Can you provide an example of missing instance that you are considering that it should have a serialVersionUID? Maybe I am missing something or maybe I am considering the class not serializable because they extends some packages that are Serializable by default like ArrayList.