selinux: don't use lsetxattr on /proc/self/fd/%d and bump selinux to v1.10.1
#3889
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc 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 |
lsetxattr always fails with ENOTSUP when it tries to relabel /proc/self/fd/%d but in this case we are acutally intrested on the actual file pointed by the `/proc/self/fd/%d` not in the `symlink` so use `Chcon` instead of `Relabel` since `Relabel` was configured here opencontainers/selinux#173 to use `lsetxattr` instead of `setxattr`. [NO NEW TESTS NEEDED] Signed-off-by: Aditya R <[email protected]>
[NO NEW TESTS NEEDED] Signed-off-by: Aditya R <[email protected]>
flouthoc
force-pushed
the
dont-use-lsetxattr-proc-fd
branch
from
90fcff9 to
552a4c6
Compare
flouthoc
changed the title
lsetxattr on /proc/self/fd/%dlsetxattr on /proc/self/fd/%d and bump selinux to v1.10.1
|
@rhatdan @giuseppe @nalind @TomSweeneyRedHat PTAL. This should fix selinux issue both in buildah and podman ( containers/podman#13687 ) |
| @@ -33,7 +32,7 @@ func runLabelStdioPipes(stdioPipe [][]int, processLabel, mountLabel string) erro | |||
| } | |||
| for i := range stdioPipe { | |||
| pipeFdName := fmt.Sprintf("/proc/self/fd/%d", stdioPipe[i][0]) | |||
| if err := label.Relabel(pipeFdName, pipeContext, false); err != nil { | |||
| if err := selinux.Chcon(pipeFdName, pipeContext, false); err != nil { | |||
There was a problem hiding this comment.
label.Relabel ends up calling selinux.Chcon, but it checks if selinux is enabled.
Why do we need this change?
There was a problem hiding this comment.
@giuseppe Relabel calls Chcon but ends up hard-coding recurse flag to true https://github.com/opencontainers/selinux/blob/main/go-selinux/label/label_linux.go#L169, which is now configured to use lsetxattr( always instead of setxattr ( the change was done here opencontainers/selinux#173 ) and for somereason lsetxattr always gives operation not supported for both rootless and root cases when labeling /proc/self/fd/%d . I think that's because lsetxattr tries to label this special symlink /proc/self/fd/%d but instead it should try to label the actual file like setxattr( used to do before.
Failure is easy to reproduce if you see containers/podman#13687 and #3875
There was a problem hiding this comment.
thanks.
What happens now if we try to relabel a real symlink (not the magic /proc/self/fd one)? Do we end up relabelling the file it points to?
There was a problem hiding this comment.
@giuseppe Yes I think now we follow the symlink via https://github.com/opencontainers/selinux/blob/main/go-selinux/selinux_linux.go#L338.
|
Let's concentrate on #3875 I now duplicate the previous behavior, reading the symlink if it exists and added an IsNotExists check, and it seems to fix the problem. |
Syscall
lsetxattralways fails withENOTSUPwhen it tries to relabel/proc/self/fd/%dbut in this case we are acutally interested in theactual file pointed by the
/proc/self/fd/%dnot in thesymlink( since that was the behavior before ) so use
Chconinstead ofRelabelsince
Relabelwas configured here Relabel: relabel links instead of their targets opencontainers/selinux#173to use
lsetxattrinstead ofsetxattr.Bump selinux to
v1.10.1to verify if it worksFixes and replaces: #3875
See: containers/common#994
[NO NEW TESTS NEEDED]: This does not adds new functionality but all existing tests must pass with selinux
v.1.10.1as of now they all break when selinux is bumped tov.1.10.1