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

Seccomp #532

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Seccomp #532

wants to merge 1 commit into from

Conversation

outscale-hmi
Copy link
Contributor

No description provided.

Copy link
Contributor

@outscale-fne outscale-fne left a comment

Choose a reason for hiding this comment

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

For me I built it correctly and didn't see anything wrong... Look's working!

src/seccomp.c Outdated
struct sigaction act;
int check = 0;
/* Set up seccomp filter */
if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)){
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems this is also call in pg_install_filter, why it it call here too ?

src/seccomp.c Outdated
{
int old_errno = errno;

printf("Attempted banned syscall number [%d] and sig [%d]\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

printf write on stdout, you should write on stderr, so fprintf("Catch sig [%d] when attemption to catch syscall: [%d]\n", sig, si->si_syscall)

Copy link
Contributor

Choose a reason for hiding this comment

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

also, I don't see you use errno ?


KILL_PROCESS,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be configurable, so take a bitmask in function parameter, and use that parameter here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is already defined in seccomp-bpf

define SECCOMP_RET_TRAP 0x00030000U /* disallow and force a SIGSYS */

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, but filter contain TRAP_PROCESS
Which doesn't work, if you want packetgraph to KILL process.
You can't put KILL_PROCESS into filter either, because that would mean packetgraph doesn't work if you want it to print process.
So you need to have a way to chose at runtime between KILL or TRAP.

src/seccomp.c Outdated
return -1;
if (prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog))
if (prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog) == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

that bug me, so if prctl success, you return -1 (which mean fail ?)

src/seccomp.c Outdated

int pg_init_seccomp(void)
int init_seccomp_filters(void)
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH, I don't understand the point of this sub function.
If you really want it please make it static.
int init_seccomp_filters(void)

src/seccomp.c Outdated
@@ -81,17 +84,59 @@ int pg_init_seccomp(void)
ALLOW_SYSCALL(gettimeofday),
ALLOW_SYSCALL(stat),
ALLOW_SYSCALL(clock_gettime),
ALLOW_SYSCALL(mprotect),
Copy link
Contributor

Choose a reason for hiding this comment

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

humm, This one is not acceptable.
I understand you need it to debug, but this call is far too dangerous.

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

Successfully merging this pull request may close these issues.

3 participants