maperror: size the error image within MAXSIZE (GHSA-qcjf-q672-q63w) - #7625
maperror: size the error image within MAXSIZE (GHSA-qcjf-q672-q63w)#7625MarkLee131 wants to merge 2 commits into
Conversation
Sorry, something went wrong.
msWMSLoadGetMapParams() stores WIDTH/HEIGHT into map->width/map->height as soon as they are parsed, but only validates them against MAXSIZE much later. Several exceptions are raised in between, so an in-image exception could be rendered at the unvalidated request dimensions, letting one unauthenticated GetMap ask for an arbitrarily large allocation. msWriteErrorImage() now falls back to 400x300 unless the map dimensions are within MAXSIZE, which covers every early-exit path, and checks msImageCreate() for NULL so a refused allocation no longer segfaults while reporting an error.
|
@MarkLee131 - the new null check for the image prevents a crash: if (img == NULL) {
/* There is nothing to draw the message on, and the error that brought us
here is already on the error list. */
if (format->refcount == 0)
msFreeOutputFormat(format);
msFree(errormsg);
return;
}
But it will return without running the isreported code below or setting headers. I'm not sure what a server would return in this case? /* the errors are reported */
while (error && error->code != MS_NOERR) {
error->isreported = MS_TRUE;
error = error->next;
}
|
Sorry, something went wrong.
Review question on MapServer#7625: the early return skips the isreported loop and the headers on purpose, so msCGIWriteError() still produces an error page.
|
That's deliberate: skipping the isreported loop is what makes the fallback work. msCGIWriteError() (mapserv.c:336) returns early when the error is already marked reported (mapservutil.c:75), so setting isreported here is what would leave the client with an empty response. Leaving it unset lets that function emit its own error page. No headers have been sent at that point either, since msWriteErrorImage() only sends them once msImageCreate() has succeeded. Tested with MAXSIZE 200000 in the mapfile so the new clamp passes, then requesting 100000x100000 with ASan refusing the allocation. Before, SEGV on unknown address 0x000000000028. After: Status: 400 Bad Request Content-Type: text/html ... msImageCreate(): Memory allocation error. Unable to create new image object. msWMSLoadGetMapParams(): WMS server error. Invalid layer(s) given in the LAYERS parameter. ... So the client gets the usual MapServer error page and the worker survives. I've pushed a comment saying so. |
Sorry, something went wrong.
|
@MarkLee131 - I've been testting this patch locally, thanks for this. I did manage to get a divide by 0 at nLines = (int)ceil((double)nTextLength / (double)nMaxCharsPerLine); causing a result of inf with a request where HEIGHT and WIDTH were 10 - &WIDTH=10&HEIGHT=10&LAYERS=no_such_layer&EXCEPTIONS=INIMAGE. This would have been an issue prior to this PR, but it would be good to add a guard against this too. Something like: if (map->width >= (nMargin * 2) + charWidth && map->width <= map->maxsize &&
map->height > 0 && map->height <= map->maxsize) {
Currently the width has to be greater than 15 to avoid the issue. |
Sorry, something went wrong.
What does this PR do?
msWMSLoadGetMapParams() stores WIDTH/HEIGHT into map->width/map->height as it parses them, but the MAXSIZE check runs about 360 lines later, with ten return msWMSException(...) sites in between. An in-image exception raised at any of them renders the error image at the unvalidated dimensions, so WIDTH=100000&HEIGHT=100000 asks for a 40 GB buffer. No valid layer name and no knowledge of the mapfile is needed. When the allocation is refused rather than granted, the worker segfaults instead, because msWriteErrorImage() does not check msImageCreate() for NULL.
The clamp at mapwms.cpp:1779 already resets to 400x300 "in case errors INIMAGE are used", but only for the exception it raises itself.
Fixed in msWriteErrorImage() rather than in the parser: the map dimensions are used only when within map->maxsize (the same predicate as mapdraw.c:330). That covers all ten paths and the non-WMS callers, and does not change which error any existing request reports. msImageCreate() is now NULL-checked.
Unpatched, the added test returns a 5000x5000 error image and fails; patched, 400x300. I ran the wxs, misc, config, renderers, query, sld, gdal and api suites against both builds and the new test is the only difference.
What are related issues/pull requests?
Fixes https://github.com/MapServer/MapServer/security/advisories/GHSA-qcjf-q672-q63w
Tasklist