Skip to content
This repository has been archived by the owner. It is now read-only.

[security] apt_key module does not verify key fingerprints #5237

Closed
ypid opened this Issue Oct 12, 2016 · 13 comments

Comments

Projects
None yet
5 participants
@ypid
Copy link

ypid commented Oct 12, 2016

ISSUE TYPE
  • Bug Report
COMPONENT NAME

apt_key module

ANSIBLE VERSION

devel

CONFIGURATION

not relevant

OS / ENVIRONMENT

not relevant

SUMMARY

apt_key module does not verify key fingerprints and imports keys based on 16 digits long id.
This is a serious problem because it is easy to generate a OpenPGP key with a desired 16 digit key IDs.
Also, hkp is unauthenticated so it it becomes trivial to offer a wrong key to a victim.

The problem is this workaround.

STEPS TO REPRODUCE
- name: Try to import key without verifying that we import the correct key
  hosts: 'dev.example.com'
  become: True

  tasks:

    - apt_key:
        id: '00000000000000000000000047AE7F72479BC94B'
        keyserver: 'hkp://pool.sks-keyservers.net'
EXPECTED RESULTS

That key does not exist. No key is added.

ACTUAL RESULTS

The key id 479BC94B is imported.

EXAMPLE FIX

Enforce correct key fingerprint with a trick such as:

mkdir /tmp/gpg && chmod 700 /tmp/gpg
gpg --homedir /tmp/gpg --recv-keys --keyserver hkp://pool.sks-keyservers.net --recv 479BC94B
gpg --homedir /tmp/gpg --export DDA2C105C4B73A6649AD2BBD47AE7F72479BC94B | apt-key add -
@ansibot

This comment has been minimized.

Copy link

ansibot commented Oct 12, 2016

@jvantuyl ping, this issue is waiting for your response.
click here for bot help

@ypid

This comment has been minimized.

Copy link
Author

ypid commented Oct 17, 2016

Considering the other open issues against apt_key it seems the current maintainer is not maintaining the module actively. What is the fallback for such a case? apt_key is in ansible-modules-core and this is a serious vulnerability.

@jvantuyl

This comment has been minimized.

Copy link
Contributor

jvantuyl commented Oct 17, 2016

I'm maintaining plenty actively. I just don't see any code being submitted, so there's nothing to be done. Someone submits something that works, I'll be glad to vet and merge it. Maintainer != free coding whenever someone feels like it. ¯_(ツ)_/¯

@jvantuyl

This comment has been minimized.

Copy link
Contributor

jvantuyl commented Oct 17, 2016

needs_contributor

@ypid

This comment has been minimized.

Copy link
Author

ypid commented Oct 17, 2016

Maintainer != free coding whenever someone feels like it.

I get that. Thanks for your response. Does Ansible have any security policy for this? I had hoped so as this is a core module.

@jvantuyl Do the security implications of this bug appear to you?

@ypid

This comment has been minimized.

Copy link
Author

ypid commented Oct 17, 2016

I just don't see any code being submitted

You saw my example code above? For properly solving this, the gnupg package in Python might be better although that will add another dependency? Probably better to do this with run_command to avoid that …

@jvantuyl

This comment has been minimized.

Copy link
Contributor

jvantuyl commented Oct 18, 2016

@ypid I see your example code. That said, it's not a pull request. Unless you have something usable, I've got about zero time to develop something, let alone test it. I don't have the resources to investigate or test this (so I'm loathe to make any deep changes). If it was in the form of a pull request I'd only have to test. That was what I meant by submitting code.

I'm not part of the Ansible core team (or any Ansible team, really). I just submitted this module and they accepted it. They never took over maintenance. It wasn't even really discussed. I never even signed up to maintain anything. One day I started getting messages from the ansibot. I can only imagine that I ended up being the maintainer by default. I also suspect that I'm not the only one in this situation. All of that said, don't assume that I'm anything other than a guy reading this when he'd rather be playing MInecraft or 3D-printing something.

So, yeah, I figured they'd take this over themselves and have a security team, etc. I agree that this is pretty critical functionality. However, that doesn't seem to be the way they work. If I were to guess, short of sufficient Twitter shaming clout (or a contract and a decently sized check), good luck getting them to pay attention to it.

@ypid

This comment has been minimized.

Copy link
Author

ypid commented Oct 19, 2016

Thanks for your response. Lets hope that Ansible comes up a proper way to handle maintenance and not just try to make the original author the default maintainer as you said.

Thanks for writing the module!

@ypid

This comment has been minimized.

Copy link
Author

ypid commented Oct 21, 2016

I reported this vulnerability here: https://www.ansible.com/security Lets see how serious the security handling of core modules is in Ansible.

@wenottingham

This comment has been minimized.

Copy link
Member

wenottingham commented Oct 21, 2016

Does reverting #2794 fix it for you?

abadger added a commit to abadger/ansible-modules-core that referenced this issue Oct 21, 2016

Only change to short IDs for delete
If the user specifies long IDs, use them for all commands except for
deleting a key.  Need to use short IDs there because of an upstream
apt_key bug.  Fixed in apt_key 1.10 (fix is present in Ubuntu 16.04 but
not Ubuntu 14.0 or some Debians).

Fixes ansible#5237

Also:

* Some style cleanups
* Return a nice user error message if the key was not found on the
  keyserver
* Make file and keyring parameters type='path' so envars and tilde are
  expanded
@abadger

This comment has been minimized.

Copy link
Member

abadger commented Oct 21, 2016

@ypid could you see if #5353 is sufficient to fix this for you (and doesn't introduce any regressions)?

Also -- I've noticed that there aren't any tests for apt_key specifically. If you'd want to add a few to /test/integration/target/apt_key in the ansible/ansible repository that would help make sure we don't regress this or any other fix as other fixes are applied. You can ping me in #ansible-devel on irc.freenode.net if you do that and need someone to review and merge it.

@abadger abadger added this to the 2.2.0 milestone Oct 21, 2016

abadger added a commit that referenced this issue Oct 22, 2016

Only change to short IDs for delete (#5353)
* Only change to short IDs for delete

If the user specifies long IDs, use them for all commands except for
deleting a key.  Need to use short IDs there because of an upstream
apt_key bug.  Fixed in apt_key 1.10 (fix is present in Ubuntu 16.04 but
not Ubuntu 14.0 or some Debians).

Fixes #5237

* Check that apt-key really erased the key

When erasing a key, apt-key does not understand how to process subkeys.
This update explicitly checks that the key_id is no longer present and
throws an error if it is.  It also hints at subkeys being a possible
problem in the error message and the documentation.

Fixes #5119

* Fix apt_key check mode with long ids

apt-key can be given a key id longer than 16 chars to more accurately
define what key to download.  However, we can use a maximum of 16
chars to verify whether a key is installed or not.  So we need to use
different lengths for the id depending on what we're doing with it.

Fixes #2622

Also:

* Some style cleanups
* Use get_bin_path to find the path to apt-key and then use that when
  invoking apt-key
* Return a nice user error message if the key was not found on the
  keyserver
* Make file and keyring parameters type='path' so envars and tilde are
  expanded

abadger added a commit that referenced this issue Oct 22, 2016

Only change to short IDs for delete (#5353)
* Only change to short IDs for delete

If the user specifies long IDs, use them for all commands except for
deleting a key.  Need to use short IDs there because of an upstream
apt_key bug.  Fixed in apt_key 1.10 (fix is present in Ubuntu 16.04 but
not Ubuntu 14.0 or some Debians).

Fixes #5237

* Check that apt-key really erased the key

When erasing a key, apt-key does not understand how to process subkeys.
This update explicitly checks that the key_id is no longer present and
throws an error if it is.  It also hints at subkeys being a possible
problem in the error message and the documentation.

Fixes #5119

* Fix apt_key check mode with long ids

apt-key can be given a key id longer than 16 chars to more accurately
define what key to download.  However, we can use a maximum of 16
chars to verify whether a key is installed or not.  So we need to use
different lengths for the id depending on what we're doing with it.

Fixes #2622

Also:

* Some style cleanups
* Use get_bin_path to find the path to apt-key and then use that when
  invoking apt-key
* Return a nice user error message if the key was not found on the
  keyserver
* Make file and keyring parameters type='path' so envars and tilde are
  expanded
@ypid

This comment has been minimized.

Copy link
Author

ypid commented Oct 22, 2016

#5353 does not fix it, please reopen.

@ypid

This comment has been minimized.

Copy link
Author

ypid commented Oct 22, 2016

Fixed by: #5357
Thanks to @abadger

@jvantuyl you might want to review the changes as well.

karmab added a commit to karmab/ansible-modules-core that referenced this issue Oct 24, 2016

Only change to short IDs for delete (ansible#5353)
* Only change to short IDs for delete

If the user specifies long IDs, use them for all commands except for
deleting a key.  Need to use short IDs there because of an upstream
apt_key bug.  Fixed in apt_key 1.10 (fix is present in Ubuntu 16.04 but
not Ubuntu 14.0 or some Debians).

Fixes ansible#5237

* Check that apt-key really erased the key

When erasing a key, apt-key does not understand how to process subkeys.
This update explicitly checks that the key_id is no longer present and
throws an error if it is.  It also hints at subkeys being a possible
problem in the error message and the documentation.

Fixes ansible#5119

* Fix apt_key check mode with long ids

apt-key can be given a key id longer than 16 chars to more accurately
define what key to download.  However, we can use a maximum of 16
chars to verify whether a key is installed or not.  So we need to use
different lengths for the id depending on what we're doing with it.

Fixes ansible#2622

Also:

* Some style cleanups
* Use get_bin_path to find the path to apt-key and then use that when
  invoking apt-key
* Return a nice user error message if the key was not found on the
  keyserver
* Make file and keyring parameters type='path' so envars and tilde are
  expanded

abadger added a commit that referenced this issue Oct 24, 2016

Backport the newest revision of apt_key from 2.2
apt_key has some handling of key ids that can be security concerns.

See this issue:
#5237

and this pair of PRs:
#5353
#5357

abadger added a commit that referenced this issue Oct 24, 2016

Backport 2.2 version of apt_key to 1.9 to fix security issues
apt_key has some handling of key ids that can be security concerns.

See this issue:
#5237

and this pair of PRs:
#5353
#5357

bdowling added a commit to bdowling/ansible-modules-core that referenced this issue Oct 28, 2016

Only change to short IDs for delete (ansible#5353)
* Only change to short IDs for delete

If the user specifies long IDs, use them for all commands except for
deleting a key.  Need to use short IDs there because of an upstream
apt_key bug.  Fixed in apt_key 1.10 (fix is present in Ubuntu 16.04 but
not Ubuntu 14.0 or some Debians).

Fixes ansible#5237

* Check that apt-key really erased the key

When erasing a key, apt-key does not understand how to process subkeys.
This update explicitly checks that the key_id is no longer present and
throws an error if it is.  It also hints at subkeys being a possible
problem in the error message and the documentation.

Fixes ansible#5119

* Fix apt_key check mode with long ids

apt-key can be given a key id longer than 16 chars to more accurately
define what key to download.  However, we can use a maximum of 16
chars to verify whether a key is installed or not.  So we need to use
different lengths for the id depending on what we're doing with it.

Fixes ansible#2622

Also:

* Some style cleanups
* Use get_bin_path to find the path to apt-key and then use that when
  invoking apt-key
* Return a nice user error message if the key was not found on the
  keyserver
* Make file and keyring parameters type='path' so envars and tilde are
  expanded
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.