You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@groovy.apache.org by Jochen Theodorou <bl...@gmx.org> on 2016/10/01 07:55:11 UTC
MethodClosure deserialization problem
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
> }
> }
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
Re: MethodClosure deserialization problem
Posted by John Wagenleitner <jo...@gmail.com>.
On Sat, Oct 1, 2016 at 10:52 PM, Jochen Theodorou <bl...@gmx.org> wrote:
> On 02.10.2016 07:50, Jochen Theodorou wrote:
>
>> On 02.10.2016 02:57, John Wagenleitner wrote:
>> [...]
>>
>
> ah yes, I forgot:
>
> private void readObject(java.io.ObjectInputStream stream) throws
>> IOException, ClassNotFoundException {
>> if (ALLOW_RESOLVE) {
>> stream.defaultReadObject();
>> }
>> throw new UnsupportedOperationException();
>> }
>>
>
> is then the new version.
>
>
> bye Jochen
>
>
>
This change looks good to me.
Re: MethodClosure deserialization problem
Posted by Jochen Theodorou <bl...@gmx.org>.
On 02.10.2016 07:50, Jochen Theodorou wrote:
> On 02.10.2016 02:57, John Wagenleitner wrote:
> [...]
ah yes, I forgot:
> private void readObject(java.io.ObjectInputStream stream) throws IOException, ClassNotFoundException {
> if (ALLOW_RESOLVE) {
> stream.defaultReadObject();
> }
> throw new UnsupportedOperationException();
> }
is then the new version.
bye Jochen
Re: MethodClosure deserialization problem
Posted by Jochen Theodorou <bl...@gmx.org>.
On 02.10.2016 02:57, John Wagenleitner wrote:
[...]
> 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.
at the point MethodClosure#readObject is called, the "default"
readObject for Closure has already happened. So regardless of what I do
in readObject in MethodClosure, those fields will have been set already.
We can still throw an Exception of course. I was only thinking that it
might be good to read all the data, that has been written first, so that
a stream with multiple objects can then continue without having to know,
that you first have to read a String to be able to continue. But I was
probably thinking too hard here (or not enough, depending on your POV ;)
) and maybe just failing is really the best option.
bye Jochen
Re: MethodClosure deserialization problem
Posted by John Wagenleitner <jo...@gmail.com>.
On Sat, Oct 1, 2016 at 12:55 AM, Jochen Theodorou <bl...@gmx.org> 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
>
Re: MethodClosure deserialization problem
Posted by Jochen Theodorou <bl...@gmx.org>.
If there are no objects, I am going to commit that tomorrow
bye Jochen
On 01.10.2016 09:55, Jochen Theodorou 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
>> }
>> }
>
> 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