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

cmd/snap-seccomp: add full complement of ptrace constants #6120

Merged
merged 2 commits into from Nov 9, 2018

Conversation

zyga
Copy link
Collaborator

@zyga zyga commented Nov 8, 2018

The snap-seccomp executable needs to understand each constant
it is given. When encountering unexpected constants it just returns
an error.

Since patch 093b366
"interfaces/browser-support, cmd/snap-seccomp: Allow read-only ptrace,
for the Breakpad crash reporter." snap-seccomp will choke on the
profiles generated for the browser-support interface.

Since patch 353aa70 "cmd/snap-seccomp:
only look for PTRACE_GETFPX?REGS where available" snap-seccomp contains
additional logic that handles various ptrace constants with
architecture-specific build tags. I'm not sure why this was done because
all of the constants are available in the C header file sys/ptrace.h

Initially I was thinking about making the parser silently ignore certain
PTRACE values but I have since reconsidered to just add the full since
they are just defined and are available at build time. After all, it
doesn't hurt if they don't work on a given architecture (at a kernel
level). We just want to white-list them in case they do work.

Fixes: https://bugs.launchpad.net/snapd/+bug/1802124
Signed-off-by: Zygmunt Krynicki me@zygoon.pl

The snap-seccomp executable needs to understand each constant
it is given. When encountering unexpected constants it just returns
an error.

Since patch 093b366
"interfaces/browser-support, cmd/snap-seccomp: Allow read-only ptrace,
for the Breakpad crash reporter." snap-seccomp will choke on the
profiles generated for the browser-support interface.

Since patch 353aa70 "cmd/snap-seccomp:
only look for PTRACE_GETFPX?REGS where available" snap-seccomp contains
additional logic that handles various ptrace constants with
architecture-specific build tags. I'm not sure why this was done because
all of the constants are available in the C header file sys/ptrace.h

Initially I was thinking about making the parser silently ignore certain
PTRACE values but I have since reconsidered to just add the full since
they are just defined and are available at build time. After all, it
doesn't hurt if they don't work on a given architecture (at a kernel
level). We just want to white-list them in case they do work.

Fixes: https://bugs.launchpad.net/snapd/+bug/1802124
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
@zyga zyga requested a review from jdstrand November 8, 2018 21:12
Copy link

@jdstrand jdstrand left a comment

Choose a reason for hiding this comment

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

I much prefer this. Thank you. Assuming the tests pass, LGTM.

@zyga
Copy link
Collaborator Author

zyga commented Nov 8, 2018

@jdstrand tests failed here but I fixed that, will push a fix as soon as one more run is finished.

Some ptrace requests are not available on all architectures. Since the
values for those requests just follow a simple sequential enumeration
it should be safe to define the missing values. The kernel doesn't
support them but we don't care about that because we just want to allow
them if they are specified by the seccomp profile.

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Copy link

@jdstrand jdstrand left a comment

Choose a reason for hiding this comment

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

Assuming the tests pass, still LGTM as an approach. This is the same as we do for others that aren't defined. I verified the kernel uses these consistently across architectures, so +1.

@bboozzoo bboozzoo merged commit ba29c10 into snapcore:master Nov 9, 2018
@chipaca
Copy link
Contributor

chipaca commented Nov 9, 2018

thank you for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants