Bug #2375

The generated rules permissions are not correct

Added by Nicolas CHARLES over 2 years ago. Updated about 2 years ago.

Status:Released Start date:2012-03-15
Priority:2 Due date:
Assignee:Matthieu CERDA % Done:

100%

Category:Webapp - Config management
Target version:2.3.8
Needs translating:No Pull Request:

Description

The generated rules have the 755 permissions, which is not great in term of security. They should have at most 750

Associated revisions

Revision 9dcaf567
Added by Matthieu CERDA about 2 years ago

Fixes #2375: The Rudder jetty init script now sets a more restrictive umask for the rudder webapps

Revision cda2327c
Added by Nicolas PERRON about 2 years ago

Merge branch 'branches/rudder/2.3' into branches/rudder/2.4

  • branches/rudder/2.3:
    Fixes #2375: The Rudder jetty init script now sets a more restrictive umask for the rudder webapps

Revision 19851318
Added by Nicolas PERRON about 2 years ago

Merge branch 'branches/rudder/2.4'

  • branches/rudder/2.4:
    Refs #2637 Escape % in timestamp for SPEC file in rudder-inventory-ldap
    Create directory before copying things into it. Refs #2637
    Last commit, fixes #2637: porting the logic to RPM packages and fixing one typo in Debian packaging
    Refs #2637: when reimporting the database, we may also have to fix the cpuSpeed attribute contents. Fix a typo in previous commit
    Refs #2637: when reimporting the database, we may also have to fix the cpuSpeed attribute contents.
    Tweak logic to export/import LDAP database on upgrading rudder-inventory-ldap for Debian. Now use a timestamped file to communicate between preinst and postinst. Refs #2637
    Tweak logic to export/import LDAP database on upgrading rudder-inventory-ldap for Debian. Refs #2637
    Restore minimal preinst targets for rudder-inventory-ldap for Debian. Refs #2637
    Tweak logic to export/import LDAP database on upgrading rudder-inventory-ldap for Debian. Refs #2637
    Refs #2637: upgrading the DB is all fine, but we also need to migrate schema at the same time. So moving that logic from rudder-webapp to rudder-inventory-ldap package.
    Tweak logic to export/import LDAP database on upgrading rudder-inventory-ldap for Debian. Refs #2637
    Tweak logic to export/import LDAP database on upgrading rudder-inventory-ldap for Debian. Refs #2637
    Add logic to export/import LDAP database on upgrading rudder-inventory-ldap for Debian. Refs #2637
    Preserve double colon for base64 values in LDAP migration script. Fixes #2636
    Fixes #2626 - Add a check for new attributes about git change message in rudder-upgrade
    TRIVIAL
    Fixes #2627 Refs 2442 - Correct wrong condition for checking LDAP existence
    Refs #2560: The rudder-upgrade script now autodetects ldap user and password from the rudder-web.properties file
    Fixes #2560: The migration script to migrate string cpuSpeed values to integers in LDAP is in rudder-webapp
    Fixes #2375: The Rudder jetty init script now sets a more restrictive umask for the rudder webapps

History

#1 Updated by Nicolas PERRON over 2 years ago

Is these permissions set by packaging ?

#2 Updated by Jonathan CLARKE over 2 years ago

  • Priority changed from 1 to 2
  • Target version changed from 2.3.7 to 2.3.8

Nicolas PERRON wrote:

Is these permissions set by packaging ?

No, this ticket is about permissions on files generated by the webapp. The contents of /var/rudder/share and /var/rudder/cfengine-community/inputs, basically.

So long as the Rudder server is secure, no access to these files can happen. When they are copied to nodes, their permissions should be changed there, but there may be a gap in between when they are world readable. Demoting priority a bit, but we should still fix this soon.

#3 Updated by François ARMAND about 2 years ago

Well, we don't have the possibility to control Unix File permission in Java (the platform independent thing).

The possibility are:

  • 1) use a different umask before starting the JVM, but that will impact ALL files created by applications under that JVM, what seems to be a bad idea ;
  • 2) run a chmod from the JVM after each creation, what is possible but not a too good idea (JVM calling Unix process is utterly slow)
  • 3) using JDK 7 to be able to use the NIO API (java.nio.file.attribute.PosixFileAttributes / Files.setPosixFilePermissions)
  • 4) have the permissions set by a CFEngine running on the server :)

And I believe I don't know anything else.

If 4) is not possible, I can implement 2), but it's likelly to be noticeable.

#4 Updated by François ARMAND about 2 years ago

  • Status changed from New to Discussion
  • Assignee changed from François ARMAND to Nicolas CHARLES

#5 Updated by Jonathan CLARKE about 2 years ago

  • Assignee changed from Nicolas CHARLES to François ARMAND

François ARMAND wrote:

Well, we don't have the possibility to control Unix File permission in Java (the platform independent thing).

The possibility are:

  • 1) use a different umask before starting the JVM, but that will impact ALL files created by applications under that JVM, what seems to be a bad idea ;
  • 2) run a chmod from the JVM after each creation, what is possible but not a too good idea (JVM calling Unix process is utterly slow)
  • 3) using JDK 7 to be able to use the NIO API (java.nio.file.attribute.PosixFileAttributes / Files.setPosixFilePermissions)
  • 4) have the permissions set by a CFEngine running on the server :)

And I believe I don't know anything else.

If 4) is not possible, I can implement 2), but it's likelly to be noticeable.

4) is possible, but I think it's already the case (not sure). However, this is not a good solution in a security problem: if we're worried about malicious users reading the files, we should not allow them to be in mode 755 at all, not even for 1 second. 2) is not sufficient for the same reason.

3) is not possible as the JRE 7 is not easily available (in distribution package repos) for all (or even most) of our supported server operating systems.

By elimination, it seems to me that 1) is the most sensible. If we assume that other malicious users may be on the Rudder server, and try and protect our data from them, we should certainly not create any files in 755. It is also easy to implement.

Can you see any reasons why this would have other negative impacts?

#6 Updated by Jonathan CLARKE about 2 years ago

Any feedback on this?

#7 Updated by François ARMAND about 2 years ago

OK, let's go for the UMASK, we will see if anything breaks - I didn't come to think to anything that should, but who knows :)

#8 Updated by Jonathan CLARKE about 2 years ago

  • Status changed from Discussion to 2
  • Assignee changed from François ARMAND to Matthieu CERDA

Matthieu, can you please add a configuration in 2.4 to force Jetty's umask to 027 (in it's init script I presume). The simplest way to test will be to regenerate promises and check their permission - they should not have 640 perms.

#9 Updated by Matthieu CERDA about 2 years ago

  • Status changed from 2 to Discussion
  • Assignee changed from Matthieu CERDA to Jonathan CLARKE

Well, I just tried now and Rudder did generate promises with these rights:

drwxr-xr-x 12 root root 4,0K juin 21 17:37 .
drwxr-xr-x 14 root root 4,0K avril 25 06:42 ..
drwx------  3 root root 4,0K juin 21 17:36 06da3556-5204-4bd7-b3b0-fa5e7bcfbbea

700 seems like a perfect mode to me :) I do not see any trouble here.

Same goes for /var/rudder/cfengine-community/inputs by the way:

-rw------- 1 root root 3,2K juin 21 17:37 /var/rudder/cfengine-community/inputs/failsafe.cf
-rw------- 1 root root  16K juin 21 17:37 /var/rudder/cfengine-community/inputs/promises.cf

Are we really sure there is a problem here ?

(This test was done on orchestrator-3, i'll try on orch-2 as well)

#10 Updated by Matthieu CERDA about 2 years ago

  • Status changed from Discussion to In progress
  • Assignee changed from Jonathan CLARKE to Matthieu CERDA

Well, that is quite a surprise, indeed orch-3 seems to be the only orchestrator doing this. The init script might have been polluted by my own umask maybe.

Whatever, I'm on it then.

#11 Updated by Matthieu CERDA about 2 years ago

  • Status changed from In progress to Discussion
  • Assignee changed from Matthieu CERDA to Jonathan CLARKE

Damn. Jetty seems to ignore the umask.

It seems we are going to need another approach, or tolerate the delay before a cf-agent run.

#12 Updated by François ARMAND about 2 years ago

Well, the JVM respects umask, and jetty (and Rudder) only call JVM methods to create files, so that seems highly improbable.

Actually, I tested in orchestrateur-3, and umask seems to be correctly applied. Note that only new files have the new umask.

I saved the non-modified jetty init script in /var/rudder/modified-files/jetty.init, and for history, this is the diff:

--- /var/rudder/modified-files/jetty.init    2012-06-25 18:03:20.000000000 +0200
+++ /etc/init.d/jetty    2012-06-25 18:04:18.000000000 +0200
@@ -436,6 +436,8 @@
 ##################################################
 case "$ACTION" in
   start)
+    UMASK="0027" 
+    echo "Setting umask to ${UMASK}" 
     echo -n "Starting Jetty: " 

     if (( NO_START )); then 
@@ -451,7 +453,7 @@
       then
         CH_USER="-c$JETTY_USER" 
       fi
-      if start-stop-daemon -S -p"$JETTY_PID" $CH_USER -d"$JETTY_HOME" -b -m -a "$JAVA" -- "${RUN_ARGS[@]}" --daemon
+      if start-stop-daemon -k ${UMASK} -S -p"$JETTY_PID" $CH_USER -d"$JETTY_HOME" -b -m -a "$JAVA" -- "${RUN_ARGS[@]}" --daemon
       then
         sleep 1
         if running "$JETTY_PID" 
@@ -463,7 +465,7 @@
       fi

     else
-
+      umask ${UMASK}  
       if [ -f "$JETTY_PID" ]
       then
         if running $JETTY_PID

I hope I didn't make any mistakes with my tests.

#13 Updated by Matthieu CERDA about 2 years ago

Well, I tried with a much more simple approach... Setting umask in my shell...

FAR's modifications seems totally valid, I approve it !

#14 Updated by Matthieu CERDA about 2 years ago

  • Status changed from Discussion to Pending technical review
  • % Done changed from 0 to 100

Applied in changeset commit:9dcaf567df0751c659484b43948b45804408423f.

#15 Updated by Matthieu CERDA about 2 years ago

  • Assignee changed from Jonathan CLARKE to Matthieu CERDA

#16 Updated by Matthieu CERDA about 2 years ago

The patch has been applied to the rudder-jetty package.

#17 Updated by Jonathan CLARKE about 2 years ago

  • Status changed from Pending technical review to Released

Looks good to me!

Also available in: Atom PDF