Skip to content

Fix error in CT compare, add negative tests for TLS pad checks #385

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

Open
ColtonWilley wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix error in CT compare, add negative tests for TLS pad checks #385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/wp_internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1172,7 +1172,7 @@ byte wp_ct_byte_mask_eq(byte a, byte b)
*/
byte wp_ct_byte_mask_ne(byte a, byte b)
{
return (((int32_t)b - a) >> 31) & (((int32_t)a - b) >> 31);
return (((int32_t)b - a) >> 31) | (((int32_t)a - b) >> 31);
Copy link
Copy Markdown
Contributor

@padelsbach padelsbach Apr 5, 2026

Choose a reason for hiding this comment

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

This is potentially a more robust fix, with less bit math:

return ~wp_ct_byte_mask_eq(a, b);

}

/**
Expand Down
162 changes: 162 additions & 0 deletions test/test_cipher.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,86 @@ int test_des3_cbc_stream(void *data)
return err;
}

/*
* Negative PKCS#7 padding test for DES3-CBC.
* Encrypt block-aligned plaintext (produces a full padding block), corrupt a
* ciphertext byte so the padding block is garbled, verify DecryptFinal rejects.
*/
int test_des3_cbc_bad_pad(void *data)
Copy link
Copy Markdown
Contributor

@padelsbach padelsbach Apr 5, 2026

Choose a reason for hiding this comment

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

I see that DES3 CBC is the only path for testing wp_ct_byte_mask_ne, currently. So this is a good add, but...

Can we add some test vectors for wp_ct_* functions? There seem to be no direct tests of these today. Just to ensure coverage.

{
int err = 0;
EVP_CIPHER *cipher = NULL;
EVP_CIPHER_CTX *ctx = NULL;
unsigned char key[24];
unsigned char iv[8];
unsigned char pt[8]; /* block-aligned: PKCS#7 adds full 8-byte pad block */
unsigned char ct[24]; /* 8 pt + 8 pad = 16, but EncryptFinal may need room */
unsigned char dec[24];
int outLen = 0, fLen = 0;

(void)data;

PRINT_MSG("DES3-CBC negative PKCS#7 padding");

memset(key, 0xAA, sizeof(key));
memset(iv, 0xBB, sizeof(iv));
memset(pt, 0x42, sizeof(pt));

cipher = EVP_CIPHER_fetch(wpLibCtx, "DES-EDE3-CBC", "");
if (cipher == NULL) {
err = 1;
}

/* Encrypt with padding. */
if (err == 0) {
ctx = EVP_CIPHER_CTX_new();
if (ctx == NULL)
err = 1;
}
if (err == 0) {
err = EVP_EncryptInit_ex(ctx, cipher, NULL, key, iv) != 1;
}
if (err == 0) {
err = EVP_EncryptUpdate(ctx, ct, &outLen, pt, (int)sizeof(pt)) != 1;
}
if (err == 0) {
err = EVP_EncryptFinal_ex(ctx, ct + outLen, &fLen) != 1;
outLen += fLen;
}
EVP_CIPHER_CTX_free(ctx);
ctx = NULL;

/* Corrupt first ciphertext block -- CBC garbles the padding block. */
if (err == 0) {
ct[0] ^= 0x01;
}

/* Decrypt -- DecryptFinal should fail due to garbled padding. */
if (err == 0) {
ctx = EVP_CIPHER_CTX_new();
if (ctx == NULL)
err = 1;
}
if (err == 0) {
err = EVP_DecryptInit_ex(ctx, cipher, NULL, key, iv) != 1;
}
if (err == 0) {
fLen = 0;
err = EVP_DecryptUpdate(ctx, dec, &fLen, ct, outLen) != 1;
}
if (err == 0) {
int ret = EVP_DecryptFinal_ex(ctx, dec + fLen, &fLen);
if (ret == 1) {
PRINT_ERR_MSG("DES3-CBC bad-pad: DecryptFinal should have failed");
err = 1;
}
}

EVP_CIPHER_CTX_free(ctx);
EVP_CIPHER_free(cipher);
return err;
}

#endif /* WP_HAVE_DES3CBC */

/******************************************************************************/
Expand Down Expand Up @@ -1326,4 +1406,86 @@ int test_aes256_cbc_multiple(void *data)

return err;
}

/*
* Negative PKCS#7 padding test for AES-256-CBC.
* Encrypt block-aligned plaintext (produces a full padding block), corrupt a
* ciphertext byte so the padding block is garbled, verify DecryptFinal rejects.
*/
int test_aes256_cbc_bad_pad(void *data)
{
int err = 0;
EVP_CIPHER *cipher = NULL;
EVP_CIPHER_CTX *ctx = NULL;
unsigned char key[32];
unsigned char iv[16];
unsigned char pt[16]; /* block-aligned: PKCS#7 adds full 16-byte pad block */
unsigned char ct[48];
unsigned char dec[48];
int outLen = 0, fLen = 0;

(void)data;

PRINT_MSG("AES-256-CBC negative PKCS#7 padding");

memset(key, 0xAA, sizeof(key));
memset(iv, 0xBB, sizeof(iv));
memset(pt, 0x42, sizeof(pt));

cipher = EVP_CIPHER_fetch(wpLibCtx, "AES-256-CBC", "");
if (cipher == NULL) {
err = 1;
}

/* Encrypt with padding. */
if (err == 0) {
ctx = EVP_CIPHER_CTX_new();
if (ctx == NULL)
err = 1;
}
if (err == 0) {
err = EVP_EncryptInit_ex(ctx, cipher, NULL, key, iv) != 1;
}
if (err == 0) {
err = EVP_EncryptUpdate(ctx, ct, &outLen, pt, (int)sizeof(pt)) != 1;
}
if (err == 0) {
err = EVP_EncryptFinal_ex(ctx, ct + outLen, &fLen) != 1;
outLen += fLen;
}
EVP_CIPHER_CTX_free(ctx);
ctx = NULL;

/* Corrupt first ciphertext block -- CBC garbles the padding block. */
if (err == 0) {
ct[0] ^= 0x01;
}

/* Decrypt -- DecryptFinal should fail due to garbled padding. */
if (err == 0) {
ctx = EVP_CIPHER_CTX_new();
if (ctx == NULL)
err = 1;
}
if (err == 0) {
err = EVP_DecryptInit_ex(ctx, cipher, NULL, key, iv) != 1;
}
if (err == 0) {
fLen = 0;
err = EVP_DecryptUpdate(ctx, dec, &fLen, ct, outLen) != 1;
}
if (err == 0) {
int ret = EVP_DecryptFinal_ex(ctx, dec + fLen, &fLen);
if (ret == 1) {
PRINT_ERR_MSG("AES-256-CBC bad-pad: DecryptFinal should have "
"failed");
err = 1;
}
}

EVP_CIPHER_CTX_free(ctx);
EVP_CIPHER_free(cipher);
return err;
}

#endif /* WP_HAVE_AESCBC */
Loading
Loading