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

Fix additional warnings due to untested attributes #404

Merged
merged 5 commits into from
Jan 31, 2022

Conversation

dabdine
Copy link
Contributor

@dabdine dabdine commented Jan 28, 2022

Description

Fixes these additional warnings that #400 did not address due to the examples missing content that could be parsed by the regular expression:

xml/http_servers.xml:1225: WARN: 'Sun Java System Application Server (formerly iPlanet Application Server, Sun ONE Application Server)' is missing an example that checks for parameter 'service.version' which is derived from a capture group
xml/operating_system.xml:59: WARN: 'Windows 2000' is missing an example that checks for parameter 'os.version' which is derived from a capture group
xml/operating_system.xml:228: WARN: 'Gentoo Linux' is missing an example that checks for parameter 'os.version' which is derived from a capture group
xml/operating_system.xml:663: WARN: 'IBM OSes' is missing an example that checks for parameter 'os.version' which is derived from a capture group
xml/operating_system.xml:679: WARN: 'HP OSes' is missing an example that checks for parameter 'os.version' which is derived from a capture group
xml/operating_system.xml:693: WARN: 'Juniper' is missing an example that checks for parameter 'os.version' which is derived from a capture group
xml/operating_system.xml:705: WARN: 'Cisco' is missing an example that checks for parameter 'os.version' which is derived from a capture group
xml/sip_user_agents.xml:286: WARN: 'Polycom RealPresence Trio Phones' is missing an example that checks for parameter 'host.mac' which is derived from a capture group
xml/smtp_banners.xml:541: WARN: 'MailEnable - Simple' is missing an example that checks for parameter 'host.name' which is derived from a capture group

Most of these (especially in operating_systems.xml) were made up to satisfy the regular expression, while others were incorporated using real (anonymized) data from internet scan engines.

After this patch, the only warnings that will remain are those that complain about fingerprints which have no test cases (<example> elements).

Motivation and Context

Clean more warnings.

How Has This Been Tested?

bundle exec rake tests

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have updated the documentation accordingly (or changes are not required).
  • I have added tests to cover my changes (or new tests are not required).
  • All new and existing tests passed.

@@ -680,6 +683,7 @@
<description>HP OSes</description>
<example os.product="HP-UX" os.family="HP-UX">HP-UX</example>
<example os.product="OpenVMS" os.family="OpenVMS">OpenVMS</example>
<example os.product="OpenVMS" os.family="OpenVMS" os.version="1.0">OpenVMS 1.0</example>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a valid version number? All of the versions listed on OpenVMS Releases start with a "V".

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed the comment on the PR description that stated these were "made up to satisfy the regular expression".

Copy link
Contributor

Choose a reason for hiding this comment

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

We should ensure that all examples have provenance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, I made this up -- though 1.0 is a valid version. Generally versions should be in numeric dotted notation for easier comparison ("is 1.2.3 less than 4.0"? is programmatically easier to ask than "is V1.2.3 less than 4.0?" -- which is done often for vulnerability testing), though I do see other places in recog that extract stuff like "V" before the version number. The exception to that rule is when you have old Cisco IOS versions like 12(T)13b (or whatever, I've really tried to erase those from my memory) where they don't really follow a dotted notation format. But if the purpose of "V" is only to denote that it is a version, there's no point in keeping it as part of the extracted version string IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having issues today with github single comments & reviews 🤦

Copy link
Contributor

@mkienow-r7 mkienow-r7 Jan 28, 2022

Choose a reason for hiding this comment

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

I agree the dotted notation portion makes sense for the parameter value. It does appear that the fingerprint regex is not correct. The table of version numbers that I linked was from a VMS Software wiki not from Wikipedia, and I found other documentation on confirming the installation of the OpenVMS operating system that shows the version number reported with a leading "V".

$ show system
OpenVMS V7.3-2 on node DS20 5-FEB-2004 15:15:16.88 Uptime 1 01:45:09

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is also documentation here:
https://marc.vos.net/books/vms/help/show/

That shows version 6.2 returning as:

$ SHOW SYSTEM
     OpenVMS 6.2 on node KRYPTN 14-DEC-1994 17:45:47:78 Uptime 2 21:53:59

However, regardless, this patch wasn't meant to make behavioral changes to any of the regular expressions. I don't think this regular expression was designed to process output from the show system command directly, as it is very limited in what it would match. Thus, I say we keep it as is, but happy to change 1.0 to 6.2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add that I'd be fine adding a conditional "V?" to the regular expression (not captured within the version capture group), but again, this regex doesn't appear to have been designed to specifically parse the output of show system (as it anchors the end of the string after the version, whereas the real output of show system continues on with a timestamp etc.).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the regular expression to conditionally match "V" as well as more test cases to support "modern" versions of OpenVMS strings in the event they're retrieved from the show system command

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting find on the SHOW SYSTEM output. Thanks for making this one more flexible.

@mkienow-r7
Copy link
Contributor

Would you please take a look at the made up parameter values and ensure they have provenance?

@dabdine
Copy link
Contributor Author

dabdine commented Jan 31, 2022

The issue I have with operating_systems.xml in particular is that nothing is defined as to what the input should be for this file. Because of that, despite these changes adding more provenance to actual versions, they may not test real world scenarios.

I'll add that the Cisco fingerprint looks entirely broken (for IOS at least) if it is to be applied to the output of show version. The fingerprint was made to be generic, but won't parse complicated Cisco IOS versions like 12.2(33)SXI1. If memory serves, in many of these cases some of the version info was received from SNMP.

@mkienow-r7
Copy link
Contributor

I appreciate you taking the time to enhance some of the fingerprints and adding more provenance. After spending more time looking at the fingerprints I agree that we need to make more sense of data source that to be used here and reassess this DB.
Thank you for the contribution @dabdine!

@mkienow-r7 mkienow-r7 merged commit 8996c20 into rapid7:master Jan 31, 2022
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.

2 participants