You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@santuario.apache.org by "Scott Cantor (JIRA)" <ji...@apache.org> on 2011/05/21 19:54:47 UTC
[jira] [Resolved] (SANTUARIO-271) Bug when signing files with big
RSA keys
[ https://issues.apache.org/jira/browse/SANTUARIO-271?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Scott Cantor resolved SANTUARIO-271.
------------------------------------
Resolution: Fixed
Fix Version/s: C++ 1.6.1
http://svn.apache.org/viewvc?view=revision&revision=1125752
Verification should work with any key sizes. Signing uses a buffer that's double the current size (should handle enormous/impractical keys), and detects when the result is too large to handle and fails instead of crashing. We can change the API in the future to improve this.
I made similar adustments to DSA and ECDSA code paths, and reviewed the WinCAPI and NSS providers, although I can't test them.
If you could test/review, I'm expecting to do a release probably around early/mid-June.
> Bug when signing files with big RSA keys
> ----------------------------------------
>
> Key: SANTUARIO-271
> URL: https://issues.apache.org/jira/browse/SANTUARIO-271
> Project: Santuario
> Issue Type: Bug
> Components: C++
> Affects Versions: C++ 1.6.0
> Environment: Ubuntu 11.04 i386, xml-security-c from SVN
> Reporter: Paulo Zanoni
> Assignee: Scott Cantor
> Labels: bug, patch
> Fix For: C++ 1.6.1
>
> Attachments: buff_overflow.cpp, reproducer.tar.gz
>
> Original Estimate: 24h
> Remaining Estimate: 24h
>
> Hello
> I hit a bug when I try to sign a file with a big RSA key.
> I'm using xml-security-c compiled from SVN on Ubuntu 11.04 i386.
> The bug is inside DSIGAlgorithmHandlerDefault::signToSafeBuffer. The function uses an array "b64Buf" with size 1024. If your signature is too big, the call to "key->signSHA1PKCS1Base64Signature()" might return 1024. After that call, if b64Buf[b64Len-1] is different from "\n" (which is usually the case), the program will do "b64buf[1024] = '\0', which is wrong (it is exactly 1 byte after the end of the buffer). Maybe this could even be considered a security vulnerability.
> I wrote a program that can reproduce the bug. If you run it with valgrind, you will see valgrind reporting the error.
> Possible solutions:
> 1: Increase the buffer size
> This solution will only allow more keys to be used, but won't solve the real problem.
> 2: Adapt the buffer size to the key
> I'm not sure how exactly this could be done, but maybe it would be easier to pass a pointer to signSHA1PKCS1Base64Signature() and make it it allocate the correct size for you (but then you'd change the library API...).
> 3: Check for the buffer overflow and return an exception
> This doesn't exactly fixes the bug, but it allows people using the library to know what is happening. After checking if "b64Len <= 0", you could check if "b64Len == 1024" and then throw an exception. My recommendation is that you implement this solution right now, and then write a proper fix later.
> 4: Combinations of solutions 1, 2 and 3
> I'll attach a .tar.gz file containing the following files:
> - buff_overflow.cpp: source code for a program that reproduces the bug
> - example.xml: file to be signed by the example program
> - rsa-private.pem: big rsa private key used to sign the file (8192 bits)
> - Makefile: use it to compile buff_overflow.cpp
> - DSIGAlgorithmHandler.patch: patch that does the following changes:
> - increases the buffer size to 2048 (solution 1)
> - creates a variable to indicate the buffer size
> - throws an exception if the bug is about to happen (solution 3)
> Valgrind output form running the reproducer on an unpatched machine:
> [paulo@foobar reproducer]$ valgrind ./buff_overflow
> ==23009== Memcheck, a memory error detector
> ==23009== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al.
> ==23009== Using Valgrind-3.5.0 and LibVEX; rerun with -h for copyright info
> ==23009== Command: ./buff_overflow
> ==23009==
> Signing XML file
> ==23009== Invalid write of size 1
> ==23009== at 0x4007D3C: strcpy (mc_replace_strmem.c:303)
> ==23009== by 0x528F916: safeBuffer::safeBuffer(char const*, unsigned int) (in /usr/lib/libxml-security-c.so.16.0.0)
> ==23009== by 0x525D995: DSIGAlgorithmHandlerDefault::signToSafeBuffer(TXFMChain*, unsigned short const*, XSECCryptoKey*, unsigned int, safeBuffer&) (in /usr/lib/libxml-security-c.so.16.0.0)
> ==23009== by 0x5267060: DSIGSignature::sign() (in /usr/lib/libxml-security-c.so.16.0.0)
> ==23009== by 0x804A535: signXmlFile(xercesc_3_1::DOMDocument*) (buff_overflow.cpp:81)
> ==23009== by 0x804A795: main (buff_overflow.cpp:117)
> ==23009== Address 0x43adfe0 is 0 bytes after a block of size 1,024 alloc'd
> ==23009== at 0x4005D2D: operator new[](unsigned int) (vg_replace_malloc.c:258)
> ==23009== by 0x528F8EC: safeBuffer::safeBuffer(char const*, unsigned int) (in /usr/lib/libxml-security-c.so.16.0.0)
> ==23009== by 0x525D995: DSIGAlgorithmHandlerDefault::signToSafeBuffer(TXFMChain*, unsigned short const*, XSECCryptoKey*, unsigned int, safeBuffer&) (in /usr/lib/libxml-security-c.so.16.0.0)
> ==23009== by 0x5267060: DSIGSignature::sign() (in /usr/lib/libxml-security-c.so.16.0.0)
> ==23009== by 0x804A535: signXmlFile(xercesc_3_1::DOMDocument*) (buff_overflow.cpp:81)
> ==23009== by 0x804A795: main (buff_overflow.cpp:117)
> ==23009==
> ==23009== Invalid read of size 1
> ==23009== at 0x4006813: strlen (mc_replace_strmem.c:275)
> ==23009== by 0x59BE8CD: xercesc_3_1::ICULCPTranscoder::transcode(char const*, xercesc_3_1::MemoryManager*) (in /usr/lib/libxerces-c-3.1.so)
> ==23009== by 0x5836ACD: xercesc_3_1::XMLString::transcode(char const*, xercesc_3_1::MemoryManager*) (in /usr/lib/libxerces-c-3.1.so)
> ==23009== by 0x528F54A: safeBuffer::sbStrToXMLCh() const (in /usr/lib/libxml-security-c.so.16.0.0)
> ==23009== by 0x52671F9: DSIGSignature::sign() (in /usr/lib/libxml-security-c.so.16.0.0)
> ==23009== by 0x804A535: signXmlFile(xercesc_3_1::DOMDocument*) (buff_overflow.cpp:81)
> ==23009== by 0x804A795: main (buff_overflow.cpp:117)
> ==23009== Address 0x43695b0 is 0 bytes after a block of size 1,024 alloc'd
> ==23009== at 0x4005D2D: operator new[](unsigned int) (vg_replace_malloc.c:258)
> ==23009== by 0x528F9FC: safeBuffer::safeBuffer() (in /usr/lib/libxml-security-c.so.16.0.0)
> ==23009== by 0x5267000: DSIGSignature::sign() (in /usr/lib/libxml-security-c.so.16.0.0)
> ==23009== by 0x804A535: signXmlFile(xercesc_3_1::DOMDocument*) (buff_overflow.cpp:81)
> ==23009== by 0x804A795: main (buff_overflow.cpp:117)
> ==23009==
> ==23009==
> ==23009== HEAP SUMMARY:
> ==23009== in use at exit: 288 bytes in 8 blocks
> ==23009== total heap usage: 7,402 allocs, 7,394 frees, 1,494,903 bytes allocated
> ==23009==
> ==23009== LEAK SUMMARY:
> ==23009== definitely lost: 0 bytes in 0 blocks
> ==23009== indirectly lost: 0 bytes in 0 blocks
> ==23009== possibly lost: 0 bytes in 0 blocks
> ==23009== still reachable: 288 bytes in 8 blocks
> ==23009== suppressed: 0 bytes in 0 blocks
> ==23009== Rerun with --leak-check=full to see details of leaked memory
> ==23009==
> ==23009== For counts of detected and suppressed errors, rerun with: -v
> ==23009== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 45 from 8)
> [paulo@foobar reproducer]$
> Cheers,
> Paulo
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira