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

fix(rules/Set Setuid or Setgid bit): use chmod syscalls instead of chmod command #765

Merged
merged 2 commits into from Aug 16, 2019

Conversation

@fntlnz
Copy link
Member

fntlnz commented Aug 13, 2019

Signed-off-by: Lorenzo Fontana lo@linux.com

What type of PR is this?

/kind bug

/kind rule-update

Any specific area of the project related to this PR?

/area rules

What this PR does / why we need it:

The "Set Setuid or Setgid bit" rule is using a check on the invocation of the chmod command in order to check if a suid or sgid flag is applied to a file.
That method is ineffective because anyone using the raw syscall or glibc can bypass that.

For example:

#include <sys/stat.h>
#include <fcntl.h>

int main ()
{
  fchmodat(AT_FDCWD, "/tmp/yo", S_ISUID, 0);
}

This PR uses the newly added fchmod, chmod, fchmodat syscalls (to sysdig)
in order to avoid that.
Which issue(s) this PR fixes:

NONE

Special notes for your reviewer:

Only works if sysdig PR draios/sysdig#1472 is merged. It adds support to the chmod syscalls we are using here.
Does this PR introduce a user-facing change?:

rule: syscalls are used to detect suid and sgid
@fntlnz

This comment has been minimized.

Copy link
Member Author

fntlnz commented Aug 13, 2019

@Kaizhe I noticed that the previous rule used a macro consider_all_chmods to disable it. I added the same macro I'm just unsure if we still need it. Can you confirm why we need it? Thanks

@fntlnz fntlnz force-pushed the feat/suid-syscalls branch from e9e7cf2 to 2b44bca Aug 13, 2019
@fntlnz

This comment has been minimized.

Copy link
Member Author

fntlnz commented Aug 13, 2019

/assign @Kaizhe
/cc @Kaizhe

@fntlnz fntlnz requested a review from Kaizhe Aug 13, 2019
@mstemm

This comment has been minimized.

Copy link
Contributor

mstemm commented Aug 13, 2019

I would also increment the required engine version, as you depend on the changes in draios/sysdig#1472 to be effective. Once the sysdig PR is merged you’ll have to increment the falco engine and checksum anyway.

@Kaizhe

This comment has been minimized.

Copy link
Contributor

Kaizhe commented Aug 13, 2019

@fntlnz good changes, couple requests here:

  1. Post the rule triggered output here to make sure the rule still works as expected.
  2. I would like to to turn it on, so replace never_true to always_true
  3. Add a customized macro user_known_chmod_applications
@poiana poiana added size/S and removed size/XS labels Aug 14, 2019
fntlnz added 2 commits Aug 13, 2019
…mod command

Signed-off-by: Lorenzo Fontana <lo@linux.com>
Signed-off-by: Lorenzo Fontana <lo@linux.com>
@fntlnz fntlnz force-pushed the feat/suid-syscalls branch from e0e47f4 to d586ec5 Aug 14, 2019
@fntlnz

This comment has been minimized.

Copy link
Member Author

fntlnz commented Aug 14, 2019

Here is the outputs @Kaizhe

Rule output examples

chmod

Syscall

chmod("/tmp/test", 04000)

Output

19:30:43.073551187: Notice Setuid or setgid bit is set via chmod (fd=<NA> filename=/tmp/test mode=S_ISUID user=ubuntu command=a.out container_id=host container_name=host image=<NA>:<NA>)

fchmodat

Syscall

fchmodat(AT_FDCWD, "/tmp/ciao", 04000)

Output

19:31:04.851600216: Notice Setuid or setgid bit is set via chmod (fd=<NA> filename=/tmp/ciao mode=S_ISUID user=ubuntu command=a.out container_id=host container_name=host image=<NA>:<NA>)

fchmod

openat(AT_FDCWD, "/tmp/test", O_RDWR)   = 3
fchmod(3, 04000)

Syscall

19:31:52.570751037: Notice Setuid or setgid bit is set via chmod (fd=<f>/tmp/test filename=<NA> mode=S_ISUID user=ubuntu command=a.out container_id=host container_name=host image=<NA>:<NA>)
@Kaizhe

This comment has been minimized.

Copy link
Contributor

Kaizhe commented Aug 14, 2019

/lgtm

@poiana poiana added the lgtm label Aug 14, 2019
@poiana

This comment has been minimized.

Copy link

poiana commented Aug 14, 2019

LGTM label has been added.

Git tree hash: ed88949aef039cf5a7a9463e3af82a935a69718a

@poiana poiana added the approved label Aug 14, 2019
@poiana

This comment has been minimized.

Copy link

poiana commented Aug 15, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Kaizhe, leodido

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fntlnz fntlnz merged commit e229cec into dev Aug 16, 2019
6 of 7 checks passed
6 of 7 checks passed
tide Not mergeable. Job Travis CI - Pull Request has not succeeded.
Build Build Successful
Details
Run tests All tests passed
Details
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/jenkins/branch This commit looks good
Details
dco All commits have Signed-off-by
Details
@poiana poiana deleted the feat/suid-syscalls branch Aug 16, 2019
@fntlnz fntlnz added this to the 0.18.0 milestone Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.