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

chore: rebuild sinker to address jan-2024 glibc updates for debian bookworm #143

Conversation

eumoh1601
Copy link
Contributor

@eumoh1601 eumoh1601 commented May 9, 2024

related to: https://github.com/influxdata/tubernetes/issues/1567

changes:

rebuild sinker to address jan-2024 glibc updates for debian bookworm

#Remove setuid/setgid bits from executables. Also If the path is /proc, don't descend into it. This effectively excludes /proc from the search.
RUN find / -path /proc -prune -o -type f \( -perm -4000 -o -perm -2000 \) -exec chmod a-s {} \;

marcdavoli
marcdavoli previously approved these changes May 10, 2024
Dockerfile Outdated
Comment on lines 26 to 37
# Remove setuid/setgid bits from executables. Also If the path is /proc, don't descend into it. This effectively excludes /proc from the search.
RUN find / -path /proc -prune -o -type f \( -perm -4000 -o -perm -2000 \) -exec chmod a-s {} \;

# Create a dedicated user and group for the application
RUN groupadd --gid 1500 sinker \
&& useradd --uid 1500 --gid sinker --shell /bin/bash --create-home sinker

# RUN apt update \
# && apt install --yes ca-certificates libssl3 --no-install-recommends \
# && rm -rf /var/lib/{apt,dpkg,cache,log} \
# && groupadd --gid 1500 sinker \
# && useradd --uid 1500 --gid sinker --shell /bin/bash --create-home sinker

Choose a reason for hiding this comment

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

Can you elaborate a little bit on why these changes are necessary?

Copy link
Collaborator

Choose a reason for hiding this comment

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

As I read this, the groupadd is simply moved to be closer to the useradd, which is fine.

The part about removing setuid/setgid executables is from https://github.com/influxdata/tubernetes/issues/1567 and the idea is that if we're going to use a base image like Debian or Ubuntu, then stripping out the setuid/setgid bits off everything in the container is a meaningful hardening measure for containers running as non-root (like sinker does and like (by far nearly) all containers should!) since vulnerabilities in libraries/etc that the setuid executable uses could allow an attacker with code exec within the container to escalate to root within the container. Since containers are not considered root strong, this could allow an attacker to escalate to root on the pod and break out of the container. Stripping out the setuid/setgid bits at container build time blocks this avenue of attack within the container.

That said, the addition of -path /proc -prune causes nothing to match. I suspect you did this to avoid the error output. You might be able to do this instead if using bash:

# Remove setuid/setgid bits from executables as a hardening measure so non-root processes can't escalate.
RUN find $(ls -d /!(dev|proc|sys)) -type f \( -perm -4000 -o -perm -2000 \) -exec chmod a-s {} \;

Otherwise, I think you'd want to change this to:

# Remove setuid/setgid bits from executables as a hardening measure so non-root processes can't escalate.
RUN find /*bin /lib* /opt /usr -type f \( -perm -4000 -o -perm -2000 \) -exec chmod a-s {} \;

This doesn't catch all directories, but it does catch all the places that setuid/setgid binaries could be installed by distro packages.

Please also remove this (now unneeded) comment:

# RUN apt update \
#     && apt install --yes ca-certificates libssl3 --no-install-recommends \
#     && rm -rf /var/lib/{apt,dpkg,cache,log} \
#     && groupadd --gid 1500 sinker \
#     && useradd --uid 1500 --gid sinker --shell /bin/bash --create-home sinker

Copy link
Collaborator

Choose a reason for hiding this comment

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

That said, the addition of -path /proc -prune causes nothing to match.

CORRECTION: I did something wrong when I tested this. I can see now that find / -path /proc -prune -o -type f \( -perm -4000 -o -perm -2000 \) -ls does match stuff. This might be a refinement you'd still be interested in though:

# Remove setuid/setgid bits from executables as a hardening measure so non-root processes can't escalate.
RUN find / \( -path /dev -o -path /proc -o -path /sys \) -prune -o -type f \( -perm -4000 -o -perm -2000 \) -exec chmod a-s {} \;

@eumoh1601 eumoh1601 requested a review from marcdavoli May 13, 2024 11:31
@eumoh1601
Copy link
Contributor Author

eumoh1601 commented May 13, 2024

Updated the newly created image tested in k8s pods using tilt. Sinker pods seems to be running as usual.

@marcdavoli marcdavoli requested a review from jdstrand May 13, 2024 14:31
Copy link

@marcdavoli marcdavoli left a comment

Choose a reason for hiding this comment

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

As discussed, let's get Jamie to take a look before we merge.

Dockerfile Outdated
Comment on lines 26 to 37
# Remove setuid/setgid bits from executables. Also If the path is /proc, don't descend into it. This effectively excludes /proc from the search.
RUN find / -path /proc -prune -o -type f \( -perm -4000 -o -perm -2000 \) -exec chmod a-s {} \;

# Create a dedicated user and group for the application
RUN groupadd --gid 1500 sinker \
&& useradd --uid 1500 --gid sinker --shell /bin/bash --create-home sinker

# RUN apt update \
# && apt install --yes ca-certificates libssl3 --no-install-recommends \
# && rm -rf /var/lib/{apt,dpkg,cache,log} \
# && groupadd --gid 1500 sinker \
# && useradd --uid 1500 --gid sinker --shell /bin/bash --create-home sinker
Copy link
Collaborator

Choose a reason for hiding this comment

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

As I read this, the groupadd is simply moved to be closer to the useradd, which is fine.

The part about removing setuid/setgid executables is from https://github.com/influxdata/tubernetes/issues/1567 and the idea is that if we're going to use a base image like Debian or Ubuntu, then stripping out the setuid/setgid bits off everything in the container is a meaningful hardening measure for containers running as non-root (like sinker does and like (by far nearly) all containers should!) since vulnerabilities in libraries/etc that the setuid executable uses could allow an attacker with code exec within the container to escalate to root within the container. Since containers are not considered root strong, this could allow an attacker to escalate to root on the pod and break out of the container. Stripping out the setuid/setgid bits at container build time blocks this avenue of attack within the container.

That said, the addition of -path /proc -prune causes nothing to match. I suspect you did this to avoid the error output. You might be able to do this instead if using bash:

# Remove setuid/setgid bits from executables as a hardening measure so non-root processes can't escalate.
RUN find $(ls -d /!(dev|proc|sys)) -type f \( -perm -4000 -o -perm -2000 \) -exec chmod a-s {} \;

Otherwise, I think you'd want to change this to:

# Remove setuid/setgid bits from executables as a hardening measure so non-root processes can't escalate.
RUN find /*bin /lib* /opt /usr -type f \( -perm -4000 -o -perm -2000 \) -exec chmod a-s {} \;

This doesn't catch all directories, but it does catch all the places that setuid/setgid binaries could be installed by distro packages.

Please also remove this (now unneeded) comment:

# RUN apt update \
#     && apt install --yes ca-certificates libssl3 --no-install-recommends \
#     && rm -rf /var/lib/{apt,dpkg,cache,log} \
#     && groupadd --gid 1500 sinker \
#     && useradd --uid 1500 --gid sinker --shell /bin/bash --create-home sinker

Copy link
Collaborator

@jdstrand jdstrand left a comment

Choose a reason for hiding this comment

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

I'm going to approve as is (now that I corrected myself), but please consider the couple things I mentioned.

@eumoh1601 eumoh1601 merged commit f4bc2ff into main May 15, 2024
2 checks passed
@eumoh1601 eumoh1601 deleted the chore/rebuild-sinker-to-address-jan-2024-glibc-updates-for-debian-bookworm branch May 15, 2024 17:58
@jdstrand
Copy link
Collaborator

@eumoh1601 thanks for working on this! :)

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

Successfully merging this pull request may close these issues.

3 participants