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

Add support to generate_stack action #4382

Merged
merged 19 commits into from
Jun 17, 2024
Merged

Conversation

CarlesDD
Copy link
Contributor

@CarlesDD CarlesDD commented Jun 6, 2024

What does this PR do?

Collects and reports stack traces whenever generate_stack action is provided by the waf.

Stack trace library frames are stripped out from reported list to ensure that truncated stack traces aren’t polluted by instrumentation frames or otherwise, allowing customers to focus only on frames relevant to them.

Stack traces are sent as part of root span using the tag _dd.stack, in meta_struct field.

Three new configurations have been added to control this feature. Check them here.

Motivation

To provide important information to the customer to understand where the vulnerability exists in their code base.

Additional Notes

N/A

APPSEC-53369

Copy link

github-actions bot commented Jun 6, 2024

Overall package size

Self size: 6.7 MB
Deduped: 61.96 MB
No deduping: 62.24 MB

Dependency sizes

name version self size total size
@datadog/native-appsec 8.0.1 15.59 MB 15.6 MB
@datadog/native-iast-taint-tracking 2.1.0 14.91 MB 14.92 MB
@datadog/pprof 5.3.0 9.85 MB 10.22 MB
protobufjs 7.2.5 2.77 MB 6.56 MB
@datadog/native-iast-rewriter 2.3.1 2.15 MB 2.24 MB
@opentelemetry/core 1.14.0 872.87 kB 1.47 MB
@datadog/native-metrics 2.0.0 898.77 kB 1.3 MB
@opentelemetry/api 1.8.0 1.21 MB 1.21 MB
import-in-the-middle 1.7.4 70.19 kB 739.86 kB
msgpack-lite 0.1.26 201.16 kB 281.59 kB
opentracing 0.14.7 194.81 kB 194.81 kB
semver 7.5.4 93.4 kB 123.8 kB
pprof-format 2.1.0 111.69 kB 111.69 kB
@datadog/sketches-js 2.1.0 109.9 kB 109.9 kB
lodash.sortby 4.7.0 75.76 kB 75.76 kB
lru-cache 7.14.0 74.95 kB 74.95 kB
ignore 5.2.4 51.22 kB 51.22 kB
int64-buffer 0.1.10 49.18 kB 49.18 kB
shell-quote 1.8.1 44.96 kB 44.96 kB
istanbul-lib-coverage 3.2.0 29.34 kB 29.34 kB
tlhunter-sorted-set 0.1.0 24.94 kB 24.94 kB
limiter 1.1.5 23.17 kB 23.17 kB
dc-polyfill 0.1.4 23.1 kB 23.1 kB
retry 0.13.1 18.85 kB 18.85 kB
jest-docblock 29.7.0 8.99 kB 12.76 kB
crypto-randomuuid 1.0.0 11.18 kB 11.18 kB
path-to-regexp 0.1.7 6.78 kB 6.78 kB
koalas 1.0.2 6.47 kB 6.47 kB
module-details-from-path 1.0.3 4.47 kB 4.47 kB

🤖 This report was automatically generated by heaviest-objects-in-the-universe

Copy link

codecov bot commented Jun 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.41%. Comparing base (5278b1c) to head (ac538f8).
Report is 1 commits behind head on master.

Current head ac538f8 differs from pull request most recent head 6df590c

Please upload reports for the commit 6df590c to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4382      +/-   ##
==========================================
+ Coverage   80.42%   88.41%   +7.98%     
==========================================
  Files           3      113     +110     
  Lines         373     4040    +3667     
  Branches       33       33              
==========================================
+ Hits          300     3572    +3272     
- Misses         73      468     +395     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pr-commenter
Copy link

pr-commenter bot commented Jun 6, 2024

Benchmarks

Benchmark execution time: 2024-06-14 14:23:04

Comparing candidate commit 6df590c in PR branch ccapell/rasp-stack-traces with baseline commit 1cd017b in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 258 metrics, 8 unstable metrics.

@CarlesDD CarlesDD marked this pull request as ready for review June 7, 2024 13:11
@CarlesDD CarlesDD requested review from a team as code owners June 7, 2024 13:11
@CarlesDD CarlesDD self-assigned this Jun 7, 2024
packages/dd-trace/src/appsec/stack_trace.js Show resolved Hide resolved
Comment on lines 43 to 52
const indexedFrames = filteredFrames.map((callSite, i) => {
return {
id: i++,
file: callSite.getFileName(),
line: callSite.getLineNumber(),
column: callSite.getColumnNumber(),
function: callSite.getFunctionName()
}
})
return cutDownFrames(indexedFrames, maxDepth)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this way, we are creating 68 unnecessary objects by default, if you merge both actions in one forEach you will be able to prevent the creation of these objects:

const resultFrames = []
const lastInitialIndex = /*calculate last beginning index */
const firstFinalIndex = /*calculate first tail index */
filteredFrames.forEach((callSite, id) => {
  if (id < lastInitialIndex || id > firstFinalIndex) {
    resultFrames.push({
      id,
      file: callSite.getFileName(),
      line: callSite.getLineNumber(),
      column: callSite.getColumnNumber(),
      function: callSite.getFunctionName()
    })
  }
})
return resultFrames

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reworked this part with a similar proposed approach. Please, check it out.

packages/dd-trace/src/config.js Outdated Show resolved Hide resolved
[...Array(10).keys()].map(i => (
{
getFileName: () => path.join(__dirname, `file${i}`),
getLineNumber: () => `${i}`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Aren't getLineNumber are getColumnNumber expecting to return a number?

Qard
Qard previously approved these changes Jun 10, 2024
Copy link
Contributor

@Qard Qard left a comment

Choose a reason for hiding this comment

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

Seems reasonable. Might be a good idea to define a more universal standard though on how we capture stack traces given that several products capture and use them. 🤔

packages/dd-trace/src/appsec/stack_trace.js Outdated Show resolved Hide resolved
packages/dd-trace/src/appsec/stack_trace.js Show resolved Hide resolved
packages/dd-trace/test/appsec/stack_trace.spec.js Outdated Show resolved Hide resolved
rootSpan.meta_struct['_dd.stack'].exploit = []
}

if (rootSpan.meta_struct['_dd.stack'].exploit.length < maxStackTraces) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If maxStackTraces is configured as 0, it shouldn't have a limit.

Suggested change
if (rootSpan.meta_struct['_dd.stack'].exploit.length < maxStackTraces) {
if (maxStackTraces < 1 || rootSpan.meta_struct['_dd.stack'].exploit.length < maxStackTraces) {

uurien
uurien previously approved these changes Jun 12, 2024
@CarlesDD CarlesDD merged commit 5be6314 into master Jun 17, 2024
134 checks passed
@CarlesDD CarlesDD deleted the ccapell/rasp-stack-traces branch June 17, 2024 09:15
tlhunter pushed a commit that referenced this pull request Jun 20, 2024
* Stack trace collection configuration

* Collect and report stack trace for appsec events

* Handle generate_stack waf action

* Fix linting in config.spec.js

* Add assertion for stack trace tag in meta_struct for express test

* Refactor reportStackTrace and some additional test

* Fix lint

* Additional assert in reportStackTrace test

* Update config

* Rework on stack trace collection

* Callsite line and column as numbers

* Update packages/dd-trace/src/appsec/stack_trace.js

Co-authored-by: Ugaitz Urien <[email protected]>

* Update packages/dd-trace/src/appsec/stack_trace.js

Co-authored-by: Ugaitz Urien <[email protected]>

* Reorder test structure

* Fix linting

* No exploit stack limit when max is set to 0 or below

* Fix filtered and capped frames case

* Fix lint

---------

Co-authored-by: Ugaitz Urien <[email protected]>
tlhunter pushed a commit that referenced this pull request Jun 20, 2024
* Stack trace collection configuration

* Collect and report stack trace for appsec events

* Handle generate_stack waf action

* Fix linting in config.spec.js

* Add assertion for stack trace tag in meta_struct for express test

* Refactor reportStackTrace and some additional test

* Fix lint

* Additional assert in reportStackTrace test

* Update config

* Rework on stack trace collection

* Callsite line and column as numbers

* Update packages/dd-trace/src/appsec/stack_trace.js

Co-authored-by: Ugaitz Urien <[email protected]>

* Update packages/dd-trace/src/appsec/stack_trace.js

Co-authored-by: Ugaitz Urien <[email protected]>

* Reorder test structure

* Fix linting

* No exploit stack limit when max is set to 0 or below

* Fix filtered and capped frames case

* Fix lint

---------

Co-authored-by: Ugaitz Urien <[email protected]>
juan-fernandez pushed a commit that referenced this pull request Jul 10, 2024
* Stack trace collection configuration

* Collect and report stack trace for appsec events

* Handle generate_stack waf action

* Fix linting in config.spec.js

* Add assertion for stack trace tag in meta_struct for express test

* Refactor reportStackTrace and some additional test

* Fix lint

* Additional assert in reportStackTrace test

* Update config

* Rework on stack trace collection

* Callsite line and column as numbers

* Update packages/dd-trace/src/appsec/stack_trace.js

Co-authored-by: Ugaitz Urien <[email protected]>

* Update packages/dd-trace/src/appsec/stack_trace.js

Co-authored-by: Ugaitz Urien <[email protected]>

* Reorder test structure

* Fix linting

* No exploit stack limit when max is set to 0 or below

* Fix filtered and capped frames case

* Fix lint

---------

Co-authored-by: Ugaitz Urien <[email protected]>
juan-fernandez pushed a commit that referenced this pull request Jul 10, 2024
* Stack trace collection configuration

* Collect and report stack trace for appsec events

* Handle generate_stack waf action

* Fix linting in config.spec.js

* Add assertion for stack trace tag in meta_struct for express test

* Refactor reportStackTrace and some additional test

* Fix lint

* Additional assert in reportStackTrace test

* Update config

* Rework on stack trace collection

* Callsite line and column as numbers

* Update packages/dd-trace/src/appsec/stack_trace.js

Co-authored-by: Ugaitz Urien <[email protected]>

* Update packages/dd-trace/src/appsec/stack_trace.js

Co-authored-by: Ugaitz Urien <[email protected]>

* Reorder test structure

* Fix linting

* No exploit stack limit when max is set to 0 or below

* Fix filtered and capped frames case

* Fix lint

---------

Co-authored-by: Ugaitz Urien <[email protected]>
juan-fernandez pushed a commit that referenced this pull request Jul 11, 2024
* Stack trace collection configuration

* Collect and report stack trace for appsec events

* Handle generate_stack waf action

* Fix linting in config.spec.js

* Add assertion for stack trace tag in meta_struct for express test

* Refactor reportStackTrace and some additional test

* Fix lint

* Additional assert in reportStackTrace test

* Update config

* Rework on stack trace collection

* Callsite line and column as numbers

* Update packages/dd-trace/src/appsec/stack_trace.js

Co-authored-by: Ugaitz Urien <[email protected]>

* Update packages/dd-trace/src/appsec/stack_trace.js

Co-authored-by: Ugaitz Urien <[email protected]>

* Reorder test structure

* Fix linting

* No exploit stack limit when max is set to 0 or below

* Fix filtered and capped frames case

* Fix lint

---------

Co-authored-by: Ugaitz Urien <[email protected]>
juan-fernandez pushed a commit that referenced this pull request Jul 11, 2024
* Stack trace collection configuration

* Collect and report stack trace for appsec events

* Handle generate_stack waf action

* Fix linting in config.spec.js

* Add assertion for stack trace tag in meta_struct for express test

* Refactor reportStackTrace and some additional test

* Fix lint

* Additional assert in reportStackTrace test

* Update config

* Rework on stack trace collection

* Callsite line and column as numbers

* Update packages/dd-trace/src/appsec/stack_trace.js

Co-authored-by: Ugaitz Urien <[email protected]>

* Update packages/dd-trace/src/appsec/stack_trace.js

Co-authored-by: Ugaitz Urien <[email protected]>

* Reorder test structure

* Fix linting

* No exploit stack limit when max is set to 0 or below

* Fix filtered and capped frames case

* Fix lint

---------

Co-authored-by: Ugaitz Urien <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants