Skip to content

Commit

Permalink
Expose bug where SecuredCatalogInfos are double wrapped
Browse files Browse the repository at this point in the history 
This patch makes sure SecuredCatalogInfo objects are always
created through DefaultSecureCatalogFactory.secure(Object, WrapperPolicy)
and logs a warning when a secured info is being created to
wrap another secured info, which ultimately can lead to
a StackOverflowError at least in ResourcePool.getDataStore(),
which serializes and deserializes the store using regular
Java serialization.
  • Loading branch information
Gabriel Roldan authored and Erik Merkle committed Mar 7, 2018
1 parent a85e17e commit 670eec9
Show file tree
Hide file tree
Showing 5 changed files with 136 additions and 30 deletions.
21 changes: 5 additions & 16 deletions src/main/src/main/java/org/geoserver/security/SecureCatalogImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
import org.geoserver.security.decorators.SecuredFeatureTypeInfo;
import org.geoserver.security.decorators.SecuredLayerGroupInfo;
import org.geoserver.security.decorators.SecuredLayerInfo;
import org.geoserver.security.decorators.SecuredObjects;
import org.geoserver.security.decorators.SecuredWMSLayerInfo;
import org.geoserver.security.decorators.SecuredWMTSLayerInfo;
import org.geoserver.security.impl.DataAccessRuleDAO;
Expand Down Expand Up @@ -533,17 +534,7 @@ else if(policy.level == AccessLevel.READ_WRITE && policy.getLimits() == null)

// otherwise we are in a mixed case where the user can read but not write, or
// cannot read but is allowed by the operation mode to access the metadata
if(info instanceof FeatureTypeInfo) {
return (T) new SecuredFeatureTypeInfo((FeatureTypeInfo) info, policy);
} else if(info instanceof CoverageInfo) {
return (T) new SecuredCoverageInfo((CoverageInfo) info, policy);
} else if(info instanceof WMSLayerInfo) {
return (T) new SecuredWMSLayerInfo((WMSLayerInfo) info, policy);
} else if(info instanceof WMTSLayerInfo) {
return (T) new SecuredWMTSLayerInfo((WMTSLayerInfo) info, policy);
} else {
throw new RuntimeException("Unknown resource type " + info.getClass());
}
return (T) SecuredObjects.secure(info, policy);
}

/**
Expand Down Expand Up @@ -587,10 +578,8 @@ else if(policy.level == AccessLevel.READ_WRITE ||
// write, or
// cannot read but is allowed by the operation mode to access the
// metadata
if (store instanceof DataStoreInfo) {
return (T) new SecuredDataStoreInfo((DataStoreInfo) store, policy);
} else if(store instanceof CoverageStoreInfo) {
return (T) new SecuredCoverageStoreInfo((CoverageStoreInfo) store, policy);
if (store instanceof DataStoreInfo || store instanceof CoverageStoreInfo) {
return (T) SecuredObjects.secure(store, policy); //new SecuredDataStoreInfo((DataStoreInfo) store, policy);
} else if (store instanceof WMSStoreInfo) {
// TODO: implement WMSStoreInfo wrappring if necessary
return store;
Expand Down Expand Up @@ -622,7 +611,7 @@ else if(policy.level == AccessLevel.READ_WRITE && policy.getLimits() == null)

// otherwise we are in a mixed case where the user can read but not write, or
// cannot read but is allowed by the operation mode to access the metadata
return new SecuredLayerInfo(layer, policy);
return (LayerInfo) SecuredObjects.secure(layer, policy);
}

/**
Expand Down
133 changes: 125 additions & 8 deletions src/main/src/main/java/org/geoserver/security/decorators/DefaultSecureCatalogFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,18 @@
*/
package org.geoserver.security.decorators;

import java.util.logging.Logger;

import org.geoserver.catalog.CoverageInfo;
import org.geoserver.catalog.CoverageStoreInfo;
import org.geoserver.catalog.DataStoreInfo;
import org.geoserver.catalog.FeatureTypeInfo;
import org.geoserver.catalog.LayerInfo;
import org.geoserver.catalog.WMTSLayerInfo;
import org.geoserver.catalog.impl.ModificationProxy;
import org.geoserver.platform.ExtensionPriority;
import org.geoserver.security.WrapperPolicy;
import org.geotools.util.logging.Logging;

/**
* Creates security wrappers for the most common catalog objects
Expand All @@ -20,7 +25,9 @@
*
*/
public class DefaultSecureCatalogFactory implements SecuredObjectFactory {


private static final Logger LOGGER = Logging.getLogger(DefaultSecureCatalogFactory.class);

public boolean canSecure(Class clazz) {
return CoverageInfo.class.isAssignableFrom(clazz)
|| CoverageStoreInfo.class.isAssignableFrom(clazz)
Expand All @@ -36,19 +43,20 @@ public Object secure(Object object, WrapperPolicy policy) {

Class clazz = object.getClass();
if (CoverageInfo.class.isAssignableFrom(clazz))
return new SecuredCoverageInfo((CoverageInfo) object, policy);
return new SecuredCoverageInfo(unwrap((CoverageInfo) object), policy);
else if (CoverageStoreInfo.class.isAssignableFrom(clazz))
return new SecuredCoverageStoreInfo((CoverageStoreInfo) object, policy);
return new SecuredCoverageStoreInfo(unwrap((CoverageStoreInfo) object), policy);
else if (DataStoreInfo.class.isAssignableFrom(clazz))
return new SecuredDataStoreInfo((DataStoreInfo) object, policy);
return new SecuredDataStoreInfo(unwrap((DataStoreInfo) object), policy);
else if (FeatureTypeInfo.class.isAssignableFrom(clazz))
return new SecuredFeatureTypeInfo((FeatureTypeInfo) object, policy);
return new SecuredFeatureTypeInfo(unwrap((FeatureTypeInfo) object), policy);
else if (LayerInfo.class.isAssignableFrom(clazz))
return new SecuredLayerInfo((LayerInfo) object, policy);
return new SecuredLayerInfo(unwrap((LayerInfo) object), policy);
else if (WMTSLayerInfo.class.isAssignableFrom(clazz))
return new SecuredWMTSLayerInfo(unwrap((WMTSLayerInfo) object), policy);
else
throw new IllegalArgumentException("Don't know how to wrap");
throw new IllegalArgumentException("Don't know how to wrap " + object);
}

/**
* Returns {@link ExtensionPriority#LOWEST} since the wrappers generated by
* this factory
Expand All @@ -57,4 +65,113 @@ public int getPriority() {
return ExtensionPriority.LOWEST;
}

private void logDoubleWrap(Object unwrapped, Object orig) {
String msg = String.format("Tried to double secure: %s already securing %s", orig, unwrapped);
LOGGER.warning(msg);
}

private void logDoubleWrap(final DataStoreInfo store) {
int proxies = 0;
int secured = 0;
DataStoreInfo object = store;
while (true) {
DataStoreInfo unwrapped;
if (object instanceof SecuredDataStoreInfo) {
secured++;
unwrapped = ((SecuredDataStoreInfo) object).unwrap(DataStoreInfo.class);
} else {
unwrapped = ModificationProxy.unwrap(object);
boolean isProxy = object != unwrapped;
if (isProxy) {
proxies++;
} else {
break;
}
}
object = unwrapped;
}

String msg = String.format("Double securing %s: proxies: %,d, SecuredDataStoreInfos: %,d",
store.getName(), proxies, secured);
LOGGER.warning(msg);
}

private WMTSLayerInfo unwrap(WMTSLayerInfo object) {
WMTSLayerInfo unwrapped = ModificationProxy.unwrap(object);
boolean isProxy = object != unwrapped;
if(unwrapped instanceof SecuredWMTSLayerInfo) {
logDoubleWrap(unwrapped, object);
// object = ((SecuredWMTSLayerInfo)unwrapped).unwrap(WMTSLayerInfo.class);
// if(isProxy) {
// object = ModificationProxy.create(object, WMTSLayerInfo.class);
// }
}
return object;
}

private LayerInfo unwrap(LayerInfo object) {
LayerInfo unwrapped = ModificationProxy.unwrap(object);
boolean isProxy = object != unwrapped;
if(unwrapped instanceof SecuredLayerInfo) {
logDoubleWrap(unwrapped, object);
// object = ((SecuredLayerInfo)unwrapped).unwrap(LayerInfo.class);
// if(isProxy) {
// object = ModificationProxy.create(object, LayerInfo.class);
// }
}
return object;
}

private FeatureTypeInfo unwrap(FeatureTypeInfo object) {
FeatureTypeInfo unwrapped = ModificationProxy.unwrap(object);
boolean isProxy = object != unwrapped;
if(unwrapped instanceof SecuredFeatureTypeInfo) {
logDoubleWrap(unwrapped, object);
// object = ((SecuredFeatureTypeInfo)unwrapped).unwrap(FeatureTypeInfo.class);
// if(isProxy) {
// object = ModificationProxy.create(object, FeatureTypeInfo.class);
// }
}
return object;
}

private CoverageStoreInfo unwrap(CoverageStoreInfo object) {
CoverageStoreInfo unwrapped = ModificationProxy.unwrap(object);
boolean isProxy = object != unwrapped;
if(unwrapped instanceof SecuredCoverageStoreInfo) {
logDoubleWrap(unwrapped, object);
// object = ((SecuredCoverageStoreInfo)unwrapped).unwrap(CoverageStoreInfo.class);
// if(isProxy) {
// object = ModificationProxy.create(object, CoverageStoreInfo.class);
// }
}
return object;
}

private CoverageInfo unwrap(CoverageInfo object) {
CoverageInfo unwrapped = ModificationProxy.unwrap(object);
boolean isProxy = object != unwrapped;
if(unwrapped instanceof SecuredDataStoreInfo) {
logDoubleWrap(unwrapped, object);
// object = ((SecuredCoverageInfo)unwrapped).unwrap(CoverageInfo.class);
// if(isProxy) {
// object = ModificationProxy.create(object, CoverageInfo.class);
// }
}
return object;
}

private DataStoreInfo unwrap(DataStoreInfo object) {
DataStoreInfo unwrapped = ModificationProxy.unwrap(object);
boolean isProxy = object != unwrapped;
if(unwrapped instanceof SecuredDataStoreInfo) {
logDoubleWrap(object);
// object = ((SecuredDataStoreInfo)unwrapped).unwrap(DataStoreInfo.class);
// if(isProxy) {
// object = ModificationProxy.create(object, DataStoreInfo.class);
// }
}
return object;
}

}
2 changes: 1 addition & 1 deletion src/main/src/main/java/org/geoserver/security/decorators/SecuredCoverageInfo.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public GridCoverageReader getGridCoverageReader(ProgressListener listener,

@Override
public CoverageStoreInfo getStore() {
return new SecuredCoverageStoreInfo(super.getStore(), policy);
return (CoverageStoreInfo) SecuredObjects.secure(super.getStore(), policy);
}

}
8 changes: 4 additions & 4 deletions src/main/src/main/java/org/geoserver/security/decorators/SecuredLayerInfo.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,13 @@ public ResourceInfo getResource() {
if (r == null)
return null;
else if (r instanceof FeatureTypeInfo)
return new SecuredFeatureTypeInfo((FeatureTypeInfo) r, policy);
return (FeatureTypeInfo) SecuredObjects.secure(r, policy);
else if (r instanceof CoverageInfo)
return new SecuredCoverageInfo((CoverageInfo) r, policy);
return (CoverageInfo) SecuredObjects.secure(r, policy);
else if (r instanceof WMSLayerInfo)
return new SecuredWMSLayerInfo((WMSLayerInfo) r, policy);
return (WMSLayerInfo) SecuredObjects.secure(r, policy);
else if (r instanceof WMTSLayerInfo)
return new SecuredWMTSLayerInfo((WMTSLayerInfo) r, policy);
return (WMTSLayerInfo) SecuredObjects.secure(r, policy);
else
throw new RuntimeException("Don't know how to make resource of type " + r.getClass());
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/src/main/java/org/geoserver/security/decorators/SecuredObjects.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public static Object secure(Object object, WrapperPolicy policy) {

// if we already know what can handle the wrapping, just do it, don't
// scan the extension points once more
Class clazz = object.getClass();
Class<?> clazz = object.getClass();
SecuredObjectFactory candidate = FACTORY_CACHE.get(clazz);

// otherwise scan and store (or complain)
Expand Down

0 comments on commit 670eec9

Please sign in to comment.