Skip to content

Fix PKCS#7 padding for block-aligned input in wc_EncryptPKCS8Key_ex #10836

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
yosuke-wolfssl wants to merge 1 commit into
base: master
Choose a base branch
from
+116 −2
Open

Fix PKCS#7 padding for block-aligned input in wc_EncryptPKCS8Key_ex #10836

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
110 changes: 110 additions & 0 deletions tests/api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,11 @@
#include <wolfssl/wolfcrypt/asn.h>
#endif

#if defined(HAVE_PKCS8) && !defined(NO_ASN)
/* for ASN tag and PBE/PKCS enum values used by the PKCS#8 encrypt tests */
#include <wolfssl/wolfcrypt/asn.h>

@Frauschi Frauschi Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 [Low] Redundant asn.h include
🔧 NIT convention

This #include <wolfssl/wolfcrypt/asn.h> under #if defined(HAVE_PKCS8) && !defined(NO_ASN) duplicates the conditional include at lines 79–81 (under WOLFSSL_SMALL_CERT_VERIFY). asn.h has include guards so it's harmless, and the new guard does correctly ensure the ASN tag / PBE enum symbols are available when the small-cert path isn't compiled.

Optional: fold this into the existing include block to avoid the duplication, or leave as-is.

#endif

#ifndef NO_DSA
#include <wolfssl/wolfcrypt/dsa.h>
#endif
Expand Down Expand Up @@ -21830,6 +21835,108 @@ static int test_wc_CreateEncryptedPKCS8Key(void)
return EXPECT_RESULT();
}

#if defined(HAVE_PKCS8) && !defined(NO_ASN) && !defined(NO_PWDBASED) && \
!defined(NO_SHA) && !defined(NO_ASN_CRYPT) && ((defined(WOLFSSL_AES_256) && \
!defined(NO_AES_CBC)) || !defined(NO_DES3) || !defined(NO_RC4))
/* Encrypt a block-aligned plaintext PKCS#8 and verify the trailing encrypted
* OCTET STRING length. expExtra is the padding expected: a full block for CBC
* ciphers, 0 for stream ciphers. Also confirms a decrypt round-trip. */
static int enc_pkcs8_pad_check(int vPKCS, int pbeOid, int encAlgId,
word32 expExtra)
{
EXPECT_DECLS;
WC_RNG rng;
byte* encKey = NULL;
word32 encKeySz = 0;
int decKeySz = 0;
word32 expEncLen = 0;
const char password[] = "Lorem ipsum dolor sit amet";
word32 passwordSz = (word32)XSTRLEN(password);
/* Block-aligned plaintext: a valid 48-byte DER SEQUENCE (a multiple of both
* the 8- and 16-byte block sizes). Content is arbitrary; only the header
* must parse so decrypt can strip padding by re-reading the DER length. */
byte plain[48];

XMEMSET(plain, 0, sizeof(plain));
plain[0] = ASN_SEQUENCE | ASN_CONSTRUCTED;
plain[1] = (byte)(sizeof(plain) - 2); /* content length = 46 */

XMEMSET(&rng, 0, sizeof(WC_RNG));
ExpectIntEQ(wc_InitRng(&rng), 0);
PRIVATE_KEY_UNLOCK();
/* Query required output size. */
ExpectIntEQ(wc_EncryptPKCS8Key(plain, (word32)sizeof(plain), NULL, &encKeySz,
password, (int)passwordSz, vPKCS, pbeOid, encAlgId, NULL, 0,
WC_PKCS12_ITT_DEFAULT, &rng, NULL), WC_NO_ERR_TRACE(LENGTH_ONLY_E));
ExpectNotNull(encKey = (byte*)XMALLOC(encKeySz, HEAP_HINT,
DYNAMIC_TYPE_TMP_BUFFER));
ExpectIntGT(wc_EncryptPKCS8Key(plain, (word32)sizeof(plain), encKey,
&encKeySz, password, (int)passwordSz, vPKCS, pbeOid, encAlgId, NULL, 0,
WC_PKCS12_ITT_DEFAULT, &rng, NULL), 0);

/* The encrypted content is the trailing OCTET STRING, extending to the end
* of the buffer. Its length is plaintext + expected pad (short-form since
* it is < 128): a full block for a block cipher, none for a stream cipher. */
expEncLen = (word32)sizeof(plain) + expExtra;
ExpectIntGE(encKeySz, expEncLen + 2);
if (EXPECT_SUCCESS() && (encKey != NULL) && (encKeySz >= expEncLen + 2)) {
ExpectIntEQ(encKey[encKeySz - expEncLen - 2], ASN_OCTET_STRING);

@Frauschi Frauschi Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 [Medium] Length check assumes short-form DER length
💡 SUGGEST test

This reads encKey[encKeySz - expEncLen - 1] as a single short-form length byte and compares it to (byte)expEncLen. That only holds while the encrypted content is < 128 bytes (short-form DER length), which is true for the current fixtures (48/56/64 bytes).

If this helper is later reused for larger keys — e.g. ML-DSA private keys, which run to several KB — the OCTET STRING length switches to long-form (0x82 | len_hi | len_lo). Both reads then land on the wrong bytes: the real tag sits two bytes earlier, and the compared value is a truncated 8-bit slice of a multi-KB length. The helper would silently false-fail (or false-pass).

Optional hardening: guard with expEncLen < 128 so a future larger-plaintext caller fails loudly rather than mis-parsing the length field. Longer term, if PKCS#8-encrypt coverage is added for ML-DSA keys, the helper would need to decode long-form lengths properly.

ExpectIntEQ(encKey[encKeySz - expEncLen - 1], (byte)expEncLen);
}

/* Round-trip: decrypt recovers the original plaintext. */
ExpectIntGT(decKeySz = wc_DecryptPKCS8Key(encKey, encKeySz, password,
(int)passwordSz), 0);
ExpectIntEQ(decKeySz, (int)sizeof(plain));
ExpectIntEQ(XMEMCMP(encKey, plain, sizeof(plain)), 0);
PRIVATE_KEY_LOCK();

XFREE(encKey, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER);
wc_FreeRng(&rng);
return EXPECT_RESULT();
}
#endif

/* PBES2 / AES-256-CBC (blockSz 16): a block-aligned plaintext must get a full
* pad block. The path the fix corrects; the helper's direct length check (not
* its round-trip, which false-passes here) is what detects a missing block. */
static int test_wc_EncryptPKCS8Key_blockAligned(void)
{
EXPECT_DECLS;
#if defined(HAVE_PKCS8) && !defined(NO_ASN) && !defined(NO_PWDBASED) \
&& defined(WOLFSSL_AES_256) && !defined(NO_AES_CBC) && !defined(NO_SHA) \
&& !defined(NO_ASN_CRYPT)
EXPECT_TEST(enc_pkcs8_pad_check(PKCS5, PBES2, AES256CBCb, AES_BLOCK_SIZE));
#endif
return EXPECT_RESULT();
}

/* PBES1 / SHA1-DES (blockSz 8): exercises the PBES1 encoder branch and a
* different block size; a block-aligned plaintext gets a full 8-byte pad
* block. */
static int test_wc_EncryptPKCS8Key_pbes1BlockAligned(void)
{
EXPECT_DECLS;
#if defined(HAVE_PKCS8) && !defined(NO_ASN) && !defined(NO_PWDBASED) \
&& !defined(NO_DES3) && !defined(NO_SHA) && !defined(NO_ASN_CRYPT)
EXPECT_TEST(enc_pkcs8_pad_check(PKCS5, PBES1_SHA1_DES, 0, DES_BLOCK_SIZE));
#endif
return EXPECT_RESULT();
}

/* PKCS12 / RC4 (blockSz 1): a stream cipher adds no padding even for a
* block-aligned plaintext. Exercises the blockSz > 1 guard in
* wc_EncryptPKCS8Key_ex. */
static int test_wc_EncryptPKCS8Key_rc4NoPad(void)
{
EXPECT_DECLS;
#if defined(HAVE_PKCS8) && !defined(NO_ASN) && !defined(NO_PWDBASED) \
&& !defined(NO_RC4) && !defined(NO_SHA) && !defined(NO_ASN_CRYPT)
EXPECT_TEST(enc_pkcs8_pad_check(1, PBE_SHA1_RC4_128, 0, 0));
#endif
return EXPECT_RESULT();
}

static int test_wc_DecryptedPKCS8Key(void)
{
EXPECT_DECLS;
Expand Down Expand Up @@ -35063,6 +35170,9 @@ TEST_CASE testCases[] = {
/* wolfCrypt ASN tests */
TEST_DECL(test_ToTraditional),
TEST_DECL(test_wc_CreateEncryptedPKCS8Key),
TEST_DECL(test_wc_EncryptPKCS8Key_blockAligned),
TEST_DECL(test_wc_EncryptPKCS8Key_pbes1BlockAligned),
TEST_DECL(test_wc_EncryptPKCS8Key_rc4NoPad),
TEST_DECL(test_wc_DecryptedPKCS8Key),
TEST_DECL(test_wc_GetPkcs8TraditionalOffset),

Expand Down
8 changes: 6 additions & 2 deletions wolfcrypt/src/asn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10345,8 +10345,12 @@ int wc_EncryptPKCS8Key_ex(byte* key, word32 keySz, byte* out, word32* outSz,
ret = GetAlgoV2(encAlgId, &encOid, &encOidSz, &pbeId, &blockSz);
}
if (ret == 0) {
padSz = (word32)((blockSz - ((int)keySz & (blockSz - 1))) &
(blockSz - 1));
/* CBC block ciphers use PKCS#7 padding: 1..blockSz bytes, a full
* block when the input is already block-aligned. Stream ciphers
* (blockSz == 1, e.g. RC4) take no padding. */
if (blockSz > 1) {
padSz = (word32)(blockSz - ((int)keySz & (blockSz - 1)));
}
ret = SetShortInt(tmpShort, &tmpIdx, (word32)itt, MAX_SHORT_SZ);
if (ret > 0) {
/* inner = OCT salt INT itt */
Expand Down
Loading