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

Resolve #389: Add line numbers to recog_verify output #390

Merged
merged 4 commits into from Dec 15, 2021

Conversation

dabdine
Copy link
Contributor

@dabdine dabdine commented Dec 5, 2021

Description

Just a simple patch to produce line numbers in parsed fingerprints.

Motivation and Context

Need this to tell which fingerprints are which when diagnosing and fixing problems reported by recog_verify

How Has This Been Tested?

Ran recog_verify
Ran bundle exec rake tests

Types of changes

  • New feature (non-breaking change which adds functionality)

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.

@dabdine
Copy link
Contributor Author

dabdine commented Dec 5, 2021

Output before this patch:

xml/http_servers.xml: WARN: 'Red Hat Stronghold Enterprise Apache' is missing an example that checks for parameter 'service.component.version' which is derived from a capture group
xml/http_servers.xml: WARN: 'Apache' is missing an example that checks for parameter 'service.version' which is derived from a capture group
xml/http_servers.xml: WARN: 'Apache' is missing an example that checks for parameter 'apache.info' which is derived from a capture group
xml/http_servers.xml: WARN: 'Microsoft IIS 1.0 - 4.0 runs on Windows NT 4.0' is missing an example that checks for parameter 'service.version' which is derived from a capture group
xml/http_servers.xml: WARN: 'Microsoft IIS new, unknown Windows version' is missing an example that checks for parameter 'service.version' which is derived from a capture group
xml/http_servers.xml: WARN: 'Microsoft .NET Remoting and Common Language Runtime (CLR)' is missing an example that checks for parameter 'service.component.version' which is derived from a capture group
xml/http_servers.xml: WARN: 'Windows CE embedded devices, including HP iPAQ, Palm Treo, Motorola phones, and many more' is missing an example that checks for parameter 'service.version' which is derived from a capture group

Output after this patch:

dabdine@nop:~/recog$ bin/recog_verify -f s -c xml/http_servers.xml
xml/http_servers.xml: WARN (line 125): 'Red Hat Stronghold Enterprise Apache' is missing an example that checks for parameter 'service.component.version' which is derived from a capture group
xml/http_servers.xml: WARN (line 176): 'Apache' is missing an example that checks for parameter 'service.version' which is derived from a capture group
xml/http_servers.xml: WARN (line 176): 'Apache' is missing an example that checks for parameter 'apache.info' which is derived from a capture group
xml/http_servers.xml: WARN (line 253): 'Microsoft IIS 1.0 - 4.0 runs on Windows NT 4.0' is missing an example that checks for parameter 'service.version' which is derived from a capture group
xml/http_servers.xml: WARN (line 379): 'Microsoft IIS new, unknown Windows version' is missing an example that checks for parameter 'service.version' which is derived from a capture group
xml/http_servers.xml: WARN (line 400): 'Microsoft .NET Remoting and Common Language Runtime (CLR)' is missing an example that checks for parameter 'service.component.version' which is derived from a capture group
xml/http_servers.xml: WARN (line 416): 'Windows CE embedded devices, including HP iPAQ, Palm Treo, Motorola phones, and many more' is missing an example that checks for parameter 'service.version' which is derived from a capture group

@dabdine
Copy link
Contributor Author

dabdine commented Dec 5, 2021

Realized we have some tests here that are looking at literal log output, so I need to update the patch to fix those as well.

@dabdine
Copy link
Contributor Author

dabdine commented Dec 5, 2021

Nokogiri under jruby apparently has compatibility problems that cause it to have line numbers that are off by one:
sparklemotion/nokogiri#1223

Tabling for now

@mkienow-r7
Copy link
Contributor

It would be nice to simplify the reporting with a simple inline line number.

xml/http_servers.xml:$LINE_NUM: WARN: ...

@tsellers-r7
Copy link
Contributor

tsellers-r7 commented Dec 6, 2021

Per the comment from @dabdine above it looks like this may be fixed in nokigiri 1.11.2. We may wish to verify and then set the minimum version in Gemfile. It appears that multiple vulnerabilities have been fixed in versions released since 1.11.2

https://github.com/sparklemotion/nokogiri/releases/tag/v1.11.2

As a note, here is the version that was used in the jruby-9.1.17.0 test: Using nokogiri 1.10.10 (java)

Edit: Correction, it appears the jruby test is using nokogiri 1.12.5 (java) which is the latest released version.

@dabdine
Copy link
Contributor Author

dabdine commented Dec 6, 2021

New patch updates the logging format (suggestion from @mkienow-r7):

xml/http_servers.xml:125: WARN: 'Red Hat Stronghold Enterprise Apache' is missing an example that checks for parameter 'service.component.version' which is derived from a capture group
xml/http_servers.xml:176: WARN: 'Apache' is missing an example that checks for parameter 'service.version' which is derived from a capture group
xml/http_servers.xml:176: WARN: 'Apache' is missing an example that checks for parameter 'apache.info' which is derived from a capture group
xml/http_servers.xml:253: WARN: 'Microsoft IIS 1.0 - 4.0 runs on Windows NT 4.0' is missing an example that checks for parameter 'service.version' which is derived from a capture group
xml/http_servers.xml:379: WARN: 'Microsoft IIS new, unknown Windows version' is missing an example that checks for parameter 'service.version' which is derived from a capture group
xml/http_servers.xml:400: WARN: 'Microsoft .NET Remoting and Common Language Runtime (CLR)' is missing an example that checks for parameter 'service.component.version' which is derived from a capture group
xml/http_servers.xml:416: WARN: 'Windows CE embedded devices, including HP iPAQ, Palm Treo, Motorola phones, and many more' is missing an example that checks for parameter 'service.version' which is derived from a capture group

@dabdine
Copy link
Contributor Author

dabdine commented Dec 6, 2021

Still haven't addressed the jruby issue, so expecting the build to fail due to the off-by-one problem Jruby has when "approximating" the line from the XML document.

@dabdine
Copy link
Contributor Author

dabdine commented Dec 6, 2021

Ah I think I know what the issue is here. I think this is due to the line numbering logic in Nokogiri under JRuby ignoring the xml prolog tag:

<?xml version="1.0"?>

EDIT: Yep, removing the XML tag from the test XML files makes this build pass. So, this is an issue with upstream Nokogiri that needs to be addressed.

@dabdine
Copy link
Contributor Author

dabdine commented Dec 9, 2021

Opened an issue for this w/Nokogiri. I added a test on their end too to demonstrate the issue. However, the Java source code seems a bit more complicated to manipulate -- I believe the entire DOM parser used would need to be rewritten. For now, I propose disabling these tests when in JRuby.

Here's the issue for tracking purposes:
sparklemotion/nokogiri#2380

Copy link
Contributor

@mkienow-r7 mkienow-r7 left a comment

Choose a reason for hiding this comment

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

This enhancement will be very helpful when tracing errors and warnings back to their source. Thank you for the contribution @dabdine!

@mkienow-r7 mkienow-r7 merged commit 89dcc16 into rapid7:master Dec 15, 2021
@dabdine dabdine deleted the resolve-389-line-numbers branch December 15, 2021 22:06
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.

None yet

3 participants