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

libselinux: off-by-one in setcon() family #417

Open
enh-google opened this issue Jan 29, 2024 · 0 comments
Open

libselinux: off-by-one in setcon() family #417

enh-google opened this issue Jan 29, 2024 · 0 comments

Comments

@enh-google
Copy link

setprocattrcon_raw() writes the C terminating '\0' (https://github.com/SELinuxProject/selinux/blob/main/libselinux/src/procattr.c#L217):

   ret = write(fd, context2, strlen(context2) + 1);

which i assume is an accident?

for C programs using libselinux, the strdup() in getprocattrcon_raw() at https://github.com/SELinuxProject/selinux/blob/main/libselinux/src/procattr.c#L160 hides this by truncating at the '\0', so callers appear to read back what they wrote (even if the kernel saw a slightly different byte sequence).

anything reading /proc directly in a language that can represent a '\0' in a string will see this extra '\0', despite it not intentionally being part of the context.

there's some weird-looking code in the kernel too:

        /* Obtain a SID for the context, if one was specified. */
        if (size && str[0] && str[0] != '\n') {
                if (str[size-1] == '\n') {
                        str[size-1] = 0;
                        size--;
                }
                error = security_context_to_sid(value, size,
                                                &sid, GFP_KERNEL);
                if (error == -EINVAL && !strcmp(name, "fscreate")) {
                        if (!has_cap_mac_admin(true)) {
                                struct audit_buffer *ab;
                                size_t audit_size;

                                /* We strip a nul only if it is at the end, otherwise the
                                 * context contains a nul and we should audit that */
                                if (str[size - 1] == '\0')
                                        audit_size = size - 1;
                                else
                                        audit_size = size;

if contexts are allowed to contain '\0's, isn't the libselinux getter API inherently broken? it'll truncate the context at the first '\0', which seems suboptimal. but it seems to me that contexts are compared in the kernel using strcmp() rather than memcmp(), so allowing '\0' in a context seems like a bug?

(i'm looking at this because i have code that just records the selinux context of a crashing process in a crash dump, and the '\0's that i would argue are inserted by libselinux are showing up there. given that it's not clear to me that the libselinux code is correct or intended, i didn't want to just duplicate the "truncate at the first '\0'" logic without at least filing a bug...)

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

1 participant