Hi, I actually also coded the "wrap the facade" path which has no interface breaking changes:
https://github.com/geoserver/geoserver/pull/1823 The SecureCatalogFacade is not a pretty sight, but changes are otherwise much more contained Cheers Andrea On Fri, Sep 16, 2016 at 12:46 PM, Andrea Aime <andrea.a...@geo-solutions.it> wrote: > Ok, > took a stab at it, pull request is here: > https://github.com/geoserver/geoserver/pull/1822 > > Most of the options to alter the I've explored have some issues (including > the chosen one): > > 1. Just adding the extra method with SortBy... breaks clients passing > null as the sortyby, because the compiler cannot choose anymore which > version to use > 2. Just changing the existing method to use SortBy... still causes > issues with code currently passing null make the implementations get a > SortBy[] {null} > 3. Adding a new method with a SortBy[] suffers from the same issues as > above > > In the end I went for 2 taking into account that the array can get null > values in and sanitizing them out in the implementations (as you say > catalog is not > really meant to be extended). > > A different approach could have been to return a SecureCatalogFacade from > SecureCatalogImpl, but the CatalogFacade interface is huge and I did not > quite like the idea of a facade wrapping a catalog wrapping other catalogs > which ultimately wrap another facade. Imho client code should just not > access the facade underlying the catalog, and indeed CSW is the only > exception. > > So this is a fix, an important one, that breaks the Catalog interface. But > that's an interface which is not really pluggable and/or meant to be > replaced. > So, any objection to backporting? If there are objections, could you > please also report how you'd fix the security issue? > > Cheers > Andrea > > > On Thu, Sep 15, 2016 at 11:40 AM, Niels Charlier <ni...@scitus.be> wrote: > >> Hi Andrea, >> >> As I can remember, the security leak was not done on purpose. I remember >> needing multiple SortBy's and being confused by the API. This security >> issue is likely an oversight on my behalf. >> >> The difference in signature between Catalog and CatalogFacade is not very >> logical/consistent. I would correct the interface. There are only three >> direct implementations (regular, secured and a decorator) in geoserver, and >> I doubt that there are other implementations with third parties, since it >> is rather CatalogFacade that you must override to make your own catalog. >> >> Kind Regards >> Niels >> >> >> >> >> On 14-09-16 18:30, Andrea Aime wrote: >> >> Hi, >> I've been asked to look at an issue with CSW that apparently is ignoring >> the >> security filters and always returning all of the layers metadata to any >> user (including anonymous ones). >> >> At first I thought the code was referring directly the internalCatalog >> bean, but >> no, it's a different variation of the same issue: instead of using the >> catalog, >> the code is extracting the CatalogFacade from it, and then queries the >> facade: >> >> https://github.com/geoserver/geoserver/blob/master/src/exten >> sion/csw/core/src/main/java/org/geoserver/csw/store/intern >> al/CatalogStoreFeatureIterator.java#L86 >> >> This ends up dodging all the security, which is not wrapped around the >> facade. >> As far as I can tell the queries are hitting the facade because there is >> a need to sort over multiple sortby in CSW, and the methods to query are >> exposed as follows: >> >> Catalog.java: >> public <T extends CatalogInfo> CloseableIterator<T> list(final >> Class<T> of, >> final Filter filter, @Nullable Integer offset, @Nullable >> Integer count, >> @Nullable SortBy sortBy); >> >> CatalogFacade.java: >> /** >> * @return an iterator over the catalog objects of the requested type >> that match the given >> * filter >> */ >> public <T extends CatalogInfo> CloseableIterator<T> list(final >> Class<T> of, >> final Filter filter, @Nullable Integer offset, @Nullable >> Integer count, >> @Nullable SortBy... sortOrder); >> >> See the difference? To make sure CSW honors security, the Catalog >> interface would have to be >> modified to support multiple sortBy. >> To avoid breaking the interface I guess I could use java 8 default >> methods which use getFacade().list(...), >> and then just override that implementation in the SecuredCatalog, but I'm >> afraid that might have non >> obvious side effects in other implementations... better be explicit and >> break the interface instead? >> >> Btw, Jira already contains a report due to this issue, but it does not >> picture the entire problem (complete >> lack of security control in CSW): >> https://osgeo-org.atlassian.net/browse/GEOS-6200 >> >> I'm also wondering if by any chance dodging security was done on purpose, >> if that's the case, >> we might want to add a system setting to get the current behavior, in >> case the original sponsors >> of the current implementation actually need it. >> >> Cheers >> Andrea >> >> -- >> == >> GeoServer Professional Services from the experts! Visit >> http://goo.gl/it488V for more information. >> == >> >> Ing. Andrea Aime >> @geowolf >> Technical Lead >> >> GeoSolutions S.A.S. >> Via di Montramito 3/A >> 55054 Massarosa (LU) >> phone: +39 0584 962313 >> fax: +39 0584 1660272 >> mob: +39 339 8844549 >> >> http://www.geo-solutions.it >> http://twitter.com/geosolutions_it >> >> *AVVERTENZE AI SENSI DEL D.Lgs. 196/2003* >> >> Le informazioni contenute in questo messaggio di posta elettronica e/o >> nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il >> loro utilizzo è consentito esclusivamente al destinatario del messaggio, >> per le finalità indicate nel messaggio stesso. Qualora riceviate questo >> messaggio senza esserne il destinatario, Vi preghiamo cortesemente di >> darcene notizia via e-mail e di procedere alla distruzione del messaggio >> stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, >> divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od >> utilizzarlo per finalità diverse, costituisce comportamento contrario ai >> principi dettati dal D.Lgs. 196/2003. >> >> >> >> The information in this message and/or attachments, is intended solely >> for the attention and use of the named addressee(s) and may be confidential >> or proprietary in nature or covered by the provisions of privacy act >> (Legislative Decree June, 30 2003, no.196 - Italy's New Data Protection >> Code).Any use not in accord with its purpose, any disclosure, reproduction, >> copying, distribution, or either dissemination, either whole or partial, is >> strictly forbidden except previous formal approval of the named >> addressee(s). If you are not the intended recipient, please contact >> immediately the sender by telephone, fax or e-mail and delete the >> information in this message that has been received in error. The sender >> does not give any warranty or accept liability as the content, accuracy or >> completeness of sent messages and accepts no responsibility for changes >> made after they were sent or for other risks which arise as a result of >> e-mail transmission, viruses, etc. >> >> ------------------------------------------------------- >> >> >> > > > -- > == > GeoServer Professional Services from the experts! Visit > http://goo.gl/it488V for more information. > == > > Ing. Andrea Aime > @geowolf > Technical Lead > > GeoSolutions S.A.S. > Via di Montramito 3/A > 55054 Massarosa (LU) > phone: +39 0584 962313 > fax: +39 0584 1660272 > mob: +39 339 8844549 > > http://www.geo-solutions.it > http://twitter.com/geosolutions_it > > *AVVERTENZE AI SENSI DEL D.Lgs. 196/2003* > > Le informazioni contenute in questo messaggio di posta elettronica e/o > nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il > loro utilizzo è consentito esclusivamente al destinatario del messaggio, > per le finalità indicate nel messaggio stesso. Qualora riceviate questo > messaggio senza esserne il destinatario, Vi preghiamo cortesemente di > darcene notizia via e-mail e di procedere alla distruzione del messaggio > stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, > divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od > utilizzarlo per finalità diverse, costituisce comportamento contrario ai > principi dettati dal D.Lgs. 196/2003. > > > > The information in this message and/or attachments, is intended solely for > the attention and use of the named addressee(s) and may be confidential or > proprietary in nature or covered by the provisions of privacy act > (Legislative Decree June, 30 2003, no.196 - Italy's New Data Protection > Code).Any use not in accord with its purpose, any disclosure, reproduction, > copying, distribution, or either dissemination, either whole or partial, is > strictly forbidden except previous formal approval of the named > addressee(s). If you are not the intended recipient, please contact > immediately the sender by telephone, fax or e-mail and delete the > information in this message that has been received in error. The sender > does not give any warranty or accept liability as the content, accuracy or > completeness of sent messages and accepts no responsibility for changes > made after they were sent or for other risks which arise as a result of > e-mail transmission, viruses, etc. > > ------------------------------------------------------- > -- == GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information. == Ing. Andrea Aime @geowolf Technical Lead GeoSolutions S.A.S. Via di Montramito 3/A 55054 Massarosa (LU) phone: +39 0584 962313 fax: +39 0584 1660272 mob: +39 339 8844549 http://www.geo-solutions.it http://twitter.com/geosolutions_it *AVVERTENZE AI SENSI DEL D.Lgs. 196/2003* Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003. The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy's New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc. -------------------------------------------------------
------------------------------------------------------------------------------
_______________________________________________ Geoserver-devel mailing list Geoserver-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geoserver-devel
