Added managing of SVG format in applytSLD - added thanks to Regione Toscana SITA #4842

luipir · 2014-01-16T08:19:17Z

Added support to svg when creating map from SLD
Added image/svg-xml MIME type suppot and creation of SYMBOL for each svg and setting MS_SYMBOL_SVG

pull request limit SVG loading only if it's not remote (http)

luipir · 2014-01-16T08:20:57Z

a clean pull request. commets referred to closed pull request:
#4836

tbonfort · 2014-01-16T09:39:32Z

mapsymbol.c

@@ -398,9 +399,18 @@ int msAddImageSymbol(symbolSetObj *symbolset, char *filename)
      symbol->full_pixmap_path = msStrdup(msBuildPath(szPath, NULL, filename));
    }
    symbol->imagepath = msStrdup(filename);
+    symbol->inmapfile = MS_TRUE;


this should not be set. it indicates the symbol block was read from the mapfile instead of the symbolset

ha ok... so it should be set at the parse sld level.

it does not need to be set at all

probabily I didn't explain well
the context is apply a SLD to mapfile
the anomaly rise up due the fact that Mark symbol are shown in mapfile but the svg symbol not
In fact for Mark symbol, the symbol->inmapfile is set to true

mapogcsld.c:1747 - msSLDGetMarkSymbol

I'm agree that, with your explication, symbol->inmapfile = MS_TRUE shouldn't be in msAddImageSymbol, my idea is this should be done at the level of parsing the sld.

I can confirm that it's not needed. The only place where it's used is when writing a mapfile to disk, and even in that use-case it is not necessary for these "shortcut" symbols.

tbonfort · 2014-01-16T09:43:08Z

so, does this one actually work, I don't see any difference with the previous one you said was failing? Again, I'm not sure this should go into the 6.4 branch without a review from the devs (loading arbitrary SVG files from the internet without validation patterns (given this is SLD) seems risky. we do however do it for pixmap symbols already): @sdlime , @dmorissette , @rouault ?

luipir · 2014-01-16T10:16:04Z

@tbonfort I didn't know that github auto update pull request... before this automatic update I saw my merge commit creating problem to travis compilation so I created a new one, sorry for the noise.

about merging in 6.4 it's up to you... in this case My pull request try to solve getting svg from local so a mediate solution could be adding a control to avoid set svg if it's an url but set it if it's a local path.

luipir · 2014-02-04T07:41:36Z

news from @sdlime , @dmorissette , @rouault ?

pcav · 2014-02-04T08:37:44Z

I understand the security implications of this PR. I think that limiting SVG loading to local filesystem should avoid any major issue: would a patch be acceptable if so modified?
In case, we are ready to fix it this way.

dmorissette · 2014-02-07T22:38:04Z

I am also concerned by the possible security implications, but am not familiar enough with the SVG format to know if there is a real risk of injection through SVG files.

On the mapserver-dev list, @sdlime suggested limiting SVG loading to local filesystem by default and adding the ability to enable the use of remote SVG or image files in SLDs using a metadata such as OWS_ALLOW_REMOTE_SLD_ASSETS, false by default. I like this suggestion.

Since this is not a trivial bug fix and is actually extending a feature, I'd suggest merging to master only (and @luipir wrote that they don't need it in 6.4 anyway).

pcav · 2014-02-09T07:19:14Z

Seems a good solution to me.

tbonfort · 2014-02-10T10:04:29Z

We should probably also be disallowing the loading of remote png/gif images, as those can also be an attack vector through vulnerabilities in giflib/libpng. I also think that adding some kind of configuration to disallow remote SLD assets, or maybe even better to require a VALIDATION entry for those assets, is the correct way to go. The problem here is that this kind of change is really not a good fit for a point-release as it breaks backwards compatibility for those that were already using loading of remote images through SLD.
I can't find any discussion on the mailing list as to the acceptance of the changes in #3662 that lead to allowing remote assets to be loaded without any further verification, it would have been needed imho...

luipir · 2014-03-03T11:29:09Z

I'll modify pull request avoiding load remote svg by default using compilation variable: OWS_ALLOW_REMOTE_SLD_ASSETS

…oscana - SITA added support to svg managing creation of SYMBOL for each svg symbol and setting MS_SYMBOL_SVG removed unused inlude added check in case externalGraphic svg is remote => by default refused

luipir · 2014-03-03T14:57:24Z

Hi, I modified pull request adding control as requested in this thread
I've also removed a USE_CURL control in mapogcsld.c:2079 because it seems usefulness and managed more internally in mapsymbol.c: msAddImageSymbol(...)

let me know

luipir · 2014-03-12T16:57:24Z

no news?

sdlime · 2014-03-12T17:13:53Z

Looks like we're good to go. @tbonfort do you want to merge into 6.4 and master?

tbonfort · 2014-03-12T17:19:19Z

I don't think this is ready for inclusion:

ExternalGraphic references an url, so the USE_CURL check in that case was correct I would say.
The OWS_ALLOW_REMOTE_SLD_ASSETS define should at least be added to the build system and be driven by cmake. Rather than it being defined at compile time, I think this should be configurable at the mapfile level (i.e. without a recompile), and not be limited to OWS/SLD (it applies to all requests with your patch). Something like CONFIG ALLOW_REMOTE_SYMBOL_ASSETS TRUE

luipir · 2014-03-12T17:45:01Z

Hi @tbonfort
"ExternalGraphic references an url, so the if you check in that case was correct I would say."
are you referring to "USE_CURL control in mapogcsld.c:2079"?
I think it's correctly managed by the next call to mapogcsld.c:211 msGetSymbolIndex(...) that call
mapfile.c:349 msAddImageSymbol(...) here mapsymbol.c:388USE_CURL is correctly managed

here no SVG is added if it is an external reference and here is managed OWS_ALLOW_REMOTE_SLD_ASSETS

about OWS_ALLOW_REMOTE_SLD_ASSETS your observation is that I should add it build system as part of pull request?

tbonfort · 2014-03-12T17:51:33Z

"USE_CURL control in mapogcsld.c:2079 (...): OK

I think it should not be a compile time option but configured through the mapfile. From your patch, it is a compile time option that cannot be set by a user as it is not managed by the build system.

luipir · 2014-03-12T19:35:58Z

@tbonfort "From your patch, it is a compile" this is because as I understood comments by @dmorissette referring to OWS_ALLOW_REMOTE_SLD_ASSETS. But if in other way, please explain me so I can change pull request.
How I can introduce a OWS_ALLOW_REMOTE_SLD_ASSETS map file option? ther's an example to replicate?

tbonfort · 2014-03-12T20:33:54Z

You can find usage of CONFIG options here: https://github.com/mapserver/mapserver/blob/branch-6-4/mapdraw.c#L207
Please do not use OWS_ OWS_ALLOW_REMOTE_SLD_ASSETS as a keyword, as this option is not limited to OWS modes or SLD. I would suggest ALLOW_REMOTE_ASSETS.

luipir · 2014-03-13T14:13:58Z

thank you @tbonfort but how can I use msTestConfigOption inside msAddImageSymbol(...)? I can't find a way to access map to get this paramter.

The only place I've access to map in the upper call... in msSLDParseExternalGraphic. I'm preparing pull request in this way

luipir · 2014-03-13T14:21:37Z

I don't believe msSLDParseExternalGraphic it the correct place to manage ALLOW_REMOTE_ASSETS parameter. In this way It will be manage only in svg entered using applaySld but I imagin this control should be general applied to all svg (and pixmap?) symbols.

the best place where to apply is in msAddImageSymbol but can't find a way to access map struct to get option parameter.

please let me know a direction

…config option instead of copilation flag

luipir · 2014-03-15T12:21:06Z

I changed pull request to align with the last comments... I didn't find a way to generalize external symbol control that should be in msAddImageSymbol instead of only in msSLDParseExternalGraphic.

I tested using sld with external svg and they are filtered by default... this filter is applied to all external symbols not only svg when importing sld

luipir · 2014-03-15T14:59:04Z

reopened with pull request in master:
#4883

luipir mentioned this pull request Jan 16, 2014

added format "image/svg+xml" to msSLDParseExternalGraphic - added thanks to Faunalia and Regione Toscana - SITA #4836

Closed

tbonfort reviewed Jan 16, 2014
View reviewed changes

added managing of SVG format in applytSLD - added thanks to Regione T…

b093463

…oscana - SITA added support to svg managing creation of SYMBOL for each svg symbol and setting MS_SYMBOL_SVG removed unused inlude added check in case externalGraphic svg is remote => by default refused

moved check to external symbol in applySld using ALLOW_REMOTE_ASSETS …

e298e6f

…config option instead of copilation flag

luipir closed this Mar 15, 2014

luipir mentioned this pull request Mar 15, 2014

Require Validation of SLD ExternalGraphic OnlineResource #4883

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added managing of SVG format in applytSLD - added thanks to Regione Toscana SITA #4842

Added managing of SVG format in applytSLD - added thanks to Regione Toscana SITA #4842

luipir commented Jan 16, 2014

luipir commented Jan 16, 2014

tbonfort Jan 16, 2014

luipir Jan 16, 2014

tbonfort Jan 16, 2014

luipir Jan 16, 2014

tbonfort Jan 16, 2014

luipir Jan 16, 2014

tbonfort commented Jan 16, 2014

luipir commented Jan 16, 2014

luipir commented Feb 4, 2014

pcav commented Feb 4, 2014

dmorissette commented Feb 7, 2014

pcav commented Feb 9, 2014

tbonfort commented Feb 10, 2014

luipir commented Mar 3, 2014

luipir commented Mar 3, 2014

luipir commented Mar 12, 2014

sdlime commented Mar 12, 2014

tbonfort commented Mar 12, 2014

luipir commented Mar 12, 2014

tbonfort commented Mar 12, 2014

luipir commented Mar 12, 2014

tbonfort commented Mar 12, 2014

luipir commented Mar 13, 2014

luipir commented Mar 13, 2014

luipir commented Mar 15, 2014

luipir commented Mar 15, 2014

Navigation Menu

Added managing of SVG format in applytSLD - added thanks to Regione Toscana SITA #4842

Added managing of SVG format in applytSLD - added thanks to Regione Toscana SITA #4842

Conversation

luipir commented Jan 16, 2014

luipir commented Jan 16, 2014

tbonfort Jan 16, 2014

Choose a reason for hiding this comment

luipir Jan 16, 2014

Choose a reason for hiding this comment

tbonfort Jan 16, 2014

Choose a reason for hiding this comment

luipir Jan 16, 2014

Choose a reason for hiding this comment

tbonfort Jan 16, 2014

Choose a reason for hiding this comment

luipir Jan 16, 2014

Choose a reason for hiding this comment

tbonfort commented Jan 16, 2014

luipir commented Jan 16, 2014

luipir commented Feb 4, 2014

pcav commented Feb 4, 2014

dmorissette commented Feb 7, 2014

pcav commented Feb 9, 2014

tbonfort commented Feb 10, 2014

luipir commented Mar 3, 2014

luipir commented Mar 3, 2014

luipir commented Mar 12, 2014

sdlime commented Mar 12, 2014

tbonfort commented Mar 12, 2014

luipir commented Mar 12, 2014

tbonfort commented Mar 12, 2014

luipir commented Mar 12, 2014

tbonfort commented Mar 12, 2014

luipir commented Mar 13, 2014

luipir commented Mar 13, 2014

luipir commented Mar 15, 2014

luipir commented Mar 15, 2014