Skip to content

Commit e0fdf3a

Browse files
author
root
committed
Corrections in Python wrappers
Management of file descriptors was leaking. The cryptfd was closed by the tlspool_starttls() call or TLS Pool but also by Python, for instance when garbage collecting the cryptfd. This allowed closing the same socket twice or, more accurately put, closing of the same file descriptor number. An intermediate process might have opened another stream with the same number, and seen it closed. Yet an other process might have opened it once again and receive spurious information from the stashed file descriptor in, say, the syslog() API or Python sockets.
1 parent 44b08fc commit e0fdf3a

File tree

4 files changed

+67
-11
lines changed

4 files changed

+67
-11
lines changed

‎lib/python/Makefile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ _tlspool.so: tlspool.c
4141
python setup.py build_ext --inplace
4242

4343
tlspool.c: py-tlspool.i ../swig-tlspool.i
44-
grep -h '^#define \(PIOF_\|PIOC_\|TLSPOOL_\)' ../../include/tlspool/commands.h ../../include/tlspool/starttls.h | grep -v 'PATH.*\\' | sort | uniq > defines.h
44+
grep -h '^#define \(PIOF_\|PIOC_\|TLSPOOL_\)' ../../include/tlspool/commands.h ../../include/tlspool/starttls.h | grep -v 'PATH.*\\' | grep -v 'windows' | sort | uniq > defines.h
4545
$(SWIG) -python -threads -o "$@" "$<"
4646

4747
.c.o:

‎lib/python/py-tlspool.i

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,19 @@ class Connection:
179179
access afterwards. The variables that are required at
180180
this point are service and, already obliged when making
181181
a new instance, cryptfd.
182+
183+
Quoting the library documentation on file handles and
184+
who should close them:
185+
186+
The cryptfd handle supplies the TLS connection that is
187+
assumed to have been setup. When the function ends,
188+
either in success or failure, this handle will no longer
189+
be available to the caller; the responsibility of closing
190+
it is passed on to the function and/or the TLS Pool.
191+
192+
And on plainfd: The file handle returned [in privdata],
193+
if it is >= 0, should be closed by the caller, both in
194+
case of success and failure.
182195
"""
183196
assert (self.cryptsk is not None)
184197
assert (self.cryptfd >= 0)
@@ -198,12 +211,18 @@ class Connection:
198211
socktp = socket.SOCK_STREAM
199212
plainsockptr = socket_data ()
200213
plainsockptr.unix_socket = self.plainfd
214+
# Clone cryptfd so _starttls() and Python can each close it
215+
unix_cryptfd = os.dup (self.cryptfd)
216+
if unix_cryptfd < 0:
217+
_tlspool.raise_errno ()
218+
# We close cryptsk now and it will not repeat during garbage collection
219+
self.cryptsk.close ()
220+
self.cryptsk = None
221+
self.cryptfd = -1
201222
# Provide None for the callback function, SWIG won't support it
202223
# We might at some point desire a library of C routine options?
203-
rv = _starttls (self.cryptfd, self.tlsdata, plainsockptr, None)
224+
rv = _starttls (unix_cryptfd, self.tlsdata, plainsockptr, None)
204225
self.plainfd = -1
205-
self.cryptfd = -1
206-
self.cryptsk = None
207226
if rv < 0:
208227
_tlspool.raise_errno ()
209228
if self.plainsk is None:
@@ -347,17 +366,24 @@ class SecurityMixIn:
347366
socktp = socket.SOCK_STREAM
348367
plainsockptr = socket_data ()
349368
plainsockptr.unix_socket = -1
350-
rv = _starttls (sox.fileno (), self.tlsdata, plainsockptr, None)
369+
unix_sox = os.dup (sox.fileno ())
370+
if unix_sox < 0:
371+
_tlspool.raise_errno ()
372+
sox.close ()
373+
rv = _starttls (unix_sox, self.tlsdata, plainsockptr, None)
351374
if rv < 0:
352375
_tlspool.raise_errno ()
353376
assert (plainsockptr.unix_socket >= 0)
354377
sox2 = socket.fromfd (plainsockptr.unix_socket, af, socktp)
378+
# Now, since socket.fromfd() used os.dup(), we close the original
379+
os.close (plainsockptr.unix_socket)
380+
plainsockptr.unix_socket = -1
355381
if type (self.request) == tuple:
382+
#TODO# This is not permitted, editing a tuple
356383
self.request [1] = sox2
357384
else:
358385
self.request = sox2
359386
self.handle_secure ()
360-
sox2.close ()
361387
362388
def startssh (self):
363389
raise NotImplementedError ("Python wrapper does not implement STARTSSH")

‎lib/python/tlspool.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6390,6 +6390,7 @@ SWIG_init(void) {
63906390
SWIG_Python_SetConstant(d, "PIOF_LIDENTRY_SKIP_NOTROOT",SWIG_From_int((int)(0x00000088)));
63916391
SWIG_Python_SetConstant(d, "PIOF_LIDENTRY_SKIP_USER",SWIG_From_int((int)(0x00000081)));
63926392
SWIG_Python_SetConstant(d, "PIOF_LIDENTRY_WANT_DBENTRY",SWIG_From_int((int)(0x00000100)));
6393+
SWIG_Python_SetConstant(d, "PIOF_STARTTLS_BOTHROLES_PEER",SWIG_From_int((int)(0x0000000f)));
63936394
SWIG_Python_SetConstant(d, "PIOF_STARTTLS_DETACH",SWIG_From_int((int)(0x00002000)));
63946395
SWIG_Python_SetConstant(d, "PIOF_STARTTLS_DOMAIN_REPRESENTS_USER",SWIG_From_int((int)(0x00008000)));
63956396
SWIG_Python_SetConstant(d, "PIOF_STARTTLS_DTLS",SWIG_From_int((int)(0x00000100)));
@@ -6408,6 +6409,7 @@ SWIG_init(void) {
64086409
SWIG_Python_SetConstant(d, "PIOF_STARTTLS_REQUEST_REMOTEID",SWIG_From_int((int)(0x00000800)));
64096410
SWIG_Python_SetConstant(d, "PIOF_STARTTLS_WITHOUT_SNI",SWIG_From_int((int)(0x00000200)));
64106411
SWIG_Python_SetConstant(d, "TLSPOOL_CTLKEYLEN",SWIG_From_int((int)(16)));
6412+
SWIG_Python_SetConstant(d, "TLSPOOL_DEFAULT_CONFIG_PATH",SWIG_FromCharPtr("/etc/tlspool.conf"));
64116413
SWIG_Python_SetConstant(d, "TLSPOOL_DEFAULT_PIDFILE_PATH",SWIG_FromCharPtr("/var/run/tlspool.pid"));
64126414
SWIG_Python_SetConstant(d, "TLSPOOL_DEFAULT_SOCKET_PATH",SWIG_FromCharPtr("/var/run/tlspool.sock"));
64136415
SWIG_Python_SetConstant(d, "TLSPOOL_IDENTITY_V2",SWIG_FromCharPtr("20151111api@tlspool.arpa2.net"));

‎lib/python/tlspool.py

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -444,6 +444,19 @@ def starttls (self):
444444
access afterwards. The variables that are required at
445445
this point are service and, already obliged when making
446446
a new instance, cryptfd.
447+
448+
Quoting the library documentation on file handles and
449+
who should close them:
450+
451+
The cryptfd handle supplies the TLS connection that is
452+
assumed to have been setup. When the function ends,
453+
either in success or failure, this handle will no longer
454+
be available to the caller; the responsibility of closing
455+
it is passed on to the function and/or the TLS Pool.
456+
457+
And on plainfd: The file handle returned [in privdata],
458+
if it is >= 0, should be closed by the caller, both in
459+
case of success and failure.
447460
"""
448461
assert (self.cryptsk is not None)
449462
assert (self.cryptfd >= 0)
@@ -463,12 +476,18 @@ def starttls (self):
463476
socktp = socket.SOCK_STREAM
464477
plainsockptr = socket_data ()
465478
plainsockptr.unix_socket = self.plainfd
479+
# Clone cryptfd so _starttls() and Python can each close it
480+
unix_cryptfd = os.dup (self.cryptfd)
481+
if unix_cryptfd < 0:
482+
_tlspool.raise_errno ()
483+
# We close cryptsk now and it will not repeat during garbage collection
484+
self.cryptsk.close ()
485+
self.cryptsk = None
486+
self.cryptfd = -1
466487
# Provide None for the callback function, SWIG won't support it
467488
# We might at some point desire a library of C routine options?
468-
rv = _starttls (self.cryptfd, self.tlsdata, plainsockptr, None)
489+
rv = _starttls (unix_cryptfd, self.tlsdata, plainsockptr, None)
469490
self.plainfd = -1
470-
self.cryptfd = -1
471-
self.cryptsk = None
472491
if rv < 0:
473492
_tlspool.raise_errno ()
474493
if self.plainsk is None:
@@ -612,17 +631,24 @@ def starttls (self):
612631
socktp = socket.SOCK_STREAM
613632
plainsockptr = socket_data ()
614633
plainsockptr.unix_socket = -1
615-
rv = _starttls (sox.fileno (), self.tlsdata, plainsockptr, None)
634+
unix_sox = os.dup (sox.fileno ())
635+
if unix_sox < 0:
636+
_tlspool.raise_errno ()
637+
sox.close ()
638+
rv = _starttls (unix_sox, self.tlsdata, plainsockptr, None)
616639
if rv < 0:
617640
_tlspool.raise_errno ()
618641
assert (plainsockptr.unix_socket >= 0)
619642
sox2 = socket.fromfd (plainsockptr.unix_socket, af, socktp)
643+
# Now, since socket.fromfd() used os.dup(), we close the original
644+
os.close (plainsockptr.unix_socket)
645+
plainsockptr.unix_socket = -1
620646
if type (self.request) == tuple:
647+
#TODO# This is not permitted, editing a tuple
621648
self.request [1] = sox2
622649
else:
623650
self.request = sox2
624651
self.handle_secure ()
625-
sox2.close ()
626652

627653
def startssh (self):
628654
raise NotImplementedError ("Python wrapper does not implement STARTSSH")
@@ -694,6 +720,7 @@ def handle_secure (self):
694720
PIOF_LIDENTRY_SKIP_NOTROOT = _tlspool.PIOF_LIDENTRY_SKIP_NOTROOT
695721
PIOF_LIDENTRY_SKIP_USER = _tlspool.PIOF_LIDENTRY_SKIP_USER
696722
PIOF_LIDENTRY_WANT_DBENTRY = _tlspool.PIOF_LIDENTRY_WANT_DBENTRY
723+
PIOF_STARTTLS_BOTHROLES_PEER = _tlspool.PIOF_STARTTLS_BOTHROLES_PEER
697724
PIOF_STARTTLS_DETACH = _tlspool.PIOF_STARTTLS_DETACH
698725
PIOF_STARTTLS_DOMAIN_REPRESENTS_USER = _tlspool.PIOF_STARTTLS_DOMAIN_REPRESENTS_USER
699726
PIOF_STARTTLS_DTLS = _tlspool.PIOF_STARTTLS_DTLS
@@ -712,6 +739,7 @@ def handle_secure (self):
712739
PIOF_STARTTLS_REQUEST_REMOTEID = _tlspool.PIOF_STARTTLS_REQUEST_REMOTEID
713740
PIOF_STARTTLS_WITHOUT_SNI = _tlspool.PIOF_STARTTLS_WITHOUT_SNI
714741
TLSPOOL_CTLKEYLEN = _tlspool.TLSPOOL_CTLKEYLEN
742+
TLSPOOL_DEFAULT_CONFIG_PATH = _tlspool.TLSPOOL_DEFAULT_CONFIG_PATH
715743
TLSPOOL_DEFAULT_PIDFILE_PATH = _tlspool.TLSPOOL_DEFAULT_PIDFILE_PATH
716744
TLSPOOL_DEFAULT_SOCKET_PATH = _tlspool.TLSPOOL_DEFAULT_SOCKET_PATH
717745
TLSPOOL_IDENTITY_V2 = _tlspool.TLSPOOL_IDENTITY_V2

0 commit comments

Comments
 (0)