Maniphest T6466

gpgme python example code contains insecure code pattern / chmod permission race condition
Closed, ResolvedPublic
Actions

Assigned To
• gniibe
Authored By
hanno
Apr 23 2023, 5:33 PM
Tags
Subscribers
• gniibe
hanno
• ikloecker
• werner

Description

The file lang/python/examples/howto/export-secret-key.py in the gpgme source code contains this code:

if result is not None:
    with open(keyfile, "wb") as f:
        f.write(result)
    os.chmod(keyfile, 0o600)

This is insecure. The problem is that there is a race condition where the private key is written to a file while it is still readable. You can find a proof of concept exploit for such issues here: https://github.com/hannob/fpracer

A more secure pattern would be this:

if result is not None:
    old_umask = os.umask(0o077)
    with open(keyfile, "wb") as f:
        f.write(result)
    os.umask(old_umask)

This makes sure the file already has secure permissions when it is opened.

This and similar coding patterns appear in these files:

lang/python/doc/src/gpgme-python-howto.org
lang/python/doc/src/gpgme-python-howto.tex~
lang/python/examples/howto/export-secret-key.py
lang/python/examples/howto/export-secret-keys.py
lang/python/examples/howto/temp-homedir-config.py

(sidenote: the .tex~ file is probably there by accident.)

Revisions and Commits

rM GPGME
rMd44a473e2716 doc: Fix Python example code.

Event Timeline

hanno created this task.Apr 23 2023, 5:33 PM
hanno created this object in space S1 Public.
• werner triaged this task as Low priority.Apr 24 2023, 8:22 AM
• werner added projects: Documentation, Python.
• werner added a subscriber: • werner.

Funny enough that Python seems not to allow to set the permission with open. Low priority because a proper umask must anyway be used on a multi-user system.

• ikloecker added a subscriber: • ikloecker.Apr 24 2023, 12:26 PM
In T6466#169934, @werner wrote:

Funny enough that Python seems not to allow to set the permission with open. Low priority because a proper umask must anyway be used on a multi-user system.

It's not that obvious, but since Python 3.3 open does allow setting the permissions by passing a tiny helper calling os.open with the desired mode as opener. See the documentation of opener (including an example) (https://docs.python.org/3/library/functions.html#open) and of os.open (https://docs.python.org/3/library/os.html#os.open).

• gniibe added a subscriber: • gniibe.Apr 25 2023, 7:43 AM

So, here are fixes. I'll apply soonish.

diff --git a/lang/python/doc/src/gpgme-python-howto.org b/lang/python/doc/src/gpgme-python-howto.org
index 2121fe65..b4367872 100644
--- a/lang/python/doc/src/gpgme-python-howto.org
+++ b/lang/python/doc/src/gpgme-python-howto.org
@@ -1612,6 +1612,7 @@ of the entire public keybox.
 
 #+BEGIN_SRC python -i
 import gpg
+import os
 import os.path
 import sys
 
@@ -1619,6 +1620,9 @@ print("""
 This script exports one or more public keys in minimised form.
 """)
 
+def open_0o600(path, flags):
+    return os.open(path, flags, mode=0o600)
+
 c = gpg.Context(armor=True)
 
 if len(sys.argv) >= 4:
@@ -1654,7 +1658,7 @@ except:
     result = c.key_export_minimal(pattern=None)
 
 if result is not None:
-    with open(keyfile, "wb") as f:
+    with open(keyfile, "wb", opener=open_0o600) as f:
         f.write(result)
 else:
     pass
@@ -1686,6 +1690,9 @@ This script exports one or more secret keys.
 The gpg-agent and pinentry are invoked to authorise the export.
 """)
 
+def open_0o600(path, flags):
+    return os.open(path, flags, mode=0o600)
+
 c = gpg.Context(armor=True)
 
 if len(sys.argv) >= 4:
@@ -1735,9 +1742,8 @@ except:
     result = c.key_export_secret(pattern=None)
 
 if result is not None:
-    with open(keyfile, "wb") as f:
+    with open(keyfile, "wb", opener=open_0o600)) as f:
         f.write(result)
-    os.chmod(keyfile, 0o600)
 else:
     pass
 #+END_SRC
diff --git a/lang/python/examples/howto/export-secret-key.py b/lang/python/examples/howto/export-secret-key.py
index eeedb84b..caae0874 100755
--- a/lang/python/examples/howto/export-secret-key.py
+++ b/lang/python/examples/howto/export-secret-key.py
@@ -35,6 +35,9 @@ This script exports one or more secret keys.
 The gpg-agent and pinentry are invoked to authorise the export.
 """)
 
+def open_0o600(path, flags):
+    return os.open(path, flags, mode=0o600)
+
 c = gpg.Context(armor=True)
 
 if len(sys.argv) >= 4:
@@ -84,8 +87,7 @@ except:
     result = c.key_export_secret(pattern=None)
 
 if result is not None:
-    with open(keyfile, "wb") as f:
+    with open(keyfile, "wb", opener=open_0o600) as f:
         f.write(result)
-    os.chmod(keyfile, 0o600)
 else:
     pass
diff --git a/lang/python/examples/howto/export-secret-keys.py b/lang/python/examples/howto/export-secret-keys.py
index 8055e4e3..32a1e4ab 100755
--- a/lang/python/examples/howto/export-secret-keys.py
+++ b/lang/python/examples/howto/export-secret-keys.py
@@ -37,6 +37,9 @@ file formats, saved in files within the user's GPG home directory.
 The gpg-agent and pinentry are invoked to authorise the export.
 """)
 
+def open_0o600(path, flags):
+    return os.open(path, flags, mode=0o600)
+
 if sys.platform == "win32":
     gpgconfcmd = "gpgconf.exe --list-dirs homedir"
 else:
@@ -119,15 +122,13 @@ except:
     b_result = b.key_export_secret(pattern=None)
 
 if a_result is not None:
-    with open(ascfile, "wb") as f:
+    with open(ascfile, "wb", opener=open_0o600) as f:
         f.write(a_result)
-    os.chmod(ascfile, 0o600)
 else:
     pass
 
 if b_result is not None:
-    with open(gpgfile, "wb") as f:
+    with open(gpgfile, "wb", opener=open_0o600) as f:
         f.write(b_result)
-    os.chmod(gpgfile, 0o600)
 else:
     pass
diff --git a/lang/python/examples/howto/temp-homedir-config.py b/lang/python/examples/howto/temp-homedir-config.py
index 897d2f9a..0a80b6d2 100755
--- a/lang/python/examples/howto/temp-homedir-config.py
+++ b/lang/python/examples/howto/temp-homedir-config.py
@@ -112,18 +112,18 @@ else:
 
 nh = "{0}/.{1}".format(userdir, new_homedir)
 
+def open_0o600(path, flags):
+    return os.open(path, flags, mode=0o600)
+
 if os.path.exists(nh) is True:
     print("The {0} directory already exists.".format(nh))
 else:
     print("Creating the {0} directory.".format(nh))
-    os.mkdir(nh)
-    os.chmod(nh, 0o700)
-    with open("{0}/{1}".format(nh, "gpg.conf"), "w") as f1:
+    os.mkdir(nh, 0o700)
+    with open("{0}/{1}".format(nh, "gpg.conf"), "w", opener=open_0o600) as f1:
         f1.write(gpgconf)
-    os.chmod("{0}/{1}".format(nh, "gpg.conf"), 0o600)
-    with open("{0}/{1}".format(nh, "gpg-agent.conf"), "w") as f2:
+    with open("{0}/{1}".format(nh, "gpg-agent.conf"), "w", opener=open_0o600) as f2:
         f2.write(gpgconf)
-    os.chmod("{0}/{1}".format(nh, "gpg-agent.conf"), 0o600)
     print("""You may now use the {0} directory as an alternative GPG homedir:
 
 gpg --homedir {0}
• ikloecker added a comment.Apr 25 2023, 10:41 AM

Note that this may not work for Python 2.7, but since those are just examples that doesn't matter that much.

• gniibe changed the task status from Open to Testing.Apr 26 2023, 1:51 AM
• gniibe claimed this task.

@ikloecker Thanks for your comment. I put a comment in the commit.

• gniibe added a commit: rMd44a473e2716: doc: Fix Python example code..Apr 26 2023, 2:18 AM
• gniibe closed this task as Resolved.May 8 2024, 8:37 AM

Fixed in gpgme 1.21.0.