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

Fix signature buffer size for RSA keys #428

Merged
merged 1 commit into from
Mar 2, 2016

Conversation

cmurphy
Copy link
Contributor

@cmurphy cmurphy commented Mar 2, 2016

When using the pyOpenSSL crypto module to sign data using a large key,
e.g. 8192 bit, a memory allocation error occurs. A test case to show
this, which comes from OpenStack Glance, is:

  $ openssl genrsa -out server.key 8192
  $ ...
  $ cat test.py
  from OpenSSL import crypto
  import uuid
  key_file = 'server.key'
  with open(key_file, 'r') as keyfile:
      key_str = keyfile.read()
  key = crypto.load_privatekey(crypto.FILETYPE_PEM, key_str)
  data = str(uuid.uuid4())
  digest = 'sha256'
  crypto.sign(key, data, digest)
  $ python test.py
  *** Error in `python': free(): invalid next size (normal): 0x0000000002879050 ***
  Aborted

Other errors that may appear to the user are:

  Segmentation Fault
  *** Error in `python': double free or corruption (!prev): 0x0000000001245300 ***
  Aborted
  *** Error in `python': munmap_chunk(): invalid pointer: 0x0000000001fde540 ***
  Aborted

The reason this happens is that the sign function of the crypto module
hard-codes the size of the signature buffer to 512 bytes (4096 bits).
An RSA key generates a signature that can be up to the size of the
private key modulus, so for an 8192 bit key, a buffer for a 4096 bit
signature is too short and causes a memory allocation error.

Technically the maximum size key this code should be able to handle is
4096 bits, but due to memory allocation alignment the problem only
becomes apparent for keys of at least 4161 bits.

This patch does two things. First, it determines the correct size of
the signature buffer, in bytes, based on the real size of the private
key, and passes that the buffer allocation instead of the static number
512. Second, it no longer passes in a signature length. This is because
the OpenSSL EVP_SignFinal function uses this argument as an output and
completely ignores it as an input[1], so there is no need for us to set
it.

This is only a problem for RSA keys, and this patch only affects RSA
keys. For DSA keys, the key size is restricted to 1024 bits (128
bytes), and the signature a DSA key will generate will be about 46
bytes, so this buffer will still be big enough for DSA signatures.

[1] https://github.com/openssl/openssl/blob/349807608f31b20af01a342d0072bb92e0b036e2/crypto/evp/p_sign.c#L74

@alex
Copy link
Member

alex commented Mar 2, 2016

oof, this is awful. can you add a test case for this?

@alex
Copy link
Member

alex commented Mar 2, 2016

(To be clear, the awful thing is our code). Thanks for working on this!

@@ -2583,9 +2583,9 @@ def sign(pkey, data, digest):
_lib.EVP_SignInit(md_ctx, digest_obj)
_lib.EVP_SignUpdate(md_ctx, data, len(data))

signature_buffer = _ffi.new("unsigned char[]", 512)
pkey_length = PKey.bits(pkey) // 8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think probably you want pkey_length = (pkey.bits() + 7) // 8, otherwise rounding down will get you for a key that's not a multiple of 8 bits.

@cmurphy cmurphy force-pushed the fix_signature_buffer_size branch from ecaba57 to ff1a64b Compare March 2, 2016 03:48
When using the pyOpenSSL crypto module to sign data using a large key,
e.g. 8192 bit, a memory allocation error occurs. A test case to show
this, which comes from OpenStack Glance, is:

```
  $ openssl genrsa -out server.key 8192
  $ ...
  $ cat test.py
  from OpenSSL import crypto
  import uuid
  key_file = 'server.key'
  with open(key_file, 'r') as keyfile:
      key_str = keyfile.read()
  key = crypto.load_privatekey(crypto.FILETYPE_PEM, key_str)
  data = str(uuid.uuid4())
  digest = 'sha256'
  crypto.sign(key, data, digest)
  $ python test.py
  *** Error in `python': free(): invalid next size (normal): 0x0000000002879050 ***
  Aborted
```

Other errors that may appear to the user are:

```
  Segmentation Fault
```

```
  *** Error in `python': double free or corruption (!prev): 0x0000000001245300 ***
  Aborted
```

```
  *** Error in `python': munmap_chunk(): invalid pointer: 0x0000000001fde540 ***
  Aborted
```

The reason this happens is that the sign function of the crypto module
hard-codes the size of the signature buffer to 512 bytes (4096 bits).
An RSA key generates a signature that can be up to the size of the
private key modulus, so for an 8192 bit key, a buffer for a 4096 bit
signature is too short and causes a memory allocation error.

Technically the maximum size key this code should be able to handle is
4096 bits, but due to memory allocation alignment the problem only
becomes apparent for keys of at least 4161 bits.

This patch does two things. First, it determines the correct size of
the signature buffer, in bytes, based on the real size of the private
key, and passes that the buffer allocation instead of the static number
512. Second, it no longer passes in a signature length. This is because
the OpenSSL EVP_SignFinal function uses this argument as an output and
completely ignores it as an input[1], so there is no need for us to set
it.

This is only a problem for RSA keys, and this patch only affects RSA
keys. For DSA keys, the key size is restricted to 1024 bits (128
bytes), and the signature a DSA key will generate will be about 46
bytes, so this buffer will still be big enough for DSA signatures.

[1] https://github.com/openssl/openssl/blob/349807608f31b20af01a342d0072bb92e0b036e2/crypto/evp/p_sign.c#L74
@cmurphy cmurphy force-pushed the fix_signature_buffer_size branch from ff1a64b to e09399b Compare March 2, 2016 03:52
@cmurphy
Copy link
Contributor Author

cmurphy commented Mar 2, 2016

Thanks for checking this @alex. I've added a test and fixed the rounding error you pointed out.

@reaperhulk
Copy link
Member

LGTM, once tests pass we can merge and it will be in the next release (which should be soon!).

Thanks!

@codecov-io
Copy link

Current coverage is 87.72%

Merging #428 into master will not affect coverage as of a2308b4

@@            master    #428   diff @@
======================================
  Files            7       7       
  Stmts         2045    2045       
  Branches       377     377       
  Methods          0       0       
======================================
  Hit           1794    1794       
  Partial        121     121       
  Missed         130     130       

Review entire Coverage Diff as of a2308b4

Powered by Codecov. Updated on successful CI builds.

@Lukasa
Copy link
Member

Lukasa commented Mar 2, 2016

LGTM as well, great catch @cmurphy!

Lukasa added a commit that referenced this pull request Mar 2, 2016
Fix signature buffer size for RSA keys
@Lukasa Lukasa merged commit e6eac51 into pyca:master Mar 2, 2016
@cmurphy
Copy link
Contributor Author

cmurphy commented Mar 2, 2016

Thanks!

@hynek
Copy link
Contributor

hynek commented Mar 2, 2016

Can someone please add a change log entry? (And maybe replace :py:obj: thru something more specific without a py prefix?)

I'd do it myself but I'm on a phone in the Moroccan desert.

reaperhulk added a commit to reaperhulk/pyopenssl that referenced this pull request Mar 2, 2016
hynek added a commit that referenced this pull request Mar 3, 2016
add changelog entry for the fix in #428
@nagyv
Copy link

nagyv commented Mar 16, 2016

Hi,

indeed a great catch, and fix. Thanks! I'm eagerly waiting for it being released.

Is there a planned release date with the above fix being included?

@Lukasa
Copy link
Member

Lukasa commented Mar 16, 2016

@nagyv I believe there are plans to release 0.16 soon™. Last I heard you might get something late this week or early next, but we promise nothing. =)

@hynek
Copy link
Contributor

hynek commented Mar 16, 2016

It’s 16.0 and we’re waiting for cryptography 1.3 because we need its bindings. :)

jsonn referenced this pull request in jsonn/pkgsrc Apr 20, 2016
Changes:
16.0.0 (2016-03-19)
-------------------
This is the first release under full stewardship of PyCA.
We have made *many* changes to make local development more pleasing.
The test suite now passes both on Linux and OS X with OpenSSL 0.9.8,
1.0.1, and 1.0.2.  It has been moved to `py.test <https://pytest.org/>`_,
all CI test runs are part of `tox <https://testrun.org/tox/>`_ and
the source code has been made fully `flake8
<https://flake8.readthedocs.org/>`_ compliant.

We hope to have lowered the barrier for contributions significantly
but are open to hear about any remaining frustrations.

Backward-incompatible changes:
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
- Python 3.2 support has been dropped.
  It never had significant real world usage and has been dropped
  by our main dependency ``cryptography``.  Affected users should
  upgrade to Python 3.3 or later.

Deprecations:
^^^^^^^^^^^^^
- The support for EGD has been removed.
  The only affected function ``OpenSSL.rand.egd()`` now uses
  ``os.urandom()`` to seed the internal PRNG instead.  Please see
  `pyca/cryptography#1636
  <https://github.com/pyca/cryptography/pull/1636>`_ for more
  background information on this decision.  In accordance with our
  backward compatibility policy ``OpenSSL.rand.egd()`` will be
  *removed* no sooner than a year from the release of 16.0.0.
  Please note that you should `use urandom
  <http://sockpuppet.org/blog/2014/02/25/safely-generate-random-numbers/>`_
  for all your secure random number needs.
- Python 2.6 support has been deprecated.
  Our main dependency ``cryptography`` deprecated 2.6 in version
  0.9 (2015-05-14) with no time table for actually dropping it.
  pyOpenSSL will drop Python 2.6 support once ``cryptography``
  does.

Changes:
^^^^^^^^
- Fixed ``OpenSSL.SSL.Context.set_session_id``,
  ``OpenSSL.SSL.Connection.renegotiate``,
  ``OpenSSL.SSL.Connection.renegotiate_pending``, and
  ``OpenSSL.SSL.Context.load_client_ca``.
  They were lacking an implementation since 0.14.  `#422
  <https://github.com/pyca/pyopenssl/pull/422>`_
- Fixed segmentation fault when using keys larger than 4096-bit to sign data.
  `#428 <https://github.com/pyca/pyopenssl/pull/428>`_
- Fixed ``AttributeError`` when ``OpenSSL.SSL.Connection.get_app_data()``
  was called before setting any app data.
  `#304 <https://github.com/pyca/pyopenssl/pull/304>`_
- Added ``OpenSSL.crypto.dump_publickey()`` to dump ``OpenSSL.crypto.PKey``
  objects that represent public keys, and ``OpenSSL.crypto.load_publickey()``
  to load such objects from serialized representations.
  `#382 <https://github.com/pyca/pyopenssl/pull/382>`_
- Added ``OpenSSL.crypto.dump_crl()`` to dump a certificate revocation
  list out to a string buffer.
  `#368 <https://github.com/pyca/pyopenssl/pull/368>`_
- Added ``OpenSSL.SSL.Connection.get_state_string()`` using the
  OpenSSL binding ``state_string_long``.
  `#358 <https://github.com/pyca/pyopenssl/pull/358>`_
- Added support for the ``socket.MSG_PEEK`` flag to
  ``OpenSSL.SSL.Connection.recv()`` and
  ``OpenSSL.SSL.Connection.recv_into()``.
  `#294 <https://github.com/pyca/pyopenssl/pull/294>`_
- Added ``OpenSSL.SSL.Connection.get_protocol_version()`` and
  ``OpenSSL.SSL.Connection.get_protocol_version_name()``.
  `#244 <https://github.com/pyca/pyopenssl/pull/244>`_
- Switched to ``utf8string`` mask by default.
  OpenSSL formerly defaulted to a ``T61String`` if there were UTF-8
  characters present.  This was changed to default to ``UTF8String``
  in the config around 2005, but the actual code didn't change it
  until late last year.  This will default us to the setting that
  actually works.  To revert this you can call
  ``OpenSSL.crypto._lib.ASN1_STRING_set_default_mask_asc(b"default")``.
  `#234 <https://github.com/pyca/pyopenssl/pull/234>`_
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants