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

Merged
merged 3 commits into from
Nov 30, 2017

Conversation

reaperhulk
Copy link
Member

@reaperhulk 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
Copy link

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
Copy link

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.

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)
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct

@alex alex merged commit e738186 into pyca:master Nov 30, 2017
@reaperhulk reaperhulk deleted the memory-management-is-hard branch November 30, 2017 14:54
bors-fusion bot referenced this pull request in fusionapp/fusion-index Dec 6, 2017
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 referenced this pull request in fusionapp/entropy Dec 6, 2017
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 referenced this pull request in fusionapp/documint Dec 6, 2017
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! 🤖
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 16, 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.

2 participants