Skip to content

Commit

Permalink
Merge pull request from GHSA-v6f3-gh5h-mqwx
Browse files Browse the repository at this point in the history
[integration] Ensure proxy files are always written securely
  • Loading branch information
fstagni committed Apr 8, 2024
2 parents c2faf3f + 635212e commit 38a18a7
Show file tree
Hide file tree
Showing 7 changed files with 39 additions and 53 deletions.
17 changes: 2 additions & 15 deletions src/DIRAC/Core/Security/ProxyFile.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from DIRAC import S_OK, S_ERROR
from DIRAC.Core.Utilities import DErrno
from DIRAC.Core.Security.DiracX import addTokenToPEM
from DIRAC.Core.Utilities.File import secureOpenForWrite
from DIRAC.Core.Security.X509Chain import X509Chain # pylint: disable=import-error
from DIRAC.Core.Security.Locations import getProxyLocation

Expand All @@ -18,26 +19,12 @@ def writeToProxyFile(proxyContents, fileName=False):
- proxyContents : string object to dump to file
- fileName : filename to dump to
"""
# Write the X509 proxy to a file
if not fileName:
try:
fd, proxyLocation = tempfile.mkstemp()
os.close(fd)
except OSError:
return S_ERROR(DErrno.ECTMPF)
fileName = proxyLocation
try:
with open(fileName, "w") as fd:
with secureOpenForWrite(fileName) as fd:
fd.write(proxyContents)
except Exception as e:
return S_ERROR(DErrno.EWF, f" {fileName}: {repr(e).replace(',)', ')')}")

# Set file permissions
try:
os.chmod(fileName, stat.S_IRUSR | stat.S_IWUSR)
except Exception as e:
return S_ERROR(DErrno.ESPF, f"{fileName}: {repr(e).replace(',)', ')')}")

# Add DiracX token to the file
proxy = X509Chain()
retVal = proxy.loadProxyFromFile(fileName)
Expand Down
13 changes: 2 additions & 11 deletions src/DIRAC/Core/Security/m2crypto/X509CRL.py
Original file line number Diff line number Diff line change
@@ -1,15 +1,13 @@
""" X509CRL is a class for managing X509CRL
This class is used to manage the revoked certificates....
"""
import stat
import os
import tempfile
import re
import datetime

import M2Crypto
from DIRAC import S_OK, S_ERROR
from DIRAC.Core.Utilities import DErrno
from DIRAC.Core.Utilities.File import secureOpenForWrite

# pylint: disable=broad-except

Expand Down Expand Up @@ -72,17 +70,10 @@ def dumpAllToFile(self, filename=False):
if not self.__loadedCert:
return S_ERROR("No certificate loaded")
try:
if not filename:
fd, filename = tempfile.mkstemp()
os.close(fd)
with open(filename, "w", encoding="ascii") as fd:
with secureOpenForWrite(filename) as fd:
fd.write(self.__pemData)
except Exception as e:
return S_ERROR(DErrno.EWF, f"{filename}: {repr(e).replace(',)', ')')}")
try:
os.chmod(filename, stat.S_IRUSR | stat.S_IWUSR)
except Exception as e:
return S_ERROR(DErrno.ESPF, f"{filename}: {repr(e).replace(',)', ')')}")
return S_OK(filename)

def hasExpired(self):
Expand Down
21 changes: 4 additions & 17 deletions src/DIRAC/Core/Security/m2crypto/X509Chain.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,7 @@
"""
import copy
import hashlib
import os
import re
import stat
import tempfile

import M2Crypto

Expand All @@ -19,6 +16,7 @@
from DIRAC.Core.Security.m2crypto.X509Certificate import X509Certificate
from DIRAC.Core.Utilities import DErrno
from DIRAC.Core.Utilities.Decorators import executeOnlyIf
from DIRAC.Core.Utilities.File import secureOpenForWrite

# Decorator to check that _certList is not empty
needCertList = executeOnlyIf("_certList", S_ERROR(DErrno.ENOCHAIN))
Expand Down Expand Up @@ -452,14 +450,10 @@ def generateProxyToFile(self, filePath, lifetime, diracGroup=False, strength=DEF
if not retVal["OK"]:
return retVal
try:
with open(filePath, "w") as fd:
with secureOpenForWrite(filePath) as fd:
fd.write(retVal["Value"])
except Exception as e:
return S_ERROR(DErrno.EWF, f"{filePath} :{repr(e).replace(',)', ')')}")
try:
os.chmod(filePath, stat.S_IRUSR | stat.S_IWUSR)
except Exception as e:
return S_ERROR(DErrno.ESPF, f"{filePath} :{repr(e).replace(',)', ')')}")
return S_OK()

@needCertList
Expand Down Expand Up @@ -835,17 +829,10 @@ def dumpAllToFile(self, filename=False):
return retVal
pemData = retVal["Value"]
try:
if not filename:
fd, filename = tempfile.mkstemp()
os.close(fd)
with open(filename, "w") as fp:
fp.write(pemData)
with secureOpenForWrite(filename) as fh:
fh.write(pemData)
except Exception as e:
return S_ERROR(DErrno.EWF, f"{filename} :{repr(e).replace(',)', ')')}")
try:
os.chmod(filename, stat.S_IRUSR | stat.S_IWUSR)
except Exception as e:
return S_ERROR(DErrno.ESPF, f"{filename} :{repr(e).replace(',)', ')')}")
return S_OK(filename)

@needCertList
Expand Down
24 changes: 24 additions & 0 deletions src/DIRAC/Core/Utilities/File.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
import sys
import re
import errno
import stat
import tempfile
from contextlib import contextmanager

# Translation table of a given unit to Bytes
# I know, it should be kB...
Expand Down Expand Up @@ -253,6 +256,27 @@ def convertSizeUnits(size, srcUnit, dstUnit):
return -sys.maxsize


@contextmanager
def secureOpenForWrite(filename=None, *, text=True):
"""Securely open a file for writing.
If filename is not provided, a file is created in tempfile.gettempdir().
The file always created with mode 600.
:param string filename: name of file to be opened
"""
if filename:
fd = os.open(
path=filename,
flags=os.O_WRONLY | os.O_CREAT | os.O_TRUNC,
mode=stat.S_IRUSR | stat.S_IWUSR,
)
else:
fd, filename = tempfile.mkstemp(text=text)
with open(fd, "w" if text else "wb", encoding="ascii") as fd:
yield fd


if __name__ == "__main__":
for p in sys.argv[1:]:
print(f"{p} : {getGlobbedTotalSize(p)} bytes")
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import os
import re
import jwt
import stat
import time
import json
import datetime

from DIRAC import S_OK, S_ERROR
from DIRAC.Core.Utilities import DErrno
from DIRAC.Core.Utilities.File import secureOpenForWrite
from DIRAC.ConfigurationSystem.Client.Helpers import Registry
from DIRAC.Resources.IdProvider.IdProviderFactory import IdProviderFactory

Expand Down Expand Up @@ -83,14 +83,10 @@ def writeToTokenFile(tokenContents, fileName):
"""
location = getTokenFileLocation(fileName)
try:
with open(location, "w") as fd:
with secureOpenForWrite(location) as fd:
fd.write(tokenContents)
except Exception as e:
return S_ERROR(DErrno.EWF, f" {location}: {repr(e)}")
try:
os.chmod(location, stat.S_IRUSR | stat.S_IWUSR)
except Exception as e:
return S_ERROR(DErrno.ESPF, f"{location}: {repr(e)}")
return S_OK(location)


Expand Down
5 changes: 2 additions & 3 deletions src/DIRAC/Interfaces/Utilities/DConfigCache.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
#!/usr/bin/env python
import os
import re
import stat
import time
import pickle
import tempfile

from DIRAC import gLogger
from DIRAC.Core.Base.Script import Script
from DIRAC.Core.Utilities.File import secureOpenForWrite
from DIRAC.ConfigurationSystem.Client.ConfigurationData import gConfigurationData


Expand Down Expand Up @@ -63,8 +63,7 @@ def cacheConfig(self):
if self.newConfig:
self.__cleanCacheDirectory()

with open(self.configCacheName, "wb") as fcache:
os.chmod(self.configCacheName, stat.S_IRUSR | stat.S_IWUSR)
with secureOpenForWrite(self.configCacheName, text=False) as fcache:
pickle.dump(gConfigurationData.mergedCFG, fcache)
else:
try:
Expand Down
4 changes: 3 additions & 1 deletion src/DIRAC/WorkloadManagementSystem/Utilities/PilotWrapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
from __future__ import print_function
import os
import io
import stat
import tempfile
import sys
Expand Down Expand Up @@ -174,7 +175,8 @@ def pilotWrapperScript(
for pfName, encodedPf in pilotFilesCompressedEncodedDict.items():
compressedString += """
try:
with open('%(pfName)s', 'wb') as fd:
fd = os.open('%(pfName)s', os.O_WRONLY | os.O_CREAT | os.O_TRUNC, stat.S_IRUSR | stat.S_IWUSR)
with io.open(fd, 'wb') as fd:
if sys.version_info < (3,):
fd.write(bz2.decompress(base64.b64decode(\"\"\"%(encodedPf)s\"\"\")))
else:
Expand Down

0 comments on commit 38a18a7

Please sign in to comment.