On Sat, Oct 1, 2016 at 12:55 AM, Jochen Theodorou <[email protected]> wrote:
> hi all,
>
> as you guys may remember we added
>
> private Object readResolve() {
>> if (ALLOW_RESOLVE) {
>> return this;
>> }
>> throw new UnsupportedOperationException();
>> }
>>
>
> to prevent proper deserialization of a MethodClosure in case we don't want
> to allow it (which is the default). Now it seems that this approach is not
> enough. I have found several articles stating that it is possible to bypass
> readResolve. In this case you would still have access to the fully
> deserialized object. Thus I suggest we do the following:
>
> private void readObject(java.io.ObjectInputStream stream) throws
>> IOException, ClassNotFoundException {
>> if (ALLOW_RESOLVE) {
>> stream.defaultReadObject();
>> } else {
>> stream.readUTF(); // read method and forget it
>> }
>> }
>>
>
>
Preventing the deserialization at this early stage before the object is
constructed I think is a good thing. But I am curious, why try to read the
method String value and forget it? Not sure if this is a concern, but even
though this will leave the method field null I think it would still try to
deserialize the fields for the super class Closure. Seems like the safer
route to take her is to throw an exception as before.
> this is very similar to the readResolve implementation of course. So we
> would still fail deserialization, only much earlier. We would still read
> the String for the method, but we would make sure it will not be assigned.
>
> So if malicious code manage to go around readResolve, it would still be
> left with a MethodClosure in which method is null, thus any try to invoke a
> method will fail with a NullpointException.
>
> Afaik this solution would be compatible to earlier versions of Groovy.
>
> What do you guys think?
>
> bye Jochen
>
