Skip to content

fix: compatibility issue between SCOSSL and TLS 1.2 ciphersuites which use HMAC in AzL3 #129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
MS-megliu wants to merge 4 commits into from
Closed

fix: compatibility issue between SCOSSL and TLS 1.2 ciphersuites which use HMAC in AzL3 #129

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
69400db
ECDHE-AES-CBC-TLS1.2: draft fix, 0 based padding and copy_mac issue
MS-megliu Jul 7, 2025
93a522b
ECDHE-SHA384-TLS1.2: working version
MS-megliu Jul 7, 2025
e5115d7
cleanup
MS-megliu Jul 7, 2025
54e25da
address commments
MS-megliu Jul 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 77 additions & 29 deletions SymCryptProvider/src/ciphers/p_scossl_aes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,19 +155,18 @@ 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,
Comment thread
MS-megliu marked this conversation as resolved.
SCOSSL_STATUS paddingStatus)
{
// MAC rotation is performed in place
BYTE rotatedMacBuf[64 + EVP_MAX_MD_SIZE];
PBYTE rotatedMac;
Comment thread
MS-megliu marked this conversation as resolved.
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 = *record;
UINT32 macEnd = (UINT32)(*recordLen); // mac end index
UINT32 macStart = macEnd - ctx->tlsMacSize;
UINT32 inMac = 0;

Expand All @@ -186,8 +185,7 @@ 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;
Expand All @@ -200,7 +198,7 @@ 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)
{
Expand All @@ -222,23 +220,74 @@ static SCOSSL_STATUS p_scossl_aes_copy_mac(_Inout_ SCOSSL_AES_CTX *ctx,
}

// MAC rotation
for (i = 0, j = 0; i < ctx->tlsMacSize; i++)
for (i = 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);
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(paddingStatusByte, macByte, randMac[i]);
}
return SCOSSL_SUCCESS;
}

SCOSSL_STATUS scossl_tls_remove_padding(PBYTE buf, SIZE_T *len)
{
UINT32 cbPadVal;
UINT32 start;
UINT32 mPaddingError = 0;
BYTE last_block[SYMCRYPT_AES_BLOCK_SIZE];
SCOSSL_STATUS ret = SCOSSL_SUCCESS;

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;
Copy link
Copy Markdown
Contributor

@samuel-lee-msft samuel-lee-msft Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*len - SYMCRYPT_AES_BLOCK_SIZE;

off by on error - start should be
(*len - 1) - SYMCRYPT_AES_BLOCK_SIZE

for (UINT32 i = 0; i < SYMCRYPT_AES_BLOCK_SIZE; i++)
last_block[i] = buf[start + i];
Copy link
Copy Markdown
Contributor

@samuel-lee-msft samuel-lee-msft Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need to copy the data to a local last_block buffer - we can validate the padding directly in buf with the correct offsets


// 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);
}

ret ^= mPaddingError & (ret ^ SYMCRYPT_INVALID_ARGUMENT);

*len -= cbPadVal;
return SCOSSL_SUCCESS;
Copy link
Copy Markdown
Contributor

@samuel-lee-msft samuel-lee-msft Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return SCOSSL_SUCCESS;

return ret;

}
Comment thread
MS-megliu marked this conversation as resolved.

static
SCOSSL_STATUS scossl_tls_add_padding(PBYTE out, SIZE_T outsize, PCBYTE in, SIZE_T inl, SIZE_T *outlen)
{
UINT32 cbPadVal = SYMCRYPT_AES_BLOCK_SIZE - (inl % SYMCRYPT_AES_BLOCK_SIZE);
SIZE_T cbTotalLen = inl + cbPadVal;

ctx->tlsMac[j++] = SYMCRYPT_OPENSSL_MASK8_SELECT(paddingStatusByte, aux3, randMac[i]);
// Check if output buffer is large enough (non-secret, so safe to branch)
if (cbTotalLen > outsize)
return SCOSSL_FAILURE;

rotateOffset++;
rotateOffset = (rotateOffset & SYMCRYPT_MASK32_LT(rotateOffset, ctx->tlsMacSize));
// 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));
}
Copy link
Copy Markdown
Contributor

@samuel-lee-msft samuel-lee-msft Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the encryption path it's actually fine to be non constant time - the caller of the encryption is the producer of the plaintext, so there is not an attack. You can use memcpy / memset as before.

The decryption path is sensitive to sidechannels because the ciphertext could be legitimate or could be tampered with over the wire.


// 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.
*outlen = cbTotalLen;
return SCOSSL_SUCCESS;
}

Expand Down Expand Up @@ -268,11 +317,11 @@ 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);
if (!scossl_tls_add_padding(out, outsize, in, inl, &inl))
{
ERR_raise(ERR_LIB_PROV, PROV_R_OUTPUT_BUFFER_TOO_SMALL);
return SCOSSL_FAILURE;
}
}

if (inl % SYMCRYPT_AES_BLOCK_SIZE != 0 ||
Expand Down Expand Up @@ -310,12 +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;
}
scError = SymCryptPaddingPkcs7Remove(
SYMCRYPT_AES_BLOCK_SIZE,
out, *outl,
out, *outl,
outl);

if (!scossl_tls_remove_padding(out, outl))
{
ERR_raise(ERR_LIB_PROV, PROV_R_CIPHER_OPERATION_FAILED);
return SCOSSL_FAILURE;
}
return p_scossl_aes_copy_mac(ctx,
out, outl,
outlPadded,
Expand Down