Why doesn't processAnnotations(Class) do allowTypes(Class) too?

[Geoffrey De Smet]

Hi all,

In preparation of 1.5,
I 've set up XStream 1.4.10 like this:
XStream xStream = new XStream();
XStream.setupDefaultSecurity(xStream);


1) When I call
xStream.processAnnotations(SolverConfig.class);
without also calling
xStream.allowTypes(new Class[]{SolverConfig.class});
I get:
Exception in thread "main"
com.thoughtworks.xstream.security.ForbiddenClassException:
org.optaplanner.core.config.solver.SolverConfig
at
com.thoughtworks.xstream.security.NoTypePermission.allows(NoTypePermission.java:26)
...

That surprises me.
Why do I need to tell XStream twice about SolverConfig? Can I submit a
jira to improve this?


2) By doing processAnnotations for the SolverConfig annotation,
it finds the annotations on ScoreDirectorFactoryConfig and PhaseConfig
(they are field types in SolverConfig).
And PhaseConfig has an @ XStreamInclude for
ConstructionHeuristicPhaseConfig.

So when I 've called
xStream.processAnnotations(SolverConfig.class);
I would expect that ConstructionHeuristicPhaseConfig is now cool too,
but it's not:
Caused by:
com.thoughtworks.xstream.security.ForbiddenClassException:
org.optaplanner.core.config.constructionheuristic.ConstructionHeuristicPhaseConfig


Philosophy for change proposal:
If a class has XStream annotations and it is somehow processed (directly
or transitively) by XStream,
then it poses no security threat (the class was build with XStream
deserialization in mind and it was explicitly registered)
and therefore shouldn't cause ForbiddenClassException.

What do you think?


wkr,
Geoffrey

[Jörg Schaible]

Hi Geoffrey,

Am Tue, 03 Oct 2017 09:41:47 +0200 schrieb Geoffrey De Smet:

> In preparation of 1.5,
> I 've set up XStream 1.4.10 like this:
> XStream xStream = new XStream();
> XStream.setupDefaultSecurity(xStream);
>
>
> 1) When I call
> xStream.processAnnotations(SolverConfig.class);
> without also calling
> xStream.allowTypes(new Class[]{SolverConfig.class});
> I get:
> Exception in thread "main"
> com.thoughtworks.xstream.security.ForbiddenClassException:
> org.optaplanner.core.config.solver.SolverConfig
> at
> com.thoughtworks.xstream.security.NoTypePermission.allows(NoTypePermission.java:26)
> ...
>
> That surprises me.
> Why do I need to tell XStream twice about SolverConfig?

Well, you have a point.

> Can I submit a
> jira to improve this?

Yes. It is an improvement request.

> 2) By doing processAnnotations for the SolverConfig annotation,
> it finds the annotations on ScoreDirectorFactoryConfig and PhaseConfig
> (they are field types in SolverConfig).
> And PhaseConfig has an @ XStreamInclude for
> ConstructionHeuristicPhaseConfig.
>
> So when I 've called
> xStream.processAnnotations(SolverConfig.class);
> I would expect that ConstructionHeuristicPhaseConfig is now cool too,
> but it's not:
> Caused by:
> com.thoughtworks.xstream.security.ForbiddenClassException:
> org.optaplanner.core.config.constructionheuristic.ConstructionHeuristicPhaseConfig
>
>
> Philosophy for change proposal:
> If a class has XStream annotations and it is somehow processed (directly
> or transitively) by XStream,
> then it poses no security threat (the class was build with XStream
> deserialization in mind and it was explicitly registered)
> and therefore shouldn't cause ForbiddenClassException.
>
> What do you think?

Well, such an automatism would have to work also without any annotation, since the same is true for every
type you're using in one of XStream's facade methods to setup aliases, et al. However, for high security
environments such an automatism might not acceptable. Therefore I am thinking of a new Permission, but I
must think about it ...

Cheers,
Jörg

[Geoffrey De Smet]

Hi Jorg,

I've send in a PR for a new permission type (that isn't actived by
default yet),
would you mind reviewing it?
https://github.com/x-stream/xstream/pull/99


"
This permission accepts any class with an XStream annotations, because
that class was designed with XStream in mind and therefore it is fair to
presume it is not vulnerable.
Jackson and JAXB follow this philosophy too and they don't have any
CVE's against this behavior.

This PR just creates and tests that class, but I strongly believe it
should also be added into XStream.setupDefaultSecurity(), so XStream 1.5
isn't harder to use than JAXB and Jackson. However, that's a different
issue/discussion, so I haven't included that change here, due to the
debatable nature of that change.
(The fact that XStream can also be used without annotations is unrelated
to this change, because just because the security framework has to be a
pain for xstream usage without annotations, it shouldn't force xstream
usage with annotations be as painful if that approach does have a less
painful solution.)