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

Implement requirement.json for injection #4019

Merged
merged 3 commits into from
Oct 30, 2024
Merged

Implement requirement.json for injection #4019

merged 3 commits into from
Oct 30, 2024

Conversation

TonyCTHsu
Copy link
Contributor

@TonyCTHsu TonyCTHsu commented Oct 22, 2024

Change log entry

None

What does this PR do?

Add requirement.json for injection.

Motivation:

Prevent injection when:

  • we know system dependencies (libc, arch) aren't satisfied
  • we know there's no point in injecting into a program (not useful, not effective, blocked by security policy)
  • we know injecting into a program would be detrimental (breaking it due to incompatibility or security policy)

Additional Notes:

JIRA

How to test the change?

Should be tested by CI in the added JSON test cases.

@TonyCTHsu TonyCTHsu force-pushed the tonycthsu/denylist branch 7 times, most recently from 4b742d0 to c75462f Compare October 22, 2024 15:54
@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.73%. Comparing base (1ec3900) to head (357ec4c).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4019   +/-   ##
=======================================
  Coverage   97.73%   97.73%           
=======================================
  Files        1338     1338           
  Lines       80246    80245    -1     
  Branches     4014     4014           
=======================================
  Hits        78430    78430           
+ Misses       1816     1815    -1     

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

@pr-commenter
Copy link

pr-commenter bot commented Oct 22, 2024

Benchmarks

Benchmark execution time: 2024-10-22 16:33:08

Comparing candidate commit c75462f in PR branch tonycthsu/denylist with baseline commit 38f6cc1 in branch master.

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

scenario:profiler - hold / resume

  • 🟥 throughput [-256863.487op/s; -255326.192op/s] or [-12.750%; -12.673%]

scenario:tracing - Propagation - Datadog

  • 🟥 throughput [-2823.523op/s; -2715.521op/s] or [-8.504%; -8.179%]

{
"arch": "x86",
"supported": true,
"min": "2.28"
Copy link
Member

Choose a reason for hiding this comment

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

2.28 is quite high for x86, do you know what symbol/dependency requires it?

running nm -D library_file | grep U will give you that answer

Copy link
Member

@lloeki lloeki Oct 30, 2024

Choose a reason for hiding this comment

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

The pipeline is using registry.ddbuild.io/images/mirror/buildpack-deps:buster as a base:

FROM registry.ddbuild.io/images/mirror/buildpack-deps:buster

With which we get a glibc 2.28:

$ ldd --version
ldd (Debian GLIBC 2.28-10+deb10u2) 2.28

So gems with native extensions are built against that.

The version requirement comes from there.

This results in the following max version referenced (== min version required):

aarch64:

tmp/arm64/3.2.0/extensions/aarch64-linux/3.2.0-static/datadog-2.4.0/datadog_profiling_loader.3.2.2_aarch64-linux.so: GLIBC_2.17
tmp/arm64/3.2.0/extensions/aarch64-linux/3.2.0-static/datadog-2.4.0/libdatadog_api.3.2_aarch64-linux.so: GLIBC_2.17
tmp/arm64/3.2.0/extensions/aarch64-linux/3.2.0-static/ffi-1.16.3/ffi_c.so: GLIBC_2.27
tmp/arm64/3.2.0/extensions/aarch64-linux/3.2.0-static/msgpack-1.7.3/msgpack/msgpack.so: GLIBC_2.17
tmp/arm64/3.2.0/gems/datadog-2.4.0/lib/datadog_profiling_loader.3.2.2_aarch64-linux.so: GLIBC_2.17
tmp/arm64/3.2.0/gems/datadog-2.4.0/lib/libdatadog_api.3.2_aarch64-linux.so: GLIBC_2.17
tmp/arm64/3.2.0/gems/ffi-1.16.3/lib/ffi_c.so: GLIBC_2.27
tmp/arm64/3.2.0/gems/libdatadog-13.1.0.1.0-aarch64-linux/vendor/libdatadog-13.1.0/aarch64-linux-musl/libdatadog-aarch64-alpine-linux-musl/lib/libdatadog_profiling.so: GLIBC_2.0
tmp/arm64/3.2.0/gems/libdatadog-13.1.0.1.0-aarch64-linux/vendor/libdatadog-13.1.0/aarch64-linux/libdatadog-aarch64-unknown-linux-gnu/lib/libdatadog_profiling.so: GLIBC_2.17
tmp/arm64/3.2.0/gems/libddwaf-1.14.0.0.0-aarch64-linux/vendor/libddwaf/libddwaf-1.14.0-linux-aarch64/lib/libddwaf.so: 
tmp/arm64/3.2.0/gems/msgpack-1.7.3/lib/msgpack/msgpack.so: GLIBC_2.17

x86_64:

tmp/x86_64/3.2.0/extensions/x86_64-linux/3.2.0-static/datadog-2.4.0/datadog_profiling_loader.3.2.2_x86_64-linux.so: GLIBC_2.2.5
tmp/x86_64/3.2.0/extensions/x86_64-linux/3.2.0-static/datadog-2.4.0/libdatadog_api.3.2_x86_64-linux.so: GLIBC_2.2.5
tmp/x86_64/3.2.0/extensions/x86_64-linux/3.2.0-static/ffi-1.16.3/ffi_c.so: GLIBC_2.27
tmp/x86_64/3.2.0/extensions/x86_64-linux/3.2.0-static/msgpack-1.7.3/msgpack/msgpack.so: GLIBC_2.14
tmp/x86_64/3.2.0/gems/datadog-2.4.0/lib/datadog_profiling_loader.3.2.2_x86_64-linux.so: GLIBC_2.2.5
tmp/x86_64/3.2.0/gems/datadog-2.4.0/lib/libdatadog_api.3.2_x86_64-linux.so: GLIBC_2.2.5
tmp/x86_64/3.2.0/gems/ffi-1.16.3/lib/ffi_c.so: GLIBC_2.27
tmp/x86_64/3.2.0/gems/libdatadog-13.1.0.1.0-x86_64-linux/vendor/libdatadog-13.1.0/x86_64-linux-musl/libdatadog-x86_64-alpine-linux-musl/lib/libdatadog_profiling.so: 
tmp/x86_64/3.2.0/gems/libdatadog-13.1.0.1.0-x86_64-linux/vendor/libdatadog-13.1.0/x86_64-linux/libdatadog-x86_64-unknown-linux-gnu/lib/libdatadog_profiling.so: GLIBC_2.16
tmp/x86_64/3.2.0/gems/libddwaf-1.14.0.0.0-x86_64-linux/vendor/libddwaf/libddwaf-1.14.0-linux-x86_64/lib/libddwaf.so: 
tmp/x86_64/3.2.0/gems/msgpack-1.7.3/lib/msgpack/msgpack.so: GLIBC_2.14

ffi is the one using GLIBC_2.27. So we can lower it to 2.27 but until we move to building against a lower glibc (which comes with its own set of issues) we're stuck with that. FYI using manylinux is under consideration.

Copy link
Member

Choose a reason for hiding this comment

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

if it helps you can pick agent build images or the ones used by the injector:

Given that you download the source codes, won't building ffi on those machines improve your coverage? Or are there specific recent symbols that are required

Copy link
Member

@lloeki lloeki Oct 30, 2024

Choose a reason for hiding this comment

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

I think memfd_create needs glibc 2.27.

It looks like libffi has fallback mechanisms if it is absent (see Memory Usage), but it is an important security feature.

Note: we're looking into removing the ffi dependency entirely mid-term.

@lloeki
Copy link
Member

lloeki commented Oct 30, 2024

Tests passed on this minimal one.

Screenshot 2024-10-30 at 18 35 12

@lloeki lloeki marked this pull request as ready for review October 30, 2024 17:35
@lloeki lloeki requested review from a team as code owners October 30, 2024 17:35
@lloeki lloeki force-pushed the tonycthsu/denylist branch from 51020e3 to 357ec4c Compare October 30, 2024 17:36
@lloeki
Copy link
Member

lloeki commented Oct 30, 2024

  • Rebased.
  • Undrafted for review.
  • Once tests pass we can merge this, with further improvements down the road, notably covering for INPLAT-103

@lloeki lloeki merged commit d361376 into master Oct 30, 2024
267 of 270 checks passed
@lloeki lloeki deleted the tonycthsu/denylist branch October 30, 2024 18:01
@github-actions github-actions bot added this to the 2.5.0 milestone Oct 30, 2024
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.

4 participants