Re: [collections] Review of proposed fix for InvokerTransformer exploit

[Thomas Neidhart]

On Mon, Nov 9, 2015 at 10:37 AM, Emmanuel Bourg  wrote:

> Le 08/11/2015 23:21, Thomas Neidhart a écrit :
>
> > please review the proposed fix for this issue here:
>
> The exception message ends with a comma, is this a typo? I suggest
> mentioning the system property in the message, such that someone
> hitting the exception immediately knows how to work around it.
>

yes, I wanted to add this information, but forgot to finish.
I also need to add a paragraph to the class javadoc outlining the change.


>
> Also:
>
> !"true".equalsIgnoreCase(deserializeProperty)
>
> is shorter than:
>
> deserializeProperty == null ||
> !deserializeProperty.equalsIgnoreCase("true")
>

yes indeed, will change too.

Thanks,

Thomas

Re: [collections] Review of proposed fix for InvokerTransformer exploit

[Emmanuel Bourg]

Le 08/11/2015 23:21, Thomas Neidhart a écrit :

> please review the proposed fix for this issue here:

The exception message ends with a comma, is this a typo? I suggest
mentioning the system property in the message, such that someone
hitting the exception immediately knows how to work around it.

Also:

!"true".equalsIgnoreCase(deserializeProperty)

is shorter than:

deserializeProperty == null || !deserializeProperty.equalsIgnoreCase("true")


Emmanuel Bourg


-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

[collections] Review of proposed fix for InvokerTransformer exploit

[Thomas Neidhart]

Hi all,

please review the proposed fix for this issue here:

http://svn.apache.org/viewvc?view=revision=1713307

Thanks,

Thomas

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [collections] Review of proposed fix for InvokerTransformer exploit

[Peter Ansell]

On 9 November 2015 at 09:21, Thomas Neidhart  wrote:
> Hi all,
>
> please review the proposed fix for this issue here:
>
> http://svn.apache.org/viewvc?view=revision=1713307

Those changes look workable to me. The main issue from my reading is
that real users of serialisation with InvokerTransformer should be
able to set it up, but by default it should not be accessible because
it is an entry point into every part of the classpath, whether they
are serializable or not. Anyone who does actually set it up possibly
still doesn't understand the full security implications, but it is out
of our hands by then.

One guide that I read yesterday that highlighted the number of ways
serialisation bugs can occur is:

http://www.oracle.com/technetwork/java/seccodeguide-139067.html#8

In particular, one further step is that we may need to do an audit of
all the serializable classes to see if we need to either change some
variables to transient, remove "Serializable", or add custom
serialisation/deserialisation as in this case. We may not be able to
change fields or Serializable until a future revision to maintain
compatibility, but it could help with the public image of commons
being heavily reused and actively caring about security by responding
promptly as we are doing now.

One small comment, in the tests, you may want to use try-finally to
set and clear the system properties, as if the test fails, the system
property settings may leak out to other tests. It is a minor thing,
because it only has any effect if the test fails, and only on one
other test in this case. It is a bigger deal in tests where the test
changes a widely used system property and must always change it back
for other tests to succeed.

Cheers,

Peter

-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Re: [collections] Review of proposed fix for InvokerTransformer exploit

[Gary Gregory]

On Sun, Nov 8, 2015 at 3:37 PM, Peter Ansell  wrote:

> On 9 November 2015 at 09:21, Thomas Neidhart 
> wrote:
> > Hi all,
> >
> > please review the proposed fix for this issue here:
> >
> > http://svn.apache.org/viewvc?view=revision=1713307
>
> Those changes look workable to me. The main issue from my reading is
> that real users of serialisation with InvokerTransformer should be
> able to set it up, but by default it should not be accessible because
> it is an entry point into every part of the classpath, whether they
> are serializable or not. Anyone who does actually set it up possibly
> still doesn't understand the full security implications, but it is out
> of our hands by then.
>
> One guide that I read yesterday that highlighted the number of ways
> serialisation bugs can occur is:
>
> http://www.oracle.com/technetwork/java/seccodeguide-139067.html#8
>
> In particular, one further step is that we may need to do an audit of
> all the serializable classes to see if we need to either change some
> variables to transient, remove "Serializable", or add custom
> serialisation/deserialisation as in this case. We may not be able to
> change fields or Serializable until a future revision to maintain
> compatibility, but it could help with the public image of commons
> being heavily reused and actively caring about security by responding
> promptly as we are doing now.
>
> One small comment, in the tests, you may want to use try-finally to
> set and clear the system properties, as if the test fails, the system
> property settings may leak out to other tests. It is a minor thing,
> because it only has any effect if the test fails, and only on one
> other test in this case. It is a bigger deal in tests where the test
> changes a widely used system property and must always change it back
> for other tests to succeed.
>

Better yet would be to write a JUnit Rule to do this, but that might be
fancier than we have the time to implement.

Gary

>
> Cheers,
>
> Peter
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
>


-- 
E-Mail: garydgreg...@gmail.com | ggreg...@apache.org
Java Persistence with Hibernate, Second Edition

JUnit in Action, Second Edition 
Spring Batch in Action 
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory