Skip to content

Fix multiple file match bugs #724

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

Merged
merged 1 commit into from
Feb 27, 2023
Merged

Conversation

kevsecurity
Copy link
Contributor

This commit fixes a number of file match bugs:

  • __filter_file_buf() was passing a length value of one less than the string length to cmpbytes() and as a result Equals and Prefix matches were testing one fewer bytes than required.

  • ASM_RCMP (used in rcmpbytes()) was decrementing the string indices before testing if they were <1. This resulted in one fewer bytes being tested than required.

  • ASM_RCMP50 comprisd of 2x ASM_RCMP20 and 1x ASM_RCMP5, totalling 45 iterations of ASM_RCMP, instead of 50.

  • __filter_file_buf() tested failed postfix matches with a forward string match. This resulted in files that started with the postfix, but didn't end with it, matching when they shouldn't.

  • cmpbytes() continued to loop to full number of iterations, even when the string length had been exhausted (inefficient).

  • Added descriptions to cmpbytes() and rcmpbytes() to aid in providing the correct parameters.

This commit fixes a number of file match bugs:

* __filter_file_buf() was passing a length value of one less than the
  string length to cmpbytes() and as a result Equals and Prefix matches
  were testing one fewer bytes than required.

* ASM_RCMP (used in rcmpbytes()) was decrementing the string indices
  before testing if they were <1. This resulted in one fewer bytes being
  tested than required.

* ASM_RCMP50 comprisd of 2x ASM_RCMP20 and 1x ASM_RCMP5, totalling 45
  iterations of ASM_RCMP, instead of 50.

* __filter_file_buf() tested failed postfix matches with a forward string
  match. This resulted in files that started with the postfix, but didn't
  end with it, matching when they shouldn't.

* cmpbytes() continued to loop to full number of iterations, even when
  the string length had been exhausted (inefficient).

* Added descriptions to cmpbytes() and rcmpbytes() to aid in providing
  the correct parameters.

Signed-off-by: Kevin Sheldrake <[email protected]>
@kevsecurity kevsecurity requested a review from a team as a code owner February 23, 2023 15:59
@jrfastab
Copy link
Contributor

I vaguely remember these things being off by one the other way and something about the \n\r chars but don't recall the details. Anyways tests should be able to verify this is all correct. (Note I only read the pr msg so far)./

@kevsecurity
Copy link
Contributor Author

I vaguely remember these things being off by one the other way and something about the \n\r chars but don't recall the details. Anyways tests should be able to verify this is all correct. (Note I only read the pr msg so far)./

Rudimentary tests (i.e. manipulating CRD and testing) seems to confirm the fixes work as expected.

@kkourt kkourt mentioned this pull request Feb 23, 2023
@kkourt
Copy link
Contributor

kkourt commented Feb 23, 2023

added an issue for adding tests to catch the issues here: #728. (My assumption is that we do not need to wait for the tests, before we merge this. Happy to wait though :) )

@kevsecurity kevsecurity merged commit 5ebb0e3 into main Feb 27, 2023
@kevsecurity kevsecurity deleted the pr/kevsecurity/fix-selector-cmp branch February 27, 2023 10:30
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