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

Cut-over to CRYPTOPP_ASSERT due to CVE-2016-7420 #277

Closed
noloader opened this issue Sep 15, 2016 · 2 comments
Closed

Cut-over to CRYPTOPP_ASSERT due to CVE-2016-7420 #277

noloader opened this issue Sep 15, 2016 · 2 comments

Comments

@noloader
Copy link
Collaborator

noloader commented Sep 15, 2016

From a recent discussion with the Debian Security Team:

On Thu, Sep 15, 2016 at 3:21 PM, Jeffrey Walton noloader@gmail.com wrote:

Thanks Florian.

this matter does not seem to be something for the Debian security
team. Debian doesn't enable coredumps by default, and crypto++
upstream doesn't document that builds without -DNDEBUG are unsafe
(say, due to denial-of-service issues caused by broken asserts).

Fair enough, done. We added information to both Readme.txt and
Install.txt; see
http://github.com/weidai11/cryptopp/commit/553049ba297d89d9e8fbf2204acb40a8a53f5cd6

Are there other places we should disseminate the information? I want
to ensure you are looking in places I expect you to look (i.e., we are
not suffering a disconnect).

For completeness, the asserts are working as expected; they are not broken. The library's build system, [GNU] Make, adds the define. The define is also added by Visual Studio without user intervention. The library is well configured in its default state.

The problem is many distros and developers fail to add -DNDEBUG for release/production when using alternate build systems, like Autotools, CMake and Eclipse. Effectively the library receives an alternate set of flags and options, and it is placed in a debug configuration. Sometimes the lack of -DNDEBUG is due to policy (Debian and Autotools), and sometimes its due to omission (regular developers under CMake, Eclipse, etc).

Mitre assigned CVE-2016-7420 for the issue.

Also see CRYPTOPP_ASSERT on the user mailing list. We discussed a library assert over a year ago. We already have it and have been waiting for a good time to cut-over to it.


There are 590 asserts in effect when the library is in a debug configuration. Debug builds are tested with out test suite, and none of them trigger when asserts are in effect. The caveat is its not attacker controlled data.

Here's a list of files which could assert in the library when using a debug configuration.

$ grep assert *.h *.cpp | cut -f 1 -d ':' | sort | uniq | tr '\n' ' '
adler32.cpp algebra.cpp algparam.cpp algparam.h asn.cpp asn.h authenc.cpp authenc.h 
basecode.cpp bench1.cpp blake2.cpp blake2.h camellia.cpp cast.cpp ccm.cpp chacha.cpp 
cmac.cpp cryptlib.cpp cryptlib.h datatest.cpp dlltest.cpp eax.cpp ec2n.cpp eccrypto.cpp 
ecp.cpp emsa2.cpp eprecomp.cpp esign.cpp fhmqv.h files.cpp filters.cpp filters.h fips140.cpp 
fipsalgt.cpp fltrimpl.h gcm.cpp gf2_32.cpp gf2n.cpp gf2n.h gfpcrypt.cpp gfpcrypt.h hkdf.h 
hmac.cpp hmqv.h hrtimer.cpp ida.cpp idea.cpp integer.cpp iterhash.cpp keccak.cpp luc.h 
mersenne.h misc.cpp misc.h modes.cpp modes.h nbtheory.cpp network.cpp oaep.cpp 
panama.cpp pkcspad.cpp polynomi.cpp pssr.cpp pubkey.h pwdbased.h queue.cpp queue.h 
rdrand.cpp rdrand.h rijndael.cpp rsa.cpp rw.cpp salsa.cpp seal.cpp secblock.h sha.cpp 
sha3.cpp shark.cpp simple.h smartptr.h socketft.cpp socketft.h sosemanuk.cpp stdcpp.h 
strciphr.cpp strciphr.h tea.cpp test.cpp trap.h trdlocal.cpp validat1.cpp validat2.cpp vmac.cpp 
wait.cpp winpipes.cpp winpipes.h words.h xtr.cpp xtr.h zdeflate.cpp zinflate.cpp zlib.cpp 
@noloader noloader added the Bug label Sep 15, 2016
@noloader noloader changed the title Library documentation lacks treatment of -DNDEBUG and Static Initialization Cut-over to CRYPTOPP_ASSERT due to CVE-2016-7420 Sep 16, 2016
noloader referenced this issue Sep 16, 2016
trap.h and CRYPTOPP_ASSERT has existed for over a year in Master. We deferred on the cut-over waiting for a minor version bump (5.7). We have to use it now due to CVE-2016-7420
noloader referenced this issue Sep 16, 2016
…bug behavior pivots on CRYPTOPP_DEBUG, and not NDEBUG (Issue 277, CVE-2016-7420)
@noloader
Copy link
Collaborator Author

noloader commented Sep 18, 2016

The documentation updates/treatment of -DNDEBUG was added to README.txt and INSTALL.txt at (master branch):

The documentation updates/treatment of C++ Static Initialization Order was added to README.txt and INSTALL.txt at (master branch):

The cut-over to CRYPTOPP_DEBUG and CRYPTOPP_ASSERT was added at (trap dev-branch):

We detected and fixed some debugging code using NDEBUG at (trap dev-branch):

We added self tests to ensure no use of NDEBUG and assert(...) in source code at (trap dev-branch):


The _trap_ dev-branch was merged into _master_ at Commit 01b4ada1482f7e02.

Our next step is to release Crypto++ 5.6.5. This report will remain open until the new version is released.

@noloader
Copy link
Collaborator Author

noloader commented Nov 6, 2016

Crypto++ 5.6.5 was released on 10/11/2016. Closing the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant