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

ls -Z doesn't seem to print selinux context #158

Closed
fawazahmed0 opened this issue Dec 24, 2019 · 20 comments
Closed

ls -Z doesn't seem to print selinux context #158

fawazahmed0 opened this issue Dec 24, 2019 · 20 comments

Comments

@fawazahmed0
Copy link

fawazahmed0 commented Dec 24, 2019

So, I have tried this in my 64 bit linux vm and also in arm64 android, the ls -Z doesn't show the selinux content
Screenshot (48)
I used the binaries from here:
http://landley.net/toybox/bin/

Also, the binaries provided in the android OS seems to print selinux without any issue

@landley
Copy link
Owner

landley commented Dec 24, 2019

Those are defconfig binaries. CONFIG_SELINUX is not enabled in the .config file produced by "make defconfig", you have to enable it in menuconfig. The android build uses a .config with CONFIG_TOYBOX_SELINUX=y

If you're arguing that CONFIG_SELINUX should be enabled in the default configuration... I suppose there's an argument for that these days. I wanted to minimize build dependencies, but I could add a compile time probe...

@fawazahmed0
Copy link
Author

@landley I can compile the binaries myself with flag set, but it will be better to have it set in defconfig, so that Android users can use your static binaries with selinux support, without the need to compile it themselves.

@landley
Copy link
Owner

landley commented Dec 25, 2019

Oh the binaries I build still wouldn't have support even if I added the probe, because:

Compile toybox.....In file included from ./toys.h:69:0, from lib/env.c:3:
./lib/lsm.h:7:29: fatal error: selinux/selinux.h: No such file or directory
#include <selinux/selinux.h>

I have to install an extra package to have support, which means when I'm cross compiling to a dozen architectures I'd have to build that external library for each architecture and put it where that cross compiler could find it. The selinux developers decided it should be a gratuitous external dependency, not part libc and not in the base OS. Therefore, toybox doesn't depend on it by default.

If there's an easy way to call the selinux plumbing directly, I'd happily add it to lib/portability.c, but I'm not cross compiling an external library for every target architecture I make binaries for as part of my release process.

@fawazahmed0
Copy link
Author

fawazahmed0 commented Dec 27, 2019

@landley Thanks for looking into it, I guess it might require external library for that, I will compile the selinux enabled version and probably keep at github repo, that will be helpful to others.

Just a little help from you is needed, when I tried compiling selinux enabled version, I got the same error as you got, installing libselinux1-dev package (in ubuntu) seems to solve that issue for me and I can successfully compile a dynamic linked version.
But I wanted a static version, and compiling that seems to throw this error:
static_build_error.txt

I have android source code at my linux vm and can easily compile a static selinux version, but I would like to compile from your source as it is the latest one.
Any help would be much appreciated

@fawazahmed0
Copy link
Author

@landley ok I got the same error while trying to build static selinux busybox and adding this:
CONFIG_EXTRA_LDLIBS="pthread pcre"
seems to solve the problem
I am not sure if toybox have something similar

@fawazahmed0
Copy link
Author

fawazahmed0 commented Dec 27, 2019

@landley ok I added "pcre pthread sepol" in scripts/make.sh at here:
for i in util crypt m resolv pcre pthread selinux sepol smack attr crypto z log iconv
and that seems to compile static selinux binary with no issue,
Maybe you should add these three libraries in make.sh and also consider adding "pthread pcre" in busybox build process
I have successfully compiled static selinux enabled toybox binary
Thanks

@landley
Copy link
Owner

landley commented Dec 28, 2019

Or just LDFLAGS="-lstatic -lpthread -lpcre" maybe?

@landley
Copy link
Owner

landley commented Dec 28, 2019

Ahem, --static not -lstatic

@fawazahmed0
Copy link
Author

fawazahmed0 commented Dec 28, 2019

Or just LDFLAGS="-static -lpthread -lpcre" maybe?

Thanks, this one is working, I tried this before, but that wasn't working, so thought there might be other way of doing it in toybox.

@fawazahmed0
Copy link
Author

Or just LDFLAGS="-static -lpthread -lpcre" maybe?

Thanks, this one is working, I tried this before, but that wasn't working, so thought there might be other way of doing it in toybox.

@landley Ignore what I wrote above, I didn't perform make clean, so that's why the above worked.
Adding libs here is what makes things work:
for i in util crypt m resolv pcre pthread selinux sepol smack attr crypto z log iconv

otherwise the libs are not picked during the compilation process

Also, I see default value for $CC and $HOSTCC as cc, I assume cc will usually point to gcc , so wouldn't it be better to directly use gcc.
I am asking this because I am using arm cross compiler (ubuntu package) and it doesn't have cc binary.
Anyways, I exported the $CC and $HOSTCC as gcc and thinks running fine now

@landley
Copy link
Owner

landley commented Dec 28, 2019

Ah right, the static link is position dependent. I'd have to move LDFLAGS to the end of the command line which doesn't work with the current code checking to see if I need to redo the library probe to regenerate generated/optlibs.dat. I'll throw that on the todo list, thanks.

clang/llvm is not gcc, android's built exclusively with llvm for years. (The gcc toolchain was deprecated and removed from the NDK and AOSP builds. Nobody wants to ship GPLv3 code, they work or sponsor work on replacements instead.)

@fawazahmed0
Copy link
Author

fawazahmed0 commented Dec 29, 2019

So, after compiling a selinux enabled toybox, still ls -Z doesn't print selinux, I will have to check how aosp builds toybox differently when I get some free time

@landley
Copy link
Owner

landley commented Dec 29, 2019

Alas, I added selinux support because Elliott asked me to, and have never had a local test environment for it. (I just built an selinux enabled ls and -Z puts a ? after every file I've examined on devuan 2.0. Whether it's the host ls or the toybox ls: same behavior.) I've pretty much been taking Elliott's word that this feature works, because I don't use it.

How would I set up an selinux test environment? (Can I just glue specific xattrs onto stuff without actually enabling selinux in the kernel?)

@fawazahmed0
Copy link
Author

fawazahmed0 commented Dec 30, 2019

yeah you can do that, don't actually have to enable selinux for that.(I tried chcon, but that doesn't seem to work for me)
This is how I did:
Just download,unzip and mount the persist.img file to an empty directory.
persist.zip
mkdir output
sudo mount -t ext4 -o loop,rw persist.img output
Go to output folder and copy any file you like (all files have selinux context attached to it, try ls -Z)
Note -a flag is important to preserve the selinux context while copying:
sudo cp -a filename pathtocopy
sudo umount output
Now
ls -Z filename
and you can see the selinux getting printed, you can use that file to test whether the ls -Z supports selinux context print or not.

I also compiled coreutils (ls -Z) at my end to make sure there is no issue at my vm in creating selinux enabled binaries and I can see compiled ls can print selinux context with no issues whatsoever.

And one more thing, I am pretty sure there is something else the android build system is doing on the top of your code and that is what adding selinux support (I used mma command to build toybox and this way android build system takes care of everything)

@landley
Copy link
Owner

landley commented Dec 30, 2019

I just did an ls -Z in there and yes, it showed something. And then I did an ls -Z with toybox and it didn't, and "ldd ls" isn't showing libselinux even though the link command line had it. Looks like i have some debugging to do, but I'm in the middle of 3 other things already at the moment.

Thanks for the test case. Added to the todo heap.

@enh-google
Copy link
Collaborator

ToT toybox ls -Z is working for me on Android, with the Android libselinux.

on the host with your repro case i see coreutils do:

lstat("camera", {st_mode=S_IFDIR|0775, st_size=4096, ...}) = 0
lgetxattr("camera", "security.selinux", "u:object_r:persist_camera_file:s"..., 255) = 34

but toybox does:

newfstatat(4, "camera", {st_mode=S_IFDIR|0775, st_size=4096, ...}, AT_SYMLINK_NOFOLLOW) = 0
openat(4, "camera", O_RDONLY|O_NOFOLLOW|O_PATH) = 5
fgetxattr(5, "security.selinux", 0x55ec2c1f7900, 255) = -1 EBADF (Bad file descriptor)
lgetxattr("/proc/self/fd/5", "security.selinux", 0x55ec2c1f7900, 255) = -1 EOPNOTSUPP (Operation not supported)

whereas on Android i'm seeing:

newfstatat(4, "product", {st_mode=S_IFDIR|0755, st_size=3488, ...}, AT_SYMLINK_NOFOLLOW) = 0
openat(4, "product", O_RDONLY|O_NOFOLLOW|O_PATH) = 5
fgetxattr(5, "security.selinux", 0x79dbe28000, 255) = -1 EBADF (Bad file descriptor)
fcntl(5, F_GETFL)                       = 0x208000 (flags O_RDONLY|O_NOFOLLOW|O_PATH)
getxattr("/proc/self/fd/5", "security.selinux", "u:object_r:system_file:s0", 255) = 26
close(5)                                = 0

looking at bionic's fgetxattr(), i see...

  // fd could be an O_PATH file descriptor, and the kernel
  // may not directly support fgetxattr() on such a file descriptor.
  // Use /proc/self/fd instead to emulate this support.

so i guess glibc (2.28) just assumes the kernel can fgetxattr() on an O_PATH fd, but linux 5.2 at least still can't.

@landley
Copy link
Owner

landley commented Jan 8, 2020

I have a tick where whenever possible I open a filehandle to things and then operate on the filehandle so you can't rename() a file out from under it between atomic operations. But in this case:

A) that strace shows it doing it wrong because it should be fstat() after the open so we know the stat and the getxattr() are at least on the same entry,

B) the two syscall version seems a lot simpler than the 4 syscall version with kernel workaround.

C) I dunno enough about selinux to say what means what hear, and it seems unlikely anything security critical is going to parse ls output?

I.E. maybe the way I wrote it is wrong and should change for better portability? I'm not a domain expert in selinux or xattrs. (I don't seem to have the lgetxattr and fgetxattr man pages installed on my system. Apparently these system calls are not supported by libc...?)

@enh-google
Copy link
Collaborator

your man pages are out of date? http://man7.org/linux/man-pages/man2/getxattr.2.html

(personally, i have a git clone of the upstream man7 project and have my MANPATH set to that.)

want me to send you a tested (on Android and Debian) patch that does what you're talking about?

@enh-google
Copy link
Collaborator

(patch sent to mailing list. tested on Android+bionic and Debian+glibc 2.28.)

@fawazahmed0
Copy link
Author

fawazahmed0 commented Jan 9, 2020

@landley I think these system calls are supported by libc, but using o_path in the function is not supported

These system calls (getxattr) have been available on Linux since kernel 2.4;
glibc support is provided since version 2.3.

Ref: http://man7.org/linux/man-pages/man2/getxattr.2.html

O_PATH (since Linux 2.6.39)
Obtain a file descriptor that can be used for two purposes: to
indicate a location in the filesystem tree and to perform
operations that act purely at the file descriptor level. The
file itself is not opened, and other file operations (e.g.,
read(2), write(2), fchmod(2), fchown(2), fgetxattr(2),
ioctl(2), mmap(2)) fail with the error EBADF.

Ref: http://man7.org/linux/man-pages/man2/open.2.html

@landley landley closed this as completed in 3609b31 Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants