F20 CVE version range checking: fix and dead code removal

gluesmith2021 commented 1 month ago

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)**
- Bug fix
- Dead code removal
What is the current behavior? (You can also link to an open issue here)**
- When a CVE version ranges specifies both a "start including" and an "end excluding" versions, the excluded end version is nevertheless reported as a vulnerable version. For example, checking for dhcp:4.3.6 returns CVE-2018-5733 but that version is excluded.
- See other information below for a test script that shows failing cases
- Not reproduced but only in code: kernel version check (sanity check?) is incorrect in at least one case as it check against an undefined variable (CVE_VER_END_INCL)
What is the new behavior (if this is a feature change)? If possible add a screenshot.
- Version ranges are properly checked
- Dead code and conditions are removed although this does not affect behavior
- See other information below for a test script that demonstrate the fix
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

No

Other information:

Tests

This test that can be run before and after merging the PR to demonstrate the bug and fixes. This part of F20 code is so error-prone that I made this test to make sure it runs fine.

There are two files:

f20-test.sh
- Tests all possible combinations of start/end included/excluded, even improbable ones.
- Place it in emba source root folder and run it there
- Note: does not test the kernel version check
CVE-2024-0000.json
- Test data
- Create folders and place it under emba_root/f20-test/nvd-test

This is not proper unit testing but demonstrate the issue nevertheless.

"Inconsequent" errors

In F20, when checking a binary version against vulnerable ranges, the combinatory nature of 'if' conditions is very error prone.

Indeed, on top of above bugs, there were a few errors that happened to make no difference in execution. Given the pattern that initially occurred four times:

if A
    if B
        ...
        continue

    if C
        ...
        continue

    if not (B and C)
        ...
        continue

    continue

The last condition actually means "if zero or one of B or C is defined", but the intent was most probably "if none of B or C is defined" which would be consistent with the code and debug output inside the 'if' code. In that case, it should have been if not (B OR C) instead (equivalent to if (not B) and (not C))

As another easy error, the first one (original line 725) also checked B and A instead of B and C, while A was obviously true at that point:

  if ! [[ -n "${CVE_VER_END_EXCL}" && -n "${CVE_VER_START_INCL}" ]]; then

Those error did not cause any misbehavior because at that point (in each of the the four occurrences) B and C are both undefined, and the total condition was always true.

Dead code removal

The above also mean that those conditions are not necessary. They were removed in this PR.

Also, the "second half" of cases (checking for end versions first) is only reached when there are no starting version (neither including nor excluding). Therefore, 'if' condition checking for those are always false and the dead code was thus removed.

comments were added for clarity

The remaining code in the second half could be made a bit shorter by flipping some conditions, but everything was left as-is, namely the debug output, to keep changes clearer.

m-1-k-3 commented 1 month ago

Thanks @gluesmith2021 for your contribution. could you do a quick check on the results of the check script.

I will walk through your modifications soon and will do some tests

m-1-k-3 commented 1 month ago

Nice ...

TEST RESULTS:
f20test-None:0.0.0: Pass
f20test-None:9.9.9: Pass
f20test-EE:1.1.3: Pass
f20test-EE:1.1.4: Pass
f20test-EE:1.2.3: Pass
f20test-EE:1.2.4: Pass
f20test-EI:1.1.3: Pass
f20test-EI:1.1.4: Pass
f20test-EI:1.2.3: Pass
f20test-EI:1.2.4: Pass
f20test-EI-EE:1.1.3: Pass
f20test-EI-EE:1.1.4: Pass
f20test-EI-EE:1.2.3: Pass
f20test-EI-EE:1.2.4: Pass
f20test-SE:1.1.3: Pass
f20test-SE:1.1.4: Pass
f20test-SE:1.2.3: Pass
f20test-SE:1.2.4: Pass
f20test-SE-EE:1.1.3: Pass
f20test-SE-EE:1.1.4: Pass
f20test-SE-EE:1.2.3: Pass
f20test-SE-EE:1.2.4: Pass
f20test-SE-EI:1.1.3: Pass
f20test-SE-EI:1.1.4: Pass
f20test-SE-EI:1.2.3: Pass
f20test-SE-EI:1.2.4: Pass
f20test-SE-EI-EE:1.1.3: Pass
f20test-SE-EI-EE:1.1.4: Pass
f20test-SE-EI-EE:1.2.3: Pass
f20test-SE-EI-EE:1.2.4: Pass
f20test-SI:1.1.3: Pass
f20test-SI:1.1.4: Pass
f20test-SI:1.2.3: Pass
f20test-SI:1.2.4: Pass
f20test-SI-EE:1.1.3: Pass
f20test-SI-EE:1.1.4: Pass
f20test-SI-EE:1.2.3: Pass
f20test-SI-EE:1.2.4: FAIL! Should NOT have found a vulnerability but did find one. **
f20test-SI-EI:1.1.3: Pass
f20test-SI-EI:1.1.4: Pass
f20test-SI-EI:1.2.3: Pass
f20test-SI-EI:1.2.4: Pass
f20test-SI-EI-EE:1.1.3: Pass
f20test-SI-EI-EE:1.1.4: Pass
f20test-SI-EI-EE:1.2.3: Pass
f20test-SI-EI-EE:1.2.4: Pass
f20test-SI-SE:1.1.3: Pass
f20test-SI-SE:1.1.4: Pass
f20test-SI-SE:1.2.3: Pass
f20test-SI-SE:1.2.4: Pass
f20test-SI-SE-EE:1.1.3: Pass
f20test-SI-SE-EE:1.1.4: Pass
f20test-SI-SE-EE:1.2.3: Pass
f20test-SI-SE-EE:1.2.4: FAIL! Should NOT have found a vulnerability but did find one. **
f20test-SI-SE-EI:1.1.3: Pass
f20test-SI-SE-EI:1.1.4: Pass
f20test-SI-SE-EI:1.2.3: Pass
f20test-SI-SE-EI:1.2.4: Pass
f20test-SI-SE-EI-EE:1.1.3: Pass
f20test-SI-SE-EI-EE:1.1.4: Pass
f20test-SI-SE-EI-EE:1.2.3: Pass
f20test-SI-SE-EI-EE:1.2.4: Pass

Thank you for your contribution!

e-m-b-a / emba