fix[PPP-5720]: remove instances of unsafe deserialization by readObject

core/src/main/java/pt/webdetails/cda/cache/CacheKey.java

@@ -18,12 +18,13 @@
 import java.io.IOException;
 import java.io.Serializable;
 import java.util.ArrayList;
+import java.util.Objects;
 
 public class CacheKey implements Serializable {
 
   private static final long serialVersionUID = -1273843592305584696L;
 
-  private ArrayList<KeyValuePair> keyValuePairs = new ArrayList<KeyValuePair>();
+  private ArrayList<KeyValuePair> keyValuePairs = new ArrayList<>();
 
   public CacheKey() {
   }
@@ -34,7 +35,7 @@ public CacheKey( String key, String value ) {
 
   public ArrayList<KeyValuePair> getKeyValuePairs() {
     if ( keyValuePairs == null ) {
-      keyValuePairs = new ArrayList<KeyValuePair>();
+      keyValuePairs = new ArrayList<>();
     }
     return keyValuePairs;
   }
@@ -47,11 +48,7 @@ public void addKeyValuePair( String key, String value ) {
 
   public void removeByKey( String key ) {
     if ( !StringUtils.isEmpty( key ) ) {
-      for ( KeyValuePair pair : getKeyValuePairs() ) {
-        if ( key.equals( pair.getKey() ) ) {
-          getKeyValuePairs().remove( pair );
-        }
-      }
+      getKeyValuePairs().removeIf( pair -> key.equals( pair.getKey() ) );
     }
   }
 
@@ -77,11 +74,7 @@ public boolean equals( final Object o ) {
 
     final CacheKey other = (CacheKey) o;
 
-    if ( keyValuePairs != null ? !keyValuePairs.equals( other.keyValuePairs ) : other.keyValuePairs != null ) {
-      return false;
-    }
-
-    return true;
+    return Objects.equals( keyValuePairs, other.keyValuePairs );
   }
 
   @Override
@@ -155,15 +148,7 @@ public boolean equals( final Object o ) {
 
       final KeyValuePair other = (KeyValuePair) o;
 
-      if ( this.key == null ? other.key != null : !this.key.equals( other.key ) ) {
-        return false;
-      }
-
-      if ( this.value == null ? other.value != null : !this.value.equals( other.value ) ) {
-        return false;
-      }
-
-      return true;
+      return Objects.equals( this.key, other.key ) && Objects.equals( this.value, other.value );
     }
 
     @Override
@@ -180,22 +165,22 @@ public String toString() {
         + "]\n";
     }
 
-  /*
-   * Serializable classes that require special handling during the serialization and deserialization process should
-   * implement the following methods: writeObject(java.io.ObjectOutputStream stream),
-   * readObject(java.io.ObjectInputStream stream)
-   *
-   * @see http://docs.oracle.com/javase/6/docs/api/java/io/ObjectInputStream.html
-   */
+    /*
+     * Serializable classes that require special handling during the serialization and deserialization process should
+     * implement the following methods: writeObject(java.io.ObjectOutputStream stream),
+     * readObject(java.io.ObjectInputStream stream)
+     *
+     * @see http://docs.oracle.com/javase/6/docs/api/java/io/ObjectInputStream.html
+     */
 
     private void readObject( java.io.ObjectInputStream in ) throws IOException, ClassNotFoundException {
-      key = (String) in.readObject();
-      value = (String) in.readObject();
+      key = in.readUTF();
+      value = in.readUTF();
     }
 
     private void writeObject( java.io.ObjectOutputStream out ) throws IOException {
-      out.writeObject( key );
-      out.writeObject( value );
+      out.writeUTF( key );
+      out.writeUTF( value );
     }
   }
 }

core/src/main/java/pt/webdetails/cda/cache/EHCacheQueryCache.java

@@ -67,7 +67,11 @@ public ExtraCacheInfo getInfo() {
     }
 
     private void writeObject( ObjectOutputStream out ) throws IOException {
-      out.writeObject( table );
+      if ( table instanceof Serializable ) {
+        out.writeObject( table );
+      } else {
+        throw new IOException("Cannot call writeObject on TableModel (object is not serializable)");
+      }
       out.writeObject( info );
     }
 
@@ -112,7 +116,12 @@ protected static synchronized Cache getCacheFromManager( final boolean switchCla
       }
     }
 
-    if ( cacheManager.cacheExists( CACHE_NAME ) == false ) {
+    if ( cacheManager == null ) {
+      logger.error( "Cache manager is null" );
+      return null;
+    }
+
+    if ( !cacheManager.cacheExists( CACHE_NAME ) ) {
       cacheManager.addCache( CACHE_NAME );
     }
 
@@ -171,7 +180,7 @@ public TableModel getTableModel( TableCacheKey key ) {
       Thread.currentThread().setContextClassLoader( this.getClass().getClassLoader() );
       final Element element = cache.get( key );
       if ( element != null ) {
-        final TableModel cachedTableModel = (TableModel) ( (CacheElement) element.getObjectValue() ).getTable();
+        final TableModel cachedTableModel = ( (CacheElement) element.getObjectValue() ).getTable();
         if ( cachedTableModel != null ) {
           if ( logger.isDebugEnabled() ) {
             // we have a entry in the cache ... great!

core/src/main/java/pt/webdetails/cda/cache/TableCacheKey.java

@@ -22,14 +22,17 @@
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
+import java.io.InputStream;
+import java.io.ObjectInputFilter;
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
 import java.io.Serializable;
-import java.io.UnsupportedEncodingException;
+import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Comparator;
 import java.util.List;
+import java.util.Objects;
 
 public class TableCacheKey implements Serializable {
 
@@ -40,7 +43,7 @@ public class TableCacheKey implements Serializable {
   private String queryType;
   private Parameter[] parameters;
 
-  private Serializable extraCacheKey;
+  private CacheKey extraCacheKey;
 
   /**
    * For serialization
@@ -50,7 +53,7 @@ protected TableCacheKey() {
 
 
   public TableCacheKey( final Connection connection, final String query,
-                        final List<Parameter> parameters, final Serializable extraCacheKey ) {
+                        final List<Parameter> parameters, final CacheKey extraCacheKey ) {
     if ( connection == null ) {
       throw new NullPointerException();
     }
@@ -69,7 +72,7 @@ public TableCacheKey( final Connection connection, final String query,
   }
 
   public TableCacheKey( final Connection connection, final String query, final String queryType,
-                        final List<Parameter> parameters, final Serializable extraCacheKey ) {
+                        final List<Parameter> parameters, final CacheKey extraCacheKey ) {
     if ( connection == null ) {
       throw new NullPointerException();
     }
@@ -123,7 +126,6 @@ public Parameter[] getParameters() {
     return parameters;
   }
 
-
   public void setParameterDataRow( ParameterDataRow parameterDataRow ) {
     //this.parameterDataRow = parameterDataRow;
     this.parameters = createParametersFromParameterDataRow( parameterDataRow );
@@ -135,7 +137,7 @@ public Object getExtraCacheKey() {
   }
 
 
-  public void setExtraCacheKey( Serializable extraCacheKey ) {
+  public void setExtraCacheKey( CacheKey extraCacheKey ) {
     this.extraCacheKey = extraCacheKey;
   }
 
@@ -147,9 +149,9 @@ private void readObject( java.io.ObjectInputStream in ) throws IOException, Clas
     //connection
     connectionHash = in.readInt();
     //query
-    query = (String) in.readObject();
+    query = in.readUTF();
     //queryTpe
-    queryType = (String) in.readObject();
+    queryType = in.readUTF();
 
     int len = in.readInt();
     Parameter[] params = new Parameter[ len ];
@@ -160,7 +162,7 @@ private void readObject( java.io.ObjectInputStream in ) throws IOException, Clas
       params[ i ] = param;
     }
     parameters = params;
-    extraCacheKey = (Serializable) in.readObject();
+    extraCacheKey = (CacheKey) in.readObject();
   }
 
   //Hazelcast will use serialized version to perform comparisons and hashcodes
@@ -169,8 +171,8 @@ private void writeObject( java.io.ObjectOutputStream out ) throws IOException {
     //binary comparison/hash will be used
 
     out.writeInt( connectionHash );
-    out.writeObject( query );
-    out.writeObject( queryType );
+    out.writeUTF( query );
+    out.writeUTF( queryType );
 
     out.writeInt( parameters.length );
     for ( Parameter param : parameters ) {
@@ -184,12 +186,11 @@ private void writeObject( java.io.ObjectOutputStream out ) throws IOException {
    * Serialize as printable <code>String</code>.
    */
   public static String getTableCacheKeyAsString( TableCacheKey cacheKey )
-    throws IOException, UnsupportedEncodingException {
+    throws IOException {
     ByteArrayOutputStream keyStream = new ByteArrayOutputStream();
     ObjectOutputStream objStream = new ObjectOutputStream( keyStream );
     cacheKey.writeObject( objStream );
-    String identifier = new String( Base64.encodeBase64( keyStream.toByteArray() ), "UTF-8" );
-    return identifier;
+    return new String( Base64.encodeBase64( keyStream.toByteArray() ), StandardCharsets.UTF_8 );
   }
 
   /**
@@ -198,12 +199,34 @@ public static String getTableCacheKeyAsString( TableCacheKey cacheKey )
   public static TableCacheKey getTableCacheKeyFromString( String encodedCacheKey )
     throws IOException, ClassNotFoundException {
     ByteArrayInputStream keyStream = new ByteArrayInputStream( Base64.decodeBase64( encodedCacheKey.getBytes() ) );
-    ObjectInputStream objStream = new ObjectInputStream( keyStream );
+
+    // We add this filter to allow only the specific classes that will be read further ahead,
+    // and block everything else from readObject. This is important because readObject can allow injection attacks.
+    ObjectInputFilter paramFilter = ObjectInputFilter.Config.createFilter(
+      "pt.webdetails.cda.dataaccess.Parameter;"
+        + "pt.webdetails.cda.dataaccess.Parameter$Type;"
+        + "pt.webdetails.cda.cache.CacheKey;"
+        + "javax.swing.table.TableModel;"
+        + "pt.webdetails.cda.cache.monitor.ExtraCacheInfo;"
+        + "pt.webdetails.cda.dataaccess.MdxDataAccess$BANDED_MODE;"
+        + "!*"
+    );
+    ObjectInputStream objStream = new FilteredObjectInputStream( keyStream, paramFilter );
     TableCacheKey cacheKey = new TableCacheKey();
     cacheKey.readObject( objStream );
     return cacheKey;
   }
 
+  // We set the filter in this custom class, because it cannot be applied after read operations have already happened,
+  // which was the case when using the normal constructor and applying the filter after class instantiation.
+  private static class FilteredObjectInputStream extends ObjectInputStream {
+    FilteredObjectInputStream( InputStream in, ObjectInputFilter filter ) throws IOException {
+      super( in );
+      setObjectInputFilter( filter );
+    }
+  }
+
+
   public boolean equals( final Object o ) {
     if ( this == o ) {
       return true;
@@ -220,14 +243,10 @@ public boolean equals( final Object o ) {
     if ( parameters != null ? !Arrays.equals( parameters, that.parameters ) : that.parameters != null ) {
       return false;
     }
-    if ( query != null ? !query.equals( that.query ) : that.query != null ) {
-      return false;
-    }
-    if ( extraCacheKey != null ? !extraCacheKey.equals( that.extraCacheKey ) : that.extraCacheKey != null ) {
+    if ( !Objects.equals( query, that.query ) ) {
       return false;
     }
-
-    return true;
+    return Objects.equals( extraCacheKey, that.extraCacheKey );
   }
 
 
@@ -264,11 +283,11 @@ public int compare( Parameter o1, Parameter o2 ) {
    * for serialization
    */
   private static Parameter[] createParametersFromParameterDataRow( final ParameterDataRow row ) {
-    ArrayList<Parameter> parameters = new ArrayList<Parameter>();
+    ArrayList<Parameter> parameters = new ArrayList<>();
     if ( row != null ) {
       for ( String name : row.getColumnNames() ) {
         Object value = row.get( name );
-        Parameter param = new Parameter( name, value != null ? value : null );
+        Parameter param = new Parameter( name, value );
         Parameter.Type type = Parameter.Type.inferTypeFromObject( value );
         param.setType( type );
         parameters.add( param );

core/src/main/java/pt/webdetails/cda/cache/monitor/ExtraCacheInfo.java

@@ -116,25 +116,25 @@ public void setTimeToLive( int timeToLive ) {
   }
 
   private void writeObject( ObjectOutputStream out ) throws IOException {
-    out.writeObject( cdaSettingsId );
-    out.writeObject( dataAccessId );
+    out.writeUTF( cdaSettingsId );
+    out.writeUTF( dataAccessId );
     out.writeLong( queryDurationMs );
     out.writeInt( nbrRows );
     out.writeLong( entryTime );
     out.writeInt( timeToLive );
-    out.writeObject( tableSnapshot != null ? tableSnapshot.toString() : null );
+    out.writeUTF( tableSnapshot != null ? tableSnapshot.toString() : null );
   }
 
   private void readObject( ObjectInputStream in ) throws IOException, ClassNotFoundException {
-    cdaSettingsId = (String) in.readObject();
-    dataAccessId = (String) in.readObject();
+    cdaSettingsId = in.readUTF();
+    dataAccessId = in.readUTF();
     queryDurationMs = in.readLong();
     nbrRows = in.readInt();
     entryTime = in.readLong();
     timeToLive = in.readInt();
 
     try {
-      tableSnapshot = new JSONObject( (String) in.readObject() );
+      tableSnapshot = new JSONObject( in.readUTF() );
     } catch ( Exception e ) {
       tableSnapshot = null;
     }