[Nagiosplug-devel] [ nagiosplug-Patches-1975646 ] check_radius wrongly handles NAS-IP-Address

[SourceForge.net]

Patches item #1975646, was opened at 2008-05-28 01:17
Message generated for change (Comment added) made by cyco_dd
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=397599&aid=1975646&group_id=29880

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Bugfix
Group: None
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Josip Rodin (shallot)
Assigned to: Nobody/Anonymous (nobody)
Summary: check_radius wrongly handles NAS-IP-Address

Initial Comment:
Hi,

The check_radius plugin is wrongly hardcoding the value of the NAS-IP-Address attribute in the packets it sends.

It gets the address from the rc_own_ipaddress() library call, and that in turn translates into calling gethostbyname() on the result of uname(). This call can easily fail, and its result can easily be unsuitable - for example when the Nagios instance uses its own virtual host, and you don't want the original system hostname leaked to the RADIUS servers you monitor with this.

Furthermore, this behaviour is inconsistent with RFC 2865, which defines the two attributes as analogous and never suggests hardcoding the value of either of them in client software.

I'm attaching a patch which adds a new option (-N) which allows the user to provide the NAS-IP-Address
attribute contents, just like they can for the other attribute (with -n).

While adding this, I also noticed that the error handling there is also broken - it does "return (ERROR_PC)", which is meaningless in the context of check_radius.c. That actually seems to be copy&waste from radiusclient-0.3.2/src/radexample.c. :) so I fixed that as well.

While debugging, I also took the opportunity to decouple the nas-identifier rc_avpair_add() instance from the initial three, because this is just bad practice to lump a fourth optional attribute into the same block with the required attributes, the error handling for which is throwing the same daft message "Out of Memory?"...

Another tidbit - the -n option, to include NAS-Identifier, requires that the radiusclient attribute dictionary includes that attribute - which it doesn't in the old library. radiusclient-ng has this fixed. I'm mentioning this just because of the recent change to the REQUIREMENTS file - just another confirmation that the change is correct, I guess :)

Please include the patch. Thanks.

(I've also filed this at http://bugs.debian.org/482947 FWIW)


----------------------------------------------------------------------

Comment By: Jan Wagner (cyco_dd)
Date: 2008-06-06 17:55

Message:
Logged In: YES 
user_id=1345239
Originator: NO

patch against 1.4.12 is available at
http://svn.debian.org/wsvn/pkg-nagios/nagios-plugins/trunk/debian/patches/37_check_radius_nas-ip-address.dpatch?op=file&rev=0&sc=0

----------------------------------------------------------------------

Comment By: Josip Rodin (shallot)
Date: 2008-06-06 14:36

Message:
Logged In: YES 
user_id=119756
Originator: YES

> Furthermore, this behaviour is inconsistent with RFC 2865, which
defines
> the two attributes as analogous and never suggests hardcoding the value
of
> either of them in client software.

I just realized that I forgot to mention the other attribute's name before
that paragraph - I'm talking about NAS-Identifier.


----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=397599&aid=1975646&group_id=29880