build_ssl_config_file sets permissions on wrong file

[John Dennis]

BaseCertificateConfigure.build_ssl_config_file() calls

Both the index and serial file have their permissions set like this:

self._set_permissions(self.ssl_config_file_name, PRIV_PERMS)

This looks like a cut-n-paste bug because instead of self.ssl_config_file_name it should have been index_file_name and serial_file_name respectively.

[Jeremy Stanley (fungi)]

Any input on what the security impact of this issue could be? Does it represent an exploitable vulnerability, and if so under what circumstances and to what effect?

[John Dennis (jdennis-a)]

It's hard to say the extent of the vulnerability. The index and serial file control how OpenSSL signs certificates when operating as a CA. These files should be "private", only the owner can read and write them. When we fail to set the correct permissions on those files the resulting permission depends on the umask (unfortunately I don't know what the umask is when this executes). The issue is someone could modify either the index or serial file. What could that do? The most likely would be to cause duplicate certificates to be generated by the CA, that's never supposed to happen. Some crypto libraries detect duplicates but others do not. The duplicates could cause SSL and token validation to fail. I don't think it would cause a breach. I think the main damage would be to cause the system to stop working. But that's just speculation. What is worrisome is this could result in an attack on the CA (Certificate Authority) and the CA is an absolutely critical component of the overall security. Just because I can't think of an attack on the CA via modifying these files doesn't mean one doesn't exist.

The other thing I just noticed is the permission that would be applied if the correct file names had been used is 0700. I don't think these files should have execute permission, which is another thing which should be fixed. Executable files are always a concern, especially of someone can modify and execute them other than the intended owner. If someone did do this another consequence is the CA would likely stop working (I have not tried to run OpenSSL as a CA with bogus files, but my recollection is it just fails).

[Adam Young (ayoung)]

Lets get the permissions fixed in the same patch, then. The above fix looks good as far as it goes.

[Thierry Carrez (ttx)]

I think this is Low vulnerability, but it still warrants an advisory in my book.

[Jeremy Stanley (fungi)]

Agreed--loose permissions on sensitive files are always a risk of some kind. I'm unconvinced that gratuitous read permissions on the CA's serial and index files are really an issue (on a plaintext CA private key this would be another matter entirely).

Additional write permissions on those are of course a concern, if that can result from this bug... are we setting the process umask to something which would permit these files to be created with o+w? This would absolutely be unusual as a default on any platforms I've used, though I suppose g+w is possible (if considerably less exploitable under typical use cases).

I'm mainly just trying to make sure that when we draft an impact description for the advisory, we appropriately characterize the situations under which this bug could be exploitable and what risks it actually presents to an environment.

[Thierry Carrez (ttx)]

Oh, misread that bug. Agree that merely giving read access to index/serial files is not really significant from a security perspective, and any system with a default umask granting write perms is likely to suffer from other issues. I'd actually not issue an advisory about this one.

[John Dennis (jdennis-a)]

Re comment #7, it's not the granting of read permission which is the issue, it's the write permission. Being able to read the serial or index file is not a security issue. However ...

I've done a lot more research into what happens with these files.

When the openssl ca command is run it updates the serial and index file, plus makes backups. In addition it writes other files into the ca directory. These new files get the ownership and group of the process running the command. Each appears to be given mode 0666 which is masked off with the umask. Normal users typically have a umask of 02, and root has a umask of 022. Because of the way the ca command backs up the serial and index file (renaming the previous file and writing a new version) whatever permissions and ownership we give those files are lost when the command runs (although the first backup retains it).

Thus the issue of ownership and permissions on the serial and index files has less to do with how we initially create the files and more with who runs the command. The command is run via keystone-manage ssl_setup | pki_setup.

keystone-manage ssl_setup | pki_setup have command line args for the keystone-user and keystone-group (symbolic user and group names). Until recently these were not required. In commit f0a9affca on 5/18/203 on master the user & group command line args became mandatory if the command was executed as root.

Devstack's stack.sh runs keystone-manage but does not specify a user or group (even if run as root).

The Red Hat install guide recommends creating a keystone system user:group and then running keystone-manage passing the keystone user:group. I presume the other install guides do the same.

Internally the keystone user:group is only used to chown files and directories if it's running as root.

However there is a bug in openssl.py, it passes the symbolic name of the user and group to chmod instead of the uid and gid respectively. This throws an exception and the setup process terminates early. I will file a separate bug on this.

The ownership and permissions of the CA files modified by the openssl ca depend on who runs the keystone-mange command.

You can run the keystone-manage command more than once. It will reset the ownship and permissions on many of the files (how depends on whether it was run as root). However it omits resetting the ownership and permission on the CA key and CA cert. Thus if you try to fix unintended ownership/permissions by running the keystone-manage command again one of the most critical files, the CA key will not be updated.

For reasons I don't understand only the group is set on directories, not the owner.

I would think root:keystone would be a better ownership than the recommended keystone:keystone.

The CA key and signing key are given 0400 but anyone with write access to the directory can rename or delete the keys. The signing key is put in the directory ssl/pprivate with permission 0700, the CA key is put in the ssl/certs directory which has permission 0755. Why isn't the CA key also in the ssl/private directory instead of the general certs directory? Certs and keys are not the same thing, certs are public while keys should be...

[Jeremy Stanley (fungi)]

Agreed--lots of the sensitive file handling here seems suboptimal and could stand to be fixed. I'm just trying to get a concise statement of what risks this presents (for inclusion in any advisory we might decide to release), and example attack vectors we can cite which are not completely contrived.

[Thierry Carrez (ttx)]

At first glance, the only way I could see these flaws in permissions handling being exploited is if they were made writeable to some other user, and no serious umask should allow that.

But this should definitely be tightened with permissions set across the board, rather than let the current user running keystone-manage mess them up... I think we can open this one up and patch it publicly, no need for an embargoed advisory ?

[Jeremy Stanley (fungi)]

I'm in favor of development in the open for this one as well, unless the reporter or keystone devs can suggest specific attack vectors we're overlooking.

[Jeremy Stanley (fungi)]

It's been a few days. Unless there are any objections before 1500 UTC tomorrow, I'll open this up as a public bug in Keystone and mark the security advisory task invalid.

[Dolph Mathews (dolph)]

Happy to see this opened.

I believe that pki_setup and ssl_setup were originally implemented primarily for developers/devstack to consume, which perhaps explains why some of these issues exist. The assumption is that a "real" deployment will instead be issued certs by an existing external authority, and be provided with that configuration through an outside deployment process.

That said, it is certainly realistic for smaller deployments to rely on these tools but I don't see any reason not to make these improvements publicly.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/43766

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/43767

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/43768

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/43769

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/43766
Committed: http://github.com/openstack/keystone/commit/56f6401d1e9be4d7b0cd6f1a8af7448a44d5d02e
Submitter: Jenkins
Branch: master

commit 56f6401d1e9be4d7b0cd6f1a8af7448a44d5d02e
Author: John Dennis <email address hidden>
Date: Mon Jul 29 16:09:10 2013 -0400

    Use correct filename for index & serial file when setting permissions

    There was a cut-n-paste bug where self._set_permissions() was called
    with the exact same filename, self.ssl_config_file_name, instead of
    the index and serial filenames. This patch uses the index and serial
    filenames as was the original intent.

    Change-Id: I571c766ac746bbbc1bedfdf1ff2b1b86363a0af0
    Fixes: bug #1206254

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/43767
Committed: http://github.com/openstack/keystone/commit/3cfe102475e11402f61ccc54d87f542bc5d48b9b
Submitter: Jenkins
Branch: master

commit 3cfe102475e11402f61ccc54d87f542bc5d48b9b
Author: John Dennis <email address hidden>
Date: Mon Aug 12 10:39:32 2013 -0400

    Modify default file/directory permissions

    * Remove execute permission on regular files (PRIV_PERMS).
    * Create file mode constants for public/private directory/file.
    * Grant read access to group members for files/directores marked private.

    Change-Id: I92563a125e6ac93762db5fda65412f9a68ef35e3
    Fixes: bug #1206254

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/43768
Committed: http://github.com/openstack/keystone/commit/4e6cf36697eeb02a420101e5e0be67072a5c9583
Submitter: Jenkins
Branch: master

commit 4e6cf36697eeb02a420101e5e0be67072a5c9583
Author: John Dennis <email address hidden>
Date: Mon Aug 12 14:53:52 2013 -0400

    Utilities to create directores, set ownership & permissions

    Add the get_unix_user(), get_unix_group(), set_permissions(),
    make_dirs() utilities to keystone/common/utils.py. These functions are
    much more robust and flexible. Ownership can be specifed either with
    symbolic names or explicit id's. Any value you do not want to override
    can be passed as None and it will be skipped.

    The BaseCertificateConfigure class in openssl.py had it's own code to
    create directories, set ownership, set permissions, etc. which was
    used for setting up certificates, key files, etc. Properly handling
    file system objects should not be a property of openssl, rather these
    should be general utilities which can be used in a variety of
    locations. Plus the openssl versions were not fully general and had
    some pecular limitations. The new utilies will be used in a subsequent
    patch which also needs this functionality but isn't in openssl code.

    Previously the openssl code was inconsistent as to whether it set the
    ownership and permissions, sometimes ownership and permission was set
    only if the file/dir did not exist, other times the ownership and
    permission were set irregardless of prior existence. The modified code
    now always sets ownerhip and permissions (if provided).

    The prior code assumed an id (uid/gid) was being passed for ownership,
    but this was not documented. The code appears to take a symbolic
    name. The cli code converted symbolic names passed on the command line
    to id's before instantiating a class which accepts ownership
    information. If you instantiated one of the class without first
    converting a name to an id it would throw an exception. The new
    utilities accept either symbolic names or id's and since these
    utilities are now used internally you can pass either a name or id and
    one is no longer dependent upon special handling inside cli argument
    parsing code to get correct behavior with these classes.

    There were several places with code like this:

        os.chown(file_name, self.use_keystone_user or -1,
                 self.use_keystone_group or -1)

    The intent would appear to be to avoid setting the uid or gid if the
    attribute had a value of None. Except this fails completely if the
    uid or gid belongs to root because those id's are zero, thus the code
    would never work with root id's. The new utilities explicitly test for
    a value of None and do not rely on the weak "truth test".

    Note: these new utilities are intended to be moved into oslo-incubator
    in the near future.

    Fixes: bug #1206254

    Change-Id: I34b811513ebfd48d91600051724949c0a96c0734

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/43769
Committed: http://github.com/openstack/keystone/commit/db19ce755b4d18be4d880bc958862c0cda71b83d
Submitter: Jenkins
Branch: master

commit db19ce755b4d18be4d880bc958862c0cda71b83d
Author: John Dennis <email address hidden>
Date: Mon Aug 26 15:13:52 2013 -0400

    Move CA key from certs directory to private directory

    Unlike certs which are public keys are private. The CA key file was
    improperly located in the certs directory with the public certs. It's
    vital to protect the CA key, it's one of the most security sensitive
    files in the system. We already had a private directory for keys,
    /etc/keystone/ssl/private where the signing key is located. This fix
    moves the CA key into /etc/keystone/ssl/private from
    /etc/keystone/ssl/certs.

    We also update all relevant files to note the change. During so it was
    observered etc/keystone.conf.sample used the wrong parent directory
    /etc/keystone/pki, the directory /etc/keystone/ssl is the directory in
    actual use.

    Fixes: bug #1206254

    Change-Id: I014e9f79093a6aa59cd5b3bb6cefa4c6dced67a6