Denials of service with SLD parameter in WMS requests #4703

Closed
wants to merge 7 commits into
from

Projects

None yet

6 participants

@rouault
Contributor
rouault commented Jul 13, 2013

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
Member

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
Member

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
Member

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
Member

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 Status

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

@coveralls

Coverage Status

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

@tbonfort
Member

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

@tbonfort
Member

@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
Member

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
Contributor
rouault commented Jul 27, 2013

@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 tbonfort added a commit to mapserver/msautotest_DEPRECATED that referenced this pull request Jul 27, 2013
@tbonfort tbonfort add remote SLD tests
- requires a http server serving the root of the msautotest directory,
  that can be started with python -m SimpleHTTPServer
- tests for mapserver/mapserver#4703
51e609f
@tbonfort tbonfort added a commit to mapserver/msautotest_DEPRECATED that referenced this pull request Jul 27, 2013
@tbonfort tbonfort add test when duplicating a NamedLayer 7a07055
@tbonfort tbonfort added a commit to mapserver/msautotest_DEPRECATED that referenced this pull request Jul 27, 2013
@tbonfort tbonfort add getmap tests with external SLDs
- check working sld
- check 404
- check invalid protocol (add file:// once mapserver/mapserver#4703 is merged)
0060e62
@tbonfort tbonfort added a commit that referenced this pull request Jul 27, 2013
@tbonfort tbonfort add external sld tests for #4703 06b73b3
@tbonfort tbonfort added a commit to mapserver/msautotest_DEPRECATED that referenced this pull request Jul 27, 2013
@tbonfort tbonfort add tests for large external SLD files 5388c00
@tbonfort tbonfort added a commit that referenced this pull request Jul 27, 2013
@tbonfort tbonfort fix denials of service in msApplySLD() (#4703)
Avoid O(N^2) looping on supplied NamedLayers
Only allow HTTP and HTTPS for curl request
unlink temporary sld file in case of error
limit remote SLD downloads to 1 Megabyte by default, can be overriden by the
  (wms|ows)_remote_sld_max_bytes metadata entry
0294d6a
@tbonfort
Member

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

@tbonfort tbonfort added a commit to mapserver/msautotest_DEPRECATED that referenced this pull request Jul 27, 2013
@tbonfort tbonfort add test for file:// rejection in curl codepath (mapserver/mapserver#… 370d10d
@havatv havatv pushed a commit to mapserver/docs that referenced this pull request Jul 27, 2013
Håvard Tveite Added documentation for wms_remote_sld_max_bytes to wfs server (mapse… c4a4f2d
@havatv
Contributor
havatv commented Jul 27, 2013

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.

@tbonfort tbonfort added a commit to mapserver/docs that referenced this pull request Jul 29, 2013
@tbonfort tbonfort fixup for remote_sld_max_bytes b58ef7e
@tbonfort tbonfort closed this Jul 29, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment