New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WMS exception may expose PostGIS connection details for users #4928

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@tbonfort
Member

tbonfort commented Feb 11, 2015

As an example an exception that I could read with my WMS client from demo.mapserver.org when PostgreSQL server was down:


msDrawMap(): Image handling error. Failed to draw layer named 'landuse_layer4'.
msPostGISLayerOpen(): Query error. Database connection failed (FATAL: database "osm" does not exist
) with connect string 'host=localhost dbname=osm user=www-data password=******** port=5432' Is the database running? Is it allowing connections? Does the specified user exist? Is the password valid? Is the database on the standard port?

Everything else than password gets exposed to all WMS users. For me connection details belong only to MS_ERRORFILE.

@dmorissette

This comment has been minimized.

Show comment
Hide comment
@dmorissette

dmorissette May 23, 2014

Contributor

There are several instances of very detailed error messages like this one in the postgis driver. Those details are useful for debugging, but you are right that it is a bit much to expose to the end user. Perhaps they could be converted to a more generic error message via msSetError(), and the details moved to a msDebug() call when layer->debug is set.

Contributor

dmorissette commented May 23, 2014

There are several instances of very detailed error messages like this one in the postgis driver. Those details are useful for debugging, but you are right that it is a bit much to expose to the end user. Perhaps they could be converted to a more generic error message via msSetError(), and the details moved to a msDebug() call when layer->debug is set.

@msmitherdc

This comment has been minimized.

Show comment
Hide comment
@msmitherdc

msmitherdc Dec 12, 2014

Contributor

@sdlime This has recently been identified by our security folks, they don't want to see SQL error messages being generated by mapserver. Is this something we could make configurable and add?

Contributor

msmitherdc commented Dec 12, 2014

@sdlime This has recently been identified by our security folks, they don't want to see SQL error messages being generated by mapserver. Is this something we could make configurable and add?

@sdlime

This comment has been minimized.

Show comment
Hide comment
@sdlime

sdlime Dec 12, 2014

Member

Another, simpler, idea would be so simply log those for debugging purposes and never return them. That way they don't leave the server. --Steve


From: Michael D. Smith [notifications@github.com]
Sent: Thursday, December 11, 2014 6:31 PM
To: mapserver/mapserver
Cc: Lime, Steve D (MNIT)
Subject: Re: [mapserver] WMS exception may expose PostGIS connection details for users (#4928)

@sdlimehttps://github.com/sdlime This has recently been identified by our security folks, they don't want to see SQL error messages being generated by mapserver. Is this something we could make configurable and add?


Reply to this email directly or view it on GitHubhttps://github.com/mapserver/mapserver/issues/4928#issuecomment-66714593.

Member

sdlime commented Dec 12, 2014

Another, simpler, idea would be so simply log those for debugging purposes and never return them. That way they don't leave the server. --Steve


From: Michael D. Smith [notifications@github.com]
Sent: Thursday, December 11, 2014 6:31 PM
To: mapserver/mapserver
Cc: Lime, Steve D (MNIT)
Subject: Re: [mapserver] WMS exception may expose PostGIS connection details for users (#4928)

@sdlimehttps://github.com/sdlime This has recently been identified by our security folks, they don't want to see SQL error messages being generated by mapserver. Is this something we could make configurable and add?


Reply to this email directly or view it on GitHubhttps://github.com/mapserver/mapserver/issues/4928#issuecomment-66714593.

@msmitherdc

This comment has been minimized.

Show comment
Hide comment
@msmitherdc

msmitherdc Dec 12, 2014

Contributor

Well, when debugging it would be nice on the web but if this is simpler, then yes, lets do that.

Contributor

msmitherdc commented Dec 12, 2014

Well, when debugging it would be nice on the web but if this is simpler, then yes, lets do that.

@sdlime

This comment has been minimized.

Show comment
Hide comment
@sdlime

sdlime Dec 12, 2014

Member

Maybe ping mapserver-dev for a preference? We should apply the same policy across all backends.


From: Michael D. Smith [notifications@github.com]
Sent: Friday, December 12, 2014 9:47 AM
To: mapserver/mapserver
Cc: Lime, Steve D (MNIT)
Subject: Re: [mapserver] WMS exception may expose PostGIS connection details for users (#4928)

Well, when debugging it would be nice on the web but if this is simpler, then yes, lets do that.


Reply to this email directly or view it on GitHubhttps://github.com/mapserver/mapserver/issues/4928#issuecomment-66789819.

Member

sdlime commented Dec 12, 2014

Maybe ping mapserver-dev for a preference? We should apply the same policy across all backends.


From: Michael D. Smith [notifications@github.com]
Sent: Friday, December 12, 2014 9:47 AM
To: mapserver/mapserver
Cc: Lime, Steve D (MNIT)
Subject: Re: [mapserver] WMS exception may expose PostGIS connection details for users (#4928)

Well, when debugging it would be nice on the web but if this is simpler, then yes, lets do that.


Reply to this email directly or view it on GitHubhttps://github.com/mapserver/mapserver/issues/4928#issuecomment-66789819.

@dmorissette

This comment has been minimized.

Show comment
Hide comment
@dmorissette

dmorissette Dec 12, 2014

Contributor

+1 to outpout only generic error messages through msSetError(), and to put the details in a corresponding msDebug() log. AFAIK the postgis driver is probably the worst case and the rest of the code base already does the right thing in most cases.

Contributor

dmorissette commented Dec 12, 2014

+1 to outpout only generic error messages through msSetError(), and to put the details in a corresponding msDebug() log. AFAIK the postgis driver is probably the worst case and the rest of the code base already does the right thing in most cases.

@msmitherdc

This comment has been minimized.

Show comment
Hide comment
@msmitherdc

msmitherdc Dec 15, 2014

Contributor

Just to note, this is not just for WMS but also for mode=map (traditional ms cgi) as well

Contributor

msmitherdc commented Dec 15, 2014

Just to note, this is not just for WMS but also for mode=map (traditional ms cgi) as well

@sdlime

This comment has been minimized.

Show comment
Hide comment
@sdlime

sdlime Dec 15, 2014

Member

And shp2img, etc… The changes are probably in mappostgis.c so anything that relies on PostGIS will see generic errors.

Member

sdlime commented Dec 15, 2014

And shp2img, etc… The changes are probably in mappostgis.c so anything that relies on PostGIS will see generic errors.

@msmitherdc

This comment has been minimized.

Show comment
Hide comment
@msmitherdc

msmitherdc Jan 19, 2015

Contributor

This might be a good one to work on at the Philly code sprint

Contributor

msmitherdc commented Jan 19, 2015

This might be a good one to work on at the Philly code sprint

@msmitherdc msmitherdc added this to the 7.0 Release milestone Feb 10, 2015

@msmitherdc

This comment has been minimized.

Show comment
Hide comment
@msmitherdc

msmitherdc Feb 11, 2015

Contributor

@tbonfort

/home/gridusr/local/src/mds_mapserver/maporaclespatial.c: In function 'msOCISetHandlers':
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c:672: warning: passing argument 1 of 'msDebug' makes pointer from integer without a cast
/home/gridusr/local/src/mds_mapserver/maperror.h:170: note: expected 'const char *' but argument is of type 'int'
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c:676: error: too few arguments to function 'msSetError'
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c: In function 'msOracleSpatialLayerOpen':
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c:1724: error: too few arguments to function 'msSetError'
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c: In function 'msOracleSpatialLayerWhichShapes':
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c:1923: error: too few arguments to function 'msSetError'
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c: In function 'msOracleSpatialLayerGetShape':
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c:2566: error: too few arguments to function 'msSetError'
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c: In function 'msOracleSpatialLayerGetAutoProjection':
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c:2768: warning: too many arguments for format
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c: In function 'msOracleSpatialLayerTranslateFilter':
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c:3488: warning: too many arguments for format
Contributor

msmitherdc commented Feb 11, 2015

@tbonfort

/home/gridusr/local/src/mds_mapserver/maporaclespatial.c: In function 'msOCISetHandlers':
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c:672: warning: passing argument 1 of 'msDebug' makes pointer from integer without a cast
/home/gridusr/local/src/mds_mapserver/maperror.h:170: note: expected 'const char *' but argument is of type 'int'
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c:676: error: too few arguments to function 'msSetError'
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c: In function 'msOracleSpatialLayerOpen':
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c:1724: error: too few arguments to function 'msSetError'
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c: In function 'msOracleSpatialLayerWhichShapes':
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c:1923: error: too few arguments to function 'msSetError'
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c: In function 'msOracleSpatialLayerGetShape':
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c:2566: error: too few arguments to function 'msSetError'
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c: In function 'msOracleSpatialLayerGetAutoProjection':
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c:2768: warning: too many arguments for format
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c: In function 'msOracleSpatialLayerTranslateFilter':
/home/gridusr/local/src/mds_mapserver/maporaclespatial.c:3488: warning: too many arguments for format
@tbonfort

This comment has been minimized.

Show comment
Hide comment
@tbonfort

tbonfort Feb 11, 2015

Member

applied in master in 9f7f14f

Member

tbonfort commented Feb 11, 2015

applied in master in 9f7f14f

@tbonfort tbonfort closed this Feb 11, 2015

tbonfort added a commit to tbonfort/mapserver that referenced this pull request Dec 5, 2016

tbonfort added a commit to tbonfort/mapserver that referenced this pull request Dec 5, 2016

tbonfort added a commit to tbonfort/mapserver that referenced this pull request Dec 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment