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

Makes docker images support non-root execution #1031

Merged
merged 7 commits into from
Jun 27, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,15 @@ RUN \
mv /grist/static-built/* /grist/static && \
rmdir /grist/static-built

# To ensure non-root users can run grist, 'other' users need read access (and execute on directories)
# This should be the case by default when copying files in.
# Only uncomment this if running into permissions issues, as it takes a long time to execute on some systems.
# RUN chmod -R o+rX /grist

# Add a user to allow de-escalating from root on startup
RUN useradd -ms /bin/bash grist
ENV GRIST_DOCKER_USER=grist\
Copy link
Member

Choose a reason for hiding this comment

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

tiny lint: maybe a space before the \ just so separation is immediately clear without needing to mentally process next line

GRIST_DOCKER_GROUP=grist
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why this isn't just a separate ENV stanza?

Copy link
Contributor Author

@Spoffy Spoffy Jun 24, 2024

Choose a reason for hiding this comment

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

Figured we may as well not create an extra docker layer for it :)

WORKDIR /grist

# Set some default environment variables to give a setup that works out of the box when
Expand Down Expand Up @@ -151,5 +160,5 @@ ENV \

EXPOSE 8484

ENTRYPOINT ["/usr/bin/tini", "-s", "--"]
ENTRYPOINT ["./sandbox/docker_entrypoint.sh"]
CMD ["node", "./sandbox/supervisor.mjs"]
40 changes: 40 additions & 0 deletions sandbox/docker_entrypoint.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
#!/usr/bin/env bash
set -Eeuo pipefail

# Runs the command provided as arguments, but attempts to configure permissions first.

important_read_dirs=("/grist" "/persist")
write_dir="/persist"
current_user_id=$(id -u)

# We want to avoid running Grist as root if possible.
# Try to setup permissions and de-elevate to a normal user.
if [[ $current_user_id == 0 ]]; then
target_user=${GRIST_DOCKER_USER:-grist}
target_group=${GRIST_DOCKER_GROUP:-grist}

# Make sure the target user owns everything that Grist needs write access to.
find $write_dir ! -user "$target_user" -exec chown "$target_user" "{}" +

# Restart as the target user, replacing the current process (replacement is needed for security).
Copy link
Contributor

Choose a reason for hiding this comment

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

What security does exec give us? If we had a parent process, a security hole in setpriv would keep the root process running, and thus potentially exploitable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly - exec throws away the parent process running as root.

Digging out the issue, it looks like it was a vuln mentioned in Gosu (an alternative to setpriv), concerning TTYs persisting across privilege boundaries:
tianon/gosu#37

The exec gets rid of the parent shell, which removes vulnerabilities where malicious code can interact with it.

# Alternative tools to setpriv are: chroot, gosu.
# Need to use `exec` to close the parent shell, to avoid vulnerabilities: https://github.com/tianon/gosu/issues/37
exec setpriv --reuid "$target_user" --regid "$target_group" --init-groups /usr/bin/env bash "$0" "$@"
fi

# Validate that this user has access to the top level of each important directory.
# There might be a benefit to testing individual files, but this is simpler as the dir may start empty.
for dir in "${important_read_dirs[@]}"; do
if ! { test -r "$dir" ;} ; then
echo "Invalid permissions, cannot read '$dir'. Aborting." >&2
exit 1
fi
done
for dir in "${important_write_dirs[@]}"; do
if ! { test -r "$dir" && test -w "$dir" ;} ; then
echo "Invalid permissions, cannot write '$dir'. Aborting." >&2
exit 1
fi
done

exec /usr/bin/tini -s -- "$@"
Loading