I'm a little uncomfortable with the setting of a default "Unique ID". If it is the same for everyone, then it is super easy to neglect setting up the ID and leave certain information clearly available to a would-be cracker. I'd like to suggest that either this be automatically generated or the following which I'd prefer.

I've observed other modules such as FCKeditor and Google Analytics will simply put a note in the status report that the module is not yet configured. Perhaps this is the safest way to handle this? The note in the status could easily point to the settings page where the unique ID could be entered.

CommentFileSizeAuthor
#18 nagios-672264-NO_default_uniqueID.patch2.03 KBhelmo
#4 Disable if not configured, set unique_ua default to blank2.57 KBflickerfly
#2 Implementation of hook_requirements1.1 KBflickerfly

Comments

kbahey’s picture

kbahey commented

The type of information that would be disclosed would not be security related, and hence are not considered a security risk by me. It is just operational in nature. The purpose here is to make it "hard enough" to find, not fool proof. If this is your defense, then it is security by obscurity, which is not really security at all.

I would like to hear from other users of the module though before considering this one way or the other ...

flickerfly’s picture

flickerfly commented
StatusFileSize
new Implementation of hook_requirements1.1 KB

Attached is a patch to implement hook_requirements providing a warning if the module is unconfigured. This one doesn't disable the module if it isn't configured. I pretty much copied it from the google analytics module, but they had it in the .module file instead of the .install. I put it in .install because that's where the API page says to put it in case installation requirements ever come in to play.

kbahey’s picture

kbahey commented
Status: Active » Needs review

Settings status for review. Please provide feedback.

flickerfly’s picture

flickerfly commented
Status: Needs review » Active
StatusFileSize
new Disable if not configured, set unique_ua default to blank2.57 KB

This patch is the same as the previous one, but with two additions.

1) It changes the default nagios_ua to a blank string instead of the current "Nagios". I figured that makes clear that it isn't set better.

2) Before providing access to the nagios status page, it checks to see if nagios_ua has been set. Otherwise, it doesn't provide access.

flickerfly’s picture

flickerfly commented

Sorry, I should have refreshed these before posting those patches so that I saw your comments.

I'd submit that the status of a site includes notice that Drupal core is unpatched and has known security problems and also that some module has some security update. These "Critical" states are important because it changes from looking for the possibility of a security hole to knowing that there is one and that they just must find it. If someone else is also watching my site like I am, they get pretty quick notice of the problem.

kbahey’s picture

kbahey commented
Status: Active » Needs review

I see your point now.

I understand the part where the patch adds a requirements thing, and I am happy to add it. What I don't get is how this patch addresses the "others can see my site's status" part. Please explain.

flickerfly’s picture

flickerfly commented

If the unique_ua is blank, it has the default setting. Therefore, if the unique_ua is not set (or is default), it tells the module to not give out any information. This is done in the same way the module restricts information when the wrong unique_ua is given.

The benefit is that the nagios module will not give up any information without a unique_ua that is truly unique instead of the default.

Is that a better explanation?

kbahey’s picture

kbahey commented

I think it is a better explanation, yes.

Let us wait for some time for other users of this module to pitch in ...

acrollet’s picture

acrollet commented

my $.02: We use this module with many sites, and therefore, I've created a script to implement it that sets our UA string with drush. Therefore, I'm not against adding an alert to the status page, but it doesn't affect us one way or another. IMO the information that the nagios status page provides is not that critical from a security standpoint - anyone trying to crack a site is just going to try the actual exploits and see if they work, rather than going through an additional step of seeing if the vulnerable update has been applied or not.

helmo’s picture

helmo commented

I've wondered why there was an unsafe default before but just changed the string every time to something unique.
Putting a notification on the status page and blocking unconfigured access seems like a clean solution. Just my $0.02

One remark about the patch, shouldn't the && below be a ||

+++ nagios.module	2 Jan 2010 01:45:14 -0000
@@ -121,9 +121,9 @@ function nagios_status_page() {
+  if ($_SERVER['HTTP_USER_AGENT'] != $ua && variable_get('nagios_ua', '') == '') {
JacobSingh’s picture

JacobSingh commented

Sorry, one nigly point here:

Shouldn't we be using access_callback for the check?

Also, I like the logic of:

if user_access('administer site configuration) || ($_SERVER['HTTP_USER_.....)

Then I can test it without hacking the code or my browser.

In fact, would be a nice feature to make a simple HTML table display of the results to use a status page for the admin. Or hook into the hook_status page if that makes sense.

-J

kbahey’s picture

kbahey commented

Seems like still there is no consensus on the best way forward with this.

I am leaning towards Jacob Singh's solution that allows an Admin to see the results via an access callback.

What do others think?

As for the hook_requirmenets parts, it seems like a no brainer to add it. Please separate it out into its own patch in a new issue, and that can go in immediately. Should not be hampered by the rest of the patch.

greg.harvey’s picture

greg.harvey
Primary language English
Location Occitanie, France
commented

hook_requirements issue raised separately here: #1110654: Implement a hook_requirements() function to make sure a unique ID is set

helmo’s picture

helmo commented

+1 for JacobSingh suggestion of adding user_access

raystuart’s picture

raystuart commented

+1 also on JacobSingh's user_access suggestion. It would make testing and patching that much quicker.

Also, I agree with the original poster on the original issue. I'd rather be more obscure with this, rather than less. Maybe even having the option of checking the incoming ip address and throwing a page not found if it doesn't match. That's not a feature request... yet... just an example of my paranoia for reference. :)

greg.harvey’s picture

greg.harvey
Primary language English
Location Occitanie, France
commented

Well Jacob's idea is gathering weight. I might make an executive decision on this and run with it...

malc0mn’s picture

malc0mn commented

+1 also on JacobSingh's user_access suggestion.

I'm also in for not showing anything at all on this page if the incorrect unique ID is not passed. I'm even for throwing a 404 or redirecting to the homepage.

helmo’s picture

helmo commented
StatusFileSize
new nagios-672264-NO_default_uniqueID.patch2.03 KB

Greg, I hope this patch helps you to make your 'executive decision'... :)

greg.harvey’s picture

greg.harvey
Primary language English
Location Occitanie, France
commented

heh, thanks... yes, I need to allocate a day next week to patching d.o projects! Thanks for this!

greg.harvey’s picture

greg.harvey
Primary language English
Location Occitanie, France
commented
Status: Needs review » Closed (fixed)

Patch in #18 committed to 6.x-1.x-dev. Thanks! =)

greg.harvey’s picture

greg.harvey
Primary language English
Location Occitanie, France
commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Closed (fixed) » Patch (to be ported)

Actually, I shouldn't close, this needs doing for D7 too.

greg.harvey’s picture

greg.harvey
Primary language English
Location Occitanie, France
commented
Status: Patch (to be ported) » Closed (fixed)

Ported to D7.