Details
Major Description
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
