Fix the risks found by PenTest #76
Conversation
Signed-off-by: Alex <pengsun@amazon.com>
the conflict with search XContent input. Signed-off-by: Alex <pengsun@amazon.com>
1. unhandled 500 server error. 2. Insecure Deserialization Signed-off-by: Alex <pengsun@amazon.com>
| "*java.time.*", | ||
| "*org.tribuo.*", | ||
| "*com.oracle.labs.*", | ||
| "[*" |
There was a problem hiding this comment.
Good question, it's a workaround of an issue of the tribuo library.
Issue:
If we serialize the KMeansModel using ObjectOutputStream and ByteArrayOutputStream to a byte array, when we
deserialize the byte[] to an Object using ObjectInputStream and ObjectInputStream, we will see some strange class names, like: "[D", "[I" and "[Lorg.tribuo.math.la.DenseVector".
Reference:
https://github.com/R3Conclave/conclave-samples/blob/6da1a92acc8d2a51975475c1ffe507e6cdfcaa08/tribuo-tutorials/enclave/src/main/resources/META-INF/native-image/serialization-config.json
Workaround Solution:
We have to put "[D", "[I" and "[Lorg.tribuo.math.la.DenseVector" to the whitelist, otherwise the validation will run failed.
There was a problem hiding this comment.
I think we'd better add concrete class path here to avoid some malicious code using this pattern.
| import java.io.ObjectOutputStream; | ||
|
|
||
| @UtilityClass | ||
| public class ModelSerDeSer { | ||
| // Welcome list includes OpenSearch ml plugin classes, JDK common classes and Tribuo libraries. | ||
| public static final String[] ACCEPT_CLASS_PATTERNS = { | ||
| "*org.opensearch.ml.*", |
There was a problem hiding this comment.
Why need to add "*" as prefix?
| ValidatingObjectInputStream validatingObjectInputStream = new ValidatingObjectInputStream(inputStream); | ||
|
|
||
| // Validate the model class type to avoid deserialization attack. | ||
| validatingObjectInputStream.accept(ACCEPT_CLASS_PATTERNS); |
There was a problem hiding this comment.
If someone wrote class with the same class path, for example "org.opensearch.ml.SomeHackClass" . Can this prevent that deserialization attack?
There was a problem hiding this comment.
Unfortunately, this validation only validate the package/class name
There was a problem hiding this comment.
Not security expert, but I feel we'd better add concrete class names to avoid some one write malicious code with these allowed patterns.
Signed-off-by: Alex <pengsun@amazon.com>
* Create JvmService instance on demand. Signed-off-by: Alex <pengsun@amazon.com> * Move the ml_parameters from XContent to the request parameters to avoid the conflict with search XContent input. Signed-off-by: Alex <pengsun@amazon.com> * Fix the security risks found by PenTest. 1. unhandled 500 server error. 2. Insecure Deserialization Signed-off-by: Alex <pengsun@amazon.com> * Remove unnecessory '*' from the welcome list of model deserializer. Signed-off-by: Alex <pengsun@amazon.com> Co-authored-by: Alex <pengsun@amazon.com>
Description
Fix the risks found by PenTest
1. unhandled 500 server error.
2. Insecure Deserialization
Issues Resolved
#73
#74
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.