Uploaded image for project: 'Santuario'
  1. Santuario
  2. SANTUARIO-400

Buffer overwrite in WinCAPICryptoSymmetricKey::encrypt() (WinCAPICryptoSymmetricKey.cpp)

    XMLWordPrintableJSON

Details

    • Bug
    • Status: Closed
    • Critical
    • Resolution: Fixed
    • C++ 1.7.0, C++ 1.7.1, C++ 1.7.2
    • C++ 1.7.3
    • C++
    • None
    • Windows 7, VS2010 SP1

    Description

      If the size of an Xml file being encrypted is 2KB + N, where N is a value less than m_blockSize, WinCAPICryptoSymmetricKey::encrypt() will generate a buffer overwrite at two locations, the second of which will cause a crash. Note that the 2 KB is the input buffer size.

      This happens because of a signed/unsigned overflow because the local variable "rounding" is greater than the method argument "inLength".

      Here are the locations where the overwrites occur:
      1. line 558
      memcpy(m_lastBlock, &inBuf[inLength - rounding], rounding);
      2. line 562 (causes a crash)
      memcpy(bufPtr, inBuf, inLength - rounding);

      There is a comment in the code that references that a fix was added to this method for bug 38365, which references very small inputs (which I presume means very small Xml files). Here's the patch in question:

      /* As per bug #38365 - we need to ensure there are enough characters to encrypt.
      Otherwise we get some nasty errors due to rounding calculations when inputs
      are too small */

      if (m_bytesInLastBlock + inLength < m_blockSize)

      { // Not enough input data memcpy(&m_lastBlock[m_bytesInLastBlock], inBuf, inLength); m_bytesInLastBlock += inLength; // Just in case we have returned an IV return outl; }

      In fact, this fix can be generalized easily to fix my problem also, since the if condition should really be

      if (inLength < m_blockSize) {

      since for very small inputs, m_bytesInLastBlock will be 0 anyway. This change allow the early return to be executed in both instances and avoids the overflow which gives a very large number and causes the crash.

      I've tested this fix in 1.7.2 and it works properly. If it could get implemented in the main code branch, that would be great.

      Attachments

        Activity

          People

            scantor Scott Cantor
            SergeLalonde Serge Lalonde
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: