[jira] [Created] (SANTUARIO-400) Buffer overwrite in WinCAPICryptoSymmetricKey::encrypt() (WinCAPICryptoSymmetricKey.cpp)

["Serge Lalonde (JIRA)" <ji...@apache.org>]

Serge Lalonde created SANTUARIO-400:
---------------------------------------

             Summary: Buffer overwrite in WinCAPICryptoSymmetricKey::encrypt() (WinCAPICryptoSymmetricKey.cpp)
                 Key: SANTUARIO-400
                 URL: https://issues.apache.org/jira/browse/SANTUARIO-400
             Project: Santuario
          Issue Type: Bug
          Components: C++
    Affects Versions: C++ 1.7.2, C++ 1.7.1, C++ 1.7.0
         Environment: Windows 7, VS2010 SP1
            Reporter: Serge Lalonde
            Assignee: Scott Cantor
            Priority: Critical
             Fix For: C++ 1.7.2


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.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)