From 69400dbdc939836d125fc1e7de21e7bed61733a0 Mon Sep 17 00:00:00 2001 From: liumegan Date: Mon, 7 Jul 2025 03:28:37 +0000 Subject: [PATCH 1/4] ECDHE-AES-CBC-TLS1.2: draft fix, 0 based padding and copy_mac issue --- SymCryptProvider/src/ciphers/p_scossl_aes.c | 77 ++++++++++++++++++--- 1 file changed, 69 insertions(+), 8 deletions(-) diff --git a/SymCryptProvider/src/ciphers/p_scossl_aes.c b/SymCryptProvider/src/ciphers/p_scossl_aes.c index bfcf606d..ee23b6a5 100644 --- a/SymCryptProvider/src/ciphers/p_scossl_aes.c +++ b/SymCryptProvider/src/ciphers/p_scossl_aes.c @@ -167,7 +167,7 @@ static SCOSSL_STATUS p_scossl_aes_copy_mac(_Inout_ SCOSSL_AES_CTX *ctx, // Random mac set in case padding removal failed BYTE randMac[EVP_MAX_MD_SIZE]; - UINT32 macEnd = *record; + UINT32 macEnd = *recordLen; UINT32 macStart = macEnd - ctx->tlsMacSize; UINT32 inMac = 0; @@ -236,12 +236,51 @@ static SCOSSL_STATUS p_scossl_aes_copy_mac(_Inout_ SCOSSL_AES_CTX *ctx, rotateOffset++; rotateOffset = (rotateOffset & SYMCRYPT_MASK32_LT(rotateOffset, ctx->tlsMacSize)); } - +//need to revisit + for (i = 0; i < ctx->tlsMacSize; i++) + { + ctx->tlsMac[i] = record[i+macStart]; + } // If we failed, we still succeed, but the MAC is set to some // random value. It's up to the caller to check the MAC. return SCOSSL_SUCCESS; } +static +SCOSSL_STATUS scossl_tls_remove_padding(unsigned char *buf, size_t *len) +{ + size_t padlen = buf[*len - 1]; // zero-based + size_t totalPad = padlen + 1; + if (totalPad > *len) + { + printf("\n[MEGLIU] remove padding, totalPad %ld > *len %ld\n", totalPad, *len); + return SCOSSL_FAILURE; + } + // Check all padding bytes equal padlen + for (size_t i = *len - totalPad; i < *len; i++) { + if (buf[i] != padlen) + return SCOSSL_FAILURE; + } + + *len -= totalPad; + return SCOSSL_SUCCESS; +} + +static int scossl_tls_add_padding(unsigned char *out, size_t outsize, const unsigned char *in, size_t inl, size_t *outlen) +{ + size_t blocksize = 16; + size_t padlen = blocksize - (inl % blocksize); + if (padlen == 0) + padlen = blocksize; + + if (inl + padlen > outsize) + return 0; // Buffer too small + + memcpy(out, in, inl); + memset(out + inl, (unsigned char)(padlen - 1), padlen); + *outlen = inl + padlen; + return 1; +} static SCOSSL_STATUS p_scossl_aes_generic_block_update(_Inout_ SCOSSL_AES_CTX *ctx, _Out_writes_bytes_(*outl) unsigned char *out, _Out_ size_t *outl, size_t outsize, _In_reads_bytes_(inl) const unsigned char *in, size_t inl) @@ -268,11 +307,22 @@ static SCOSSL_STATUS p_scossl_aes_generic_block_update(_Inout_ SCOSSL_AES_CTX *c if (ctx->encrypt) { - // in == out - SymCryptPaddingPkcs7Add( - SYMCRYPT_AES_BLOCK_SIZE, - in, inl, - out, outsize, &inl); + // TLS 1.1+ uses explicit IV and TLS-style (zero-based) padding + if (ctx->tlsVersion >= TLS1_1_VERSION) + { + if (!scossl_tls_add_padding(out, outsize, in, inl, &inl)) + { + ERR_raise(ERR_LIB_PROV, PROV_R_OUTPUT_BUFFER_TOO_SMALL); + return SCOSSL_FAILURE; + } + } + else + { + SymCryptPaddingPkcs7Add( + SYMCRYPT_AES_BLOCK_SIZE, + in, inl, + out, outsize, &inl); + } } if (inl % SYMCRYPT_AES_BLOCK_SIZE != 0 || @@ -301,7 +351,18 @@ static SCOSSL_STATUS p_scossl_aes_generic_block_update(_Inout_ SCOSSL_AES_CTX *c case DTLS1_BAD_VER: out += SYMCRYPT_AES_BLOCK_SIZE; *outl -= SYMCRYPT_AES_BLOCK_SIZE; - __attribute__ ((fallthrough)); + outlPadded = *outl; + + if (ctx->tlsMacSize > *outl) + { + ERR_raise(ERR_LIB_PROV, PROV_R_CIPHER_OPERATION_FAILED); + return SCOSSL_FAILURE; + } + scossl_tls_remove_padding(out, outl); + return p_scossl_aes_copy_mac(ctx, + out, outl, + outlPadded, + SymCryptMapUint32(scError, SCOSSL_FAILURE, scErrorMap, 1)); case TLS1_VERSION: outlPadded = *outl; From 93a522bdbd887240a59a821af6ef4190750cc2b0 Mon Sep 17 00:00:00 2001 From: liumegan Date: Mon, 7 Jul 2025 04:58:13 +0000 Subject: [PATCH 2/4] ECDHE-SHA384-TLS1.2: working version --- SymCryptProvider/src/ciphers/p_scossl_aes.c | 45 +++++++-------------- 1 file changed, 15 insertions(+), 30 deletions(-) diff --git a/SymCryptProvider/src/ciphers/p_scossl_aes.c b/SymCryptProvider/src/ciphers/p_scossl_aes.c index ee23b6a5..96b42c62 100644 --- a/SymCryptProvider/src/ciphers/p_scossl_aes.c +++ b/SymCryptProvider/src/ciphers/p_scossl_aes.c @@ -155,19 +155,16 @@ static SCOSSL_STATUS p_scossl_aes_generic_decrypt_init(_Inout_ SCOSSL_AES_CTX *c // This function is adapted from ssl3_cbc_copy_mac in ssl/record/tls_pad.c // and runs in constant time. In case of bad padding, a random MAC is assigned instead static SCOSSL_STATUS p_scossl_aes_copy_mac(_Inout_ SCOSSL_AES_CTX *ctx, - _In_reads_bytes_(buflen) unsigned char *record, _Inout_ SIZE_T *recordLen, + _In_reads_bytes_(buflen) unsigned char *record, + _Inout_ SIZE_T *recordLen, SIZE_T recordLenPadded, SCOSSL_STATUS paddingStatus) { - // MAC rotation is performed in place BYTE rotatedMacBuf[64 + EVP_MAX_MD_SIZE]; PBYTE rotatedMac; - BYTE aux1, aux2, aux3, mask; - BYTE paddingStatusByte = (BYTE) (paddingStatus & 0xff); - // Random mac set in case padding removal failed BYTE randMac[EVP_MAX_MD_SIZE]; - UINT32 macEnd = *recordLen; + UINT32 macEnd = (UINT32)(*recordLen); // mac end index UINT32 macStart = macEnd - ctx->tlsMacSize; UINT32 inMac = 0; @@ -178,8 +175,6 @@ static SCOSSL_STATUS p_scossl_aes_copy_mac(_Inout_ SCOSSL_AES_CTX *ctx, OPENSSL_free(ctx->tlsMac); ctx->tlsMac = NULL; - // Public info, safe to branch - // No mac to copy if (ctx->tlsMacSize == 0) { return paddingStatus; @@ -187,7 +182,6 @@ static SCOSSL_STATUS p_scossl_aes_copy_mac(_Inout_ SCOSSL_AES_CTX *ctx, *recordLen -= ctx->tlsMacSize; - // Generate random bytes in case of bad padding if (RAND_bytes_ex(ctx->libctx, randMac, ctx->tlsMacSize, 0) <= 0) { return SCOSSL_FAILURE; @@ -201,13 +195,12 @@ static SCOSSL_STATUS p_scossl_aes_copy_mac(_Inout_ SCOSSL_AES_CTX *ctx, rotatedMac = rotatedMacBuf + ((0 - (SIZE_T)rotatedMacBuf) & 0x3f); - // Public info, safe to branch if (recordLenPadded > ctx->tlsMacSize + 255 + 1) { scanStart = recordLenPadded - (ctx->tlsMacSize + 255 + 1); } - // Find and extract MAC + // Extract MAC bytes in constant time memset(rotatedMac, 0, ctx->tlsMacSize); for (i = scanStart, j = 0; i < recordLenPadded; i++) { @@ -221,30 +214,23 @@ static SCOSSL_STATUS p_scossl_aes_copy_mac(_Inout_ SCOSSL_AES_CTX *ctx, j &= SYMCRYPT_MASK32_LT(j, ctx->tlsMacSize); } - // MAC rotation - for (i = 0, j = 0; i < ctx->tlsMacSize; i++) - { - // in case cache-line is 32 bytes, load from both lines and select appropriately - aux1 = rotatedMac[rotateOffset & ~0x20]; - aux2 = rotatedMac[rotateOffset | 0x20]; - mask = (BYTE) SYMCRYPT_MASK32_EQ(rotateOffset & !0x20, rotateOffset); - aux3 = SYMCRYPT_OPENSSL_MASK8_SELECT(mask, aux1, aux2); - + // Convert paddingStatus to full 0xFF mask or 0x00 + BYTE mask = paddingStatus ? 0xFF : 0x00; - ctx->tlsMac[j++] = SYMCRYPT_OPENSSL_MASK8_SELECT(paddingStatusByte, aux3, randMac[i]); - - rotateOffset++; - rotateOffset = (rotateOffset & SYMCRYPT_MASK32_LT(rotateOffset, ctx->tlsMacSize)); - } -//need to revisit + // Rotate back MAC by rotateOffset, in constant time for (i = 0; i < ctx->tlsMacSize; i++) { - ctx->tlsMac[i] = record[i+macStart]; + BYTE macByte = 0; + for (j = 0; j < ctx->tlsMacSize; j++) { + UINT32 match = SYMCRYPT_MASK32_EQ(j, (rotateOffset + i) % ctx->tlsMacSize); + macByte |= rotatedMac[j] & match; + } + ctx->tlsMac[i] = SYMCRYPT_OPENSSL_MASK8_SELECT(mask, macByte, randMac[i]); } - // If we failed, we still succeed, but the MAC is set to some - // random value. It's up to the caller to check the MAC. + return SCOSSL_SUCCESS; } + static SCOSSL_STATUS scossl_tls_remove_padding(unsigned char *buf, size_t *len) { @@ -253,7 +239,6 @@ SCOSSL_STATUS scossl_tls_remove_padding(unsigned char *buf, size_t *len) if (totalPad > *len) { - printf("\n[MEGLIU] remove padding, totalPad %ld > *len %ld\n", totalPad, *len); return SCOSSL_FAILURE; } // Check all padding bytes equal padlen From e5115d7f2e2f0de3229cb062bff7bdead7588ee5 Mon Sep 17 00:00:00 2001 From: liumegan Date: Mon, 7 Jul 2025 16:16:33 +0000 Subject: [PATCH 3/4] cleanup --- SymCryptProvider/src/ciphers/p_scossl_aes.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/SymCryptProvider/src/ciphers/p_scossl_aes.c b/SymCryptProvider/src/ciphers/p_scossl_aes.c index 96b42c62..1121e2cb 100644 --- a/SymCryptProvider/src/ciphers/p_scossl_aes.c +++ b/SymCryptProvider/src/ciphers/p_scossl_aes.c @@ -160,6 +160,7 @@ static SCOSSL_STATUS p_scossl_aes_copy_mac(_Inout_ SCOSSL_AES_CTX *ctx, SIZE_T recordLenPadded, SCOSSL_STATUS paddingStatus) { + // MAC rotation is performed in place BYTE rotatedMacBuf[64 + EVP_MAX_MD_SIZE]; PBYTE rotatedMac; BYTE randMac[EVP_MAX_MD_SIZE]; @@ -175,13 +176,15 @@ static SCOSSL_STATUS p_scossl_aes_copy_mac(_Inout_ SCOSSL_AES_CTX *ctx, OPENSSL_free(ctx->tlsMac); ctx->tlsMac = NULL; + // Public info, safe to branch + // No mac to copy if (ctx->tlsMacSize == 0) { return paddingStatus; } *recordLen -= ctx->tlsMacSize; - + if (RAND_bytes_ex(ctx->libctx, randMac, ctx->tlsMacSize, 0) <= 0) { return SCOSSL_FAILURE; @@ -194,13 +197,14 @@ static SCOSSL_STATUS p_scossl_aes_copy_mac(_Inout_ SCOSSL_AES_CTX *ctx, } rotatedMac = rotatedMacBuf + ((0 - (SIZE_T)rotatedMacBuf) & 0x3f); - + + // Public info, safe to branch if (recordLenPadded > ctx->tlsMacSize + 255 + 1) { scanStart = recordLenPadded - (ctx->tlsMacSize + 255 + 1); } - // Extract MAC bytes in constant time + // Find and extract MAC memset(rotatedMac, 0, ctx->tlsMacSize); for (i = scanStart, j = 0; i < recordLenPadded; i++) { @@ -216,8 +220,7 @@ static SCOSSL_STATUS p_scossl_aes_copy_mac(_Inout_ SCOSSL_AES_CTX *ctx, // Convert paddingStatus to full 0xFF mask or 0x00 BYTE mask = paddingStatus ? 0xFF : 0x00; - - // Rotate back MAC by rotateOffset, in constant time + // MAC rotation for (i = 0; i < ctx->tlsMacSize; i++) { BYTE macByte = 0; @@ -227,7 +230,6 @@ static SCOSSL_STATUS p_scossl_aes_copy_mac(_Inout_ SCOSSL_AES_CTX *ctx, } ctx->tlsMac[i] = SYMCRYPT_OPENSSL_MASK8_SELECT(mask, macByte, randMac[i]); } - return SCOSSL_SUCCESS; } From 54e25da8b67c6ef0af3da663addb36e906e1078e Mon Sep 17 00:00:00 2001 From: liumegan Date: Mon, 7 Jul 2025 21:54:50 +0000 Subject: [PATCH 4/4] address commments --- SymCryptProvider/src/ciphers/p_scossl_aes.c | 114 ++++++++++---------- 1 file changed, 57 insertions(+), 57 deletions(-) diff --git a/SymCryptProvider/src/ciphers/p_scossl_aes.c b/SymCryptProvider/src/ciphers/p_scossl_aes.c index 1121e2cb..2b35af73 100644 --- a/SymCryptProvider/src/ciphers/p_scossl_aes.c +++ b/SymCryptProvider/src/ciphers/p_scossl_aes.c @@ -163,6 +163,7 @@ static SCOSSL_STATUS p_scossl_aes_copy_mac(_Inout_ SCOSSL_AES_CTX *ctx, // MAC rotation is performed in place BYTE rotatedMacBuf[64 + EVP_MAX_MD_SIZE]; PBYTE rotatedMac; + BYTE paddingStatusByte = (BYTE) (paddingStatus & 0xff); BYTE randMac[EVP_MAX_MD_SIZE]; UINT32 macEnd = (UINT32)(*recordLen); // mac end index @@ -218,8 +219,6 @@ static SCOSSL_STATUS p_scossl_aes_copy_mac(_Inout_ SCOSSL_AES_CTX *ctx, j &= SYMCRYPT_MASK32_LT(j, ctx->tlsMacSize); } - // Convert paddingStatus to full 0xFF mask or 0x00 - BYTE mask = paddingStatus ? 0xFF : 0x00; // MAC rotation for (i = 0; i < ctx->tlsMacSize; i++) { @@ -228,46 +227,70 @@ static SCOSSL_STATUS p_scossl_aes_copy_mac(_Inout_ SCOSSL_AES_CTX *ctx, UINT32 match = SYMCRYPT_MASK32_EQ(j, (rotateOffset + i) % ctx->tlsMacSize); macByte |= rotatedMac[j] & match; } - ctx->tlsMac[i] = SYMCRYPT_OPENSSL_MASK8_SELECT(mask, macByte, randMac[i]); + ctx->tlsMac[i] = SYMCRYPT_OPENSSL_MASK8_SELECT(paddingStatusByte, macByte, randMac[i]); } return SCOSSL_SUCCESS; } -static -SCOSSL_STATUS scossl_tls_remove_padding(unsigned char *buf, size_t *len) +SCOSSL_STATUS scossl_tls_remove_padding(PBYTE buf, SIZE_T *len) { - size_t padlen = buf[*len - 1]; // zero-based - size_t totalPad = padlen + 1; + UINT32 cbPadVal; + UINT32 start; + UINT32 mPaddingError = 0; + BYTE last_block[SYMCRYPT_AES_BLOCK_SIZE]; + SCOSSL_STATUS ret = SCOSSL_SUCCESS; - if (totalPad > *len) - { + if (*len == 0 || (*len % SYMCRYPT_AES_BLOCK_SIZE) != 0) return SCOSSL_FAILURE; + + cbPadVal = buf[*len - 1] + 1; + mPaddingError |= SymCryptMask32IsZeroU31(cbPadVal); + + // If cbPadVal is greater than cbBlockSize, + // we have to limit cbPadVal to be at most equal to cbBlockSize. + cbPadVal = 1 + ((cbPadVal - 1) & (SYMCRYPT_AES_BLOCK_SIZE - 1)); + // Copy last block to local buffer to ensure data-invariant access + start = *len - SYMCRYPT_AES_BLOCK_SIZE; + for (UINT32 i = 0; i < SYMCRYPT_AES_BLOCK_SIZE; i++) + last_block[i] = buf[start + i]; + + // Validate padding in constant time + for (UINT32 i = 0; i < SYMCRYPT_AES_BLOCK_SIZE; i++) { + UINT32 mask = (i >= SYMCRYPT_AES_BLOCK_SIZE - cbPadVal); + UINT32 diff = last_block[i] ^ cbPadVal; + mPaddingError |= (diff & -mask); } - // Check all padding bytes equal padlen - for (size_t i = *len - totalPad; i < *len; i++) { - if (buf[i] != padlen) - return SCOSSL_FAILURE; - } - *len -= totalPad; + ret ^= mPaddingError & (ret ^ SYMCRYPT_INVALID_ARGUMENT); + + *len -= cbPadVal; return SCOSSL_SUCCESS; } -static int scossl_tls_add_padding(unsigned char *out, size_t outsize, const unsigned char *in, size_t inl, size_t *outlen) +static +SCOSSL_STATUS scossl_tls_add_padding(PBYTE out, SIZE_T outsize, PCBYTE in, SIZE_T inl, SIZE_T *outlen) { - size_t blocksize = 16; - size_t padlen = blocksize - (inl % blocksize); - if (padlen == 0) - padlen = blocksize; - - if (inl + padlen > outsize) - return 0; // Buffer too small - - memcpy(out, in, inl); - memset(out + inl, (unsigned char)(padlen - 1), padlen); - *outlen = inl + padlen; - return 1; + UINT32 cbPadVal = SYMCRYPT_AES_BLOCK_SIZE - (inl % SYMCRYPT_AES_BLOCK_SIZE); + SIZE_T cbTotalLen = inl + cbPadVal; + + // Check if output buffer is large enough (non-secret, so safe to branch) + if (cbTotalLen > outsize) + return SCOSSL_FAILURE; + + // Copy input to output + for (UINT32 i = 0; i < inl; i++) + out[i] = in[i]; + + // Write padding in constant time + for (UINT32 i = 0; i < SYMCRYPT_AES_BLOCK_SIZE; i++) { + UINT32 mask = (UINT32)(i < cbPadVal); // mask = 1 if i < padval, else 0 + out[inl + i] = (BYTE)((out[inl + i] & ~mask) | ((cbPadVal - 1) & mask)); + } + + *outlen = cbTotalLen; + return SCOSSL_SUCCESS; } + static SCOSSL_STATUS p_scossl_aes_generic_block_update(_Inout_ SCOSSL_AES_CTX *ctx, _Out_writes_bytes_(*outl) unsigned char *out, _Out_ size_t *outl, size_t outsize, _In_reads_bytes_(inl) const unsigned char *in, size_t inl) @@ -294,21 +317,10 @@ static SCOSSL_STATUS p_scossl_aes_generic_block_update(_Inout_ SCOSSL_AES_CTX *c if (ctx->encrypt) { - // TLS 1.1+ uses explicit IV and TLS-style (zero-based) padding - if (ctx->tlsVersion >= TLS1_1_VERSION) + if (!scossl_tls_add_padding(out, outsize, in, inl, &inl)) { - if (!scossl_tls_add_padding(out, outsize, in, inl, &inl)) - { - ERR_raise(ERR_LIB_PROV, PROV_R_OUTPUT_BUFFER_TOO_SMALL); - return SCOSSL_FAILURE; - } - } - else - { - SymCryptPaddingPkcs7Add( - SYMCRYPT_AES_BLOCK_SIZE, - in, inl, - out, outsize, &inl); + ERR_raise(ERR_LIB_PROV, PROV_R_OUTPUT_BUFFER_TOO_SMALL); + return SCOSSL_FAILURE; } } @@ -338,6 +350,8 @@ static SCOSSL_STATUS p_scossl_aes_generic_block_update(_Inout_ SCOSSL_AES_CTX *c case DTLS1_BAD_VER: out += SYMCRYPT_AES_BLOCK_SIZE; *outl -= SYMCRYPT_AES_BLOCK_SIZE; + __attribute__ ((fallthrough)); + case TLS1_VERSION: outlPadded = *outl; if (ctx->tlsMacSize > *outl) @@ -345,25 +359,11 @@ static SCOSSL_STATUS p_scossl_aes_generic_block_update(_Inout_ SCOSSL_AES_CTX *c ERR_raise(ERR_LIB_PROV, PROV_R_CIPHER_OPERATION_FAILED); return SCOSSL_FAILURE; } - scossl_tls_remove_padding(out, outl); - return p_scossl_aes_copy_mac(ctx, - out, outl, - outlPadded, - SymCryptMapUint32(scError, SCOSSL_FAILURE, scErrorMap, 1)); - case TLS1_VERSION: - outlPadded = *outl; - - if (ctx->tlsMacSize > *outl) + if (!scossl_tls_remove_padding(out, outl)) { ERR_raise(ERR_LIB_PROV, PROV_R_CIPHER_OPERATION_FAILED); return SCOSSL_FAILURE; } - scError = SymCryptPaddingPkcs7Remove( - SYMCRYPT_AES_BLOCK_SIZE, - out, *outl, - out, *outl, - outl); - return p_scossl_aes_copy_mac(ctx, out, outl, outlPadded,