From 2839a6d144a23d7841e53512e88af2b1bf4dfc7a Mon Sep 17 00:00:00 2001 From: Yuval Harpaz Date: Wed, 21 Jul 2021 20:32:40 +0300 Subject: [PATCH] Adding Add/Remove Padding for PKCS7 --- inc/symcrypt.h | 8 +- lib/paddingPkcs7.c | 17 ++- lib/scsTools.c | 2 +- unittest/inc/test_lib.h | 3 + unittest/lib/CMakeLists.txt | 1 + unittest/lib/main.cpp | 4 +- unittest/lib/sources | 1 + unittest/lib/testPaddingPkcs7.cpp | 202 ++++++++++++++++++++++++++++++ unittest/lib/testScsTools.cpp | 125 ++++++++++++------ 9 files changed, 314 insertions(+), 49 deletions(-) create mode 100644 unittest/lib/testPaddingPkcs7.cpp diff --git a/inc/symcrypt.h b/inc/symcrypt.h index ad2df7a..557f541 100644 --- a/inc/symcrypt.h +++ b/inc/symcrypt.h @@ -3023,7 +3023,7 @@ SymCryptPaddingPkcs7Add( SIZE_T* pcbResult); // // Prerequisites: -// cbBlockSize is a power of 2 and <= 256 +// cbBlockSize is a power of 2 and < 256 // cbDst >= cbSrc - cbSrc % cbBlockSize + cbBlockSize // // Add PKCS7 block padding to a message @@ -3057,7 +3057,7 @@ SymCryptPaddingPkcs7Remove( SIZE_T* pcbResult); // // Prerequisites: -// - cbBlockSize is a power of 2 and <= 256 +// - cbBlockSize is a power of 2 and < 256 // - cbSrc is a multiple of cbBlockSize // - cbSrc is greater than zero (at least equals to cbBlockSize) // @@ -3078,10 +3078,6 @@ SymCryptPaddingPkcs7Remove( // Even if an error is returned, the pbDst buffer may or may not contain data from the message. // Callers should wipe the buffer even if an error is returned. // -// In case that the padding is invalid and the output buffer is too small, both the -// errors above are detected, but SYMCRYPT_INVALID_ARGUMENT gets precedence over -// SYMCRYPT_BUFFER_TOO_SMALL and the returned value will be SYMCRYPT_INVALID_ARGUMENT. -// // Note: Removal of PKCS7 padding is extremely sensitive to side channels. // For example, if a message is encrypted with AES-CBC and the attacker can modify // the ciphertext and then determine whether a padding error occurrs during decryption, diff --git a/lib/paddingPkcs7.c b/lib/paddingPkcs7.c index a12e46c..83f962c 100644 --- a/lib/paddingPkcs7.c +++ b/lib/paddingPkcs7.c @@ -21,7 +21,7 @@ SymCryptPaddingPkcs7Add( SIZE_T cbDataLastBlock; // dwDataLastBlock is the number of bytes of data at the final block. SIZE_T cbResult = 0; // This variable must always have a valid value when we finish the function. - SYMCRYPT_ASSERT(cbBlockSize <= 256); // cbBlockSize must be <= 256 + SYMCRYPT_ASSERT(cbBlockSize < 256); // cbBlockSize must be < 256 SYMCRYPT_ASSERT((cbBlockSize & (cbBlockSize - 1)) == 0); // cbBlockSize must be a power of 2 // @@ -82,12 +82,18 @@ SymCryptPaddingPkcs7Remove( SIZE_T cbResult; // This variable must always have a valid value when we finish the function. - SYMCRYPT_ASSERT(cbBlockSize <= 256); // cbBlockSize must be <= 256 + SYMCRYPT_ASSERT(cbBlockSize < 256); // cbBlockSize must be < 256 SYMCRYPT_ASSERT((cbBlockSize & (cbBlockSize - 1)) == 0); // cbBlockSize must be a power of 2 SYMCRYPT_ASSERT((cbSrc & (cbBlockSize - 1)) == 0); // cbSrc is a multiple of cbBlockSize SYMCRYPT_ASSERT(cbSrc > 0); // cbSrc is greaten than zero cbPadVal = (UINT32)pbSrc[cbSrc - 1]; + + // check the Padding to make sure it is valid. + mPaddingError |= SymCryptMask32IsZeroU31(cbPadVal) | SymCryptMask32LtU31((UINT32)cbBlockSize, cbPadVal); + + // If cbPadVal is greater than cbSrc, SYMCRYPT_INVALID_ARGUMENT will be returned + // and cbResult will not be the right value. cbResult = cbSrc - cbPadVal; // @@ -121,15 +127,14 @@ SymCryptPaddingPkcs7Remove( // // Validating padding // - + // If cbPadVal is greater than cbBlockSize, + // we have to limit cbPadVal to be at most equal to cbBlockSize. + cbPadVal = 1 + ((cbPadVal - 1) & (cbBlockSize - 1)); cbMsg32 = (UINT32)(cbBlockSize - cbPadVal); //check Dst buffer length to make sure it is possible copy the whole message (not including the padding). mBufferSizeError |= SymCryptMask32LtU31(cbDst32, cbMsg32); - // check the Padding to make sure it is valid. - mPaddingError |= SymCryptMask32IsZeroU31(cbPadVal) | SymCryptMask32LtU31((UINT32)cbBlockSize, cbPadVal); - // // Final Block processing // diff --git a/lib/scsTools.c b/lib/scsTools.c index e07eb0d..8bad9de 100644 --- a/lib/scsTools.c +++ b/lib/scsTools.c @@ -364,7 +364,7 @@ SymCryptScsRotateBuffer( UINT32 SYMCRYPT_CALL SymCryptMapUint32(UINT32 u32Input, UINT32 u32Default, PCSYMCRYPT_UINT32_MAP pcMap, SIZE_T nMap) { - UINT32 mask = 0; + UINT32 mask; UINT32 u32Output = u32Default; for (SIZE_T i = 0; i < nMap; ++i) diff --git a/unittest/inc/test_lib.h b/unittest/inc/test_lib.h index 597baf1..73766c2 100644 --- a/unittest/inc/test_lib.h +++ b/unittest/inc/test_lib.h @@ -1096,6 +1096,9 @@ testScsTable(); VOID testScsTools(); +VOID +testPaddingPkcs7(); + VOID testEcc(); diff --git a/unittest/lib/CMakeLists.txt b/unittest/lib/CMakeLists.txt index ff7e0f7..bf87951 100644 --- a/unittest/lib/CMakeLists.txt +++ b/unittest/lib/CMakeLists.txt @@ -33,6 +33,7 @@ set(SOURCES testScsTable.cpp testScsTools.cpp perf.cpp + testPaddingPkcs7.cpp ) # Append Windows-specific sources diff --git a/unittest/lib/main.cpp b/unittest/lib/main.cpp index adb92b3..be1e8cb 100644 --- a/unittest/lib/main.cpp +++ b/unittest/lib/main.cpp @@ -1548,7 +1548,9 @@ runFunctionalTests() testArithmetic(); testScsTable(); - + + testPaddingPkcs7(); + testScsTools(); testRsaSignAlgorithms(); diff --git a/unittest/lib/sources b/unittest/lib/sources index c05610b..5faa2b2 100644 --- a/unittest/lib/sources +++ b/unittest/lib/sources @@ -73,6 +73,7 @@ SOURCES= \ testRsaEnc.cpp \ testDh.cpp \ testDsa.cpp \ + testPaddingPkcs7.cpp \ I386_SOURCES = \ savexmm.asm \ diff --git a/unittest/lib/testPaddingPkcs7.cpp b/unittest/lib/testPaddingPkcs7.cpp new file mode 100644 index 0000000..f589952 --- /dev/null +++ b/unittest/lib/testPaddingPkcs7.cpp @@ -0,0 +1,202 @@ +// +// Copyright (c) Microsoft Corporation. Licensed under the MIT license. +// + +#include "precomp.h" + +#define MAX_BUFFER_SIZE 10000 +#define MAX_PKCS7_BLOCK_SIZE 128 +#define MAX_DEST_BUFFER_SIZE 2 * (MAX_BUFFER_SIZE + MAX_PKCS7_BLOCK_SIZE) + +SIZE_T +verifyAddPadding(SIZE_T cbBlockSize, PCBYTE pbSrc, UINT32 cbSrc, PBYTE pbDst, UINT32 cbDst) +{ + SIZE_T cbResult = cbSrc; + + // Making copy of the Dst buffer before padding for validation + PBYTE pbDstCpy = new BYTE[cbDst]; + memcpy(pbDstCpy, pbDst, cbDst); + + // Calculating padding parameters for validation + UINT32 cbPadVal = (UINT32)(cbBlockSize - (cbSrc % cbBlockSize)); + UINT32 cbPredictedResult = cbSrc + cbPadVal; + + SymCryptPaddingPkcs7Add(cbBlockSize, pbSrc, cbSrc, pbDst, cbDst, &cbResult); + + // Verify cbResult value + CHECK(cbResult == cbPredictedResult, "testPaddingPkcs7Add : cbResult is incorrect."); + + // Verify pbDst data (msg, padding, unchanged data) + for (UINT32 i = 0; i < cbDst; ++i) + { + if (i < cbSrc) + { + CHECK(pbSrc[i] == pbDst[i], "testPaddingPkcs7Add: message was not copied to destination as expected!"); + continue; + } + if (i < cbResult) + { + CHECK((BYTE)cbPadVal == pbDst[i], "testPaddingPkcs7Add: invalid padded value!"); + continue; + } + CHECK(pbDst[i] == pbDstCpy[i], "testPaddingPkcs7Add: data after padding was changed!"); + } + + delete [] pbDstCpy; + return cbResult; +} + +VOID +verifyRemovePadding(SIZE_T cbBlockSize, PCBYTE pbSrc, UINT32 cbSrc, PBYTE pbDst, UINT32 cbDst) +{ + + SIZE_T cbResult = cbSrc; + UINT32 cbPadVal = pbSrc[cbSrc - 1]; + UINT32 cbMsg = cbSrc - (cbPadVal % (cbBlockSize + 1 )); + + bool validPadding = (cbPadVal != 0 && cbPadVal <= cbBlockSize) ? true : false; + bool bufferTooSmall = (cbDst < cbSrc - cbBlockSize) ? true : false; + bool validBuffer = (cbDst < cbMsg) ? false : true; + + // Making copy of the Dst buffer for validation + PBYTE pbDstCpy = new BYTE[cbDst]; + memcpy(pbDstCpy, pbDst, cbDst); + + SYMCRYPT_ERROR err = SymCryptPaddingPkcs7Remove(cbBlockSize, pbSrc, cbSrc, pbDst, cbDst, &cbResult); + + // if cbDst is less than block size, we don't need to check the padding. + if (!bufferTooSmall) + { + for (UINT32 i = cbMsg; i < cbSrc; ++i) + { + if (pbSrc[i] != cbPadVal) + { + validPadding = false; + } + } + } + else + { + if (!validPadding) + { + CHECK(((err == SYMCRYPT_BUFFER_TOO_SMALL) || (err == SYMCRYPT_INVALID_ARGUMENT)), + "SymCryptPaddingPkcs7Remove: not the expected error was returned."); + } + else + { + CHECK(err == SYMCRYPT_BUFFER_TOO_SMALL, "testPaddingPkcs7Remove: not the expected error was returned."); + } + } + + if (validBuffer && validPadding) + { + CHECK(err == SYMCRYPT_NO_ERROR, + "SymCryptPaddingPkcs7Remove: not the expected error was returned."); + CHECK(cbResult == cbMsg, "testPaddingPkcs7Remove: cbResult is incorrect."); + + // validate data at pbDst + for (UINT32 i = 0; i < cbDst; ++i) + { + if (!bufferTooSmall && i < cbMsg) + { + CHECK(pbSrc[i] == pbDst[i], "testPaddingPkcs7Remove: message was not copied to destination as expected!"); + continue; + } + + CHECK(pbDstCpy[i] == pbDst[i], "testPaddingPkcs7Remove: data after padding was changed!"); + } + } + else if (validBuffer && !validPadding) + { + CHECK(err == SYMCRYPT_INVALID_ARGUMENT, + "SymCryptPaddingPkcs7Remove: not the expected error was returned."); + } + else if (!validBuffer && validPadding) + { + CHECK(err == SYMCRYPT_BUFFER_TOO_SMALL, + "SymCryptPaddingPkcs7Remove: not the expected error was returned."); + } + else if (!validBuffer && !validPadding) + { + CHECK(((err == SYMCRYPT_BUFFER_TOO_SMALL) || (err == SYMCRYPT_INVALID_ARGUMENT)), + "SymCryptPaddingPkcs7Remove: not the expected error was returned."); + } + + delete [] pbDstCpy; +} + + +VOID +testPaddingPkcs7() +{ + UINT32 cbMsg; + UINT32 cbAddPad; + UINT32 cbRemovePad; + + // Prerequisites: cbBlockSize is a power of 2 and <= 128 + SIZE_T cbBlockSize; + GENRANDOM(&cbBlockSize, sizeof(cbBlockSize)); + cbBlockSize = SymCryptRoundUpPow2Sizet(1 + (cbBlockSize % MAX_PKCS7_BLOCK_SIZE)); + + GENRANDOM(&cbMsg, sizeof(cbMsg)); + cbMsg = cbMsg % MAX_BUFFER_SIZE; + + GENRANDOM(&cbRemovePad, sizeof(cbRemovePad)); + cbRemovePad = cbRemovePad % MAX_BUFFER_SIZE; + + // Prerequisites: cbDst >= cbSrc - cbSrc % cbBlockSize + cbBlockSize + GENRANDOM(&cbAddPad, sizeof(cbAddPad)); + cbAddPad = cbAddPad % MAX_DEST_BUFFER_SIZE; + cbAddPad = cbAddPad + (cbMsg + (UINT32)cbBlockSize - (cbMsg % cbBlockSize)); + + + // Generating both Src and Dst random buffers + PBYTE pbMsg = new BYTE[cbMsg]; + PBYTE pbAddPad = new BYTE[cbAddPad]; + PBYTE pbRemovePad = new BYTE[cbRemovePad]; + + GENRANDOM(pbMsg, cbMsg); + GENRANDOM(pbAddPad, cbAddPad); + GENRANDOM(pbRemovePad, cbRemovePad); + + // Check Add padding process + UINT32 cbResult = (UINT32) verifyAddPadding(cbBlockSize, pbMsg, cbMsg, pbAddPad, cbAddPad); + + // Check Remove padding process with validated padded buffer + verifyRemovePadding(cbBlockSize, pbAddPad, cbResult, pbRemovePad, cbRemovePad); + + // Checking the remove padding process with manipulated padded data. + // In this test we will generate a padding value and a position and replace one of + // the padded bytes with this random value. Then we will validate the remove padding. + UINT32 cbPadValDummy; + GENRANDOM(&cbPadValDummy, sizeof(cbPadValDummy)); + if (MAX_PKCS7_BLOCK_SIZE <= cbPadValDummy) + { + cbPadValDummy = cbPadValDummy % MAX_PKCS7_BLOCK_SIZE; + } + UINT32 cbPadVal = cbResult - cbMsg; + UINT32 rndPadPos = cbMsg; + if (cbPadVal != 0) + { + rndPadPos = rndPadPos + (cbPadValDummy % cbPadVal); + } + UINT32 cbSrcDummy = cbResult; + PBYTE pbSrcDummy = new BYTE[cbSrcDummy]; + memcpy(pbSrcDummy, pbAddPad, cbResult); + memset(pbSrcDummy + rndPadPos, cbPadValDummy, 1); + + UINT32 cbDstDummy; + GENRANDOM(&cbDstDummy, sizeof(cbDstDummy)); + cbDstDummy = cbDstDummy % MAX_BUFFER_SIZE; + PBYTE pbDstDummy = new BYTE[cbDstDummy]; + GENRANDOM(pbDstDummy, cbDstDummy); + + verifyRemovePadding(cbBlockSize, pbSrcDummy, cbSrcDummy, pbDstDummy, cbDstDummy); + + // Cleanup + delete [] pbMsg; + delete [] pbAddPad; + delete [] pbRemovePad; + delete [] pbSrcDummy; + delete [] pbDstDummy; +} diff --git a/unittest/lib/testScsTools.cpp b/unittest/lib/testScsTools.cpp index 128ffd2..901a939 100644 --- a/unittest/lib/testScsTools.cpp +++ b/unittest/lib/testScsTools.cpp @@ -6,6 +6,7 @@ #include "precomp.h" +#define MAX_MAP_UINT32_LEN 99 VOID testScsRotateBuffer() @@ -127,51 +128,105 @@ testBasicMaskFunctions() } +VOID +initMapUint32(PSYMCRYPT_UINT32_MAP pMap, UINT32 uMapLen) +{ + UINT32 uDuplication; + GENRANDOM(&uDuplication, sizeof(uDuplication)); + + // Limiting the number of duplicates of 'from' values to be at most uMapLen + uDuplication = uDuplication % uMapLen; + + // Generating fully random map + for (UINT32 i = 0; i < uMapLen; ++i) + { + GENRANDOM(&(pMap[i].from), sizeof(pMap[i].from)); + GENRANDOM(&(pMap[i].to), sizeof(pMap[i].to)); + } + + // forcing duplications + for (UINT32 i = 0; i < uDuplication; ++i) + { + UINT32 copyFromEntry; + UINT32 copyToEntry; + + GENRANDOM(©FromEntry, sizeof(copyFromEntry)); + GENRANDOM(©ToEntry, sizeof(copyToEntry)); + + copyFromEntry = copyFromEntry % uMapLen; + copyToEntry = copyToEntry % uMapLen; + + pMap[copyToEntry].from = pMap[copyFromEntry].from; + } +} + +bool +validateMapping(UINT32 u32Input, PCSYMCRYPT_UINT32_MAP pMap, UINT32 uMapLen) +{ + UINT32 u32Default; + UINT32 u32Result; + + bool result = false; + bool exists = false; + + GENRANDOM(&u32Default, sizeof(u32Default)); + + u32Result = SymCryptMapUint32(u32Input, u32Default, pMap, uMapLen); + + for (UINT32 i = 0; i < uMapLen; ++i) + { + if (pMap[i].from == u32Input) + { + exists = true; + if (pMap[i].to == u32Result) + { + result = true; + break; + } + } + } + + if (!exists && u32Result == u32Default) + { + result = true; + } + + return result; +} + VOID testScsMapUint32() { - UINT32 u32Input; - UINT32 u32Default; - UINT32 u32From1; - UINT32 u32To1; - UINT32 u32From2; - UINT32 u32To2; - UINT32 u32To3; + UINT32 uMapLen; + GENRANDOM(&uMapLen, sizeof(uMapLen)); - GENRANDOM(&u32Default, sizeof(u32Default)); - GENRANDOM(&u32From1, sizeof(u32From1)); - GENRANDOM(&u32From2, sizeof(u32From2)); - GENRANDOM(&u32To1, sizeof(u32To1)); - GENRANDOM(&u32To2, sizeof(u32To2)); - GENRANDOM(&u32To3, sizeof(u32To3)); + // Limiting the length of the map to be between 1 to 100. + // We want to avoid a creation of huge map. + uMapLen = (uMapLen % MAX_MAP_UINT32_LEN) + 1; - SYMCRYPT_UINT32_MAP pMap[3] = { - { u32From1, u32To1 }, - { u32From2, u32To2 }, - { u32From2, u32To3 } }; // multiple map entries may have the same 'from' + PSYMCRYPT_UINT32_MAP pMap = new SYMCRYPT_UINT32_MAP[uMapLen]; + CHECK(pMap != NULL, "Out of memory"); - // - // Case 1: u32Input matches the 'from' field of only one entry - // + // Initiating map with random values + initMapUint32(pMap, uMapLen); - CHECK(SymCryptMapUint32(u32From1, u32Default, pMap, 2) == u32To1, "SymCryptMapUint32"); - - // - // Case 2: u32Input matches the 'from' field of multiple entries - // - CHECK(SymCryptMapUint32(u32From2, u32Default, pMap, 2) == u32To2, "SymCryptMapUint32"); - - // - // Case 3: u32Input doesn't match the 'from' field of any entry - // - do + // Validating SymCryptMapUint32 result for every 'from' value in map + for (UINT32 i = 0; i < uMapLen; ++i) { - GENRANDOM(&u32Input, sizeof(u32Input)); - } - while (u32Input == u32From1 || u32Input == u32From2); + CHECK(validateMapping(pMap[i].from, pMap, uMapLen), "SymCryptMapUint32"); + } - CHECK(SymCryptMapUint32(u32Input, u32Default, pMap, 2) == u32Default, "SymCryptMapUint32"); + // Validating SymCryptMapUint32 result for random inputs + for (UINT32 i = 0; i < MAX_MAP_UINT32_LEN; ++i) + { + UINT32 u32RndInput; + GENRANDOM(&u32RndInput, sizeof(u32RndInput)); + CHECK(validateMapping(u32RndInput, pMap, uMapLen), "SymCryptMapUint32"); + } + + //cleanup + delete [] pMap; } VOID