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

F20 CVE version range checking: fix and dead code removal #1165

Merged
merged 4 commits into from
May 27, 2024

Conversation

gluesmith2021
Copy link

@gluesmith2021 gluesmith2021 commented May 23, 2024

  • 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

f20-test.zip

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.

@gluesmith2021 gluesmith2021 changed the title F20 fix and simplify CVE version ranges checking F20 CVE version range checking: fix and dead code removal May 23, 2024
@m-1-k-3
Copy link
Member

m-1-k-3 commented May 23, 2024

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

image

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

@m-1-k-3 m-1-k-3 added bug Something isn't working cve-search Some cve-search question/issue EMBA labels May 23, 2024
@m-1-k-3
Copy link
Member

m-1-k-3 commented May 27, 2024

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!

Copy link
Member

@m-1-k-3 m-1-k-3 left a comment

Choose a reason for hiding this comment

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

Thank your for great contribution and fixing my buggy code. Hope to see you again :)

@m-1-k-3 m-1-k-3 merged commit c24b928 into e-m-b-a:master May 27, 2024
13 checks passed
@gluesmith2021 gluesmith2021 deleted the f20_fix_simplify_version_ranges branch June 4, 2024 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cve-search Some cve-search question/issue EMBA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants