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.

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

Added snmp_security check plugin for various SNMP checks #403

Merged
merged 28 commits into from
Jan 25, 2022

Conversation

Jed-Giblin
Copy link
Contributor

This solves #355 by adding a single plugin that has two tests:

  1. Check if SNMP version 1 or 2c is in use
  2. If version 3 is in use, validate that noAuthNoPriv isn't being utilized.

Copy link
Member

@ericwb ericwb left a comment

Choose a reason for hiding this comment

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

Could the blacklist plugin be used instead of defining a new plugin?

doc/source/plugins/b508_snmp_weak_cryptography.rst Outdated Show resolved Hide resolved
doc/source/plugins/b508_snmp_weak_cryptography.rst Outdated Show resolved Hide resolved
examples/snmp.py Outdated
@@ -0,0 +1,9 @@
from pysnmp.hlapi import UsmUserData
Copy link
Member

Choose a reason for hiding this comment

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

Alpha sort: UsmUserData comes after CommunityData

@@ -0,0 +1,8 @@
-----------------------------
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
-----------------------------
----------------------------

@@ -0,0 +1,8 @@
-----------------------------
B508: snmp_weak_cryptography
-----------------------------
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
-----------------------------
----------------------------

bandit/plugins/snmp_security_check.py Show resolved Hide resolved
bandit/plugins/snmp_security_check.py Show resolved Hide resolved
if context.call_args_count == 1 or context.call_args_count == 1:
return bandit.Issue(
severity=bandit.MEDIUM,
confidence=bandit.MEDIUM,
Copy link
Member

Choose a reason for hiding this comment

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

Is the confidence not HIGH?

severity=bandit.MEDIUM,
confidence=bandit.MEDIUM,
text="You should not use SNMPv3 without encryption. "
"noAuthNoPriv is an insecure method of transport.",
Copy link
Member

Choose a reason for hiding this comment

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

What about authNoPriv? Seems equally bad.

context.check_call_arg_value("mpModel", 1):
return bandit.Issue(
severity=bandit.MEDIUM,
confidence=bandit.MEDIUM,
Copy link
Member

Choose a reason for hiding this comment

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

Is the confidence not HIGH?

@Jed-Giblin
Copy link
Contributor Author

@ericwb fixed up the lint errors you pointed out and corrected the duplicate code. As for using the blacklist plugin, I would think it should be its own plugin since its a very narrow use case

examples/snmp.py Outdated Show resolved Hide resolved
bandit/plugins/snmp_security_check.py Outdated Show resolved Hide resolved
bandit/plugins/snmp_security_check.py Outdated Show resolved Hide resolved
Comment on lines 3 to 15
# Copyright (c) 2018 SolarWinds, Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may
# not use this file except in compliance with the License. You may obtain
# a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
# License for the specific language governing permissions and limitations
# under the License.
Copy link
Member

Choose a reason for hiding this comment

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

Switch to SPDX short form of the license.

@ericwb ericwb enabled auto-merge (squash) January 25, 2022 05:52
Copy link
Member

@ericwb ericwb left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants