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.

Sign up for GitHub

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

Jump to bottom

clang: check header ownership #4928

Merged
merged 1 commit into from
Mar 6, 2024
Merged

clang: check header ownership #4928

merged 1 commit into from
Mar 6, 2024

Conversation

brendangregg
Copy link
Member

Example testing with a brendan-owned /tmp/kheaders file (note the "ERROR:" message):

~/bcc/build$ sudo /usr/share/bcc/tools/biosnoop
ERROR: header file ownership unexpected: /tmp/kheaders-5.15.47-internal
<built-in>:1:10: fatal error: './include/linux/kconfig.h' file not found
#include "./include/linux/kconfig.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
Traceback (most recent call last):
  File "/usr/share/bcc/tools/biosnoop", line 335, in <module>
    b = BPF(text=bpf_text)
  File "/usr/lib/python3/dist-packages/bcc-0.1.5+6cd27218-py3.10.egg/bcc/__init__.py", line 479, in __init__
Exception: Failed to compile BPF module <text>
~/bcc/build$ ls -lhd /tmp/kheaders-5.15.47-internal
drwxrwxr-x 2 brendan dev 4.0K Mar  6 02:50 /tmp/kheaders-5.15.47-internal

No error when chown'd back to root.

jordalgo
jordalgo reviewed Mar 6, 2024
View reviewed changes
src/cc/frontends/clang/kbuild_helper.cc
int ret;
if ((ret = stat(f, &buffer)) == 0) {
if (buffer.st_uid != uid) {
std::cout << "ERROR: header file ownership unexpected: " << std::string(f) << "\n";
Copy link

Choose a reason for hiding this comment

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

Nit: is it useful to also print out the expected uid and the one in the file: st_uid?

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 think this is close enough: says this filename has this type of error. End-user should likely see this and run ls -l. There's related codepaths that can error with no such hint, which is pretty hard to debug, which is why I put a big clue in. :-)

@jordalgo
Copy link

jordalgo commented Mar 6, 2024

Do we need to check the ld cache ownership too? https://github.com/iovisor/bcc/blob/master/src/cc/bcc_proc.c#L400

@brendangregg
Copy link
Member Author

Thanks, although the ld cache is only in /etc, right? /etc should already be locked down, so we wouldn't need to check. If unprivileged users can create files in /etc, I imagine there's a ton of other issues.

@jordalgo jordalgo mentioned this pull request Mar 6, 2024
Merged
3 tasks
@yonghong-song yonghong-song merged commit 008ea09 into iovisor:master Mar 6, 2024
12 checks passed
@jeromemarchand
Copy link
Contributor

I'm afraid this is not fixed yet. The example above fails only because it does not contain the include/linux/kconfig.h file.
If a user builds a valid kheaders tree, with possibly a few chosen alteration, we're going to show an error mesage, but we're still going to use the compromised headers.

$ sudo /usr/share/bcc/tools/execsnoop 
ERROR: header file ownership unexpected: /tmp/kheaders-6.8.0-63.fc41.x86_64
PCOMM            PID     PPID    RET ARGS
ls               1690    1426      0 /usr/bin/ls --color=auto

jeromemarchand added a commit to jeromemarchand/bcc that referenced this pull request May 6, 2024
@jeromemarchand
clang: fail when the kheaders ownership is wrong (iovisor#4928)
8e267b0 
file_exists_and_ownedby() returns -1 when the file exists but its
ownership is unexpected, which is very misleading since anything non
zero is interpreted as true and a function with such a name is
expected to return a boolean. So currently all this does, is write a
warning message, and continues as if nothing is wrong.

Make file_exists_and_ownedby() returns false when the ownership is
wrong and have get_proc_kheaders() fails when this happen. Also have
all the *exists* functions return bool to avoid such issues in the
future.

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
jeromemarchand added a commit to jeromemarchand/bcc that referenced this pull request May 6, 2024
@jeromemarchand
clang: fail when the kheaders ownership is wrong (iovisor#4928)
bf3c339 
file_exists_and_ownedby() returns -1 when the file exists but its
ownership is unexpected, which is very misleading since anything non
zero is interpreted as true and a function with such a name is
expected to return a boolean. So currently all this does, is write a
warning message, and continues as if nothing is wrong.

Make file_exists_and_ownedby() returns false when the ownership is
wrong and have get_proc_kheaders() fails when this happen. Also have
all the *exists* functions return bool to avoid such issues in the
future.

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
yonghong-song pushed a commit that referenced this pull request May 8, 2024
@jeromemarchand
clang: fail when the kheaders ownership is wrong (#4928)
7bf2459 
file_exists_and_ownedby() returns -1 when the file exists but its
ownership is unexpected, which is very misleading since anything non
zero is interpreted as true and a function with such a name is
expected to return a boolean. So currently all this does, is write a
warning message, and continues as if nothing is wrong.

Make file_exists_and_ownedby() returns false when the ownership is
wrong and have get_proc_kheaders() fails when this happen. Also have
all the *exists* functions return bool to avoid such issues in the
future.

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
jeromemarchand added a commit to jeromemarchand/bcc that referenced this pull request May 16, 2024
@jeromemarchand
clang: fail when the kheaders ownership is wrong (iovisor#4928)
135c7f0 
file_exists_and_ownedby() returns -1 when the file exists but its
ownership is unexpected, which is very misleading since anything non
zero is interpreted as true and a function with such a name is
expected to return a boolean. So currently all this does, is write a
warning message, and continues as if nothing is wrong.

Make file_exists_and_ownedby() returns false when the ownership is
wrong and have get_proc_kheaders() fails when this happen. Also have
all the *exists* functions return bool to avoid such issues in the
future.

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
yonghong-song pushed a commit that referenced this pull request May 17, 2024
@jeromemarchand
clang: fail when the kheaders ownership is wrong (#4928) (#4985)
5bc97bb 
file_exists_and_ownedby() returns -1 when the file exists but its
ownership is unexpected, which is very misleading since anything non
zero is interpreted as true and a function with such a name is
expected to return a boolean. So currently all this does, is write a
warning message, and continues as if nothing is wrong.

Make file_exists_and_ownedby() returns false when the ownership is
wrong and have get_proc_kheaders() fails when this happen. Also have
all the *exists* functions return bool to avoid such issues in the
future.

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
dkruces pushed a commit to dkruces/bcc that referenced this pull request Nov 28, 2024
@brendangregg @dkruces
clang: check header ownership (iovisor#4928)
565b244 
Example testing with a brendan-owned /tmp/kheaders file (note the "ERROR:" message):

~/bcc/build$ sudo /usr/share/bcc/tools/biosnoop
ERROR: header file ownership unexpected: /tmp/kheaders-5.15.47-internal
<built-in>:1:10: fatal error: './include/linux/kconfig.h' file not found
#include "./include/linux/kconfig.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
Traceback (most recent call last):
  File "/usr/share/bcc/tools/biosnoop", line 335, in <module>
    b = BPF(text=bpf_text)
  File "/usr/lib/python3/dist-packages/bcc-0.1.5+6cd27218-py3.10.egg/bcc/__init__.py", line 479, in __init__
Exception: Failed to compile BPF module <text>
~/bcc/build$ ls -lhd /tmp/kheaders-5.15.47-internal
drwxrwxr-x 2 brendan dev 4.0K Mar  6 02:50 /tmp/kheaders-5.15.47-internal

No error when chown'd back to root.
dkruces pushed a commit to dkruces/bcc that referenced this pull request Nov 28, 2024
@jeromemarchand @dkruces
clang: fail when the kheaders ownership is wrong (iovisor#4928) (iovi…
cc9cc43 
…sor#4985)

file_exists_and_ownedby() returns -1 when the file exists but its
ownership is unexpected, which is very misleading since anything non
zero is interpreted as true and a function with such a name is
expected to return a boolean. So currently all this does, is write a
warning message, and continues as if nothing is wrong.

Make file_exists_and_ownedby() returns false when the ownership is
wrong and have get_proc_kheaders() fails when this happen. Also have
all the *exists* functions return bool to avoid such issues in the
future.

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jordalgo jordalgo jordalgo approved these changes

@drzaeus77 drzaeus77 Awaiting requested review from drzaeus77 drzaeus77 is a code owner

@yonghong-song yonghong-song Awaiting requested review from yonghong-song yonghong-song is a code owner

@4ast 4ast Awaiting requested review from 4ast 4ast is a code owner

@davemarchevsky davemarchevsky Awaiting requested review from davemarchevsky davemarchevsky is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@brendangregg @jordalgo @jeromemarchand @yonghong-song