Santuario
  1. Santuario
  2. SANTUARIO-271

Bug when signing files with big RSA keys

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: C++ 1.6.0
    • Fix Version/s: C++ 1.6.1
    • Component/s: C++
    • Security Level: Public (Public issues, viewable by everyone)
    • Labels:
    • Environment:
      Ubuntu 11.04 i386, xml-security-c from SVN

      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

      1. reproducer.tar.gz
        7 kB
        Paulo Zanoni
      2. buff_overflow.cpp
        4 kB
        Paulo Zanoni

        Activity

        Scott Cantor made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Scott Cantor made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s C++ 1.6.1 [ 12316452 ]
        Resolution Fixed [ 1 ]
        Hide
        Scott Cantor added a comment -

        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.

        Show
        Scott Cantor added a comment - 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.
        Paulo Zanoni made changes -
        Attachment buff_overflow.cpp [ 12479765 ]
        Hide
        Paulo Zanoni added a comment -

        Attached new version of the "reproducer", now with signature validation code. As I mentioned, valgrind might not warn about errors on that part of the code.

        Show
        Paulo Zanoni added a comment - Attached new version of the "reproducer", now with signature validation code. As I mentioned, valgrind might not warn about errors on that part of the code.
        Hide
        Paulo Zanoni added a comment -

        Hello again

        After some more investigation, it seems that if you increase the buffer size to 2048 bits (inside DSIGAlgorithmHandlerDefault::signToSafeBuffer) you will have to increase the buffer sizes in more places, not only when signing but also when checking signatures.

        One place I discovered was inside OpenSSLCryptoKeyRSA::verifySHA1PKCS1Base64Signature. Unfortunately Valgrind doesn't report an error there, but what I could find is that the "sigVal" buffer needs to be increased. (note: if you replace "unsigned char sigval[1024]" with "unsigned char *sigVal = new unsigned char[1024]" you will see Valgrind report errors with that buffer (since valgrind can deal better with memory allocation)).

        In a test, I replaced the "1024" value with "sigLen" value, so the line inside OpenSSLCryptoKeyRSA::verifySHA1PKCS1Base64Signature looked like:
        "unsigned char sigVal[sigLen];"
        Because, as far as I understood, "sigLen" seems to be always bigger than what we will store inside the sigVal buffer. This seems to have worked for me. I'm not sure all compilers will support this feature.

        I also realized that, at least in my common cases (smaller keys), sigLen is usually 349bytes (because the signature is 256bytes), so changing from "1024" to "sigLen" will even represent less memory usage in the "common case".

        Of course, if such a change was needed inside OpenSSL-specific code, we'll probably have to change other pieces of the code.

        Show
        Paulo Zanoni added a comment - Hello again After some more investigation, it seems that if you increase the buffer size to 2048 bits (inside DSIGAlgorithmHandlerDefault::signToSafeBuffer) you will have to increase the buffer sizes in more places, not only when signing but also when checking signatures. One place I discovered was inside OpenSSLCryptoKeyRSA::verifySHA1PKCS1Base64Signature. Unfortunately Valgrind doesn't report an error there, but what I could find is that the "sigVal" buffer needs to be increased. (note: if you replace "unsigned char sigval [1024] " with "unsigned char *sigVal = new unsigned char [1024] " you will see Valgrind report errors with that buffer (since valgrind can deal better with memory allocation)). In a test, I replaced the "1024" value with "sigLen" value, so the line inside OpenSSLCryptoKeyRSA::verifySHA1PKCS1Base64Signature looked like: "unsigned char sigVal [sigLen] ;" Because, as far as I understood, "sigLen" seems to be always bigger than what we will store inside the sigVal buffer. This seems to have worked for me. I'm not sure all compilers will support this feature. I also realized that, at least in my common cases (smaller keys), sigLen is usually 349bytes (because the signature is 256bytes), so changing from "1024" to "sigLen" will even represent less memory usage in the "common case". Of course, if such a change was needed inside OpenSSL-specific code, we'll probably have to change other pieces of the code.
        Hide
        Paulo Zanoni added a comment -

        Well... Your understanding of the code is way better than mine. Any solution would be okay for me =)

        But if I try to implement, I'll try to not break any current API. Maybe there is a way to write solution 2 without changing any API.

        Show
        Paulo Zanoni added a comment - Well... Your understanding of the code is way better than mine. Any solution would be okay for me =) But if I try to implement, I'll try to not break any current API. Maybe there is a way to write solution 2 without changing any API.
        Hide
        Scott Cantor added a comment -

        Over 3192 or so, certainly over 4096, you need ECC. I don't think people expect 8192 bit keys to be workable.

        As far as making API changes is concerned, adding APIs is probably ok, but changing existing functions is probably more than I want to do in most cases. If I do anything in that area it will be starting from scratch on top of my own XML code and limited to XML Sig 2.0.

        Is one possibility just to have a method to call to get the buffer size to pass in based on the key? Two calls, but API-compatible.

        Show
        Scott Cantor added a comment - Over 3192 or so, certainly over 4096, you need ECC. I don't think people expect 8192 bit keys to be workable. As far as making API changes is concerned, adding APIs is probably ok, but changing existing functions is probably more than I want to do in most cases. If I do anything in that area it will be starting from scratch on top of my own XML code and limited to XML Sig 2.0. Is one possibility just to have a method to call to get the buffer size to pass in based on the key? Two calls, but API-compatible.
        Hide
        Paulo Zanoni added a comment -

        Well, it's your call =)

        Btw, just out of curiosity: what do you see as the future, instead of big RSA keys? 2048 bits is pretty common these days, 8192 is just two steps away.

        Another question: if I ever implement "solution 2" in a decent way, will you consider accepting the patch?

        Thanks for your quick answer.

        Show
        Paulo Zanoni added a comment - Well, it's your call =) Btw, just out of curiosity: what do you see as the future, instead of big RSA keys? 2048 bits is pretty common these days, 8192 is just two steps away. Another question: if I ever implement "solution 2" in a decent way, will you consider accepting the patch? Thanks for your quick answer.
        Hide
        Scott Cantor added a comment -

        Sorry for making you repeat. Reason I ask is that if the conventional wisdom is that RSA has no future at that size, we could take the "easy" way out as you described in solution 3, and not go overboard trying to handle it.

        Show
        Scott Cantor added a comment - Sorry for making you repeat. Reason I ask is that if the conventional wisdom is that RSA has no future at that size, we could take the "easy" way out as you described in solution 3, and not go overboard trying to handle it.
        Hide
        Paulo Zanoni added a comment -

        As I said, the key used is 8192 bits. I know this size is really considered "too big" for today, but in the future it might be a problem. We should at least "fail gracefully" when we detect the error (see "solution 3" and my patch)

        Show
        Paulo Zanoni added a comment - As I said, the key used is 8192 bits. I know this size is really considered "too big" for today, but in the future it might be a problem. We should at least "fail gracefully" when we detect the error (see "solution 3" and my patch)
        Paulo Zanoni made changes -
        Field Original Value New Value
        Attachment reproducer.tar.gz [ 12478991 ]
        Hide
        Paulo Zanoni added a comment -

        Files used to reproduce the bug

        Show
        Paulo Zanoni added a comment - Files used to reproduce the bug
        Hide
        Scott Cantor added a comment -

        Just curious, how big a key is involved?

        Show
        Scott Cantor added a comment - Just curious, how big a key is involved?
        Paulo Zanoni created issue -

          People

          • Assignee:
            Scott Cantor
            Reporter:
            Paulo Zanoni
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 24h
              24h
              Remaining:
              Remaining Estimate - 24h
              24h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development