Re: [PR] reject non-SerializableObject classes in SerializableObject.readClass [lucene]
dweiss commented on PR #16138: URL: https://github.com/apache/lucene/pull/16138#issuecomment-4594905686 > The backport is failing errorprone checks in 10x (although not on main for some reason?) The code on 10x uses old(er) errorprone - this change ignores static volatile fields, so it passes on main. https://github.com/google/error-prone/commit/5d4c57deb9127eb7c87a1abd74d4511361c866a1 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] reject non-SerializableObject classes in SerializableObject.readClass [lucene]
uschindler commented on PR #16138: URL: https://github.com/apache/lucene/pull/16138#issuecomment-4591585730 I fixed the check already on #16158 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] reject non-SerializableObject classes in SerializableObject.readClass [lucene]
romseygeek commented on PR #16138: URL: https://github.com/apache/lucene/pull/16138#issuecomment-4591549930 The backport is failing errorprone checks in 10x (although not on main for some reason?) ``` > Task :lucene:spatial3d:compileTestJava FAILED -- | /opt/buildkite-agent/builds/bk-agent-prod-gcp-1780306213479883048/elastic/apache-lucene-periodic/lucene/spatial3d/src/test/org/apache/lucene/spatial3d/geom/TestSerializableObjectClassFilter.java:29: warning: [NonFinalStaticField] Static fields should almost always be final. | static volatile boolean nonSerializableConstructed = false, | ^ | (see https://errorprone.info/bugpattern/NonFinalStaticField) | /opt/buildkite-agent/builds/bk-agent-prod-gcp-1780306213479883048/elastic/apache-lucene-periodic/lucene/spatial3d/src/test/org/apache/lucene/spatial3d/geom/TestSerializableObjectClassFilter.java:30: warning: [NonFinalStaticField] Static fields should almost always be final. | nonSerializableClassInitialized = false; | ^ | (see https://errorprone.info/bugpattern/NonFinalStaticField) | error: warnings found and -Werror specified | 1 error | 2 warnings ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] reject non-SerializableObject classes in SerializableObject.readClass [lucene]
uschindler commented on PR #16138: URL: https://github.com/apache/lucene/pull/16138#issuecomment-4582855334 Backport done in https://github.com/apache/lucene/commit/f5d93f7ab575911dc98d470654e201351edec206 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] reject non-SerializableObject classes in SerializableObject.readClass [lucene]
uschindler merged PR #16138: URL: https://github.com/apache/lucene/pull/16138 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] reject non-SerializableObject classes in SerializableObject.readClass [lucene]
uschindler commented on PR #16138: URL: https://github.com/apache/lucene/pull/16138#issuecomment-4582808965 I applied the suggested changes. I think this is ready to go. The method signatures changed to be more type safe, this is fine because it is internal API anyways. I had to call `MethodHandles.lookup()` on each call unfortunately as interfaces do not allow private static fields (why the hell?). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] reject non-SerializableObject classes in SerializableObject.readClass [lucene]
uschindler commented on code in PR #16138:
URL: https://github.com/apache/lucene/pull/16138#discussion_r3328590204
##
lucene/spatial3d/src/test/org/apache/lucene/spatial3d/geom/TestSerializableObjectClassFilter.java:
##
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.lucene.spatial3d.geom;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import org.apache.lucene.tests.util.LuceneTestCase;
+
+public class TestSerializableObjectClassFilter extends LuceneTestCase {
+
+ static volatile boolean nonSerializableConstructed = false;
+
+ /** A SerializableObject that is not registered in {@link StandardObjects}.
*/
+ public static class CustomShape implements SerializableObject {
+final int value;
+
+public CustomShape(int value) {
+ this.value = value;
+}
+
+public CustomShape(InputStream inputStream) throws IOException {
+ this.value = SerializableObject.readInt(inputStream);
+}
+
+@Override
+public void write(OutputStream outputStream) throws IOException {
+ SerializableObject.writeInt(outputStream, value);
+}
+ }
+
+ /** Not a SerializableObject, but instantiable from an InputStream. */
+ public static class NotASerializableObject {
Review Comment:
it should no only make sure it is constructed, it should have a static
initializer to make sure the stati initializer is not executed (which is the
biggest security issue here):
```java
static {
nonSerializableInitialized = true;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] reject non-SerializableObject classes in SerializableObject.readClass [lucene]
uschindler commented on code in PR #16138:
URL: https://github.com/apache/lucene/pull/16138#discussion_r3328590204
##
lucene/spatial3d/src/test/org/apache/lucene/spatial3d/geom/TestSerializableObjectClassFilter.java:
##
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.lucene.spatial3d.geom;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import org.apache.lucene.tests.util.LuceneTestCase;
+
+public class TestSerializableObjectClassFilter extends LuceneTestCase {
+
+ static volatile boolean nonSerializableConstructed = false;
+
+ /** A SerializableObject that is not registered in {@link StandardObjects}.
*/
+ public static class CustomShape implements SerializableObject {
+final int value;
+
+public CustomShape(int value) {
+ this.value = value;
+}
+
+public CustomShape(InputStream inputStream) throws IOException {
+ this.value = SerializableObject.readInt(inputStream);
+}
+
+@Override
+public void write(OutputStream outputStream) throws IOException {
+ SerializableObject.writeInt(outputStream, value);
+}
+ }
+
+ /** Not a SerializableObject, but instantiable from an InputStream. */
+ public static class NotASerializableObject {
Review Comment:
it should no only make sure it is constructed, it should have a static
initializer:
```java
static {
nonSerializableInitialized = true;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] reject non-SerializableObject classes in SerializableObject.readClass [lucene]
uschindler commented on code in PR #16138:
URL: https://github.com/apache/lucene/pull/16138#discussion_r3328587959
##
lucene/spatial3d/src/test/org/apache/lucene/spatial3d/geom/TestSerializableObjectClassFilter.java:
##
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.lucene.spatial3d.geom;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import org.apache.lucene.tests.util.LuceneTestCase;
+
+public class TestSerializableObjectClassFilter extends LuceneTestCase {
+
+ static volatile boolean nonSerializableConstructed = false;
+
+ /** A SerializableObject that is not registered in {@link StandardObjects}.
*/
+ public static class CustomShape implements SerializableObject {
+final int value;
+
+public CustomShape(int value) {
+ this.value = value;
+}
+
+public CustomShape(InputStream inputStream) throws IOException {
+ this.value = SerializableObject.readInt(inputStream);
+}
+
+@Override
+public void write(OutputStream outputStream) throws IOException {
+ SerializableObject.writeInt(outputStream, value);
+}
+ }
+
+ /** Not a SerializableObject, but instantiable from an InputStream. */
+ public static class NotASerializableObject {
+public NotASerializableObject(InputStream inputStream) {
+ nonSerializableConstructed = true;
+}
+ }
+
+ public void testCustomClassRoundTrips() throws IOException {
+CustomShape shape = new CustomShape(42);
+ByteArrayOutputStream out = new ByteArrayOutputStream();
+SerializableObject.writeObject(out, shape);
+ByteArrayInputStream in = new ByteArrayInputStream(out.toByteArray());
+SerializableObject copy = SerializableObject.readObject(in);
+assertTrue(copy instanceof CustomShape);
+assertEquals(42, ((CustomShape) copy).value);
+ }
+
+ public void testRejectsNonSerializableObjectClass() throws IOException {
+nonSerializableConstructed = false;
+ByteArrayOutputStream out = new ByteArrayOutputStream();
+SerializableObject.writeBoolean(out, false);
+SerializableObject.writeString(out,
NotASerializableObject.class.getName());
+ByteArrayInputStream in = new ByteArrayInputStream(out.toByteArray());
+IOException e = expectThrows(IOException.class, () ->
SerializableObject.readObject(in));
Review Comment:
with the suggested change above it would cause a `ClassCastException`. Then
we need no check on exception message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] reject non-SerializableObject classes in SerializableObject.readClass [lucene]
uschindler commented on code in PR #16138:
URL: https://github.com/apache/lucene/pull/16138#discussion_r3328584549
##
lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/SerializableObject.java:
##
@@ -216,7 +216,13 @@ static Class readClass(final InputStream inputStream)
return StandardObjects.CODE_REGISTRY.get(index);
} else {
String className = readString(inputStream);
- return Class.forName(className);
+ // Load without initializing and confirm the named class is actually a
SerializableObject
+ // before it can be instantiated, so a crafted stream cannot load
arbitrary classes.
+ Class clazz = Class.forName(className, false,
SerializableObject.class.getClassLoader());
Review Comment:
Instead of doing the isAssignable check, it should maybe directly cast the
class:
```java
Class clazz = LOOKUP.findClass(className)
.asSubclass(SerializableObject.class);
```
Then you do not need to do any extra checks and the Exception is an expected
one.
Also better change all method signatures like the return values of this
method and the readObject parameters to take `Class` instead of `Class`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] reject non-SerializableObject classes in SerializableObject.readClass [lucene]
uschindler commented on code in PR #16138:
URL: https://github.com/apache/lucene/pull/16138#discussion_r3328584549
##
lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/SerializableObject.java:
##
@@ -216,7 +216,13 @@ static Class readClass(final InputStream inputStream)
return StandardObjects.CODE_REGISTRY.get(index);
} else {
String className = readString(inputStream);
- return Class.forName(className);
+ // Load without initializing and confirm the named class is actually a
SerializableObject
+ // before it can be instantiated, so a crafted stream cannot load
arbitrary classes.
+ Class clazz = Class.forName(className, false,
SerializableObject.class.getClassLoader());
Review Comment:
Instead of doing the isAssignable check, it should maybe directly cast the
class:
```java
Class clazz =
LOOKUP.findClass(className).cast(SerializableObject.class);
```
Then you do not need to do any extra checks and the Exception is an expected
one.
Also better change all method signatures like the return values of this
method and the readObject parameters to take `Class` instead of `Class`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] reject non-SerializableObject classes in SerializableObject.readClass [lucene]
uschindler commented on code in PR #16138:
URL: https://github.com/apache/lucene/pull/16138#discussion_r3328584549
##
lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/SerializableObject.java:
##
@@ -216,7 +216,13 @@ static Class readClass(final InputStream inputStream)
return StandardObjects.CODE_REGISTRY.get(index);
} else {
String className = readString(inputStream);
- return Class.forName(className);
+ // Load without initializing and confirm the named class is actually a
SerializableObject
+ // before it can be instantiated, so a crafted stream cannot load
arbitrary classes.
+ Class clazz = Class.forName(className, false,
SerializableObject.class.getClassLoader());
Review Comment:
Instead of doing the isAssignable check, it should maybe directly cast the
class:
``java
Class clazz =
LOOKUP.findClass(className).cast(SerializableObject.class);
```
Then you do not need to do any extra checks and the Exception is an expected
one.
Also better change all method signatures like the return values of this
method and the readObject parameters to take `Class` instead of `Class`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] reject non-SerializableObject classes in SerializableObject.readClass [lucene]
uschindler commented on code in PR #16138:
URL: https://github.com/apache/lucene/pull/16138#discussion_r3328584549
##
lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/SerializableObject.java:
##
@@ -216,7 +216,13 @@ static Class readClass(final InputStream inputStream)
return StandardObjects.CODE_REGISTRY.get(index);
} else {
String className = readString(inputStream);
- return Class.forName(className);
+ // Load without initializing and confirm the named class is actually a
SerializableObject
+ // before it can be instantiated, so a crafted stream cannot load
arbitrary classes.
+ Class clazz = Class.forName(className, false,
SerializableObject.class.getClassLoader());
Review Comment:
Instead of doing the isAssignable check, it should maybe directly cast the
class:
``java
Class clazz =
LOOKUP.findClass(className).cast(SerializableObject.class);
``
Then you do not need to do any extra checks and the Exception is an expected
one.
Also better change all method signatures like the return values of this
method and the readObject parameters to take `Class` instead of `Class`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] reject non-SerializableObject classes in SerializableObject.readClass [lucene]
uschindler commented on code in PR #16138:
URL: https://github.com/apache/lucene/pull/16138#discussion_r3328572079
##
lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/SerializableObject.java:
##
@@ -216,7 +216,13 @@ static Class readClass(final InputStream inputStream)
return StandardObjects.CODE_REGISTRY.get(index);
} else {
String className = readString(inputStream);
- return Class.forName(className);
+ // Load without initializing and confirm the named class is actually a
SerializableObject
+ // before it can be instantiated, so a crafted stream cannot load
arbitrary classes.
+ Class clazz = Class.forName(className, false,
SerializableObject.class.getClassLoader());
Review Comment:
Instead of the antique clasloader magic, the preferable thing is to load the
class with a static final `MethodHandles#lookup()` which is created for this
class. This would make the whole thing behave like an `LDC` instruction, that
does NOT load the class and initializes it. Instead it looks up the class and
makes sure it is accessible from this class.
So basically:
```java
private static final LOOKUP = MethodHandles.lookup();
// later in code here:
Class clazz = LOOKUP.findClass(className);
```
It also does not return null and works like an `LDC` instruction.
Anyways, I'd like to rewrite the rest of the code to use MethodHandles, too.
But that's outside of the scope of this PR.
##
lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/SerializableObject.java:
##
@@ -216,7 +216,13 @@ static Class readClass(final InputStream inputStream)
return StandardObjects.CODE_REGISTRY.get(index);
} else {
String className = readString(inputStream);
- return Class.forName(className);
+ // Load without initializing and confirm the named class is actually a
SerializableObject
+ // before it can be instantiated, so a crafted stream cannot load
arbitrary classes.
+ Class clazz = Class.forName(className, false,
SerializableObject.class.getClassLoader());
Review Comment:
The pro on this approach is that it also makes sure that we can access the
class (it is visible to us, so invoking ctor works and so on).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] reject non-SerializableObject classes in SerializableObject.readClass [lucene]
metsw24-max commented on PR #16138: URL: https://github.com/apache/lucene/pull/16138#issuecomment-4581875263 Good call, added a Bug Fixes entry under 10.5.0. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] reject non-SerializableObject classes in SerializableObject.readClass [lucene]
metsw24-max commented on code in PR #16138:
URL: https://github.com/apache/lucene/pull/16138#discussion_r3328242123
##
lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/SerializableObject.java:
##
@@ -216,7 +216,13 @@ static Class readClass(final InputStream inputStream)
return StandardObjects.CODE_REGISTRY.get(index);
} else {
String className = readString(inputStream);
- return Class.forName(className);
+ // Load without initializing and confirm the named class is actually a
SerializableObject
+ // before it can be instantiated, so a crafted stream cannot load
arbitrary classes.
+ Class clazz = Class.forName(className, false,
SerializableObject.class.getClassLoader());
+ if (!SerializableObject.class.isAssignableFrom(clazz)) {
Review Comment:
Done, switched to == false.
##
lucene/spatial3d/src/test/org/apache/lucene/spatial3d/geom/TestSerializableObjectClassFilter.java:
##
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.lucene.spatial3d.geom;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import org.apache.lucene.tests.util.LuceneTestCase;
+
+public class TestSerializableObjectClassFilter extends LuceneTestCase {
+
+ static volatile boolean nonSerializableConstructed = false;
+
+ /** A SerializableObject that is not registered in {@link StandardObjects}.
*/
+ public static class CustomShape implements SerializableObject {
+final int value;
+
+public CustomShape(int value) {
+ this.value = value;
+}
+
+public CustomShape(InputStream inputStream) throws IOException {
+ this.value = SerializableObject.readInt(inputStream);
+}
+
+@Override
+public void write(OutputStream outputStream) throws IOException {
+ SerializableObject.writeInt(outputStream, value);
+}
+ }
+
+ /** Not a SerializableObject, but instantiable from an InputStream. */
+ public static class NotASerializableObject {
+public NotASerializableObject(InputStream inputStream) {
+ nonSerializableConstructed = true;
+}
+ }
+
+ public void testCustomClassRoundTrips() throws IOException {
+CustomShape shape = new CustomShape(42);
+ByteArrayOutputStream out = new ByteArrayOutputStream();
+SerializableObject.writeObject(out, shape);
+ByteArrayInputStream in = new ByteArrayInputStream(out.toByteArray());
+SerializableObject copy = SerializableObject.readObject(in);
+assertTrue(copy instanceof CustomShape);
+assertEquals(42, ((CustomShape) copy).value);
+ }
+
+ public void testRejectsNonSerializableObjectClass() throws IOException {
+nonSerializableConstructed = false;
+ByteArrayOutputStream out = new ByteArrayOutputStream();
+SerializableObject.writeBoolean(out, false);
+SerializableObject.writeString(out,
NotASerializableObject.class.getName());
+ByteArrayInputStream in = new ByteArrayInputStream(out.toByteArray());
+expectThrows(IOException.class, () -> SerializableObject.readObject(in));
Review Comment:
Added an assert that the message contains the class name.
##
lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/SerializableObject.java:
##
@@ -216,7 +216,13 @@ static Class readClass(final InputStream inputStream)
return StandardObjects.CODE_REGISTRY.get(index);
} else {
String className = readString(inputStream);
- return Class.forName(className);
+ // Load without initializing and confirm the named class is actually a
SerializableObject
+ // before it can be instantiated, so a crafted stream cannot load
arbitrary classes.
+ Class clazz = Class.forName(className, false,
SerializableObject.class.getClassLoader());
Review Comment:
Class.forName doesn't return null, it throws ClassNotFoundException for an
unknown name, so that path is the same as before (readClass already declares
it). The new check only kicks in once the class resolves.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on
Re: [PR] reject non-SerializableObject classes in SerializableObject.readClass [lucene]
shubhamsrkdev commented on PR #16138: URL: https://github.com/apache/lucene/pull/16138#issuecomment-4577338967 We might need a Changes.txt entry for this one? We are introducing a new exception? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] reject non-SerializableObject classes in SerializableObject.readClass [lucene]
shubhamsrkdev commented on code in PR #16138:
URL: https://github.com/apache/lucene/pull/16138#discussion_r3325485475
##
lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/SerializableObject.java:
##
@@ -216,7 +216,13 @@ static Class readClass(final InputStream inputStream)
return StandardObjects.CODE_REGISTRY.get(index);
} else {
String className = readString(inputStream);
- return Class.forName(className);
+ // Load without initializing and confirm the named class is actually a
SerializableObject
+ // before it can be instantiated, so a crafted stream cannot load
arbitrary classes.
+ Class clazz = Class.forName(className, false,
SerializableObject.class.getClassLoader());
+ if (!SerializableObject.class.isAssignableFrom(clazz)) {
Review Comment:
In this repo we prefer:
```suggestion
if (SerializableObject.class.isAssignableFrom(clazz) == false) {
```
instead.
##
lucene/spatial3d/src/test/org/apache/lucene/spatial3d/geom/TestSerializableObjectClassFilter.java:
##
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.lucene.spatial3d.geom;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import org.apache.lucene.tests.util.LuceneTestCase;
+
+public class TestSerializableObjectClassFilter extends LuceneTestCase {
+
+ static volatile boolean nonSerializableConstructed = false;
+
+ /** A SerializableObject that is not registered in {@link StandardObjects}.
*/
+ public static class CustomShape implements SerializableObject {
+final int value;
+
+public CustomShape(int value) {
+ this.value = value;
+}
+
+public CustomShape(InputStream inputStream) throws IOException {
+ this.value = SerializableObject.readInt(inputStream);
+}
+
+@Override
+public void write(OutputStream outputStream) throws IOException {
+ SerializableObject.writeInt(outputStream, value);
+}
+ }
+
+ /** Not a SerializableObject, but instantiable from an InputStream. */
+ public static class NotASerializableObject {
+public NotASerializableObject(InputStream inputStream) {
+ nonSerializableConstructed = true;
+}
+ }
+
+ public void testCustomClassRoundTrips() throws IOException {
+CustomShape shape = new CustomShape(42);
+ByteArrayOutputStream out = new ByteArrayOutputStream();
+SerializableObject.writeObject(out, shape);
+ByteArrayInputStream in = new ByteArrayInputStream(out.toByteArray());
+SerializableObject copy = SerializableObject.readObject(in);
+assertTrue(copy instanceof CustomShape);
+assertEquals(42, ((CustomShape) copy).value);
+ }
+
+ public void testRejectsNonSerializableObjectClass() throws IOException {
+nonSerializableConstructed = false;
+ByteArrayOutputStream out = new ByteArrayOutputStream();
+SerializableObject.writeBoolean(out, false);
+SerializableObject.writeString(out,
NotASerializableObject.class.getName());
+ByteArrayInputStream in = new ByteArrayInputStream(out.toByteArray());
+expectThrows(IOException.class, () -> SerializableObject.readObject(in));
Review Comment:
nit: can we assert on exception message also?
##
lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/SerializableObject.java:
##
@@ -216,7 +216,13 @@ static Class readClass(final InputStream inputStream)
return StandardObjects.CODE_REGISTRY.get(index);
} else {
String className = readString(inputStream);
- return Class.forName(className);
+ // Load without initializing and confirm the named class is actually a
SerializableObject
+ // before it can be instantiated, so a crafted stream cannot load
arbitrary classes.
+ Class clazz = Class.forName(className, false,
SerializableObject.class.getClassLoader());
Review Comment:
Could this be null? What would be the behavior then, same as before?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the
