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.

Sign up for GitHub

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

Jump to bottom

IAST support for commons fileupload #6089

Merged
merged 11 commits into from
Nov 16, 2023
Merged

IAST support for commons fileupload #6089

merged 11 commits into from
Nov 16, 2023

Conversation

DDJavierSantos
Copy link
Contributor

@DDJavierSantos DDJavierSantos commented Oct 24, 2023 •
edited by jira bot
Loading

What Does This Do

Instrument the commons fileupload form parser so form properties are marked as tainted.

Motivation

To cover all tainted inputs.

Additional Notes

Jira ticket: APPSEC-11821

@DDJavierSantos DDJavierSantos requested a review from a team as a code owner October 24, 2023 08:24
@pr-commenter
Copy link

pr-commenter bot commented Oct 24, 2023 •
edited
Loading

Benchmarks

Startup

Parameters

Baseline Candidate
commit 1.24.0-SNAPSHOT~e4541b1774 1.24.0-SNAPSHOT~a8d6ccaee5
config baseline candidate
See matching parameters
Baseline Candidate
application insecure-bank insecure-bank
module Agent Agent
parent None None
variant iast iast

Summary

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

Startup time reports for insecure-bank 
gantt
    title insecure-bank - global startup overhead: candidate=1.24.0-SNAPSHOT~a8d6ccaee5, baseline=1.24.0-SNAPSHOT~e4541b1774

    dateFormat X
    axisFormat %s
section tracing
Agent [baseline] (1.044 s) : 0, 1043585
Total [baseline] (8.776 s) : 0, 8775511
Agent [candidate] (1.031 s) : 0, 1031143
Total [candidate] (8.781 s) : 0, 8780559
section iast
Agent [baseline] (1.16 s) : 0, 1160218
Total [baseline] (9.344 s) : 0, 9343728
Agent [candidate] (1.146 s) : 0, 1146495
Total [candidate] (9.28 s) : 0, 9279804
section iast_TELEMETRY_OFF
Agent [baseline] (1.14 s) : 0, 1140159
Total [baseline] (9.301 s) : 0, 9300562
Agent [candidate] (1.154 s) : 0, 1154176
Total [candidate] (9.348 s) : 0, 9347650
Loading
  • baseline results
Module Variant Duration Δ tracing
Agent tracing 1.044 s -
Agent iast 1.16 s 116.633 ms (11.2%)
Agent iast_TELEMETRY_OFF 1.14 s 96.574 ms (9.3%)
Total tracing 8.776 s -
Total iast 9.344 s 568.216 ms (6.5%)
Total iast_TELEMETRY_OFF 9.301 s 525.05 ms (6.0%)
  • candidate results
Module Variant Duration Δ tracing
Agent tracing 1.031 s -
Agent iast 1.146 s 115.353 ms (11.2%)
Agent iast_TELEMETRY_OFF 1.154 s 123.033 ms (11.9%)
Total tracing 8.781 s -
Total iast 9.28 s 499.246 ms (5.7%)
Total iast_TELEMETRY_OFF 9.348 s 567.091 ms (6.5%) 
gantt
    title insecure-bank - break down per module: candidate=1.24.0-SNAPSHOT~a8d6ccaee5, baseline=1.24.0-SNAPSHOT~e4541b1774

    dateFormat X
    axisFormat %s
section tracing
BytebuddyAgent [baseline] (653.553 ms) : 0, 653553
BytebuddyAgent [candidate] (645.406 ms) : 0, 645406
GlobalTracer [baseline] (297.421 ms) : 0, 297421
GlobalTracer [candidate] (294.759 ms) : 0, 294759
AppSec [baseline] (49.761 ms) : 0, 49761
AppSec [candidate] (48.793 ms) : 0, 48793
Remote Config [baseline] (668.773 µs) : 0, 669
Remote Config [candidate] (656.079 µs) : 0, 656
Telemetry [baseline] (7.322 ms) : 0, 7322
Telemetry [candidate] (7.214 ms) : 0, 7214
section iast
BytebuddyAgent [baseline] (775.509 ms) : 0, 775509
BytebuddyAgent [candidate] (764.772 ms) : 0, 764772
GlobalTracer [baseline] (276.886 ms) : 0, 276886
GlobalTracer [candidate] (274.498 ms) : 0, 274498
AppSec [baseline] (46.971 ms) : 0, 46971
AppSec [candidate] (46.922 ms) : 0, 46922
Remote Config [baseline] (581.745 µs) : 0, 582
Remote Config [candidate] (561.214 µs) : 0, 561
Telemetry [baseline] (9.849 ms) : 0, 9849
Telemetry [candidate] (10.564 ms) : 0, 10564
IAST [baseline] (15.639 ms) : 0, 15639
IAST [candidate] (14.833 ms) : 0, 14833
section iast_TELEMETRY_OFF
BytebuddyAgent [baseline] (757.194 ms) : 0, 757194
BytebuddyAgent [candidate] (768.264 ms) : 0, 768264
GlobalTracer [baseline] (274.522 ms) : 0, 274522
GlobalTracer [candidate] (279.007 ms) : 0, 279007
AppSec [baseline] (46.539 ms) : 0, 46539
AppSec [candidate] (47.324 ms) : 0, 47324
Remote Config [baseline] (590.394 µs) : 0, 590
Remote Config [candidate] (599.987 µs) : 0, 600
Telemetry [baseline] (10.4 ms) : 0, 10400
Telemetry [candidate] (8.59 ms) : 0, 8590
IAST [baseline] (16.646 ms) : 0, 16646
IAST [candidate] (15.656 ms) : 0, 15656
Loading
Startup time reports for petclinic 
gantt
    title petclinic - global startup overhead: candidate=1.24.0-SNAPSHOT~a8d6ccaee5, baseline=1.24.0-SNAPSHOT~e4541b1774

    dateFormat X
    axisFormat %s
section tracing
Agent [baseline] (1.041 s) : 0, 1041212
Total [baseline] (9.347 s) : 0, 9346746
Agent [candidate] (1.041 s) : 0, 1040841
Total [candidate] (9.34 s) : 0, 9339594
section appsec
Agent [baseline] (1.12 s) : 0, 1119605
Total [baseline] (9.348 s) : 0, 9347932
Agent [candidate] (1.121 s) : 0, 1121204
Total [candidate] (9.343 s) : 0, 9342688
section iast
Agent [baseline] (1.149 s) : 0, 1148883
Total [baseline] (9.512 s) : 0, 9512088
Agent [candidate] (1.147 s) : 0, 1147280
Total [candidate] (9.489 s) : 0, 9489164
section profiling
Agent [baseline] (1.215 s) : 0, 1214939
Total [baseline] (9.475 s) : 0, 9475477
Agent [candidate] (1.22 s) : 0, 1220283
Total [candidate] (9.508 s) : 0, 9507553
Loading
  • baseline results
Module Variant Duration Δ tracing
Agent tracing 1.041 s -
Agent appsec 1.12 s 78.393 ms (7.5%)
Agent iast 1.149 s 107.671 ms (10.3%)
Agent profiling 1.215 s 173.727 ms (16.7%)
Total tracing 9.347 s -
Total appsec 9.348 s 1.185 ms (0.0%)
Total iast 9.512 s 165.342 ms (1.8%)
Total profiling 9.475 s 128.73 ms (1.4%)
  • candidate results
Module Variant Duration Δ tracing
Agent tracing 1.041 s -
Agent appsec 1.121 s 80.363 ms (7.7%)
Agent iast 1.147 s 106.438 ms (10.2%)
Agent profiling 1.22 s 179.442 ms (17.2%)
Total tracing 9.34 s -
Total appsec 9.343 s 3.094 ms (0.0%)
Total iast 9.489 s 149.571 ms (1.6%)
Total profiling 9.508 s 167.959 ms (1.8%) 
gantt
    title petclinic - break down per module: candidate=1.24.0-SNAPSHOT~a8d6ccaee5, baseline=1.24.0-SNAPSHOT~e4541b1774

    dateFormat X
    axisFormat %s
section tracing
BytebuddyAgent [baseline] (652.114 ms) : 0, 652114
BytebuddyAgent [candidate] (651.446 ms) : 0, 651446
GlobalTracer [baseline] (296.883 ms) : 0, 296883
GlobalTracer [candidate] (297.51 ms) : 0, 297510
AppSec [baseline] (49.514 ms) : 0, 49514
AppSec [candidate] (49.33 ms) : 0, 49330
Remote Config [baseline] (669.428 µs) : 0, 669
Remote Config [candidate] (665.949 µs) : 0, 666
Telemetry [baseline] (7.239 ms) : 0, 7239
Telemetry [candidate] (7.289 ms) : 0, 7289
section appsec
BytebuddyAgent [baseline] (645.225 ms) : 0, 645225
BytebuddyAgent [candidate] (646.399 ms) : 0, 646399
GlobalTracer [baseline] (293.757 ms) : 0, 293757
GlobalTracer [candidate] (295.016 ms) : 0, 295016
AppSec [baseline] (138.934 ms) : 0, 138934
AppSec [candidate] (138.086 ms) : 0, 138086
Remote Config [baseline] (653.523 µs) : 0, 654
Remote Config [candidate] (639.133 µs) : 0, 639
Telemetry [baseline] (6.736 ms) : 0, 6736
Telemetry [candidate] (6.747 ms) : 0, 6747
section iast
BytebuddyAgent [baseline] (765.969 ms) : 0, 765969
BytebuddyAgent [candidate] (765.581 ms) : 0, 765581
GlobalTracer [baseline] (274.98 ms) : 0, 274980
GlobalTracer [candidate] (274.865 ms) : 0, 274865
AppSec [baseline] (46.672 ms) : 0, 46672
AppSec [candidate] (46.462 ms) : 0, 46462
Remote Config [baseline] (580.885 µs) : 0, 581
Remote Config [candidate] (570.945 µs) : 0, 571
Telemetry [baseline] (8.536 ms) : 0, 8536
Telemetry [candidate] (6.447 ms) : 0, 6447
IAST [baseline] (17.763 ms) : 0, 17763
IAST [candidate] (19.022 ms) : 0, 19022
section profiling
ProfilingAgent [baseline] (87.337 ms) : 0, 87337
ProfilingAgent [candidate] (87.78 ms) : 0, 87780
BytebuddyAgent [baseline] (656.637 ms) : 0, 656637
BytebuddyAgent [candidate] (660.883 ms) : 0, 660883
GlobalTracer [baseline] (359.786 ms) : 0, 359786
GlobalTracer [candidate] (360.167 ms) : 0, 360167
AppSec [baseline] (48.786 ms) : 0, 48786
AppSec [candidate] (48.636 ms) : 0, 48636
Remote Config [baseline] (647.88 µs) : 0, 648
Remote Config [candidate] (662.667 µs) : 0, 663
Telemetry [baseline] (7.476 ms) : 0, 7476
Telemetry [candidate] (7.455 ms) : 0, 7455
Profiling [baseline] (87.361 ms) : 0, 87361
Profiling [candidate] (87.807 ms) : 0, 87807
Loading

Load

Parameters

Baseline Candidate
commit 1.24.0-SNAPSHOT~e4541b1774 1.24.0-SNAPSHOT~a8d6ccaee5
config baseline candidate
end_time 2023-11-15T14:52:42 2023-11-15T15:09:09
start_time 2023-11-15T14:52:30 2023-11-15T15:08:56
See matching parameters
Baseline Candidate
application insecure-bank insecure-bank
variant iast iast

Summary

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

Request duration reports for insecure-bank 
gantt
    title insecure-bank - request duration [CI 0.99] : candidate=1.24.0-SNAPSHOT~a8d6ccaee5, baseline=1.24.0-SNAPSHOT~e4541b1774
    dateFormat X
    axisFormat %s
section baseline
no_agent (363.541 µs) : 342, 385
.   : milestone, 364,
iast (461.448 µs) : 441, 482
.   : milestone, 461,
iast_FULL (525.334 µs) : 505, 546
.   : milestone, 525,
iast_INACTIVE (438.79 µs) : 418, 459
.   : milestone, 439,
iast_TELEMETRY_OFF (468.767 µs) : 448, 490
.   : milestone, 469,
tracing (434.859 µs) : 414, 456
.   : milestone, 435,
section candidate
no_agent (367.712 µs) : 347, 389
.   : milestone, 368,
iast (460.456 µs) : 440, 481
.   : milestone, 460,
iast_FULL (529.164 µs) : 509, 550
.   : milestone, 529,
iast_INACTIVE (436.241 µs) : 416, 457
.   : milestone, 436,
iast_TELEMETRY_OFF (464.758 µs) : 443, 486
.   : milestone, 465,
tracing (434.812 µs) : 414, 456
.   : milestone, 435,
Loading
  • baseline results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 363.541 µs [342.485 µs, 384.598 µs] -
iast 461.448 µs [440.812 µs, 482.084 µs] 97.907 µs (26.9%)
iast_FULL 525.334 µs [504.717 µs, 545.951 µs] 161.793 µs (44.5%)
iast_INACTIVE 438.79 µs [418.229 µs, 459.352 µs] 75.249 µs (20.7%)
iast_TELEMETRY_OFF 468.767 µs [447.709 µs, 489.824 µs] 105.225 µs (28.9%)
tracing 434.859 µs [413.54 µs, 456.179 µs] 71.318 µs (19.6%)
  • candidate results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 367.712 µs [346.506 µs, 388.918 µs] -
iast 460.456 µs [439.873 µs, 481.039 µs] 92.744 µs (25.2%)
iast_FULL 529.164 µs [508.593 µs, 549.736 µs] 161.452 µs (43.9%)
iast_INACTIVE 436.241 µs [415.636 µs, 456.845 µs] 68.528 µs (18.6%)
iast_TELEMETRY_OFF 464.758 µs [443.387 µs, 486.128 µs] 97.045 µs (26.4%)
tracing 434.812 µs [413.818 µs, 455.806 µs] 67.1 µs (18.2%)
Request duration reports for petclinic 
gantt
    title petclinic - request duration [CI 0.99] : candidate=1.24.0-SNAPSHOT~a8d6ccaee5, baseline=1.24.0-SNAPSHOT~e4541b1774
    dateFormat X
    axisFormat %s
section baseline
no_agent (1.364 ms) : 1345, 1384
.   : milestone, 1364,
appsec (1.712 ms) : 1687, 1738
.   : milestone, 1712,
iast (1.502 ms) : 1478, 1526
.   : milestone, 1502,
profiling (1.473 ms) : 1448, 1498
.   : milestone, 1473,
tracing (1.434 ms) : 1409, 1459
.   : milestone, 1434,
section candidate
no_agent (1.362 ms) : 1343, 1381
.   : milestone, 1362,
appsec (1.712 ms) : 1687, 1736
.   : milestone, 1712,
iast (1.461 ms) : 1437, 1485
.   : milestone, 1461,
profiling (1.488 ms) : 1462, 1514
.   : milestone, 1488,
tracing (1.453 ms) : 1428, 1478
.   : milestone, 1453,
Loading
  • baseline results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 1.364 ms [1.345 ms, 1.384 ms] -
appsec 1.712 ms [1.687 ms, 1.738 ms] 348.078 µs (25.5%)
iast 1.502 ms [1.478 ms, 1.526 ms] 137.855 µs (10.1%)
profiling 1.473 ms [1.448 ms, 1.498 ms] 108.775 µs (8.0%)
tracing 1.434 ms [1.409 ms, 1.459 ms] 69.69 µs (5.1%)
  • candidate results
Variant Request duration [CI 0.99] Δ no_agent
no_agent 1.362 ms [1.343 ms, 1.381 ms] -
appsec 1.712 ms [1.687 ms, 1.736 ms] 349.335 µs (25.6%)
iast 1.461 ms [1.437 ms, 1.485 ms] 98.407 µs (7.2%)
profiling 1.488 ms [1.462 ms, 1.514 ms] 125.711 µs (9.2%)
tracing 1.453 ms [1.428 ms, 1.478 ms] 90.776 µs (6.7%)

@smola smola requested review from a team and ValentinZakharov October 24, 2023 09:10
@smola smola added the comp: asm iast Application Security Management (IAST) label Oct 24, 2023
@smola smola changed the title IAST - Commons fileupload instrumentation IAST support for commons fileupload Oct 24, 2023
@DDJavierSantos DDJavierSantos force-pushed the jsantos/Commons_Fileupload_Instrumentation branch from 4dc54db to 93c78f6 Compare October 26, 2023 11:36
@DDJavierSantos DDJavierSantos force-pushed the jsantos/Commons_Fileupload_Instrumentation branch 3 times, most recently from 9207c4b to bd984a2 Compare November 2, 2023 12:07
@DDJavierSantos DDJavierSantos force-pushed the jsantos/Commons_Fileupload_Instrumentation branch from c2e5935 to 53f9290 Compare November 8, 2023 14:23
bm1549
bm1549 reviewed Nov 8, 2023
View reviewed changes
Copy link
Contributor

@bm1549 bm1549 left a comment

Choose a reason for hiding this comment

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

Question - should this code be owned by the client library team or ASM?
If it should be ASM, please update the CODEOWNERS accordingly

@DDJavierSantos DDJavierSantos force-pushed the jsantos/Commons_Fileupload_Instrumentation branch from 3980552 to b0fe0b9 Compare November 8, 2023 19:23
@DDJavierSantos DDJavierSantos force-pushed the jsantos/Commons_Fileupload_Instrumentation branch 3 times, most recently from c65ec86 to 8a2d379 Compare November 13, 2023 12:21
@smola @DDJavierSantos
Update system-tests to 6998b307ce0e8d206ca6eb99045651dcc8bc853f (#6199)
fc3f4bd 
Fix error when trying to get class stream for Mockito mocks (#6183)

Fix error when serializing skippable tests whose names contain non-ASCII characters (#6182)

Fail fast when tracer versions do not match in parent and child processes (#6168)

initial instrumentation
@DDJavierSantos DDJavierSantos force-pushed the jsantos/Commons_Fileupload_Instrumentation branch from 8a2d379 to fc3f4bd Compare November 13, 2023 20:31
@smola
Copy link
Member

smola commented Nov 14, 2023 •
edited
Loading

@bm1549 Question - should this code be owned by the client library team or ASM? If it should be ASM, please update the CODEOWNERS accordingly

Yes, as long as there are no unrelated instrumentations in the module, we should own it. @DDJavierSantos can you update codeowners to add the path to this module to our team?

@DDJavierSantos @bm1549
Update .github/CODEOWNERS
d34ed65 
Co-authored-by: Brian Marks <[email protected]>
@DDJavierSantos DDJavierSantos merged commit df6decb into master Nov 16, 2023
68 of 71 checks passed
@DDJavierSantos DDJavierSantos deleted the jsantos/Commons_Fileupload_Instrumentation branch November 16, 2023 10:04
@github-actions github-actions bot added this to the 1.25.0 milestone Nov 16, 2023
mcculls added a commit that referenced this pull request Nov 16, 2023
@mcculls
Move integrations-core submodule back to 7.49.0 (was accidentally rev…
1b0471a 
…erted to 7.47.0 by #6089)
@mcculls
Copy link
Contributor

mcculls commented Nov 16, 2023

FYI, this PR accidentally reverted the version of the integrations-core submodule to 7.47.0

I've fixed this on master, but you can stop this from happening again with this git config:

git config --local submodule.recurse true

you just need to apply it to your local checkout of dd-trace-java as mentioned in CONTRIBUTING.md

@smola smola mentioned this pull request Nov 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manuel-alvarez-alvarez manuel-alvarez-alvarez manuel-alvarez-alvarez approved these changes

@ValentinZakharov ValentinZakharov ValentinZakharov approved these changes

@bm1549 bm1549 bm1549 approved these changes

@dougqh dougqh Awaiting requested review from dougqh dougqh is a code owner automatically assigned from DataDog/apm-java

@am312 am312 Awaiting requested review from am312 am312 is a code owner automatically assigned from DataDog/apm-java

@smola smola Awaiting requested review from smola

Assignees
No one assigned
Labels
comp: asm iast Application Security Management (IAST)
Projects
None yet
Milestone
1.25.0
Development

Successfully merging this pull request may close these issues.
6 participants
@DDJavierSantos @smola @mcculls @manuel-alvarez-alvarez @ValentinZakharov @bm1549