new plugin for Cambium cnMatrix Ethernet Switches with SFP#6184
new plugin for Cambium cnMatrix Ethernet Switches with SFP#6184rmorandell-pgum wants to merge 5 commits into
Conversation
(cherry picked from commit 129ab769ef5398495ba82576cbbaeac7ed3a68af)
This reverts commit 129ab769ef5398495ba82576cbbaeac7ed3a68af.
This reverts commit 55b8c92.
| } | ||
|
|
||
| $self->{sfp}->{$instance}->{module_temperature} = undef; | ||
| if (defined($result->{biasCurrent}) && $result->{moduleTemperature} != 0 && $result->{moduleTemperature} != -32768) { |
There was a problem hiding this comment.
Module temperature assignment is gated by defined($result->{biasCurrent}), which can suppress valid moduleTemperature values.
| if (defined($result->{biasCurrent}) && $result->{moduleTemperature} != 0 && $result->{moduleTemperature} != -32768) { | |
| if (defined($result->{moduleTemperature}) && $result->{moduleTemperature} != 0 && $result->{moduleTemperature} != -32768) { |
Details
✨ AI Reasoning
The control condition for storing module temperature checks a different field than the one being stored. This introduces an unintended and incorrect dependency: temperature availability is tied to bias current presence. That leads to valid module temperature values not being recorded.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| if (defined($self->{option_results}->{display_transform_src})) { | ||
| $self->{option_results}->{display_transform_dst} = '' if (!defined($self->{option_results}->{display_transform_dst})); | ||
|
|
||
| $self->{safe}->reval("\$assign_var =~ s{$self->{option_results}->{display_transform_src}}{$self->{option_results}->{display_transform_dst}}", |
There was a problem hiding this comment.
User-controlled options (display_transform_src/dst) are interpolated into code passed to Safe->reval, enabling code/regex injection. Avoid eval by performing regex substitution without building code strings, or properly validate/escape these option values.
Details
✨ AI Reasoning
The code dynamically constructs and evaluates Perl code using Safe->reval with patterns coming from command-line options (display_transform_src and display_transform_dst). These option values are user-controlled at runtime. Although Safe is used, reval still executes interpreted code built from potentially untrusted input and thus can allow injection of unintended code or cause unsafe regex operations. The problematic construct appears where a substitution expression is built as a string and passed to reval. This harms security by allowing arbitrary code execution or DoS via crafted regexes. Fixing by avoiding eval (use direct regex operations with proper escaping or compile-safe regex routines) would meaningfully reduce risk and is feasible in this PR scope.
🔧 How do I fix it?
Use parameterized queries with placeholders, array-based command execution (no shell interpretation), or properly escaped arguments using vetted libraries. Avoid dynamic queries/commands built with user input concatenation.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| my $display = defined($self->{option_results}->{add_interface_name}) ? | ||
| $instance . '-' . exists($sfp_ports->{$instance}->[1]) ? $sfp_ports->{$instance}->[1] : '' : $instance; |
There was a problem hiding this comment.
The my $display = ... ternary expression is precedence-ambiguous and evaluates with unintended grouping, producing incorrect display values when add_interface_name is set.
| my $display = defined($self->{option_results}->{add_interface_name}) ? | |
| $instance . '-' . exists($sfp_ports->{$instance}->[1]) ? $sfp_ports->{$instance}->[1] : '' : $instance; | |
| my $display = defined($self->{option_results}->{add_interface_name}) ? | |
| $instance . '-' . (exists($sfp_ports->{$instance}->[1]) ? $sfp_ports->{$instance}->[1] : '') : $instance; |
Details
✨ AI Reasoning
The display value construction uses a nested conditional expression whose grouping is not aligned with the intended meaning. Because of operator precedence, the condition effectively evaluates a concatenated string rather than directly selecting between full display formats. This makes the resulting display text inconsistent with the surrounding logic and can drop the port prefix unexpectedly.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| if (defined($result->{biasCurrent}) && $result->{voltage} != 0 && $result->{voltage} != -32768) { | ||
| $self->{sfp}->{$instance}->{voltage} = $result->{voltage} / 1000; | ||
| } | ||
|
|
||
| $self->{sfp}->{$instance}->{module_temperature} = undef; | ||
| if (defined($result->{biasCurrent}) && $result->{moduleTemperature} != 0 && $result->{moduleTemperature} != -32768) { |
There was a problem hiding this comment.
Voltage assignment is gated by defined($result->{biasCurrent}), so voltage data can be incorrectly dropped when bias current is absent.
Show fix
| if (defined($result->{biasCurrent}) && $result->{voltage} != 0 && $result->{voltage} != -32768) { | |
| $self->{sfp}->{$instance}->{voltage} = $result->{voltage} / 1000; | |
| } | |
| $self->{sfp}->{$instance}->{module_temperature} = undef; | |
| if (defined($result->{biasCurrent}) && $result->{moduleTemperature} != 0 && $result->{moduleTemperature} != -32768) { | |
| if (defined($result->{voltage}) && $result->{voltage} != 0 && $result->{voltage} != -32768) { | |
| $self->{sfp}->{$instance}->{voltage} = $result->{voltage} / 1000; | |
| } | |
| $self->{sfp}->{$instance}->{module_temperature} = undef; | |
| if (defined($result->{moduleTemperature}) && $result->{moduleTemperature} != 0 && $result->{moduleTemperature} != -32768) { |
Details
✨ AI Reasoning
A condition that controls whether voltage is stored checks a different metric variable. This creates a hard dependency between unrelated measurements. As a result, valid voltage readings can be ignored solely because bias current is missing, which is incorrect behavior from control-flow alone.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
Community contributors
Description
Modes Available:
Type of change
How this pull request can be tested ?
CAMBIUM-NETWORKS-SYSTEM-MIB.txt
CAMBIUM-NETWORKS-TRANSCEIVER-MIB.txt
cambium-cnMatrix.snmpwalk.txt
Checklist