Skip to content
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.

Sign up for GitHub

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

Jump to bottom

updated HariSekhonUtils.pm allow vlans in isInterface #46

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

updated HariSekhonUtils.pm allow vlans in isInterface #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions HariSekhonUtils.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2183,9 +2183,16 @@ sub isInt ($;$) {
sub isInterface ($) {
my $interface = shift;
defined($interface) || return;
my $vlan;
($interface, $vlan) = split(/\./, $interface);
$interface = isAlNum($interface) or return;
if ($vlan) {
isInt($vlan) or return;
($vlan > 0 && $vlan < 4095) or return;
$interface .= "." . $vlan
}
# TODO: consider checking if the interface actually exists on the system
$interface =~ /^((?:em|eth|bond|lo|docker)\d+|lo|veth[A-Fa-f0-9]+)$/ or return;
Copy link
Owner

@HariSekhon HariSekhon Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think removing this line checking the format on the interface name needs some kind of reinstating to avoid weakening the check.

Unit tests need to be extended a bit to test failure of a fake interface called hari0 instead of eth0 for example, which fails on this branch, which highlights the weakening without the regex validation.

Also, the regex test should be done to untaint the returned interface. The code base runs entirely in taint security mode and relies on known good specific regexes to untaint user inputs. This patch inadvertently removes the untainting.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for the response.
I've dared to removed the regex check, because its IMHO not possible to craft a regex to match interface names other than alnum - ie. the former regex did not worked for brX, tailscaleX, tunX, wlanX, wmnetX just to name a few. btw. interface hari0 is also valid interface name ;-)

$ ip link add link eth0 name hari0 type vlan id 101
hari0: flags=4098<BROADCAST,MULTICAST>  mtu 1500
        ether 96:00:03:6c:82:23  txqueuelen 1000  (Ethernet)
        RX packets 0  bytes 0 (0.0 B)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 0  bytes 0 (0.0 B)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0

Also, the isInterface() was not used till now anywhere except of validate_interface(), that was mentioned only in the comment of check_linux_interface.pl (https://github.com/HariSekhon/Nagios-Plugins/blob/master/check_linux_interface.pl#L61).

The untainting is done in isAlnum and isInt (as it was till now, directly in check_linux_interface.pl), but I can improve it more if you have some suggestion how. One possibility can be, to close this todo
# TODO: consider checking if the interface actually exists on the system
and check interface name against /proc/net/dev.

return $1;
return $interface;
}


Expand Down Expand Up @@ -3681,7 +3688,7 @@ sub validate_int ($$;$$) {
sub validate_interface ($) {
my $interface = shift;
defined($interface) || usage "interface not defined";
$interface = isInterface($interface) || usage "invalid interface defined: must be either eth<N>, bond<N> or lo<N>";
$interface = isInterface($interface) || usage "invalid interface defined: must be either alfanumeric or alfanumber.number";
vlog_option("interface", $interface);
return $interface;
}
Expand Down
3 changes: 2 additions & 1 deletion t/HariSekhonUtils.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,11 +666,12 @@ is(validate_integer(6,"six",5,7), 6, 'validate_integer(6,"six",5,7)');

# ============================================================================ #
is(isInterface("eth0"), "eth0", 'isInterface("eth0")');
is(isInterface("eth0.100"), "eth0.100", 'isInterface("eth0.100")');
is(isInterface("bond3"), "bond3", 'isInterface("bond3")');
is(isInterface("lo"), "lo", 'isInterface("lo")');
is(isInterface("docker0"), "docker0", 'isInterface("docker0")');
is(isInterface("vethfa1b2c3"), "vethfa1b2c3", 'isInterface("vethfa1b2c3")');
ok(!isInterface("vethfa1b2z3"), 'isInterface("vethfa1b2z3")');
is(isInterface("vethfa1b2c3.100"), "vethfa1b2c3.100", 'isInterface("vethfa1b2c3.100")');
ok(!isInterface('b@interface'), '!isInterface(\'b@dinterface\'');

is(validate_interface("eth0"), "eth0", 'validate_interface("eth0")');
Expand Down