Prevents panics during ELF parsing from crashing process #30725

brycekahle · 2024-11-04T19:56:48Z

What does this PR do?

Uses the safeelf package everywhere, which handles panics during ELF parsing.
Prevents the usage of debug/elf with depguard.
Handles if elf.Section.ReaderAt is nil, which is possible.

Motivation

The debug/elf package is known to have several bugs/limitations which result in panics. Since system-probe parses many arbitrary binaries, this change will prevent those binaries from crashing system-probe if they are malformed in some way.

Describe how to test/QA your changes

Existing automated tests pass.

Possible Drawbacks / Trade-offs

Additional Notes

paulcacheux

LGTM for agent-security files

grantseltzer

LGTM

Not sure why but on your branch I got errors saying pcap.h couldn't be found. It's probably unrelated but if that means anything to you then do with that information what you will.

pkg/network/usm/ebpf_ssl.go

pkg/ebpf/uprobes/inspector.go

pkg/network/go/bininspect/symbols.go

val06 · 2024-11-05T09:16:30Z

pkg/util/safeelf/elf.go

+// Any panic during parsing is turned into an error. This is necessary since
+// there are a bunch of unfixed bugs in debug/elf.
+//
+// https://github.com/golang/go/issues?q=is%3Aissue+is%3Aopen+debug%2Felf+in%3Atitle


which issues from that list actually concern you?

debug/elf switched to use saferio under the hood and I think now it is considered a safe package

There have been panics fixed as recently as Go 1.22 due to slice bounds. This is a defensive approach, since we are parsing uncontrolled input.

we are using go 1.22 for the datadog-agent repo. I still don't see which item from the list you provided in the comment concerns you

I am not sure i understand the decision to introduce the panic wrapper specifically for the elf parsing case.
if we want to avoid panics we can make a general wrapper for the agent

I still don't see which item from the list you provided in the comment concerns you

This comment comes from cilium/ebpf which I grabbed some of the code from. I do share the concern though. Someone from the Go team even responded to me with this:

But running go-fuzz on the package is likely to re-reproduce it (good idea anyway b/c there are likely more bugs after this one)

Separately:

if we want to avoid panics we can make a general wrapper for the agent

It is not possible to have a general panic/recover wrapper and still maintain correct state or control flow. You must do it deep and where you have knowledge of the called code behavior. In this case we know the debug/elf parsing is not using Goroutines, so we are safe to use panic/recover.

Regardless of the above, this is a defensive approach that costs us nothing.

cit-pr-commenter · 2024-11-05T19:17:32Z

Go Package Import Differences

Baseline: 5ed352e
Comparison: 8a34060

binary	os	arch	change
agent	linux	amd64	+1, -0 +github.com/DataDog/datadog-agent/pkg/util/safeelf
agent	linux	arm64	+1, -0 +github.com/DataDog/datadog-agent/pkg/util/safeelf
iot-agent	linux	amd64	+1, -0 +github.com/DataDog/datadog-agent/pkg/util/safeelf
iot-agent	linux	arm64	+1, -0 +github.com/DataDog/datadog-agent/pkg/util/safeelf
heroku-agent	linux	amd64	+1, -0 +github.com/DataDog/datadog-agent/pkg/util/safeelf
cluster-agent	linux	amd64	+1, -0 +github.com/DataDog/datadog-agent/pkg/util/safeelf
cluster-agent	linux	arm64	+1, -0 +github.com/DataDog/datadog-agent/pkg/util/safeelf
process-agent	linux	amd64	+1, -0 +github.com/DataDog/datadog-agent/pkg/util/safeelf
process-agent	linux	arm64	+1, -0 +github.com/DataDog/datadog-agent/pkg/util/safeelf
heroku-process-agent	linux	amd64	+1, -0 +github.com/DataDog/datadog-agent/pkg/util/safeelf
system-probe	linux	amd64	+1, -0 +github.com/DataDog/datadog-agent/pkg/util/safeelf
system-probe	linux	arm64	+1, -0 +github.com/DataDog/datadog-agent/pkg/util/safeelf

agent-platform-auto-pr · 2024-11-05T19:45:01Z

Test changes on VM

Use this command from test-infra-definitions to manually test this PR changes on a VM:

inv create-vm --pipeline-id=48860913 --os-family=ubuntu

Note: This applies to commit 8a34060

cit-pr-commenter · 2024-11-05T19:50:25Z

Regression Detector

Regression Detector Results

Metrics dashboard
Target profiles
Run ID: da2b05d1-dcfc-4137-b9e3-750fc4a67d22

Baseline: 5ed352e
Comparison: 8a34060
Diff

Optimization Goals: ✅ No significant changes detected

Fine details of change detection per experiment

perf	experiment	goal	Δ mean %	Δ mean % CI	trials	links
➖	pycheck_lots_of_tags	% cpu utilization	+1.11	[-2.30, +4.53]	1	Logs
➖	quality_gate_idle_all_features	memory utilization	+0.78	[+0.68, +0.88]	1	Logs bounds checks dashboard
➖	otel_to_otel_logs	ingress throughput	+0.52	[-0.16, +1.21]	1	Logs
➖	quality_gate_idle	memory utilization	+0.41	[+0.36, +0.46]	1	Logs bounds checks dashboard
➖	file_to_blackhole_1000ms_latency	egress throughput	+0.12	[-0.36, +0.60]	1	Logs
➖	uds_dogstatsd_to_api_cpu	% cpu utilization	+0.07	[-0.65, +0.80]	1	Logs
➖	tcp_dd_logs_filter_exclude	ingress throughput	-0.00	[-0.01, +0.01]	1	Logs
➖	uds_dogstatsd_to_api	ingress throughput	-0.00	[-0.09, +0.08]	1	Logs
➖	file_to_blackhole_100ms_latency	egress throughput	-0.01	[-0.32, +0.31]	1	Logs
➖	file_to_blackhole_1000ms_latency_linear_load	egress throughput	-0.01	[-0.49, +0.48]	1	Logs
➖	file_to_blackhole_300ms_latency	egress throughput	-0.03	[-0.22, +0.17]	1	Logs
➖	file_to_blackhole_0ms_latency	egress throughput	-0.03	[-0.49, +0.43]	1	Logs
➖	file_to_blackhole_500ms_latency	egress throughput	-0.08	[-0.32, +0.17]	1	Logs
➖	tcp_syslog_to_blackhole	ingress throughput	-0.09	[-0.13, -0.04]	1	Logs
➖	file_tree	memory utilization	-0.09	[-0.21, +0.03]	1	Logs
➖	basic_py_check	% cpu utilization	-3.72	[-7.39, -0.06]	1	Logs

Bounds Checks: ❌ Failed

perf	experiment	bounds_check_name	replicates_passed	links
❌	quality_gate_idle	memory_usage	0/10	bounds checks dashboard
❌	file_to_blackhole_0ms_latency	lost_bytes	2/10
❌	quality_gate_idle_all_features	memory_usage	9/10	bounds checks dashboard
✅	file_to_blackhole_0ms_latency	memory_usage	10/10
✅	file_to_blackhole_1000ms_latency	memory_usage	10/10
✅	file_to_blackhole_1000ms_latency_linear_load	memory_usage	10/10
✅	file_to_blackhole_100ms_latency	lost_bytes	10/10
✅	file_to_blackhole_100ms_latency	memory_usage	10/10
✅	file_to_blackhole_300ms_latency	memory_usage	10/10
✅	file_to_blackhole_500ms_latency	memory_usage	10/10

Explanation

Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%

Performance changes are noted in the perf column of each table:

✅ = significantly better comparison variant performance
❌ = significantly worse comparison variant performance
➖ = no significant change in performance

A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".

For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:

Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
Its configuration does not mark it "erratic".

pkg/network/go/bininspect/symbols.go

Kaderinho

I don't fully understand code in util/safeelf but I don't see anything strange either 👍 I would suggest to add codeowners for util/safeelf if you consider adding / modify this folder later

brycekahle · 2024-11-07T19:42:54Z

@guyarb I've addressed your comments.

brycekahle · 2024-11-12T22:09:33Z

/merge

dd-devflow · 2024-11-12T22:09:41Z

Devflow running: `/merge`

View all feedbacks in Devflow UI.

2024-11-12 22:09:40 UTC ℹ️ MergeQueue: pull request added to the queue

The median merge time in main is 23m.

2024-11-12 22:12:19 UTC 🚨 MergeQueue: This merge request is in error

Gitlab pipeline didn't start on its own and we were unable to create it... Please retry.

brycekahle · 2024-11-12T22:12:55Z

/merge

dd-devflow · 2024-11-12T22:13:06Z

Devflow running: `/merge`

View all feedbacks in Devflow UI.

2024-11-12 22:13:04 UTC ℹ️ MergeQueue: pull request added to the queue

The median merge time in main is 23m.

2024-11-12 22:23:13 UTC ❌ MergeQueue: The build pipeline contains failing jobs for this merge request

Build pipeline has failing jobs for 13972bc:

⚠️ Do NOT retry failed jobs directly (why?).

What to do next?

Investigate the failures and when ready, re-add your pull request to the queue!
Any question, go check the FAQ.

Details

Since those jobs are not marked as being allowed to fail, the pipeline will most likely fail.
Therefore, and to allow other builds to be processed, this merge request has been rejected and the pipeline got canceled.

brycekahle · 2024-11-12T23:08:31Z

/merge

dd-devflow · 2024-11-12T23:08:39Z

Devflow running: `/merge`

View all feedbacks in Devflow UI.

2024-11-12 23:08:39 UTC ℹ️ MergeQueue: waiting for PR to be ready

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.

2024-11-12 23:09:42 UTC ℹ️ MergeQueue: pull request added to the queue

The median merge time in main is 23m.

brycekahle added changelog/no-changelog bugfix/security team/ebpf-platform qa/done QA done before merge and regressions are covered by tests labels Nov 4, 2024

brycekahle added this to the 7.61.0 milestone Nov 4, 2024

brycekahle requested review from a team as code owners November 4, 2024 19:56

brycekahle requested a review from a team November 4, 2024 19:56

brycekahle requested review from a team as code owners November 4, 2024 19:56

brycekahle requested a review from Kaderinho November 4, 2024 19:56

github-actions bot added component/system-probe long review PR is complex, plan time to review it labels Nov 4, 2024

robertjli approved these changes Nov 4, 2024

View reviewed changes

paulcacheux approved these changes Nov 4, 2024

View reviewed changes

grantseltzer approved these changes Nov 4, 2024

View reviewed changes

guyarb reviewed Nov 4, 2024

View reviewed changes

pkg/network/usm/ebpf_ssl.go Outdated Show resolved Hide resolved

guyarb reviewed Nov 4, 2024

View reviewed changes

pkg/ebpf/uprobes/inspector.go Outdated Show resolved Hide resolved

guyarb reviewed Nov 4, 2024

View reviewed changes

pkg/network/go/bininspect/symbols.go Outdated Show resolved Hide resolved

gjulianm approved these changes Nov 5, 2024

View reviewed changes

KevinFairise2 approved these changes Nov 5, 2024

View reviewed changes

val06 reviewed Nov 5, 2024

View reviewed changes

guyarb reviewed Nov 6, 2024

View reviewed changes

pkg/network/go/bininspect/symbols.go Outdated Show resolved Hide resolved

Kaderinho approved these changes Nov 6, 2024

View reviewed changes

ogaca-dd approved these changes Nov 7, 2024

View reviewed changes

guyarb approved these changes Nov 8, 2024

View reviewed changes

brycekahle added 8 commits November 12, 2024 14:28

use safeelf to prevent panics when parsing ELF

31b9ad1

ensure Section.ReaderAt is non-nil

9b6dc37

add lint ignores

a134cc3

move ReaderAt check out of loop

43ff180

remove copy of SanitizeUprobeAddresses

e37b679

add to CODEOWNERS

53fa0c7

use errors.New

5c85965

fix cuda elf

8a34060

brycekahle force-pushed the bryce.kahle/safeelf branch from b272967 to 8a34060 Compare November 12, 2024 22:30

dd-mergequeue bot merged commit b0adcb9 into main Nov 12, 2024
288 of 297 checks passed

dd-mergequeue bot deleted the bryce.kahle/safeelf branch November 12, 2024 23:28

mftoure pushed a commit that referenced this pull request Nov 21, 2024

Prevents panics during ELF parsing from crashing process (#30725)

c586ecf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevents panics during ELF parsing from crashing process #30725

Prevents panics during ELF parsing from crashing process #30725

brycekahle commented Nov 4, 2024 •

edited

Loading

paulcacheux left a comment

grantseltzer left a comment

val06 Nov 5, 2024

brycekahle Nov 5, 2024

val06 Nov 6, 2024 •

edited

Loading

brycekahle Nov 6, 2024 •

edited

Loading

brycekahle Nov 7, 2024

cit-pr-commenter bot commented Nov 5, 2024 •

edited

Loading

agent-platform-auto-pr bot commented Nov 5, 2024 •

edited

Loading

cit-pr-commenter bot commented Nov 5, 2024 •

edited

Loading

Fine details of change detection per experiment

Explanation

Kaderinho left a comment •

edited

Loading

brycekahle commented Nov 7, 2024

brycekahle commented Nov 12, 2024

dd-devflow bot commented Nov 12, 2024 •

edited

Loading

brycekahle commented Nov 12, 2024

dd-devflow bot commented Nov 12, 2024 •

edited

Loading

brycekahle commented Nov 12, 2024

dd-devflow bot commented Nov 12, 2024 •

edited

Loading

Prevents panics during ELF parsing from crashing process #30725

Prevents panics during ELF parsing from crashing process #30725

Conversation

brycekahle commented Nov 4, 2024 • edited Loading

What does this PR do?

Motivation

Describe how to test/QA your changes

Possible Drawbacks / Trade-offs

Additional Notes

paulcacheux left a comment

Choose a reason for hiding this comment

grantseltzer left a comment

Choose a reason for hiding this comment

val06 Nov 5, 2024

Choose a reason for hiding this comment

brycekahle Nov 5, 2024

Choose a reason for hiding this comment

val06 Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

brycekahle Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

brycekahle Nov 7, 2024

Choose a reason for hiding this comment

cit-pr-commenter bot commented Nov 5, 2024 • edited Loading

Go Package Import Differences

agent-platform-auto-pr bot commented Nov 5, 2024 • edited Loading

Test changes on VM

cit-pr-commenter bot commented Nov 5, 2024 • edited Loading

Regression Detector

Regression Detector Results

Optimization Goals: ✅ No significant changes detected

Fine details of change detection per experiment

Bounds Checks: ❌ Failed

Explanation

Kaderinho left a comment • edited Loading

Choose a reason for hiding this comment

brycekahle commented Nov 7, 2024

brycekahle commented Nov 12, 2024

dd-devflow bot commented Nov 12, 2024 • edited Loading

Devflow running: /merge

brycekahle commented Nov 12, 2024

dd-devflow bot commented Nov 12, 2024 • edited Loading

Devflow running: /merge

What to do next?

brycekahle commented Nov 12, 2024

dd-devflow bot commented Nov 12, 2024 • edited Loading

Devflow running: /merge

brycekahle commented Nov 4, 2024 •

edited

Loading

val06 Nov 6, 2024 •

edited

Loading

brycekahle Nov 6, 2024 •

edited

Loading

cit-pr-commenter bot commented Nov 5, 2024 •

edited

Loading

agent-platform-auto-pr bot commented Nov 5, 2024 •

edited

Loading

cit-pr-commenter bot commented Nov 5, 2024 •

edited

Loading

Kaderinho left a comment •

edited

Loading

dd-devflow bot commented Nov 12, 2024 •

edited

Loading

Devflow running: `/merge`

dd-devflow bot commented Nov 12, 2024 •

edited

Loading

Devflow running: `/merge`

dd-devflow bot commented Nov 12, 2024 •

edited

Loading

Devflow running: `/merge`