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

test: include strace openat test #46150

Merged
merged 1 commit into from
Feb 23, 2023

Conversation

RafaelGSS
Copy link
Member

Signed-off-by: RafaelGSS [email protected]

Opening it for early feedback. We need to find a way to do it cross-platform, either with other files (openat-linux-syscall, openat-osx-syscall, openat-windows-syscall) or with some magic cross-platform tool.

The idea is to address nodejs/security-wg#827. The CVE-2022-32222 is an example of the purpose of this test.

cc: @nodejs/security-wg @mhdawson

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jan 9, 2023
test/parallel/test-strace-openat-openssl.js Outdated Show resolved Hide resolved

const file = line.match(/"(.*?)"/)[1];
// skip .so reading attempt
if (file.match(/.+\.so(\.?)/) !== null) {
Copy link
Member

Choose a reason for hiding this comment

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

It might actually not be that bad to assert these as well – if we added a new .so dependency on Linux, we might want to know about that?

Copy link
Member Author

Choose a reason for hiding this comment

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

In my case, it's reading the .so from shared locations, for instance: "/home/rafaelgss/.gvm/pkgsets/go1.15/global/overlay/lib/libpthread.so.0". How would we handle it?

Copy link
Member

Choose a reason for hiding this comment

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

You could assert just the filename, ignoring the rest of the path, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure. That CVE mentioned in the PR description is really about it. Reading a file/library from an unexpected path.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, please don’t consider this a blocking comment. This would test something different, that’s true, but it does seem valuable to test it regardless.

test/parallel/test-strace-openat-openssl.js Outdated Show resolved Hide resolved
t.js Outdated Show resolved Hide resolved
@bnoordhuis
Copy link
Member

AssertionError [ERR_ASSERTION]: /proc/self/cmdline is not in the list of allowed openat calls

I don't think you're going to be able to avoid this. Different versions of glibc open different files, to say nothing of other libcs.

If you want to go fancy, you could wait until right before the child process does require('crypto') and attach with strace -p <pid>; will involve syncing over an IPC channel.

Another issue you or someone else inevitably is going to run into is that strace won't work on locked down systems (sysctl kernel.yama.ptrace_scope > 1)

@RafaelGSS
Copy link
Member Author

I don't think you're going to be able to avoid this. Different versions of glibc open different files, to say nothing of other libcs.

What about skipping /proc/* fd?

Another issue you or someone else inevitably is going to run into is that strace won't work on locked down systems (sysctl kernel.yama.ptrace_scope > 1)

Well, we can also run it only on CI, so I assume it will work all the time.

@bnoordhuis
Copy link
Member

What about skipping /proc/* fd?

That won't be enough. glibc can for any number of reasons decide to open files in /etc, /proc, /sys, /lib, etc.

@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 21, 2023
@RafaelGSS
Copy link
Member Author

Requesting CI just to see how many use cases I would need to cover. Another viable option would be just logging it before publishing a release, however, it would require an extra step for a releaser, which is, definitely something I don't want.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 21, 2023
@nodejs-github-bot
Copy link
Collaborator

@RafaelGSS
Copy link
Member Author

What about skipping /proc/* fd?

That won't be enough. glibc can for any number of reasons decide to open files in /etc, /proc, /sys, /lib, etc.

Wouldn't it be consistent in the CI machine at least? Well, I think we won't have a better way to pursue this work, right?

@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 30, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2023
@nodejs-github-bot
Copy link
Collaborator

@RafaelGSS RafaelGSS force-pushed the add-strace-openat-test branch 2 times, most recently from 8c82218 to 534a61b Compare February 1, 2023 12:37
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 6, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 6, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@RafaelGSS
Copy link
Member Author

@bnoordhuis It seems to be fixed (considering we are skipping the test in a few situations), but based on your comment looks like we should also skip it when it's not running in the CI.

Do you think it will be flaky somehow?

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 22, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 22, 2023
@nodejs-github-bot
Copy link
Collaborator

@RafaelGSS RafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 23, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 23, 2023
@nodejs-github-bot nodejs-github-bot merged commit 86362b7 into nodejs:main Feb 23, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 86362b7

targos pushed a commit that referenced this pull request Mar 13, 2023
Signed-off-by: RafaelGSS <[email protected]>
PR-URL: #46150
Reviewed-By: Michael Dawson <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
Signed-off-by: RafaelGSS <[email protected]>
PR-URL: #46150
Reviewed-By: Michael Dawson <[email protected]>
codebytere added a commit to electron/electron that referenced this pull request Apr 18, 2023
codebytere added a commit to electron/electron that referenced this pull request Apr 18, 2023
* chore: bump node in DEPS to v18.16.0

* build,test: add proper support for IBM i

nodejs/node#46739

* lib: enforce use of trailing commas

nodejs/node#46881

* src: add initial support for single executable applications

nodejs/node#45038

* lib: do not crash using workers with disabled shared array buffers

nodejs/node#41023

* src: remove shadowed variable in OptionsParser::Parse

nodejs/node#46672

* src: allow embedder control of code generation policy

nodejs/node#46368

* src: allow optional Isolate termination in node::Stop()

nodejs/node#46583

* lib: fix BroadcastChannel initialization location

nodejs/node#46864

* chore: fixup patch indices

* chore: sync filenames.json

* fix: add simdutf dep to src/inspector BUILD.gn

- nodejs/node#46471
- nodejs/node#46472

* deps: replace url parser with Ada

nodejs/node#46410

* tls: support automatic DHE

nodejs/node#46978

* fixup! src: add initial support for single executable applications

* http: unify header treatment

nodejs/node#46528

* fix: libc++ buffer overflow in string_view ctor

nodejs/node#46410

* test: include strace openat test

nodejs/node#46150

* fixup! fixup! src: add initial support for single executable applications

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants