Dhcp version strings and blacklist fix#1163
Conversation
config/bin_version_strings.cfg
Outdated
| isc-dhclient;;ISC;"isc-dhclient-[0-9](\.[0-9]+)+?-ESV-R[0-9]$";"sed -r 's/isc-dhclient-([0-9](\.[0-9]+)+?(-ESV-R[0-9])?)$/dhcp_client:\1/'"; | ||
| isc-dhcpd;;ISC;"Internet\ Systems\ Consortium\ DHCP\ Server\ [0-9](\.[0-9]+)+?([a-z][0-9])?$";"sed -r 's/Internet\ Systems\ Consortium\ DHCP\ Server\ ([0-9](\.[0-9]+)+?([a-z][0-9])?)$/dhcp:\1/'"; | ||
| isc-dhcpd;;ISC;"Internet\ Systems\ Consortium\ DHCP\ Server\ [0-9](\.[0-9]+)+?([a-z][0-9])?$";"sed -r 's/Internet\ Systems\ Consortium\ DHCP\ Server\ ([0-9](\.[0-9]+)+?([a-z][0-9])?)$/dhcpd:\1/'"; | ||
| isc-dhcpd;;ISC;"isc-dhcpd-[0-9](\.[0-9]+)+?$";"sed -r 's/isc-dhcpd-([0-9](\.[0-9]+)+?)$/dhcp:\1/'"; |
There was a problem hiding this comment.
wondering on these rules as they do not fit the results on my strings output from your binary. They probably match the emulation output (s115) but not teh static checks (s09). Why not using the multi-grep rule from here for static checking?
There was a problem hiding this comment.
btw. ist this a public firmware you are using for testing? Can you provide a download link?
There was a problem hiding this comment.
wondering on these rules as they do not fit the results on my strings output from your binary. They probably match the emulation output (s115) but not teh static checks (s09). Why not using the multi-grep rule from here for static checking?
Indeed, as far as the binaries attached to the issue are concerned, strings can only be found through emulation as static analysis won't find them as is in the binary.
There might be other build of the same binaries in the wild that do include those whole strings for static check. I don't know. But given that we don't have such samples for now, I could add no_static to the rules.
I'm not sure at all about multi_grep. There are other occurrences version-like strings such as 1.1 and 9.10 in dhcpd binary, and we don't want to match those as binary versions. There could be many others in another build of dhcpd. Requiring a three-number version would work in this particular case, but is prone to fail the same way in other builds (and would also fail on genuine two-number versions of dhcpd if there are any)
That firmware is unfortunately not public. I also tried to create a mini-firmware version by keeping only a a few generic binaries that could be shared (also because as it stand now, even when excluding S13, S14 and S16 from sbom-default it still takes 9 hours on a 32-core system), but emulation would never run. Can't tell why.
I'll try with no_static and re-analyze before committing.
There was a problem hiding this comment.
ok, for static analysis it is fine and the dynamic strings are not much different to the already known. So, I think we are fine after adjusting.
"multi_grep" is currently only available in static mode. It is on the todo list for dynamic analysis as well as combining multiple rules (no_static, multi_grep and further rules in the future)
config/bin_version_strings.cfg
Outdated
| isc-dhclient;;ISC;"isc-dhclient-[0-9](\.[0-9]+)+?$";"sed -r 's/isc-dhclient-([0-9](\.[0-9]+)+?)$/isc:dhcp_client:\1/'"; | ||
| isc-dhclient;;ISC;"isc-dhclient-[0-9](\.[0-9]+)+?-([ABPabp]|rc|RC)[0-3]$";"sed -r 's/isc-dhclient-([0-9](\.[0-9]+)+?(-([ABPabp]|rc|RC)[0-3])?)$/isc:dhcp_client:\1/'"; | ||
| isc-dhclient;;ISC;"isc-dhclient-[0-9](\.[0-9]+)+?-ESV-R[0-9]$";"sed -r 's/isc-dhclient-([0-9](\.[0-9]+)+?(-ESV-R[0-9])?)$/isc:dhcp_client:\1/'"; | ||
| isc-dhclient;;ISC;"Internet\ Systems\ Consortium\ DHCP\ Client\ [0-9](\.[0-9]+)+?([a-z][0-9])?$";"sed -r 's/Internet\ Systems\ Consortium\ DHCP\ Client\ ([0-9](\.[0-9]+)+?([a-z][0-9])?)$/dhcp_client:\1/'"; |
There was a problem hiding this comment.
with #1166 we should be able to add the isc as the vendor to the sed commands of all these dhcp identifiers. Would you be interested to address this in your PR?
There was a problem hiding this comment.
ok, adding isc: prefix back. #1166 makes it work properly (and is now required for this PR)
| fi | ||
|
|
||
| if [[ "${BIN_BLACKLIST[*]}" == *"$(basename "${FULL_BIN_PATH}")"* ]]; then | ||
| if echo "${BIN_BLACKLIST[@]}" | grep -q -F -w "$(basename "${FULL_BIN_PATH}")"; then |
There was a problem hiding this comment.
I think we do not need the extra echo and grep here:
if [[ "${BIN_BLACKLIST[*]}" == *" $(basename "${FULL_BIN_PATH}") "* ]]; then
Does this fit your solution?
There was a problem hiding this comment.
I don't know if it does. Does it work with the first and last item as well?
Sorry, I don't have time for any more testing. I just know that my proposed solution works. Feel free to implement whichever you like that works.
New feature and related fix
What is the current behavior? (You can also link to an open issue here)
isc:dhcp_client:version) is not properly handled by F20 that looks forisc:versioninstead ofdhcp_client:versiondchpif the blacklist configuration containsudhcpdWhat is the new behavior (if this is a feature change)? If possible add a screenshot.
isc:dhcpandisc:dhcpd)isc:dhcp_client:*version string replaced withdhcp_client:*so that F20 can find CVEsWorking example:
dhcpis found and not blacklisted in S115. Blacklist still function as intended. For example:No