Skip to content

Conversation

@jandro996
Copy link
Member

@jandro996 jandro996 commented Jul 6, 2023

What Does This Do

  • Add mark attribute to Range
  • Provide a mark for each vulnerabilityType
  • Provide a new method get all ranges that are not marked for a vulnerability type
  • Overload taintString and taintObject methods with a mark parameter to allow tainting with marks
  • Improve highestPriorityRange algorithm using marks

Motivation

Add an exclusion mark system to avoid reporting vulnerabilities if all its ranges are marked as excluded for that type of vulnerability.

Additional Notes

  • Use an int bit field for flagging system to improve performance
  • Remove previous Range constructor to maintain ranges inmutables
  • highestPriorityRange algorithm can be more accurate but we decided to keep it simple for performance

@jandro996 jandro996 added tag: do not merge Do not merge changes tag: no release notes Changes to exclude from release notes comp: asm iast Application Security Management (IAST) labels Jul 6, 2023
@pr-commenter
Copy link

pr-commenter bot commented Jul 6, 2023

Benchmarks

Parameters

Baseline Candidate
commit 1.18.0-SNAPSHOT~48fe24d67b 1.18.0-SNAPSHOT~af984886ba
config baseline candidate
See matching parameters
Baseline Candidate
module Agent Agent
parent None None

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 22 cases.

@jandro996 jandro996 force-pushed the alejandro.gonzalez/range_marks branch from 31fef31 to 5967f11 Compare July 6, 2023 11:17
@jandro996 jandro996 changed the title Add marks to Range Add vulnerabilities flagging system Jul 6, 2023
@jandro996 jandro996 force-pushed the alejandro.gonzalez/range_marks branch from 487478c to b4ccc06 Compare July 6, 2023 11:55
@jandro996 jandro996 force-pushed the alejandro.gonzalez/range_marks branch from b4ccc06 to 24c5494 Compare July 6, 2023 12:01
@jandro996 jandro996 marked this pull request as ready for review July 6, 2023 13:02
@jandro996 jandro996 requested review from a team and DDJavierSantos July 6, 2023 13:02
@jandro996 jandro996 removed the tag: do not merge Do not merge changes label Jul 6, 2023
@smola smola changed the title Add vulnerabilities flagging system Add marks to exclude ranges from vulnerability reporting Jul 7, 2023
@jandro996 jandro996 requested a review from smola July 7, 2023 08:45
Copy link
Member

@smola smola left a comment

Choose a reason for hiding this comment

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

It looks good. But I think we're missing skipping marked ranges from vulnerability reports. If there is one secure range and one non-secure, the secure one should be omitted. This might change in the future if/when we have a spec on how to send this additional info to the backend.

@jandro996
Copy link
Member Author

It looks good. But I think we're missing skipping marked ranges from vulnerability reports. If there is one secure range and one non-secure, the secure one should be omitted. This might change in the future if/when we have a spec on how to send this additional info to the backend.

I'm working on it

…and only add to the evidence not marked ranges
@jandro996
Copy link
Member Author

It looks good. But I think we're missing skipping marked ranges from vulnerability reports. If there is one secure range and one non-secure, the secure one should be omitted. This might change in the future if/when we have a spec on how to send this additional info to the backend.

Done!

@jandro996 jandro996 requested a review from smola July 10, 2023 05:23
@jandro996 jandro996 merged commit 71ab4bb into master Jul 10, 2023
@jandro996 jandro996 deleted the alejandro.gonzalez/range_marks branch July 10, 2023 17:10
@github-actions github-actions bot added this to the 1.18.0 milestone Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp: asm iast Application Security Management (IAST) tag: no release notes Changes to exclude from release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants