Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ssl.wrap_socket() is incompatible with servers that drop privileges, due to keyfile requirement #48073

Closed
forest mannequin opened this issue Sep 9, 2008 · 13 comments
Closed
Labels
stdlib Python modules in the Lib dir type-security A security issue

Comments

@forest
Copy link
Mannequin

forest mannequin commented Sep 9, 2008

BPO 3823
Nosy @jcea, @pitrou, @giampaolo, @anacrolix

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2010-06-29.16:18:08.137>
created_at = <Date 2008-09-09.18:25:18.562>
labels = ['type-security', 'library']
title = 'ssl.wrap_socket() is incompatible with servers that drop privileges, due to keyfile requirement'
updated_at = <Date 2010-06-29.16:18:08.136>
user = 'https://bugs.python.org/forest'

bugs.python.org fields:

activity = <Date 2010-06-29.16:18:08.136>
actor = 'pitrou'
assignee = 'janssen'
closed = True
closed_date = <Date 2010-06-29.16:18:08.137>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2008-09-09.18:25:18.562>
creator = 'forest'
dependencies = []
files = []
hgrepos = []
issue_num = 3823
keywords = []
message_count = 13.0
messages = ['72891', '72905', '72955', '72976', '72977', '72982', '72983', '72984', '72986', '103242', '104938', '108901', '108916']
nosy_count = 7.0
nosy_names = ['jcea', 'janssen', 'pitrou', 'forest', 'giampaolo.rodola', 'hodgestar', 'anacrolix']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue3823'
versions = ['Python 3.2']

@forest
Copy link
Mannequin Author

forest mannequin commented Sep 9, 2008

SSLSocket() and ssl.wrap_socket() accept private keys only as paths to
their location on the file system. This means that a server can only
support SSL if it has read access to its private key file at the time
when client connections arrive, which is a problem for servers that bind
to their socket and drop privileges as soon as they start up.

In other words, the new ssl module's API prevents its use in servers
that follow best practices that are prevalent in the unix world.

If SSLSocket() and ssl.wrap_socket() were updated to accept private keys
as strings (or open file-like objects or some such), the problem would
go away. Moreover, it would allow a program to keep private keys cached
in memory, which might slightly improve performance over reading them
from the file system on every new connection.

@forest forest mannequin added stdlib Python modules in the Lib dir type-security A security issue labels Sep 9, 2008
@forest forest mannequin changed the title ssl.wrap_socket() is incompatible with unprivileged servers, due to keyfile requirement ssl.wrap_socket() is incompatible with servers that drop privileges, due to keyfile requirement Sep 9, 2008
@forest
Copy link
Mannequin Author

forest mannequin commented Sep 9, 2008

This problem also exists in the add-on ssl module for python < 2.6:
http://pypi.python.org/pypi/ssl/

@hodgestar
Copy link
Mannequin

hodgestar mannequin commented Sep 10, 2008

I've dug around in the code a bit and the keyfile, certfile and ca_certs
filename arguments to SSLSocket.__init__ are passed down into
newPySSLObject in _ssl.c and from there directly to SSL_CTX_* function
from OpenSSL so making these arguments allow file-like objects is going
to be non-trivial.

The options I see are:

  • Write the file-like objects out to named temporary files and pass
    those through to OpenSSL (seems like a nasty hack and prone to all sorts
    of problems).

  • Change the which OpenSSL functions are used to setup the certificate
    (I definitely don't think this could go into 2.6 or 3.0 at this stage;
    also see analysis of current OpenSSL usage below for more difficulties)

  • Add an SSL CTX wrapper object and allow that to be passed down to
    newPySSLObject instead of the filenames. Then the CTX object could be
    created before dropping privileges (I think this is probably also too
    big a change to be considered for 2.6 or 3.0 at this point, but it's
    what looks best to me at the moment).

The current situation in _ssl.c:

  • keyfile is loaded using SSL_CTX_use_PrivateKey_file(...) which loads
    the first certificate from keyfile into ctx. We could replace this with
    SSL_CTX_use_PrivateKey(SSL_CTX *ctx, EVP_PKEY *pkey) but we'd have to
    load the key ourselves and make sure to follow what OpenSSL does to
    maintain compatibility.

  • certfile is loaded with SSL_CTX_use_certificate_chain_file(...) which
    reads in all the certificates from certfile into ctx. We could read the
    certificates in ourselves and them load them one by one using
    SSL_CTX_use_certificate(...) and then SSL_CTX_add_extra_chain_cert(...).

  • ca_certs is loaded using SSL_CTX_load_verify_locations(...). As fasr
    as I can see there is no convenient replacement function for this in
    OpenSSL. SSL_CTX_set_client_CA_list(...) will load a list of certificate
    names but doesn't load the certificates themselves (so verification
    won't be done with them) and SSL_CTX_add_client_CA(...) has the same
    issue.

One could use SSL_CTX_set_cert_store(...) to register callbacks (and
then presumably one can do whatever one wants and can get around the
ca_certs issue) but the man page for SSL_CTX_set_cert_store has the
rather disheartening "Currently no detailed documentation on how to use
the X509_STORE object is available."

All this comes with the proviso that I just started digging into the
OpenSSL manpages today so I'm a long way from being an expert. :)

I can probably find time to create a patch with tests once we have a
clear direction to go in.

@forest: If you have an details on how non-Python servers go about
loading certificates and then dropping privileges using OpenSSL, that
would be extremely useful.

@janssen
Copy link
Mannequin

janssen mannequin commented Sep 10, 2008

Thanks, Simon.

I remember digging through all this last year, and finally deciding
to keep things simple and use the strategy the current codebase uses.

It almost sounds like we'd need to create Key and Certificate objects
in the _ssl module, which could be used to load up all the keys and/or
certificates the server uses, before it changes UID (and presumbably
loses access to the files the data is kept in). I was resisting
going down that path; there's a lot of complexity there I want to avoid.
But much of the mechanism of a Certificate object is already there;
perhaps adding an opaque Key object wouldn't be too bad.

@exarkun
Copy link
Mannequin

exarkun mannequin commented Sep 10, 2008

Both M2Crypto and pyOpenSSL expose certificate and key objects and have
seen lots of real-world use. Following their lead would be sensible.

@janssen
Copy link
Mannequin

janssen mannequin commented Sep 10, 2008

Sure -- but the point of the SSL module was to do something
small and compatible with the existing socket.ssl module; I really don't
want to get into a full-fledged Python interface to OpenSSL. Perhaps
incremental progress would be OK...

@exarkun
Copy link
Mannequin

exarkun mannequin commented Sep 10, 2008

I'm just suggesting that if the ssl module *is* going to gain
certificate and key objects, looking at existing APIs and perhaps
emulating them, to the extent that they provide functionality which the
ssl module is also going to provide, is a good idea. Perhaps we could
actually get the various APIs to begin to converge (where they overlap)
instead of producing further divergence.

@forest
Copy link
Mannequin Author

forest mannequin commented Sep 10, 2008

Simon: I wish I could offer guidance here, but I'm afraid that I too am
reading some of these openssl man pages for the first time.

I agree that writing to a temporary file would be bad.

Accepting file-like objects from python code would be nice, but isn't
really necessary. Simple strings would suffice. It's easy enough for
application code to read the strings from the appropriate files. Of
course, the ssl module (or an underlying library) would need a way to
determine the data format within the strings. Unfortunately, I didn't
notice an equivalent of SSL_CTX_use_PrivateKey_file() that accepts a
file's contents instead of its path. SSL_CTX_use_RSAPrivateKey() looks
like the next best thing.

much of the mechanism of a Certificate object is already there;
perhaps adding an opaque Key object wouldn't be too bad."

That's encouraging.

From the openssl docs I've read so far, it looks like certificates and
keys have several formats and use patterns. That seems to me like
another argument in favor of supporting separate Certificate and Key
objects, even if they're only minimally implemented to begin with. In
other words, I don't imagine the presence of Certificate and Key objects
would muck up the ssl module's api.

In order to keep compatibility with socket.ssl, perhaps any new
certificate and key objects should be accepted as alternatives to the
existing certfile and keyfile args, but the latter should still be allowed?

@exarkun
Copy link
Mannequin

exarkun mannequin commented Sep 10, 2008

You can load a private key from a string by creating a memory BIO and
using PEM_read_bio_PrivateKey or d2i_PrivateKey_bio.

This is how pyOpenSSL implements its load_privatekey API. You can see
the code here:

http://bazaar.launchpad.net/~exarkun/pyopenssl/trunk/annotate/70?file_id=crypto.c-20080219014912-qyb7kjf196jhzlyv-128

@giampaolo
Copy link
Contributor

Any update about this issue?
This should be marked as high priority, imho.

@pitrou
Copy link
Member

pitrou commented May 4, 2010

There's a patch in bpo-8550 to expose SSL contexts as first-class objects. It allows you to create first your context object(s) and load certificates, then drop privileges, then create sockets using this/these contexts.
In any case, resolution can't happen before 3.2. 2.7 is in feature freeze now.

@anacrolix
Copy link
Mannequin

anacrolix mannequin commented Jun 29, 2010

I take it this is out for 2.7, will it be in 3.2?

@pitrou
Copy link
Member

pitrou commented Jun 29, 2010

I take it this is out for 2.7, will it be in 3.2?

The patch for bpo-8550 has been committed. The SSLContext API should solve this issue:
http://docs.python.org/dev/py3k/library/ssl.html#ssl.SSLContext.wrap_socket

If it doesn't, please explain why.

@pitrou pitrou closed this as completed Jun 29, 2010
@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-security A security issue
Projects
None yet
Development

No branches or pull requests

2 participants