Uploaded image for project: 'Commons IO'
  1. Commons IO
  2. IO-487

ValidatingObjectInputStream contribution - restrict which classes can be deserialized

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.5
    • Component/s: Utilities
    • Labels:
    • Flags:
      Patch

      Description

      As discussed on the commons dev list I'd like to contribute my SLING-5288 code to commons-io. I'll attach a patch.

      Update: this is committed now, see [1] for an example.

      [1] https://svn.apache.org/repos/asf/commons/proper/io/trunk/src/test/java/org/apache/commons/io/serialization/MoreComplexObjectTest.java

      1. IO-487.patch
        18 kB
        Bertrand Delacretaz
      2. IO-487.patch
        18 kB
        Bertrand Delacretaz
      3. IO-487.patch
        18 kB
        Bertrand Delacretaz
      4. IO-487.patch
        44 kB
        Adrian Crum
      5. IO-487.patch
        46 kB
        Adrian Crum
      6. IO-487-name-regex-acceptor.patch
        12 kB
        Niall Pemberton
      7. IO-487.patch
        23 kB
        Adrian Crum
      8. IO-487-2.patch
        24 kB
        Niall Pemberton
      9. IO-487-matchers.patch
        23 kB
        Bertrand Delacretaz
      10. IO-487-accept-reject.patch
        30 kB
        Bertrand Delacretaz
      11. IO-487.patch
        51 kB
        Adrian Crum
      12. IO-487-accept-reject-2.patch
        31 kB
        Bertrand Delacretaz

        Issue Links

          Activity

          Hide
          jonenst Jon Harper added a comment -

          Hi,
          just adding a comment here as this is the best documentation I have found for this feature. (This is actually what Thomas Neidhart said in the comment just before mine, but I didn't understand it. At least I assume that's what he meant)

          java.lang.String will not be resolved

          I confirm that you can not blacklist java.lang.String. It will always be whitelisted and it is like this by default. And java.lang.String is the only object that is like this.

          This is because this algorithm works by using the readResolve call of the ClassDesc (which comes before the object in the stream). Looking at https://docs.oracle.com/javase/7/docs/platform/serialization/spec/protocol.html , java.lang.String is the only one that doesn't have a ClassDesc.

          So primitive types and String are always whitelisted; all other types (including arrays and boxed variants of primitives types) need to be whitelisted (either through a package java.lang.* or individually) to allow deserializing all the transitive fields of all the objects needed to deserialize the top object.
          Cheers,
          Jon

          Show
          jonenst Jon Harper added a comment - Hi, just adding a comment here as this is the best documentation I have found for this feature. (This is actually what Thomas Neidhart said in the comment just before mine, but I didn't understand it. At least I assume that's what he meant) java.lang.String will not be resolved I confirm that you can not blacklist java.lang.String. It will always be whitelisted and it is like this by default. And java.lang.String is the only object that is like this. This is because this algorithm works by using the readResolve call of the ClassDesc (which comes before the object in the stream). Looking at https://docs.oracle.com/javase/7/docs/platform/serialization/spec/protocol.html , java.lang.String is the only one that doesn't have a ClassDesc. So primitive types and String are always whitelisted; all other types (including arrays and boxed variants of primitives types) need to be whitelisted (either through a package java.lang.* or individually) to allow deserializing all the transitive fields of all the objects needed to deserialize the top object. Cheers, Jon
          Hide
          tn Thomas Neidhart added a comment -

          btw. some observations from a few tests that I made:

          java.lang.String will not be resolved

          When whitelisting a class, also all serializable super-classes must be whitelisted.

          Show
          tn Thomas Neidhart added a comment - btw. some observations from a few tests that I made: java.lang.String will not be resolved When whitelisting a class, also all serializable super-classes must be whitelisted.
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          Or create static ClassNameMatcher members for common class categories. The ClassNameMatcher implementations are immutable.

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - Or create static ClassNameMatcher members for common class categories. The ClassNameMatcher implementations are immutable.
          Hide
          chris@christopherschultz.net Christopher Schultz added a comment -

          Instantiating the java.lang.Class object for a class is probably not terribly risky, but there are certainly scenarios where untrusted classes could be loaded... if their static initializers are run, there is an opportunity for Bad Things to happen.

          But if you were worried about such a thing, you'd use a ClassNameMatcher instead.

          To improve performance, one could keep a lookup table of className -> java.lang.Class that you update only when the class name is acceptable. That would allow you to safely perform type-checking in a ClassMatcher, but only under certain conditions.

          For instance, let's say that I am willing to allow java.util.List and anything that implements that interface (dangerous, but illustrative). If I have a com.foo.SpecialList, the only way to check to see whether com.foo.SpecialList will be acceptable is to check the class hierarchy to see if it implements that interface (or any others registered, of course). I don't see a way around this unless you want to use commons-bcel to inspect .class files without formally-loading them into the ClassLoader and risking the execution of their static initializers.

          Without something like a ClassMatcher, it will often be very difficult to specify every possible class that you might want to allow for deserialization.

          Show
          chris@christopherschultz.net Christopher Schultz added a comment - Instantiating the java.lang.Class object for a class is probably not terribly risky, but there are certainly scenarios where untrusted classes could be loaded... if their static initializers are run, there is an opportunity for Bad Things to happen. But if you were worried about such a thing, you'd use a ClassNameMatcher instead. To improve performance, one could keep a lookup table of className -> java.lang.Class that you update only when the class name is acceptable. That would allow you to safely perform type-checking in a ClassMatcher, but only under certain conditions. For instance, let's say that I am willing to allow java.util.List and anything that implements that interface (dangerous, but illustrative). If I have a com.foo.SpecialList, the only way to check to see whether com.foo.SpecialList will be acceptable is to check the class hierarchy to see if it implements that interface (or any others registered, of course). I don't see a way around this unless you want to use commons-bcel to inspect .class files without formally-loading them into the ClassLoader and risking the execution of their static initializers. Without something like a ClassMatcher, it will often be very difficult to specify every possible class that you might want to allow for deserialization.
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          Regarding the various usability suggestions I think those are good ideas. OTOH the code in its current form might be good enough for a first release which will help gather feedback from users. I'll be busy with other things myself for the next ten days or so, don't wait for me!

          Show
          bdelacretaz Bertrand Delacretaz added a comment - Regarding the various usability suggestions I think those are good ideas. OTOH the code in its current form might be good enough for a first release which will help gather feedback from users. I'll be busy with other things myself for the next ten days or so, don't wait for me!
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          To match against Class objects you'd need to instantiate those Class objects based on the ObjectStreamClass that's passed to the ObjectInputStream.resolveClass method. You'd then have to be very careful not to initialize any unwanted classes, as that might execute arbitrary code.

          It's probably easier to keep things safe when working via class names only, for data that comes from the outside.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - To match against Class objects you'd need to instantiate those Class objects based on the ObjectStreamClass that's passed to the ObjectInputStream.resolveClass method. You'd then have to be very careful not to initialize any unwanted classes, as that might execute arbitrary code. It's probably easier to keep things safe when working via class names only, for data that comes from the outside.
          Hide
          ebourg Emmanuel Bourg added a comment -

          Another usability suggestion: if the type T is trusted, then T[] should be automatically trusted without calling accept("[LT").

          Show
          ebourg Emmanuel Bourg added a comment - Another usability suggestion: if the type T is trusted, then T[] should be automatically trusted without calling accept("[LT") .
          Hide
          tn Thomas Neidhart added a comment -

          The ClassNameMatcher as it is now implemented is quite easy to use, but it would be more powerful if we would have a ClassMatcher interface that would match on Class instances rather than on plain strings.
          This would allow some checks that are not really possible right now, like accept or reject any sub-class of an interface.

          Show
          tn Thomas Neidhart added a comment - The ClassNameMatcher as it is now implemented is quite easy to use, but it would be more powerful if we would have a ClassMatcher interface that would match on Class instances rather than on plain strings. This would allow some checks that are not really possible right now, like accept or reject any sub-class of an interface.
          Hide
          ebourg Emmanuel Bourg added a comment -

          Another idea we could consider, if trusting some packages or classes by default isn't desirable we could provide one or several preconfigured instances of ValidatingObjectInputStream. For example ValidatingObjectInputStream.DEFAULT would provide an implementation accepting basic types (java.lang.*, Date, URL, etc). ValidatingObjectInputStream.ALL would accept everything and would then be restricted with reject() calls.

          The preconfigured instances can either be provided as static fields (ValidatingObjectInputStream will have to become immutable similarly to CSVFormat) or by static methods.

          Show
          ebourg Emmanuel Bourg added a comment - Another idea we could consider, if trusting some packages or classes by default isn't desirable we could provide one or several preconfigured instances of ValidatingObjectInputStream. For example ValidatingObjectInputStream.DEFAULT would provide an implementation accepting basic types (java.lang.*, Date, URL, etc). ValidatingObjectInputStream.ALL would accept everything and would then be restricted with reject() calls. The preconfigured instances can either be provided as static fields ( ValidatingObjectInputStream will have to become immutable similarly to CSVFormat ) or by static methods.
          Hide
          ebourg Emmanuel Bourg added a comment -

          What about trusting java.lang by default?

          Show
          ebourg Emmanuel Bourg added a comment - What about trusting java.lang by default?
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          If you have to declare any accepted class, you might be surprised how many of it you're actually using (Joerg Schaible) .

          Indeed. I have added a MoreComplexObjectTest [1] which demonstrates this, using 3 variants: trust java.lang packages, trust all java packages, and a blacklist-only mode.

          The "trust java" variant is not too bad:

          new ValidatingObjectInputStream(inputStream)
            .accept(MoreComplexObject.class)
            .accept("java.*","[Ljava.*")
          

          But of course it depends on one's concrete cases.

          [1] https://svn.apache.org/repos/asf/commons/proper/io/trunk/src/test/java/org/apache/commons/io/serialization/MoreComplexObjectTest.java

          Show
          bdelacretaz Bertrand Delacretaz added a comment - If you have to declare any accepted class, you might be surprised how many of it you're actually using ( Joerg Schaible ) . Indeed. I have added a MoreComplexObjectTest [1] which demonstrates this, using 3 variants: trust java.lang packages, trust all java packages, and a blacklist-only mode. The "trust java" variant is not too bad: new ValidatingObjectInputStream(inputStream) .accept(MoreComplexObject.class) .accept( "java.*" , "[Ljava.*" ) But of course it depends on one's concrete cases. [1] https://svn.apache.org/repos/asf/commons/proper/io/trunk/src/test/java/org/apache/commons/io/serialization/MoreComplexObjectTest.java
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -
          Show
          bdelacretaz Bertrand Delacretaz added a comment - Done, http://svn.apache.org/r1715240
          Hide
          krosenvold Kristian Rosenvold added a comment -

          Yes please !

          Show
          krosenvold Kristian Rosenvold added a comment - Yes please !
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          Ran the Cobertura coverage with "mvn site", target/site/cobertura/index.html reports 100% coverage for the org.apache.commons.io.serialization package.

          Should I add an item to src/changes/changes.xml myself, or how does that work?

          Show
          bdelacretaz Bertrand Delacretaz added a comment - Ran the Cobertura coverage with "mvn site", target/site/cobertura/index.html reports 100% coverage for the org.apache.commons.io.serialization package. Should I add an item to src/changes/changes.xml myself, or how does that work?
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          Added the class name in the InvalidClassException, as discussed above - Adrian Crum is right that it doesn't provided any additional info to a potential attacker. http://svn.apache.org/r1715221

          Show
          bdelacretaz Bertrand Delacretaz added a comment - Added the class name in the InvalidClassException, as discussed above - Adrian Crum is right that it doesn't provided any additional info to a potential attacker. http://svn.apache.org/r1715221
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          If I try to exploit code by desrializing MyExploit.class, and the exception says "Class 'MyExploit' not accepted" - what information have I gained?

          None indeed, you are absolutely right. I will modify ValidatingObjectInputStreamTest to include the class name.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - If I try to exploit code by desrializing MyExploit.class, and the exception says "Class 'MyExploit' not accepted" - what information have I gained? None indeed, you are absolutely right. I will modify ValidatingObjectInputStreamTest to include the class name.
          Hide
          bdelacretaz Bertrand Delacretaz added a comment - - edited

          I have committed IO-487-accept-reject-2.patch with minor javadoc changes as http://svn.apache.org/r1715217

          Show
          bdelacretaz Bertrand Delacretaz added a comment - - edited I have committed IO-487 -accept-reject-2.patch with minor javadoc changes as http://svn.apache.org/r1715217
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          Without the class name, the exception is not useful to the developer. What information is being disclosed to an attacker? If I try to exploit code by desrializing MyExploit.class, and the exception says "Class 'MyExploit' not accepted" - what information have I gained?

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - Without the class name, the exception is not useful to the developer. What information is being disclosed to an attacker? If I try to exploit code by desrializing MyExploit.class, and the exception says "Class 'MyExploit' not accepted" - what information have I gained?
          Hide
          ebourg Emmanuel Bourg added a comment -

          The name isn't included on purpose to avoid disclosing too much information to an attacker.

          That said, we could display the name only when a debug mode is enabled.

          Show
          ebourg Emmanuel Bourg added a comment - The name isn't included on purpose to avoid disclosing too much information to an attacker. That said, we could display the name only when a debug mode is enabled.
          Hide
          niallp Niall Pemberton added a comment -

          Go for it - looks good to me, the only minor comment I have is, can you include the className in the exception message:

          throw new InvalidClassException("Class '" + className + "' not accepted");

          Show
          niallp Niall Pemberton added a comment - Go for it - looks good to me, the only minor comment I have is, can you include the className in the exception message: throw new InvalidClassException("Class '" + className + "' not accepted");
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          at least you spelled it right, that's no so common

          Show
          bdelacretaz Bertrand Delacretaz added a comment - at least you spelled it right, that's no so common
          Hide
          garydgregory Gary Gregory added a comment -

          This is also DelacretazObjectInputStream ...

          Show
          garydgregory Gary Gregory added a comment - This is also DelacretazObjectInputStream ...
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          RestrictedObjectInputStream maybe, but ValidatingObjectInputStream works for me.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - RestrictedObjectInputStream maybe, but ValidatingObjectInputStream works for me.
          Hide
          chris@christopherschultz.net Christopher Schultz added a comment -

          I would suggest Filter[ing]ObjectInputStream, except it doesn't actually /filter/... it merely ... objects to things.

          RestrictingObjectInputStream? VetoingObjectInputStream? SentinelObjectInputStream? PrudishObjectInputStream?

          Show
          chris@christopherschultz.net Christopher Schultz added a comment - I would suggest Filter [ing] ObjectInputStream, except it doesn't actually /filter/... it merely ... objects to things. RestrictingObjectInputStream? VetoingObjectInputStream? SentinelObjectInputStream? PrudishObjectInputStream?
          Hide
          garydgregory Gary Gregory added a comment -

          I like ValidatingObjectInputStream for the name.

          Show
          garydgregory Gary Gregory added a comment - I like ValidatingObjectInputStream for the name.
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          if nobody objects you can even do it yourself since the repository is open to all Apache committers.

          Ok, I'll wait about 24 hours in case there are objections.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - if nobody objects you can even do it yourself since the repository is open to all Apache committers. Ok, I'll wait about 24 hours in case there are objections.
          Hide
          ebourg Emmanuel Bourg added a comment -

          Its looks ready to be committed to me, and if nobody objects you can even do it yourself since the repository is open to all Apache committers.

          As for the name I don't know. SanitizingObjectInputStream maybe? SafeObjectInputStream wasn't that bad.

          Show
          ebourg Emmanuel Bourg added a comment - Its looks ready to be committed to me, and if nobody objects you can even do it yourself since the repository is open to all Apache committers. As for the name I don't know. SanitizingObjectInputStream maybe? SafeObjectInputStream wasn't that bad.
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          Here's an updated IO-487-accept-reject-2.patch that adds a protected invalidClassNameFound method to ValidatingObjectInputStream, as suggested by Emmanuel Bourg. That method could be overridden to log invalid classes instead of failing, and it also includes the comment about not logging the invalid class name.

          Do you guys think this can be committed? I guess what's important is to agree on the API-like elements which are only the ClassNameMatcher interface and the public/protected methods of ValidatingObjectInputStream.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - Here's an updated IO-487 -accept-reject-2.patch that adds a protected invalidClassNameFound method to ValidatingObjectInputStream , as suggested by Emmanuel Bourg . That method could be overridden to log invalid classes instead of failing, and it also includes the comment about not logging the invalid class name. Do you guys think this can be committed? I guess what's important is to agree on the API-like elements which are only the ClassNameMatcher interface and the public/protected methods of ValidatingObjectInputStream .
          Hide
          ebourg Emmanuel Bourg added a comment -

          Or move the throw InvalidClassException to a protected method that can be overridden to log the rejected classes.

          Show
          ebourg Emmanuel Bourg added a comment - Or move the throw InvalidClassException to a protected method that can be overridden to log the rejected classes.
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          For that you can write a ClassNameMatcher that accepts everything and logs names.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - For that you can write a ClassNameMatcher that accepts everything and logs names.
          Hide
          britter Benedikt Ritter added a comment -

          The general design philosophy at commons is, that our components don't write to a log since they are so low level (there are exceptions from this rule however...).

          Show
          britter Benedikt Ritter added a comment - The general design philosophy at commons is, that our components don't write to a log since they are so low level (there are exceptions from this rule however...).
          Hide
          chris@christopherschultz.net Christopher Schultz added a comment -

          I made a suggestion on the tomcat-user mailing list where we have been discussing the same thing, and are likely to use your implementation once it's complete: allow for the [Whatever]InputStream to be put into a mode where it merely reports (via log @ INFO level) which classes would have been rejected. This will allow a developer to run in this mode to ensure that there aren't any classes being used that are expected to be deserialized during legitimate uses of the application, but aren't matching the currently-configured "accept" criteria.

          Yes, this can be done by watching for UnsupportedOperationException/InvalidClassException, but it will require the developer to re-build and re-try many times to get all of the various classes taken care of. With this feature, someone could enable the logging, run the application normally, and end up with a complete list of classes that need to be "allowed" by grepping the log file.

          Show
          chris@christopherschultz.net Christopher Schultz added a comment - I made a suggestion on the tomcat-user mailing list where we have been discussing the same thing, and are likely to use your implementation once it's complete: allow for the [Whatever] InputStream to be put into a mode where it merely reports (via log @ INFO level) which classes would have been rejected. This will allow a developer to run in this mode to ensure that there aren't any classes being used that are expected to be deserialized during legitimate uses of the application, but aren't matching the currently-configured "accept" criteria. Yes, this can be done by watching for UnsupportedOperationException/InvalidClassException, but it will require the developer to re-build and re-try many times to get all of the various classes taken care of. With this feature, someone could enable the logging, run the application normally, and end up with a complete list of classes that need to be "allowed" by grepping the log file.
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          The IO-487-accept-reject.patch uses a different and much simpler (IMO) API that makes it more foolproof, so I would much prefer that variant.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - The IO-487 -accept-reject.patch uses a different and much simpler (IMO) API that makes it more foolproof, so I would much prefer that variant.
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          Updated patch with Niall's changes.

          The biggest problem I see with this issue is we have multiple contributers working on different versions of the patches. There needs to be better coordination.

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - Updated patch with Niall's changes. The biggest problem I see with this issue is we have multiple contributers working on different versions of the patches. There needs to be better coordination.
          Hide
          garydgregory Gary Gregory added a comment -

          Right, accept(MyClass.class) is fine as long as we translate that to Class#getName() internally.

          Show
          garydgregory Gary Gregory added a comment - Right, accept(MyClass.class) is fine as long as we translate that to Class#getName() internally.
          Hide
          bdelacretaz Bertrand Delacretaz added a comment - - edited

          You mean in methods like accept(MyClass.class) ?

          One can always use accept(MyClass.class.getName()) instead, using the wildcard syntax. And internally we use the FQCN anyway, so not sure if that's a problem. As you're going to deserialize those classes they need to be loaded anyway.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - - edited You mean in methods like accept(MyClass.class) ? One can always use accept(MyClass.class.getName()) instead, using the wildcard syntax. And internally we use the FQCN anyway, so not sure if that's a problem. As you're going to deserialize those classes they need to be loaded anyway.
          Hide
          garydgregory Gary Gregory added a comment -

          I would be careful using class objects in the API, because of class loader issues. Internally, we should always use FQ class names, right?

          Show
          garydgregory Gary Gregory added a comment - I would be careful using class objects in the API, because of class loader issues. Internally, we should always use FQ class names, right?
          Hide
          ebourg Emmanuel Bourg added a comment -

          The base types should be accepted by default I think (primitive wrappers, arrays, enums, String, Date, URL, File...).

          Accepting a hierarchy is a good idea, something like acceptInstancesOf() maybe? On the other hand, instead of having a proliferation of methods we could rely on the Java 8 syntax and write accept(c -> List.class.isAssignableFrom(c)).

          Show
          ebourg Emmanuel Bourg added a comment - The base types should be accepted by default I think (primitive wrappers, arrays, enums, String, Date, URL, File...). Accepting a hierarchy is a good idea, something like acceptInstancesOf() maybe? On the other hand, instead of having a proliferation of methods we could rely on the Java 8 syntax and write accept(c -> List.class.isAssignableFrom(c)) .
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          Makes sense, we could provide a set of standard ClassNameMatchers along those lines.

          Best might be to add a few tests that demonstrate those needs, so we can create some standard matchers. My own use cases are very limited in terms of class space, so if others have good examples they're welcome.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - Makes sense, we could provide a set of standard ClassNameMatchers along those lines. Best might be to add a few tests that demonstrate those needs, so we can create some standard matchers. My own use cases are very limited in terms of class space, so if others have good examples they're welcome.
          Hide
          ebourg Emmanuel Bourg added a comment -

          Ok understood I didn't parse the method properly. All classes are rejected by default, and reject() specifies exceptions to what was accept()ed. The javadoc of the accept/reject methods is clear, a few examples in the class javadoc would be good though.

          Show
          ebourg Emmanuel Bourg added a comment - Ok understood I didn't parse the method properly. All classes are rejected by default, and reject() specifies exceptions to what was accept()ed. The javadoc of the accept/reject methods is clear, a few examples in the class javadoc would be good though.
          Hide
          joehni Joerg Schaible added a comment -

          If you have to declare any accepted class, you might be surprised how many of it you're actually using. It might be good to support ClassNameMatcher implementations that accept:

          • the primitive types and their boxed variants
          • very common types like String, UUID, Date, File
          • any array type if the base type is also allowed
          • a class hierarchy (e.g. List and derived)
          • proxy types
          • (special) lambda types
          Show
          joehni Joerg Schaible added a comment - If you have to declare any accepted class, you might be surprised how many of it you're actually using. It might be good to support ClassNameMatcher implementations that accept: the primitive types and their boxed variants very common types like String, UUID, Date, File any array type if the base type is also allowed a class hierarchy (e.g. List and derived) proxy types (special) lambda types
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          ...any class is rejected unless it's explicitly accepted. Calling reject() has no real effect on the end result.

          I think the IO-487-accept-reject.patch logic makes sense considering the use of wildcard matchers:

          By default nothing is accepted, to force users to think about what they accept. If you don't call accept, nothing works and I think it's good. This allows you to just tell your developers to use ValidatingObjectInputStream and they'll discover the rest by themselves.

          The most common case (and recommended IMO) is probably to just accept a single or a few classes, like accept(MyClass.class) or accept(org.mycompany.safestuff.*). This is a pure whitelisting mode.

          If you want a whitelist with a few exceptions you can do something like

          accept("org.*").reject("org.badguys.*")

          Such a wide accept pattern is not recommended but works if you know what you're doing. And the order of the accept/reject calls is not important, reject always wins which is good for security.

          If you want to use a standard blacklist on top of whatever you accept, just to be on the safe side, you can require your users to always call reject(YOUR_STANDARD_BLACKLIST)

          Does this make sense?

          Show
          bdelacretaz Bertrand Delacretaz added a comment - ...any class is rejected unless it's explicitly accepted. Calling reject() has no real effect on the end result. I think the IO-487 -accept-reject.patch logic makes sense considering the use of wildcard matchers: By default nothing is accepted, to force users to think about what they accept. If you don't call accept , nothing works and I think it's good. This allows you to just tell your developers to use ValidatingObjectInputStream and they'll discover the rest by themselves. The most common case (and recommended IMO) is probably to just accept a single or a few classes, like accept(MyClass.class) or accept(org.mycompany.safestuff.*) . This is a pure whitelisting mode. If you want a whitelist with a few exceptions you can do something like accept("org.*").reject("org.badguys.*") Such a wide accept pattern is not recommended but works if you know what you're doing. And the order of the accept/reject calls is not important, reject always wins which is good for security. If you want to use a standard blacklist on top of whatever you accept, just to be on the safe side, you can require your users to always call reject(YOUR_STANDARD_BLACKLIST) Does this make sense?
          Hide
          ebourg Emmanuel Bourg added a comment -

          I intentionally didn't do that as security folks sometimes complain that these sorts of things disclose too much information to an attacker.

          Ok, in this case I suggest adding a comment explaining this choice, otherwise someone may be tempted to change this later without knowing.

          Show
          ebourg Emmanuel Bourg added a comment - I intentionally didn't do that as security folks sometimes complain that these sorts of things disclose too much information to an attacker. Ok, in this case I suggest adding a comment explaining this choice, otherwise someone may be tempted to change this later without knowing.
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          I'd suggest adding the name of the class rejected to the InvalidClassException

          I intentionally didn't do that as security folks sometimes complain that these sorts of things disclose too much information to an attacker. If adding the name is fine with the usual Commons best practices I'm fine with that.

          I'll look at your other comments later today, hopefully.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - I'd suggest adding the name of the class rejected to the InvalidClassException I intentionally didn't do that as security folks sometimes complain that these sorts of things disclose too much information to an attacker. If adding the name is fine with the usual Commons best practices I'm fine with that. I'll look at your other comments later today, hopefully.
          Hide
          ebourg Emmanuel Bourg added a comment -

          The API looks good to me. I'd suggest adding the name of the class rejected to the InvalidClassException (there is a constructor for that).

          I have one question regarding the accept/reject logic though. If I read the validateClassName method properly, any class is rejected unless it's explicitly accepted. Calling reject() has no real effect on the end result. The logic should be adjusted a bit I think, I'm not sure but maybe something like this:

          • if reject is called but not accept, accept everything but the classes rejected
          • if accept is called but not reject, reject everything but the classes accepted
          • if both accept and reject are called, reject everything but the classes accepted (it sounds safer this way)
          Show
          ebourg Emmanuel Bourg added a comment - The API looks good to me. I'd suggest adding the name of the class rejected to the InvalidClassException (there is a constructor for that). I have one question regarding the accept/reject logic though. If I read the validateClassName method properly, any class is rejected unless it's explicitly accepted. Calling reject() has no real effect on the end result. The logic should be adjusted a bit I think, I'm not sure but maybe something like this: if reject is called but not accept, accept everything but the classes rejected if accept is called but not reject, reject everything but the classes accepted if both accept and reject are called, reject everything but the classes accepted (it sounds safer this way)
          Hide
          bdelacretaz Bertrand Delacretaz added a comment - - edited

          Here's IO-487-accept-reject.patch with the suggested accept/reject syntax (also at https://github.com/bdelacretaz/commons-io/tree/IO-487)

          ValidatingObjectInputStreamTest has a number of examples.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - - edited Here's IO-487 -accept-reject.patch with the suggested accept/reject syntax (also at https://github.com/bdelacretaz/commons-io/tree/IO-487 ) ValidatingObjectInputStreamTest has a number of examples.
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          ...we can reuse FilenameUtils.wildcardMatch(String, String)...

          Ah great, I'll implement the accept/reject variant now.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - ...we can reuse FilenameUtils.wildcardMatch(String, String)... Ah great, I'll implement the accept/reject variant now.
          Hide
          ebourg Emmanuel Bourg added a comment -

          For the simplified pattern syntax we can reuse FilenameUtils.wildcardMatch(String, String), also in commons-io. The accept(String) method would not accept a Perl regexp to keep things simple.

          Show
          ebourg Emmanuel Bourg added a comment - For the simplified pattern syntax we can reuse FilenameUtils.wildcardMatch(String, String) , also in commons-io. The accept(String) method would not accept a Perl regexp to keep things simple.
          Hide
          ebourg Emmanuel Bourg added a comment -

          accept/reject is even better (I'd prefer accept(Pattern) though).

          So the API would look like this:

          accept(String...)
          accept(Class...)
          accept(Pattern)
          accept(ClassNameMatcher)
          
          reject(String...)
          reject(Class...)
          reject(Pattern)
          reject(ClassNameMatcher)
          

          and the implementations of ClassNameMatcher remain package private.

          Show
          ebourg Emmanuel Bourg added a comment - accept/reject is even better (I'd prefer accept(Pattern) though). So the API would look like this: accept( String ...) accept( Class ...) accept(Pattern) accept(ClassNameMatcher) reject( String ...) reject( Class ...) reject(Pattern) reject(ClassNameMatcher) and the implementations of ClassNameMatcher remain package private.
          Hide
          sebb@apache.org Sebb added a comment -

          Wildcard matching such as withClass("com.bar.Bar*") uses a syntax which AFAIK is not directly supported by Java or Commons.

          Implementation will require analysis of the parameter in order to either convert it to a Java regex or to directly implement the new search syntax.
          This is likely to be non-trivial.
          Also the syntax will need to be clearly documented and tested.

          The other fixed parameter syntaxes look OK.

          Show
          sebb@apache.org Sebb added a comment - Wildcard matching such as withClass("com.bar.Bar*") uses a syntax which AFAIK is not directly supported by Java or Commons. Implementation will require analysis of the parameter in order to either convert it to a Java regex or to directly implement the new search syntax. This is likely to be non-trivial. Also the syntax will need to be clearly documented and tested. The other fixed parameter syntaxes look OK.
          Hide
          bdelacretaz Bertrand Delacretaz added a comment - - edited

          Or maybe

          ObjectInputStream ois = 
            new ValidatingObjectInputStream(is)
            .accept(com.foo.Foo.class, Integer.class)
            .accept("com.bar.Bar*")
            .reject("com.baz.*")
          

          You'd need to process those simplified regex but considering the conventions on class names it should be sufficient to map dots to \. and stars to .*

          And also include acceptPattern(Pattern p) and rejectPattern(Pattern p) for edge cases. Or maybe better, accept(ClassNameMatcher m) and reject(ClassNameMatcher m)

          Show
          bdelacretaz Bertrand Delacretaz added a comment - - edited Or maybe ObjectInputStream ois = new ValidatingObjectInputStream(is) .accept(com.foo.Foo.class, Integer .class) .accept( "com.bar.Bar*" ) .reject( "com.baz.*" ) You'd need to process those simplified regex but considering the conventions on class names it should be sufficient to map dots to \. and stars to .* And also include acceptPattern(Pattern p) and rejectPattern(Pattern p) for edge cases. Or maybe better, accept(ClassNameMatcher m) and reject(ClassNameMatcher m)
          Hide
          ebourg Emmanuel Bourg added a comment -

          What about an even simpler syntax like:

          ObjectInputStream ois = 
            new ValidatingObjectInputStream(is)
            .withClass(com.foo.Foo.class)
            .withClass("com.bar.Bar*")
            .withoutPackage("com.baz")
          

          The various matcher are implementation details that could be hidden behind friendly named methods.

          Show
          ebourg Emmanuel Bourg added a comment - What about an even simpler syntax like: ObjectInputStream ois = new ValidatingObjectInputStream(is) .withClass(com.foo.Foo.class) .withClass( "com.bar.Bar*" ) .withoutPackage( "com.baz" ) The various matcher are implementation details that could be hidden behind friendly named methods.
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          Based on all those great ideas, here's a variant (IO-487-matchers.patch) that I find simpler and more foolproof to use, the single-class setup code is now

          ObjectInputStream ois = 
            new ValidatingObjectInputStream(is)
            .withWhitelist(new FullClassNameMatcher(MyClass.class.getName()))
          

          And allowing a full package except for a specific class would be

          ObjectInputStream ois = 
            new ValidatingObjectInputStream(is)
            .withWhitelist(new RegexClassNameMatcher("com\\.example\\.foo.*"),
            .withBlacklist(com.example.foo.SomeBadClass.class.getName())
          

          Someone said they prefer include/exclude instead of black/whitelists. I don't mind, it's just that the latter are common terms in security discussions.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - Based on all those great ideas, here's a variant ( IO-487 -matchers.patch) that I find simpler and more foolproof to use, the single-class setup code is now ObjectInputStream ois = new ValidatingObjectInputStream(is) .withWhitelist( new FullClassNameMatcher(MyClass.class.getName())) And allowing a full package except for a specific class would be ObjectInputStream ois = new ValidatingObjectInputStream(is) .withWhitelist( new RegexClassNameMatcher( "com\\.example\\.foo.*" ), .withBlacklist(com.example.foo.SomeBadClass.class.getName()) Someone said they prefer include/exclude instead of black/whitelists. I don't mind, it's just that the latter are common terms in security discussions.
          Hide
          joehni Joerg Schaible added a comment -

          Please use the InvalidClassException with a proper reason (e.g. "security restrictions: class rejected"). We had to detect that even recent jBoss releases start to behave very badly if the object serialization is broken in an unexpected way (we managed to throw a NPE). A restart of jBoss was actually the only way to solve the issue until the next NPE happened. This might apply to other app servers, too.

          Show
          joehni Joerg Schaible added a comment - Please use the InvalidClassException with a proper reason (e.g. "security restrictions: class rejected"). We had to detect that even recent jBoss releases start to behave very badly if the object serialization is broken in an unexpected way (we managed to throw a NPE). A restart of jBoss was actually the only way to solve the issue until the next NPE happened. This might apply to other app servers, too.
          Hide
          krosenvold Kristian Rosenvold added a comment -

          IntersectionClassAcceptor: Does a "Set" of acceptors make any sense when the ClassAcceptors dont implement equals/hashcode ?

          I'm wondering if this should really be moved to the "org.apache.commons.io.input" package, possibly with the acceptors in a sub-package ? (I'm discussing this with myself kind-of. But I think so....)

          Show
          krosenvold Kristian Rosenvold added a comment - IntersectionClassAcceptor: Does a "Set" of acceptors make any sense when the ClassAcceptors dont implement equals/hashcode ? I'm wondering if this should really be moved to the "org.apache.commons.io.input" package, possibly with the acceptors in a sub-package ? (I'm discussing this with myself kind-of. But I think so....)
          Hide
          niallp Niall Pemberton added a comment -

          Attaching IO-487-2.patch which includes the following:

          • Change the ClassAcceptor's accept() method to return a boolean
          • RegexpClassAcceptor (in place of WhiteRegexpClassAcceptor & BlackRegexpClassAcceptor)
          • NameClassAcceptor (in place of WhitelistClassAcceptor & BlacklistClassAcceptor)
          • add NotClassAcceptor (to provide blacklist/exclusion functionality of above implementations)
          Show
          niallp Niall Pemberton added a comment - Attaching IO-487 -2.patch which includes the following: Change the ClassAcceptor's accept() method to return a boolean RegexpClassAcceptor (in place of WhiteRegexpClassAcceptor & BlackRegexpClassAcceptor) NameClassAcceptor (in place of WhitelistClassAcceptor & BlacklistClassAcceptor) add NotClassAcceptor (to provide blacklist/exclusion functionality of above implementations)
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          That sounds great! I will make those changes and wrap this up tomorrow.

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - That sounds great! I will make those changes and wrap this up tomorrow.
          Hide
          niallp Niall Pemberton added a comment -

          Last comment! I think the ClassAcceptor's accept() method should be changed to return a boolean, and move the exception to be thrown to the resolveClass() method of the ObjectInputStream.

          I think that would make ClassAcceptor more re-usable in its own right outside of this specific use-case. It would also make composing composite ClassAcceptors from other implementations easier to implement.

          Show
          niallp Niall Pemberton added a comment - Last comment! I think the ClassAcceptor's accept() method should be changed to return a boolean, and move the exception to be thrown to the resolveClass() method of the ObjectInputStream. I think that would make ClassAcceptor more re-usable in its own right outside of this specific use-case. It would also make composing composite ClassAcceptors from other implementations easier to implement.
          Hide
          niallp Niall Pemberton added a comment -

          OK, makes sense.

          Show
          niallp Niall Pemberton added a comment - OK, makes sense.
          Hide
          niallp Niall Pemberton added a comment -

          No, thats not true - IntersectionClassAcceptor is something different from what I was suggesting. Thinking about it though, probably just providing a "NotClassAcceptor" implementation which reverses the effect of a delegate ClassAcceptor that its constructed with would provide this for any ClassAcceptor and then no need for the separate included/exclude (blacklist/whitelist) implementations.

          Show
          niallp Niall Pemberton added a comment - No, thats not true - IntersectionClassAcceptor is something different from what I was suggesting. Thinking about it though, probably just providing a "NotClassAcceptor" implementation which reverses the effect of a delegate ClassAcceptor that its constructed with would provide this for any ClassAcceptor and then no need for the separate included/exclude (blacklist/whitelist) implementations.
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          Latest patch with the new class names. I'm still working on the unit tests. I can't get Cobertura to work with maven, so I'm trying to get it to work with ant.

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - Latest patch with the new class names. I'm still working on the unit tests. I can't get Cobertura to work with maven, so I'm trying to get it to work with ant.
          Hide
          garydgregory Gary Gregory added a comment -

          I like UnsupportedOperationException better FWIW.

          Show
          garydgregory Gary Gregory added a comment - I like UnsupportedOperationException better FWIW.
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          Agreed, but I think UnsupportedOperationException describes things better. InvalidClassException implies that the class to be deserialized can't be found, while UnsupportedOperationException makes it clear that class deserialization is not supported (or allowed).

          Either one is fine with me.

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - Agreed, but I think UnsupportedOperationException describes things better. InvalidClassException implies that the class to be deserialized can't be found, while UnsupportedOperationException makes it clear that class deserialization is not supported (or allowed). Either one is fine with me.
          Hide
          niallp Niall Pemberton added a comment -

          Also an alternative to throwing UnsupportedOperationException would be to throw InvalidClassException (which is an ObjectStreamException)

          Show
          niallp Niall Pemberton added a comment - Also an alternative to throwing UnsupportedOperationException would be to throw InvalidClassException (which is an ObjectStreamException)
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          Niall - That is what the IntersectionClassAcceptor is used for.

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - Niall - That is what the IntersectionClassAcceptor is used for.
          Hide
          niallp Niall Pemberton added a comment -

          An alternative would be for single RegexClassAcceptor & NameClassAcceptor implementations that do both include & exclude. Attaching patch containing implementations.

          Show
          niallp Niall Pemberton added a comment - An alternative would be for single RegexClassAcceptor & NameClassAcceptor implementations that do both include & exclude. Attaching patch containing implementations.
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          I agree Include/Exclude sounds better.

          I will spend some time building out the unit tests, then provide an updated patch.

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - I agree Include/Exclude sounds better. I will spend some time building out the unit tests, then provide an updated patch.
          Hide
          niallp Niall Pemberton added a comment -

          Personally I prefer SafeObjectInputStream over Validating....
          Also would prefer Exclude/Include rather than Black/White

          Show
          niallp Niall Pemberton added a comment - Personally I prefer SafeObjectInputStream over Validating.... Also would prefer Exclude/Include rather than Black/White
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          RestrictedObjectInputStream?

          Show
          bdelacretaz Bertrand Delacretaz added a comment - RestrictedObjectInputStream?
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          RestrictedObjectInputStream?

          Show
          bdelacretaz Bertrand Delacretaz added a comment - RestrictedObjectInputStream?
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          I just noticed my patch utility is creating duplicates within the patch. Let me know if you have any problems applying the patch and I will fix it.

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - I just noticed my patch utility is creating duplicates within the patch. Let me know if you have any problems applying the patch and I will fix it.
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          Updated patch that includes the suggestions made so far.

          I think the latest API is flexible and easy to use.

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - Updated patch that includes the suggestions made so far. I think the latest API is flexible and easy to use.
          Hide
          garydgregory Gary Gregory added a comment -

          This is clearly an IO class.

          Show
          garydgregory Gary Gregory added a comment - This is clearly an IO class.
          Hide
          chris@christopherschultz.net Christopher Schultz added a comment -

          I'm no expert in "commons land", but would this class be better-placed into commons-lang? I can see reasonable arguments for either commons-lang or commons-io.

          Show
          chris@christopherschultz.net Christopher Schultz added a comment - I'm no expert in "commons land", but would this class be better-placed into commons-lang? I can see reasonable arguments for either commons-lang or commons-io.
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          Agreed.

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - Agreed.
          Hide
          chris@christopherschultz.net Christopher Schultz added a comment -

          RegexpClassAcceptor could be simpler by reducing the whitelists and blacklists to a single Pattern for each: anyone can use | in a regular expression to support many patterns.

          Show
          chris@christopherschultz.net Christopher Schultz added a comment - RegexpClassAcceptor could be simpler by reducing the whitelists and blacklists to a single Pattern for each: anyone can use | in a regular expression to support many patterns.
          Hide
          garydgregory Gary Gregory added a comment -

          Ah, OK, thank you for the update. Since are still kibitzing, then I'd like to open the class name up for debate. To me "SafeObjectInputStream" leads me to ask "safe from what?" and "How safe is it really?" Perhaps a name focused on the "doing" rather than the "intent" would be better. Maybe "AcceptorObjectInputStream", "CheckedObjectInputStream", "CheckingObjectInputStream", "ValidatingObjectInputStream". None of these are great but so far I like "ValidatingObjectInputStream".

          Show
          garydgregory Gary Gregory added a comment - Ah, OK, thank you for the update. Since are still kibitzing, then I'd like to open the class name up for debate. To me "SafeObjectInputStream" leads me to ask "safe from what?" and "How safe is it really?" Perhaps a name focused on the "doing" rather than the "intent" would be better. Maybe "AcceptorObjectInputStream", "CheckedObjectInputStream", "CheckingObjectInputStream", "ValidatingObjectInputStream". None of these are great but so far I like "ValidatingObjectInputStream".
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          Gary - right now I think we are trying to settle on an API. If there is consensus, I would be happy to write unit tests.

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - Gary - right now I think we are trying to settle on an API. If there is consensus, I would be happy to write unit tests.
          Hide
          garydgregory Gary Gregory added a comment -

          Is someone wiling to look at code coverage reports with Jacoco and/or Cobertura to make sure as much of this new code is tested?

          Show
          garydgregory Gary Gregory added a comment - Is someone wiling to look at code coverage reports with Jacoco and/or Cobertura to make sure as much of this new code is tested?
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          You are welcome!

          Show
          bdelacretaz Bertrand Delacretaz added a comment - You are welcome!
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          Improved patch attached. Added missing @Overrides, added missing JavaDocs, added more ClassAcceptor implementations, added thread safety.

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - Improved patch attached. Added missing @Overrides, added missing JavaDocs, added more ClassAcceptor implementations, added thread safety.
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          If you don't mind, I would like to make a few improvements to your patch.

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - If you don't mind, I would like to make a few improvements to your patch.
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          Another update...just a comment change.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - Another update...just a comment change.
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          Here's an updated patch that uses UnsupportedOperationException, good idea.

          A package-based ClassAcceptor sounds like a good idea, don't have time to write this right now.

          I think RegexpClassAcceptor can be useful for code with a suboptimal package organization, but that could also be made optional and not included in the library, I don't know how much you want to minimize the size of commons-io.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - Here's an updated patch that uses UnsupportedOperationException, good idea. A package-based ClassAcceptor sounds like a good idea, don't have time to write this right now. I think RegexpClassAcceptor can be useful for code with a suboptimal package organization, but that could also be made optional and not included in the library, I don't know how much you want to minimize the size of commons-io.
          Hide
          adrianc@hlmksw.com Adrian Crum added a comment -

          Some suggestions:

          1. Use UnsupportedOperationException instead of custom exception class.
          2. Maybe use a package whitelist/blacklist instead of regular expressions (regexes seem like overkill to me).

          Show
          adrianc@hlmksw.com Adrian Crum added a comment - Some suggestions: 1. Use UnsupportedOperationException instead of custom exception class. 2. Maybe use a package whitelist/blacklist instead of regular expressions (regexes seem like overkill to me).
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          Forgot to mention good contributions from Alexander Klimetschek on that SLING-5288 code, please also credit him if you accept the patch (alexkli at a.o).

          Show
          bdelacretaz Bertrand Delacretaz added a comment - Forgot to mention good contributions from Alexander Klimetschek on that SLING-5288 code, please also credit him if you accept the patch (alexkli at a.o).

            People

            • Assignee:
              Unassigned
              Reporter:
              bdelacretaz Bertrand Delacretaz
            • Votes:
              0 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development