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

Clean up hosts strings before using them (20.08) #1352

Merged

Conversation

timopollmeier
Copy link
Member

@timopollmeier timopollmeier commented Nov 12, 2020

What:
Before sending hosts strings to the OSPd-OpenVAS scanner and before
calculating the number of hosts, leading zeroes in IPv4 addresses
are removed.

Why:
The addresses would be considered invalid otherwise, resulting in the hosts count to be reported as -1.
Starting a scan with a target using an IPv4 address with leading zeroes would also cause the scanner to crash.

How:
This was tested by:

  1. Calling the max_hosts SQL function containing IPv4 addresses with extra zeroes, e.g.
    SELECT max_hosts('192.168.123.001-192.168.123.035', '192.168.123.033');
    SELECT max_hosts('192.168.123.000/26, host001.example.com, 127.000.000.001', '');
    which should now return a positive number of hosts instead of -1.
  2. Creating a target, modifying it via SQL to use a IPv4 address with extra zeroes.
    (e.g. UPDATE targets SET hosts='192.168.123.045' WHERE name='ipv4test';)
    A scan using it should run the same as one with the same address without extra zeroes and not end in the "Interrupted" status.
  3. Temporarily adding extra debug output to functions where the new clean_hosts_string function was added to verify IPv4 addresses are cleaned up in the cases for single addresses, simple host ranges (e.g. 192.168.123.001-192.168.123.035) and CIDR notation ranges are handled while extra zeroes in hostnames like host001.example.com are kept intact.

Checklist:

Before sending hosts strings to the OSPd-OpenVAS scanner and before
calculating the number of hosts, leading zeroes in IPv4 addresses
are removed because the addresses would be considered invalid otherwise.
The function did not recognize short form IPv4 ranges like
"192.168.123.001-010", so the zeroes were not cleaned up.
@timopollmeier timopollmeier marked this pull request as ready for review November 12, 2020 15:51
@ArnoStiefvater
Copy link
Member

Would it be a good idea to add this function to the gvm-libs/base/hosts.c ?
And some tests would be nice for this function as they should be easy to write (see gvm-libs/base/networking_tests.c).

src/manage_utils.c Outdated Show resolved Hide resolved
src/manage.c Show resolved Hide resolved
src/manage_utils.c Show resolved Hide resolved
src/manage_utils.c Outdated Show resolved Hide resolved
@mattmundell
Copy link
Contributor

Would it be a good idea to add this function to the gvm-libs/base/hosts.c ?

Maybe yes. Our rule from way back is that it only goes in libs when it's used by two modules.

And some tests would be nice for this function as they should be easy to write (see gvm-libs/base/networking_tests.c).

Agreed.

@ArnoStiefvater
Copy link
Member

Would it be a good idea to add this function to the gvm-libs/base/hosts.c ?

Maybe yes. Our rule from way back is that it only goes in libs when it's used by two modules.

Maybe gvm_hosts_t * gvm_hosts_new_with_max (const gchar *hosts_str, unsigned int max_hosts) should be using this function to sanitize the string?

The variable `clean_exclude_hosts` is only used in the if block checking
`exclude_hosts` so it only needs to be declared there.
The strings for the cleaned hosts are copied by osp_target_new, so they
need to be freed by the caller.
Comments were added to clarify what the matching and replacement regular
expressions do.
@timopollmeier timopollmeier marked this pull request as draft November 17, 2020 11:04
Before this fix the regex matched some non-leading zeroes in numbers
such as "100". Also, the limits of digit groups are now more clearly
defined as the start/end of the line and non-digit characters.
@timopollmeier
Copy link
Member Author

I've added some unit tests and fixed a problem with non-leading zeroes being removed that I found with one of the test cases.

@timopollmeier timopollmeier marked this pull request as ready for review November 17, 2020 13:56
@mattmundell mattmundell merged commit b3c7a38 into greenbone:gvmd-20.08 Nov 17, 2020
@timopollmeier timopollmeier deleted the clean-hosts-string-20.08 branch October 15, 2021 12:07
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