Skip to content

Commit

Permalink
Univention (UCS): change umask only temporary
Browse files Browse the repository at this point in the history 
Before, the univention-bareos UCS listener module did change the umask permanently.
However this impacts ofter functionality of the UCS Listener.
Therefore the change the umask only temporary.
  • Loading branch information
@joergsteffens @franku
joergsteffens authored and franku committed May 28, 2019
1 parent c5e3d4d commit da72d6e
Showing 1 changed file with 17 additions and 15 deletions.
32 changes: 17 additions & 15 deletions core/platforms/univention/univention-bareos.py
View file
Expand Up @@ -142,32 +142,34 @@ def createClientSecret(client_name):

char_set = string.ascii_uppercase + string.digits + string.ascii_lowercase
password=''.join(random.sample(char_set*40,40))
os.umask(077)
oldumask = os.umask(0o077)
with open(path,'w') as f:
f.write(password)
os.chown(path,-1,0)
os.umask(oldumask)

return password



def removeClientJob(client_name):
path=JOBS_PATH+'/'+client_name+'.include'
os.remove(path)
path=JOBS_PATH+'/'+client_name+'.include'
os.remove(path)

def createClientJob(client_name,client_type,enable='Yes'):
password=getClientSecret(client_name)
path=JOBS_PATH+'/'+client_name+'.include'
templatefile=JOBS_PATH+'/'+client_type+'.template'
os.umask(077)
with open(templatefile,'r') as f:
content=f.read()

t=string.Template(content)
with open(path,"w") as f:
f.write(t.substitute(enable=enable, password=password, client_name=client_name))
os.chown(path,-1,bareos_gid)
os.chmod(path,stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP)
password=getClientSecret(client_name)
path=JOBS_PATH+'/'+client_name+'.include'
templatefile=JOBS_PATH+'/'+client_type+'.template'
with open(templatefile,'r') as f:
content=f.read()

t=string.Template(content)
oldumask = os.umask(0o077)
with open(path,"w") as f:
f.write(t.substitute(enable=enable, password=password, client_name=client_name))
os.chown(path,-1,bareos_gid)
os.chmod(path,stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP)
os.umask(oldumask)

def disableClientJob(client_name,client_type):
createClientJob(client_name,client_type,'No')
Expand Down

7 comments on commit da72d6e

@pmhahn
Copy link

@pmhahn pmhahn commented on da72d6e Jun 3, 2019 •
edited

Choose a reason for hiding this comment

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

If you change the umask, it's better to wrap it into a try: .. finally: ... block as otherwise you risk obscure cases, where an exception (IOError, KeyboardInterrupted, SystemExit, ...) is thrown and the umask is not restored.
So my recommendation is to use:

oldumask = os.umask(0o600)
try:
    ....
finally:
    os.umask(oldumask)

or not change the process-global umask at all by using my approach from https://help.univention.com/t/check-univention-replication-fails-after-some-time/9282/9

@joergsteffens
Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. I added a commit (#178) with the try/finally statement. I decided against the patch you proposed, as I preferred creating the file with limited permissions instead of changing the permission after writing sensible data into it.

@pmhahn
Copy link

@pmhahn pmhahn commented on da72d6e Jul 24, 2019

Choose a reason for hiding this comment

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

I decided against the patch you proposed, as I preferred creating the file with limited permissions instead of changing the permission after writing sensible data into it.

That is why I called os.fchmod() and os.fchown() before writing data.
But you are right that there still would be a vulnerability between the open() and those calls if umask would allow creating world-writeable files.

@arogge
Copy link
Member

@arogge arogge commented on da72d6e Jul 24, 2019

Choose a reason for hiding this comment

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

using os.open() instead of open() allows to set the file-mask while creating the file. No fiddling with the umask or calls to chmod required.

@pmhahn
Copy link

@pmhahn pmhahn commented on da72d6e Jul 24, 2019 •
edited

Choose a reason for hiding this comment

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

using os.open() instead of open() allows to set the file-mask while creating the file. No fiddling with the umask or calls to chmod required.

Yes, I know of os.open(), but please be aware that umask is still masked out from its mode argument.
Just FYI, no need for additional actions.

@arogge
Copy link
Member

@arogge arogge commented on da72d6e Jul 24, 2019

Choose a reason for hiding this comment

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

You're right, but IMO the umask is up to the user so she can restrict permissions of created files and directories even more.
Maybe we should look at this again @franku @joergsteffens ?

@pmhahn
Copy link

@pmhahn pmhahn commented on da72d6e Jul 24, 2019

Choose a reason for hiding this comment

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

If I remember correctly a process being daemonized should call umask(0). A quick search did not show any rational for this.
In contrast to that man:systemd.exec(5) lists umask(0o0022) as its default, which looks sane for to me: creating word-writeable files is very rare. For security sensitive daemons I would even go for 0o027 or 0o077 depending on if the daemon has a per-user-group or not.

Please sign in to comment.