geopublisher leaks geoserver user/pass credentials to users with editor profile

[landry]

when the geopublisher is enabled, an user with enough credentials (ie Editor profile) can get access to the full content of geoserver-nodes.xml, and thus to the user/pass couple used by geonetwork to publish data in remote geoservers.

To reproduce, login with an editor profile, and GET /srv/eng/geoserver.publisher?action=LIST. The full content of geoserver-nodes.xml is returned, and contains the user/pass. It's because in web/src/main/java/org/fao/geonet/services/publisher/Do.java, list() directly returns geoserverConfig without sanitizing it from sensitive informations.

Since on the client side GeoPublisherPanel.js only needs id/name/adminUrl/wfsUrl/wmsUrl/wcsUrl/stylerUrl/namespacePrefix informations the user and pass nodes should be removed from the returned XML. <!-- comments --> could be stripped too to reduce size :)

On a sidenote, GeoPublisherPanel is defined in two redundant places : web/src/main/webapp/scripts/editor/app.GeoPublisherPanel.js web-client/src/main/resources/apps/js/GeoNetwork/lib/GeoNetwork/widgets/editor/GeoPublisherPanel.js

The first one seems unused (and registers a wrong ext class 'gn_geopublihserpanel') so maybe it should be removed to avoid confusion ?

[landry]

Can someone look into this ? this is rather bad to ship GN 2.8.0 with that..

[landry]

Fwiw the fix is quite simple, by adding :

String user = node.getChildText("user"); String password = node.getChildText("password");

+ node.removeChild("user"); + node.removeChild("password");

in web/src/main/java/org/fao/geonet/services/publisher/Do.java, init() method when the geoserver-nodes.xml file is parsed. It doesnt trim the xml comments, but at least it's more secure.

[landry]

--- web/src/main/java/org/fao/geonet/services/publisher/Do.java (revision 9275)
+++ web/src/main/java/org/fao/geonet/services/publisher/Do.java (working copy)
@@ -149,6 +149,10 @@
                        String namespaceUrl = node.getChildText("namespaceUrl");
                        String user = node.getChildText("user");
                        String password = node.getChildText("password");
+                       node.removeChild("user");
+                       node.removeChild("password");

is what i meant..

[jesseeichar]

Thanks for the report. I have committed the fix to master and 2.8.x

[landry]

I think the fix is wrong.. you still want user/password to be passed to GeoServerNode ctor so that it can actually connect to the geoserver. The problem here is that geoserverConfig xml is returned directly in list() (which is returned to users with editor profile creds), including user/password. We want to remove that info from the xml, but still read/parse it to pass it to GeoServerNode(). On a sidenote, your commit broke the build by removing user/password String declaration :)

[jesseeichar]

Odd. I wonder how I did that. I made the change then ran it... must have accidently deleted change while checking that it did what I thought.

I have fixed my mistake.

You can see the file at: ​https://github.com/geonetwork/core-geonetwork/blob/master/web/src/main/java/org/fao/geonet/services/publisher/Do.java

[jesseeichar]

What the heck!. Something is going on on my computer. The commits I am pushing are incorrect... I need to try again

[jesseeichar]

No, Just turned out I was being a fool. I have it correct finally. Doing to many things at once :P.