Denials of service with SLD parameter in WMS requests

[tbonfort]

I've identified several flows in the handling of the SLD parameter of WMS GetFeature requests :

• [1] you can pass SLD=file:///some/path/on/file/system. This will read the local file, but as the curl return code is 0 and not 200, the download will be considered as failed. I don't think that a remote attacker could get back that content, but it can lead to big local files being read and stored in the temporary file : 30 second timeout * 40 MB/sec = 1.2 GB

--> We could check in msHTTPGetFile() that the URL starts with http:// or https://

• [2] a valid HTTP URL could also lead to a big file being read. The 30 second timeout can be sufficient for large files being transfered if you have a connexion with large bandwith. This could also potentially lead to huge memory allocation (let's say that the path is a path to a huge raster file) in msSLDApplySLDURL() when it ingests the file.

--> Could be mitigated by making sure that msHTTPWriteFct (in the case of dowloading a SLD or an external graphic from msSLDParseExternalGraphic()) stops after downloading, let's say 1 MB, 10 MB ?

• [3] msSLDApplySLDURL() doesn't unlink the temporary file in case of failure, which leads to accumulation of potentially big files --> can be easily fixed
• [4] Even a 1 MB SLD can be large enough to make the CPU of a MapServer instance busy for a few thousand years.

For example:

<?xml version="1.0"?>
<StyledLayerDescriptor version="1.0.0">
<NamedLayer><Name>bla</Name></NamedLayer>
<NamedLayer><Name>bla</Name></NamedLayer>
[ repeated 10000 times ]
<NamedLayer><Name>bla</Name></NamedLayer>
</StyledLayerDescriptor>

Breaking in the debugger shows that the time is spent in msGetLayerIndex(). Indeed, the issue is in msSLDApplySLD() that has :

for (m=0; m<nLayers; m++) {
    for (l=0; l<nLayers; l++) {
        nIndex = msGetLayerIndex(map, pasLayers[m].name);

The complexity of this is O(nLayers^3).

The above complexity could likely be reduced in O(nLayers * log(nLayers)) by using smart data structures (hashtable, etc...). An easier workaround would be to validate that nLayers isn't too large, 100, 1000 ? For reference, on my PC, 1000 layers means 13 seconds.

• [5] you can block a whole MapServer cluster for 30 seconds with a single request. Look at the following request on my local lighttpd server that has been configured with a maximum of 4 simultaneous instances :

The structure is : GET_FEATURE_URL&SLD=url_encode_of(GET_FEATURE_URL&SLD=url_encode_of(GET_FEATURE_URL&SLD=url_encode_of(GET_FEATURE_URL&SLD=url_encode_of(GET_FEATURE_URL))))

Example :
http://127.0.0.1:80/cgi-bin/mapserv.fcgi?map=/home/even/mapserver/git/mapserver/msautotest/wxs/wms_sld.map&SERVICE=WMS&VERSION=1.3.0&REQUEST=GetMap&CRS=EPSG:4326&WIDTH=560&HEIGHT=350&LAYERS=road_styles,road_styles,road_styles&FORMAT=image/png&BGCOLOR=0xFFFFFF&TRANSPARENT=FALSE&EXCEPTIONS=INIMAGE&BBOX=44.6139013125,-66.998804375,48.0904731875,-61.436289375&SLD=http%3A%2F%2F127.0.0.1%3A80%2Fcgi-bin%2Fmapserv.fcgi%3Fmap%3D%2Fhome%2Feven%2Fmapserver%2Fgit%2Fmapserver%2Fmsautotest%2Fwxs%2Fwms_sld.map%26SERVICE%3DWMS%26VERSION%3D1.3.0%26REQUEST%3DGetMap%26CRS%3DEPSG%3A4326%26WIDTH%3D560%26HEIGHT%3D350%26LAYERS%3Droad_styles%2Croad_styles%2Croad_styles%26FORMAT%3Dimage%2Fpng%26BGCOLOR%3D0xFFFFFF%26TRANSPARENT%3DFALSE%26EXCEPTIONS%3DINIMAGE%26BBOX%3D44.6139013125%2C-66.998804375%2C48.0904731875%2C-61.436289375%26SLD%3Dhttp%253A%252F%252F127.0.0.1%253A80%252Fcgi-bin%252Fmapserv.fcgi%253Fmap%253D%252Fhome%252Feven%252Fmapserver%252Fgit%252Fmapserver%252Fmsautotest%252Fwxs%252Fwms_sld.map%2526SERVICE%253DWMS%2526VERSION%253D1.3.0%2526REQUEST%253DGetMap%2526CRS%253DEPSG%253A4326%2526WIDTH%253D560%2526HEIGHT%253D350%2526LAYERS%253Droad_styles%252Croad_styles%252Croad_styles%2526FORMAT%253Dimage%252Fpng%2526BGCOLOR%253D0xFFFFFF%2526TRANSPARENT%253DFALSE%2526EXCEPTIONS%253DINIMAGE%2526BBOX%253D44.6139013125%252C-66.998804375%252C48.0904731875%252C-61.436289375%2526SLD%253Dhttp%25253A%25252F%25252F127.0.0.1%25253A80%25252Fcgi-bin%25252Fmapserv.fcgi%25253Fmap%25253D%25252Fhome%25252Feven%25252Fmapserver%25252Fgit%25252Fmapserver%25252Fmsautotest%25252Fwxs%25252Fwms_sld.map%252526SERVICE%25253DWMS%252526VERSION%25253D1.3.0%252526REQUEST%25253DGetMap%252526CRS%25253DEPSG%25253A4326%252526WIDTH%25253D560%252526HEIGHT%25253D350%252526LAYERS%25253Droad_styles%25252Croad_styles%25252Croad_styles%252526FORMAT%25253Dimage%25252Fpng%252526BGCOLOR%25253D0xFFFFFF%252526TRANSPARENT%25253DFALSE%252526EXCEPTIONS%25253DINIMAGE%252526BBOX%25253D44.6139013125%25252C-66.998804375%25252C48.0904731875%25252C-61.436289375%252526SLD%25253Dhttp%2525253A%2525252F%2525252F127.0.0.1%2525253A80%2525252Fcgi-bin%2525252Fmapserv.fcgi%2525253Fmap%2525253D%2525252Fhome%2525252Feven%2525252Fmapserver%2525252Fgit%2525252Fmapserver%2525252Fmsautotest%2525252Fwxs%2525252Fwms_sld.map%25252526SERVICE%2525253DWMS%25252526VERSION%2525253D1.3.0%25252526REQUEST%2525253DGetMap%25252526CRS%2525253DEPSG%2525253A4326%25252526WIDTH%2525253D560%25252526HEIGHT%2525253D350%25252526LAYERS%2525253Droad_styles%2525252Croad_styles%2525252Croad_styles%25252526FORMAT%2525253Dimage%2525252Fpng%25252526BGCOLOR%2525253D0xFFFFFF%25252526TRANSPARENT%2525253DFALSE%25252526EXCEPTIONS%2525253DINIMAGE%25252526BBOX%2525253D44.6139013125%2525252C-66.998804375%2525252C48.0904731875%2525252C-61.436289375

--> Could be mitigated by validating that the SLD URL doesn't contain % and ? characters ? And that it has a .xml or .sld extension ?

Of course all of this can be avoided by defining "ows_sld_enabled" "false" in the mapfile, but as true is the default value, many servers are potentially vulnerable. The documentation doesn't really warns the user about the implications of enabling SLD. Perhaps should the default value be changed to "false" for 6.4 or 7.0 ?

[aperi2007]

Of course all of this can be avoided by defining "ows_sld_enabled" "false" in the mapfile, but as true is the default
value, many servers are potentially vulnerable. The documentation doesn't really warns the user about the
implications of enabling SLD. Perhaps should the default value be changed to "false" for 6.4 or 7.0 ?

Set
"ows_sld_enabled" "false"
will resolve, but also deny the use of sld remote.

I guess a good solution could be to allow the use of sld remote only from selected sources.
So a solution like the "ows_allowed_ip_list" to allow the use of sld to some one and deny it to all the others could resolve.

[tbonfort]

milestoning this for 6.4 so we don't forget it. Can anyone familiar with SLD comment on the proposed fixes? ping @mkofahl @dmorissette @tomkralidis

[tomkralidis]

I like the size limit put forth in/for msHTTPWriteFct. We could default to, like, 1MB, and if it's reached, throw a ServiceExceptionReport saying file size exceeds configured default.

We could also add the ability for users to set this with `ows_filesize_limit" "bytes" (which could be used for many other areas potentially in the code where MapServer is fetching remote resources.

[tbonfort]

for the file:// part of this issue, see http://curl.haxx.se/libcurl/c/curl_easy_setopt.html#CURLOPTPROTOCOLS where we would only allow http and https (we might want to add yet another metadata to extend that if needed by the user)

[tbonfort]

For point [4], in 08ac591 I have moved the msGetLayerIndex call out of the inner loop. In theory it would need to be called in the inner loop as the inner loop is potentially adding layers, however the added layers are added to the end of the layer list, which means that nIndex does not change.

[coveralls]

Coverage remained the same when pulling bf8b932 on tbonfort:b4703 into 471cdf4 on mapserver:master.

[coveralls]

Coverage remained the same when pulling 8c5828a on tbonfort:b4703 into 471cdf4 on mapserver:master.

[tbonfort]

for point [3] we now unlink the temporary file in case of error, in 1765c1e

[tbonfort]

@rouault do I understand correctly that point [5] is a case where the server is recursively calling itself? In this case it may actually lead to a complete denial of service, once you have occupied all the webserver's workers. I'm not sure how to mitigate this however, as it seems that such a request is compliant with the spec.

[tbonfort]

For point [2] I have added a default limit of 1 Megabyte to remote SLD size in 0abb694 + typo in 664c7dc . cc @dmorissette . The (ows|wms)_remote_sld_max_bytes metadata value to override this will need to be documented (cc @havatv). We also need to setup some autotests for remote SLD handling.

[rouault]

@tbonfort about [5], yes the server is calling itself recursively and it can completely block itself. However I'm not sure if it is really critical, since an hostile client could just issue X requests with a SLD that points to a URL that timeouts. The trick is [5] is just some fun to do it with just one request, in case a policy on the server would block simultaneous request from the same IP for example.

[tbonfort]

points [1] through [4] rebased,squashed and applied to master in 0294d6a . For [5] I have no idea what to do.

[havatv]

Sorry, typo in the commit message - should be "wms server" (not "wfs server").
Also very short description. Please comment if there is a need for more information there.