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 a memory leak and a potential UAF and also #722 #723

Merged
merged 3 commits into from Nov 30, 2017

Conversation

Projects
None yet
2 participants
@reaperhulk
Member

reaperhulk commented Nov 29, 2017

In the callback we were creating an X509 object that didn't own its own memory so if you retained a reference to it after the callback completed you could end up in a UAF situation.

In PKCS12 we freed the stack but freeing the stack doesn't free the underlying objects. This meant that any PKCS12 with cacerts was leaking memory every load.

Finally, we clean up one other little area where we can use _from_raw_x509_ptr.

This needs CHANGELOG entries, but it also can't be merged until we backport pyca/cryptography#4028 and release a 2.1.4 (and then upgrade our minimum dep in pyOpenSSL to 2.1.4, d'oh)

@codecov

This comment has been minimized.

codecov bot commented Nov 29, 2017

Codecov Report

Merging #723 into master will decrease coverage by 2.5%.
The diff coverage is 95.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #723      +/-   ##
==========================================
- Coverage   97.03%   94.52%   -2.51%     
==========================================
  Files          18       18              
  Lines        5669     5684      +15     
  Branches      394      394              
==========================================
- Hits         5501     5373     -128     
- Misses        112      249     +137     
- Partials       56       62       +6
Impacted Files Coverage Δ
tests/test_ssl.py 93.98% <100%> (-5.13%) ⬇️
src/OpenSSL/SSL.py 90.54% <100%> (-4.34%) ⬇️
src/OpenSSL/crypto.py 96.43% <80%> (-0.44%) ⬇️
tests/test_crypto.py 98.54% <0%> (-0.38%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f724786...eae36b8. Read the comment docs.

@codecov

This comment has been minimized.

codecov bot commented Nov 29, 2017

Codecov Report

Merging #723 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #723      +/-   ##
==========================================
+ Coverage   97.03%   97.04%   +<.01%     
==========================================
  Files          18       18              
  Lines        5669     5682      +13     
  Branches      394      394              
==========================================
+ Hits         5501     5514      +13     
  Misses        112      112              
  Partials       56       56
Impacted Files Coverage Δ
tests/test_ssl.py 99.11% <100%> (ø) ⬆️
src/OpenSSL/SSL.py 94.88% <100%> (ø) ⬆️
src/OpenSSL/crypto.py 96.86% <100%> (-0.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f724786...4f0b2fa. Read the comment docs.

@reaperhulk reaperhulk force-pushed the reaperhulk:memory-management-is-hard branch from eae36b8 to 915d1d8 Nov 29, 2017

pycacert = X509.__new__(X509)
pycacert._x509 = _lib.sk_X509_value(cacerts, i)
x509 = _lib.sk_X509_value(cacerts, i)
pycacert = X509._from_raw_x509_ptr(x509)

This comment has been minimized.

@reaperhulk

reaperhulk Nov 29, 2017

Member

I'd like a double check here. I'm not confident this is right. Does sk_X509_free free the underlying X509 * in the stack? If so then this is wrong, but that would also mean the current code in master is just a giant UAF.

This comment has been minimized.

@alex

alex Nov 29, 2017

Member

sk_x509_free only frees the stack itself, not any of the things it contains, afaik

This comment has been minimized.

@reaperhulk

reaperhulk Nov 29, 2017

Member

Okay, if that's true (which is what I thought but it's amazing how quickly you start second guessing yourself when fixing memory bugs) then should be right.

This comment has been minimized.

@alex

alex Nov 29, 2017

Member

So the memory approach here is that each X509 object is responsible for hte X509* and the cacerts var is only responsible for freeing the stack?

This comment has been minimized.

@reaperhulk

reaperhulk Nov 29, 2017

Member

Correct

reaperhulk added some commits Nov 29, 2017

@alex

alex approved these changes Nov 30, 2017

@alex alex merged commit e738186 into pyca:master Nov 30, 2017

3 checks passed

codecov/patch 100% of diff hit (target 97.03%)
Details
codecov/project 97.04% (+<.01%) compared to f724786
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@reaperhulk reaperhulk deleted the reaperhulk:memory-management-is-hard branch Nov 30, 2017

bors-fusion bot added a commit to fusionapp/fusion-index that referenced this pull request Dec 6, 2017

Merge #170
170: Scheduled weekly dependency update for week 49 r=mithrandi a=pyup-bot




## Updates
Here's a list of all the updates bundled in this pull request. I've added some links to make it easier for you to find all the information you need.
<table align="center">

<tr>
<td><b>cryptography</b></td>
<td align="center">2.1.3</td>
<td align="center">&raquo;</td>
<td align="center">2.1.4</td>
<td>
     <a href="https://pypi.python.org/pypi/cryptography">PyPI</a> | <a href="https://pyup.io/changelogs/cryptography/">Changelog</a> | <a href="https://github.com/pyca/cryptography">Repo</a> 

</td>

<tr>
<td><b>eliot</b></td>
<td align="center">1.2.0</td>
<td align="center">&raquo;</td>
<td align="center">1.3.0</td>
<td>
     <a href="https://pypi.python.org/pypi/eliot">PyPI</a> | <a href="https://pyup.io/changelogs/eliot/">Changelog</a> | <a href="https://github.com/ClusterHQ/eliot/">Repo</a> 

</td>

<tr>
<td><b>hypothesis</b></td>
<td align="center">3.38.5</td>
<td align="center">&raquo;</td>
<td align="center">3.40.1</td>
<td>
     <a href="https://pypi.python.org/pypi/hypothesis">PyPI</a> | <a href="https://pyup.io/changelogs/hypothesis/">Changelog</a> | <a href="https://github.com/HypothesisWorks/hypothesis/issues">Repo</a> 

</td>

<tr>
<td><b>pyopenssl</b></td>
<td align="center">17.4.0</td>
<td align="center">&raquo;</td>
<td align="center">17.5.0</td>
<td>
     <a href="https://pypi.python.org/pypi/pyopenssl">PyPI</a> | <a href="https://pyup.io/changelogs/pyopenssl/">Changelog</a> | <a href="https://pyopenssl.org/">Homepage</a> | <a href="http://pythonhosted.org/pyOpenSSL/">Docs</a> 

</td>

</tr>
</table>



## Changelogs


### eliot 1.2.0 -> 1.3.0

>### 1.3.0









### hypothesis 3.38.5 -> 3.40.1

>### 3.40.1

>-------------------

>This release makes two changes:

>* It makes the calculation of some of the metadata that Hypothesis uses for
>  shrinking occur lazily. This should speed up performance of test case
>  generation a bit because it no longer calculates information it doesn&#39;t need.
>* It improves the shrinker for certain classes of nested examples. e.g. when
>  shrinking lists of lists, the shrinker is now able to concatenate two
>  adjacent lists together into a single list. As a result of this change,
>  shrinking may get somewhat slower when the minimal example found is large.

>-------------------


>### 3.40.0

>-------------------

>This release improves how various ways of seeding Hypothesis interact with the
>example database:

>* Using the example database with :func:`~hypothesis.seed` is now deprecated.
>  You should set ``database=None`` if you are doing that. This will only warn
>  if you actually load examples from the database while using ``seed``.
>* The :attr:`~hypothesis.settings.derandomize` will behave the same way as
>  ``seed``.
>* Using ``--hypothesis-seed`` will disable use of the database.
>* If a test used examples from the database, it will not suggest using a seed
>  to reproduce it, because that won&#39;t work.

>This work was funded by `Smarkets &lt;https://smarkets.com/&gt;`_.

>-------------------


>### 3.39.0

>-------------------

>This release adds a new health check that checks if the smallest &quot;natural&quot;
>possible example of your test case is very large - this will tend to cause
>Hypothesis to generate bad examples and be quite slow.

>This work was funded by `Smarkets &lt;https://smarkets.com/&gt;`_.

>-------------------


>### 3.38.9

>-------------------

>This is a documentation release to improve the documentation of shrinking
>behaviour for Hypothesis&#39;s strategies.

>-------------------


>### 3.38.8

>-------------------

>This release improves the performance of
>:func:`~hypothesis.strategies.characters` when using ``blacklist_characters``
>and :func:`~hypothesis.strategies.from_regex` when using negative character
>classes.

>The problems this fixes were found in the course of work funded by
>`Smarkets &lt;https://smarkets.com/&gt;`_.

>-------------------


>### 3.38.7

>-------------------

>This is a patch release for :func:`~hypothesis.strategies.from_regex`, which
>had a bug in handling of the :obj:`python:re.VERBOSE` flag (:issue:`992`).
>Flags are now handled correctly when parsing regex.

>-------------------


>### 3.38.6

>-------------------

>This patch changes a few byte-string literals from double to single quotes,
>thanks to an update in :pypi:`unify`.  There are no user-visible changes.

>-------------------






### pyopenssl 17.4.0 -> 17.5.0

>### 17.5.0

>-------------------


>Backward-incompatible changes:
>^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

>* The minimum ``cryptography`` version is now 2.1.4.


>Deprecations:
>^^^^^^^^^^^^^

>*none*


>Changes:
>^^^^^^^^

>- Fixed a potential use-after-free in the verify callback and resolved a memory leak when loading PKCS12 files with ``cacerts``.
>  `723 &lt;https://github.com/pyca/pyopenssl/pull/723&gt;`_
>- Added ``Connection.export_keying_material`` for RFC 5705 compatible export of keying material.
>  `725 &lt;https://github.com/pyca/pyopenssl/pull/725&gt;`_

>----












That's it for now!

Happy merging! 🤖

bors-fusion bot added a commit to fusionapp/entropy that referenced this pull request Dec 6, 2017

Merge #163
163: Scheduled weekly dependency update for week 49 r=mithrandi a=pyup-bot




## Updates
Here's a list of all the updates bundled in this pull request. I've added some links to make it easier for you to find all the information you need.
<table align="center">

<tr>
<td><b>cryptography</b></td>
<td align="center">2.1.3</td>
<td align="center">&raquo;</td>
<td align="center">2.1.4</td>
<td>
     <a href="https://pypi.python.org/pypi/cryptography">PyPI</a> | <a href="https://pyup.io/changelogs/cryptography/">Changelog</a> | <a href="https://github.com/pyca/cryptography">Repo</a> 

</td>

<tr>
<td><b>pyopenssl</b></td>
<td align="center">17.4.0</td>
<td align="center">&raquo;</td>
<td align="center">17.5.0</td>
<td>
     <a href="https://pypi.python.org/pypi/pyopenssl">PyPI</a> | <a href="https://pyup.io/changelogs/pyopenssl/">Changelog</a> | <a href="https://pyopenssl.org/">Homepage</a> | <a href="http://pythonhosted.org/pyOpenSSL/">Docs</a> 

</td>

</tr>
</table>



## Changelogs


### pyopenssl 17.4.0 -> 17.5.0

>### 17.5.0

>-------------------


>Backward-incompatible changes:
>^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

>* The minimum ``cryptography`` version is now 2.1.4.


>Deprecations:
>^^^^^^^^^^^^^

>*none*


>Changes:
>^^^^^^^^

>- Fixed a potential use-after-free in the verify callback and resolved a memory leak when loading PKCS12 files with ``cacerts``.
>  `723 &lt;https://github.com/pyca/pyopenssl/pull/723&gt;`_
>- Added ``Connection.export_keying_material`` for RFC 5705 compatible export of keying material.
>  `725 &lt;https://github.com/pyca/pyopenssl/pull/725&gt;`_

>----












That's it for now!

Happy merging! 🤖

bors-fusion bot added a commit to fusionapp/documint that referenced this pull request Dec 6, 2017

Merge #121
121: Scheduled weekly dependency update for week 49 r=mithrandi a=pyup-bot




## Updates
Here's a list of all the updates bundled in this pull request. I've added some links to make it easier for you to find all the information you need.
<table align="center">

<tr>
<td><b>cryptography</b></td>
<td align="center">2.1.3</td>
<td align="center">&raquo;</td>
<td align="center">2.1.4</td>
<td>
     <a href="https://pypi.python.org/pypi/cryptography">PyPI</a> | <a href="https://pyup.io/changelogs/cryptography/">Changelog</a> | <a href="https://github.com/pyca/cryptography">Repo</a> 

</td>

<tr>
<td><b>pyopenssl</b></td>
<td align="center">17.4.0</td>
<td align="center">&raquo;</td>
<td align="center">17.5.0</td>
<td>
     <a href="https://pypi.python.org/pypi/pyopenssl">PyPI</a> | <a href="https://pyup.io/changelogs/pyopenssl/">Changelog</a> | <a href="https://pyopenssl.org/">Homepage</a> | <a href="http://pythonhosted.org/pyOpenSSL/">Docs</a> 

</td>

</tr>
</table>



## Changelogs


### pyopenssl 17.4.0 -> 17.5.0

>### 17.5.0

>-------------------


>Backward-incompatible changes:
>^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

>* The minimum ``cryptography`` version is now 2.1.4.


>Deprecations:
>^^^^^^^^^^^^^

>*none*


>Changes:
>^^^^^^^^

>- Fixed a potential use-after-free in the verify callback and resolved a memory leak when loading PKCS12 files with ``cacerts``.
>  `723 &lt;https://github.com/pyca/pyopenssl/pull/723&gt;`_
>- Added ``Connection.export_keying_material`` for RFC 5705 compatible export of keying material.
>  `725 &lt;https://github.com/pyca/pyopenssl/pull/725&gt;`_

>----












That's it for now!

Happy merging! 🤖
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment