Skip to content

Conversation

rubys
Copy link
Contributor

@rubys rubys commented Mar 4, 2023

@rails-bot rails-bot bot added the railties label Mar 4, 2023
@byroot
Copy link
Member

byroot commented Mar 4, 2023

What is the reasoning for this? Making them non-editable?

It sounds OK to me on paper, but that's a rather uncommon setup so I fear it might break some rare gems.

@rubys
Copy link
Contributor Author

rubys commented Mar 4, 2023

Security, yes.

For years, my primary development platform was Ubuntu, where I need to run sudo to install a gem, and provide my password to bundle update.

These days I split my time with MacOSX, where brew eliminates the need for this.

So, while this may be an uncommon setup for development machines, I will argue that this is the right choice for deployed machines. Particularly machines that are Debian based.

@n-rodriguez
Copy link

n-rodriguez commented Mar 6, 2023

Be careful with this one or you could run into this : mikel/mail#1489 (comment)

I fixed the issue with this :

# Cleanup final image and harden permissions on Ruby lib dir
RUN \
  # remove useless files/directories
  rm -rf /app/app/frontend && \
  rm -f /app/log/*.log && \
  rm -f /app/bin/webpacker && \
  rm -rf /app/vendor/bundle/ruby/3.2.0/cache && \
  \
  # fix directories owner
  chown -R root.root /app/vendor /app/.bundle && \
  \
  # fix directories permissions
  find /app/app /app/config /app/db /app/lib /app/vendor -type d -exec chmod 755 {} \; && \
  \
  # fix files permissions
  find /app -type f > /tmp/fix_perms.txt && \
  cat /tmp/fix_perms.txt | grep -v '\/app\/public_assets' | grep -v '\.so' | grep -v '\/bin\/' | grep -v '\/exe\/' | xargs chmod 444; \
  cat /tmp/fix_perms.txt | grep '\.so'    | xargs chmod 555; \
  cat /tmp/fix_perms.txt | grep '\/bin\/' | xargs chmod 555; \
  cat /tmp/fix_perms.txt | grep '\/exe\/' | xargs chmod 555; \
  rm -f /tmp/fix_perms.txt && \
  \
  # check files permissions
  find /app -type f -not -perm 444 -exec ls -hal {} \; > /tmp/check_perms.txt && \
  if [[ -z $(cat /tmp/check_perms.txt | grep -v '\/app\/tmp' | grep -v '\/app\/public_assets\/' | grep -v '\.so' | grep -v -- -r-xr-xr-x) ]]; then echo "OK!"; else echo "NOK!" && exit 1; fi; \
  rm -f /tmp/check_perms.txt

What it does? Basically it chmod 555 executable files and chmod 444 the other ones.

Notes: I did this because I also ran into mikel/mail#1489 (comment) and rather than fixing this single issue I choose the opposite approach : make my Docker image fully read only.

@zzak
Copy link
Member

zzak commented Mar 6, 2023

I'm also not seeing the docker CI job here, maybe it didn't run because b31e549 wasn't merged yet?

@byroot
Copy link
Member

byroot commented Mar 6, 2023

@zzak yeah I pushed that change after seeing Sam's PR didn't have that step.

you could run into this : mikel/mail#1489 (comment)

Yeah, that's the kind of things that make me think it might not be worth it.

IMO, The security gain is very limited here in my opinion, it would only prevent some kind of threats where the threat actors can only write in gems, and not in /rails, it's not totally improbable, but not super likely.

So I'm not really keen on risking to hit this type of issues however rare they are for such a limited gain.

@rubys
Copy link
Contributor Author

rubys commented Mar 6, 2023

In my opinion, the right long term direction is to lock down rails, i.e. limit writes to specific directories like tmp, storage, db, and log. That's super ugly in shell scripts, but could very easily be a rake task.

Rare cases where you need write access to, for example, gems could be handled by developers modifying the generated Dockerfiles, at which point the responsibility clearly shifts to them.

rubys added a commit to rubys/rails that referenced this pull request Mar 7, 2023
Current Dockerfile generated by Rails runs as a non-root user
which prevents modification of the operating system but leaves
wide open all gems and the application itself.

This change locks down the application gems and only opens up
access to the following directories: db, log, storage, tmp

This is a even more secure alternative to

rails#47580
@rubys
Copy link
Contributor Author

rubys commented Mar 8, 2023

Closing in favor of #47594

@rubys rubys closed this Mar 8, 2023
dhh pushed a commit that referenced this pull request Mar 8, 2023
Current Dockerfile generated by Rails runs as a non-root user
which prevents modification of the operating system but leaves
wide open all gems and the application itself.

This change locks down the application gems and only opens up
access to the following directories: db, log, storage, tmp

This is a even more secure alternative to

#47580
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants