Skip to content

Commit 02ecb85

Browse files
claudeclaude
committed
LMS: address review findings on Wconversion cleanup
- Fix small-variant signing regression (review finding bonus): reverting word16 sum -> unsigned int in wc_lmots_q_expand broke the small-variant checksum loop, which relies on arithmetic wrapping at 2^16. Restore word16 sum with explicit (word16) casts at every assignment so the -Wconversion warning is silenced without changing semantics. Caught by the new runtime test job (also added). - Fix 16-bit shift UB (review #1): the matrix doesn't cover 16-bit, but on word32 = unsigned long targets `1U << params->height` is undefined for height >= 16. Restore (word32)1U on every shift whose exponent can reach height (LMS_Q_AT_LEVEL macro and four open-coded sites). - Add msgSz <= 0 check to wc_LmsKey_Verify (review #2): wc_LmsKey_Sign already rejects non-positive msgSz; the public verify entry point did not. Without it, the (word32)msgSz cast forwarded a huge value to wc_hss_verify and caused buffer over-read. - Add a test_lms_runtime CI job (review #11): the existing matrix only builds; it cannot catch a semantic regression like the q_expand one above. The new job builds --enable-lms and --enable-lms=yes,small and runs testwolfcrypt for both. - Make wc_hss_make_key loop index word32 (review #10) so the (int)cast on HSS_MAX_LEVELS in the second-loop bound is no longer needed. - Add LMOTS_Y_LEN as the canonical name for the LM-OTS y[] length and alias LMS_PRIV_Y_TREE_LEN to it (review #6); switch the non-cache call sites in wc_hss_sign / wc_lms_sig_copy / wc_hss_sign_build_sig to LMOTS_Y_LEN to match intent. Verified: 15 matrix rows still build clean under the conversion flags; testwolfcrypt LMS Vfy / LMS pass for --enable-lms, --enable-lms=yes,small, and --enable-lms=yes,small,sha256-192,shake256; bench_lms shows no regression. https://claude.ai/code/session_01EJmy1bKDgHseTwZ5Qqpu1g
1 parent c0c79a6 commit 02ecb85

4 files changed

Lines changed: 80 additions & 40 deletions

File tree

‎.github/workflows/wolfCrypt-Wconversion.yml‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,3 +55,29 @@ jobs:
5555
echo "running ./configure ${{ matrix.config }}"
5656
./configure ${{ matrix.config }} || $(exit 3)
5757
make -j 4 || $(exit 4)
58+
59+
# Compiler-clean (above) doesn't catch a bad cast that silently changes
60+
# the wire output. Run testwolfcrypt's lms_test under the same conversion
61+
# flags on the default and small LMS variants.
62+
test_lms_runtime:
63+
strategy:
64+
matrix:
65+
config: [
66+
'--enable-lms CPPFLAGS="-Wconversion -Warith-conversion -Wenum-conversion -Wfloat-conversion -Wsign-conversion -Wcast-qual"',
67+
'--enable-lms=yes,small CPPFLAGS="-Wconversion -Warith-conversion -Wenum-conversion -Wfloat-conversion -Wsign-conversion -Wcast-qual"',
68+
]
69+
name: test LMS runtime
70+
if: github.repository_owner == 'wolfssl'
71+
runs-on: ubuntu-24.04
72+
timeout-minutes: 6
73+
steps:
74+
- uses: actions/checkout@v4
75+
name: Checkout wolfSSL
76+
77+
- name: Build and run wolfCrypt LMS tests
78+
run: |
79+
./autogen.sh || $(exit 2)
80+
echo "running ./configure ${{ matrix.config }}"
81+
./configure ${{ matrix.config }} || $(exit 3)
82+
make -j 4 || $(exit 4)
83+
./wolfcrypt/test/testwolfcrypt || $(exit 5)

‎wolfcrypt/src/wc_lms.c‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1492,6 +1492,7 @@ int wc_LmsKey_GetSigLen(const LmsKey* key, word32* len)
14921492
* @param [in] msgSz Length of the message in bytes.
14931493
* @return 0 on success.
14941494
* @return BAD_FUNC_ARG when a key, sig or msg is NULL.
1495+
* @return BAD_FUNC_ARG when msgSz is not greater than 0.
14951496
* @return SIG_VERIFY_E when signature did not verify message.
14961497
* @return BAD_STATE_E when wrong state for operation.
14971498
* @return BUFFER_E when sigSz is invalid for parameters.
@@ -1505,6 +1506,9 @@ int wc_LmsKey_Verify(LmsKey* key, const byte* sig, word32 sigSz,
15051506
if ((key == NULL) || (sig == NULL) || (msg == NULL)) {
15061507
ret = BAD_FUNC_ARG;
15071508
}
1509+
if ((ret == 0) && (msgSz <= 0)) {
1510+
ret = BAD_FUNC_ARG;
1511+
}
15081512
/* Check state. */
15091513
if ((ret == 0) && (key->state != WC_LMS_STATE_OK) &&
15101514
(key->state != WC_LMS_STATE_VERIFYONLY)) {

‎wolfcrypt/src/wc_lms_impl.c‎

Lines changed: 42 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -689,7 +689,11 @@ static WC_INLINE int wc_lmots_q_expand(byte* q, word8 n, word8 w, word8 ls,
689689
byte* qe)
690690
{
691691
int ret = 0;
692-
unsigned int sum;
692+
/* sum is word16: the small-variant checksum loop below relies on
693+
* arithmetic wrapping at 2^16 (sum <<= w then sum >> (16 - w) reads the
694+
* top byte of the rolled value). Switching to a wider type would let
695+
* those high bits leak into subsequent reads. */
696+
word16 sum;
693697
unsigned int i;
694698

695699
#ifndef WOLFSSL_WC_LMS_SMALL
@@ -699,27 +703,27 @@ static WC_INLINE int wc_lmots_q_expand(byte* q, word8 n, word8 w, word8 ls,
699703
/* No expansion required, just copy. */
700704
XMEMCPY(qe, q, n);
701705
/* Start sum with all 2^w - 1s and subtract from that. */
702-
sum = 0xffU * n;
706+
sum = (word16)(0xffU * n);
703707
/* For each byte of the hash. */
704708
for (i = 0; i < n; i++) {
705709
/* Subtract coefficient from sum. */
706-
sum -= q[i];
710+
sum = (word16)(sum - q[i]);
707711
}
708712
/* Put coefficients of checksum on the end. */
709713
qe[n + 0] = (word8)(sum >> 8);
710714
qe[n + 1] = (word8)(sum );
711715
break;
712716
/* Winternitz width of 4. */
713717
case 4:
714-
sum = 2U * 0xfU * n;
718+
sum = (word16)(2U * 0xfU * n);
715719
/* For each byte of the hash. */
716720
for (i = 0; i < n; i++) {
717721
/* Get coefficient. */
718722
qe[0] = (q[i] >> 4) ;
719723
qe[1] = (q[i] ) & 0xf;
720724
/* Subtract coefficients from sum. */
721-
sum -= qe[0];
722-
sum -= qe[1];
725+
sum = (word16)(sum - qe[0]);
726+
sum = (word16)(sum - qe[1]);
723727
/* Move to next coefficients. */
724728
qe += 2;
725729
}
@@ -730,7 +734,7 @@ static WC_INLINE int wc_lmots_q_expand(byte* q, word8 n, word8 w, word8 ls,
730734
break;
731735
/* Winternitz width of 2. */
732736
case 2:
733-
sum = 4U * 0x3U * n;
737+
sum = (word16)(4U * 0x3U * n);
734738
/* For each byte of the hash. */
735739
for (i = 0; i < n; i++) {
736740
/* Get coefficients. */
@@ -739,10 +743,10 @@ static WC_INLINE int wc_lmots_q_expand(byte* q, word8 n, word8 w, word8 ls,
739743
qe[2] = (q[i] >> 2) & 0x3;
740744
qe[3] = (q[i] ) & 0x3;
741745
/* Subtract coefficients from sum. */
742-
sum -= qe[0];
743-
sum -= qe[1];
744-
sum -= qe[2];
745-
sum -= qe[3];
746+
sum = (word16)(sum - qe[0]);
747+
sum = (word16)(sum - qe[1]);
748+
sum = (word16)(sum - qe[2]);
749+
sum = (word16)(sum - qe[3]);
746750
/* Move to next coefficients. */
747751
qe += 4;
748752
}
@@ -755,7 +759,7 @@ static WC_INLINE int wc_lmots_q_expand(byte* q, word8 n, word8 w, word8 ls,
755759
break;
756760
/* Winternitz width of 1. */
757761
case 1:
758-
sum = 8U * 0x01U * n;
762+
sum = (word16)(8U * 0x01U * n);
759763
/* For each byte of the hash. */
760764
for (i = 0; i < n; i++) {
761765
/* Get coefficients. */
@@ -768,14 +772,14 @@ static WC_INLINE int wc_lmots_q_expand(byte* q, word8 n, word8 w, word8 ls,
768772
qe[6] = (q[i] >> 1) & 0x1;
769773
qe[7] = (q[i] ) & 0x1;
770774
/* Subtract coefficients from sum. */
771-
sum -= qe[0];
772-
sum -= qe[1];
773-
sum -= qe[2];
774-
sum -= qe[3];
775-
sum -= qe[4];
776-
sum -= qe[5];
777-
sum -= qe[6];
778-
sum -= qe[7];
775+
sum = (word16)(sum - qe[0]);
776+
sum = (word16)(sum - qe[1]);
777+
sum = (word16)(sum - qe[2]);
778+
sum = (word16)(sum - qe[3]);
779+
sum = (word16)(sum - qe[4]);
780+
sum = (word16)(sum - qe[5]);
781+
sum = (word16)(sum - qe[6]);
782+
sum = (word16)(sum - qe[7]);
779783
/* Move to next coefficients. */
780784
qe += 8;
781785
}
@@ -811,7 +815,7 @@ static WC_INLINE int wc_lmots_q_expand(byte* q, word8 n, word8 w, word8 ls,
811815

812816
if (ret == 0) {
813817
/* Start sum with all 2^w - 1s and subtract from that. */
814-
sum = ((1U << w) - 1U) * ((n * 8U) / w);
818+
sum = (word16)(((1U << w) - 1U) * ((n * 8U) / w));
815819
/* For each byte of the hash. */
816820
for (i = 0; i < n; i++) {
817821
/* Get next byte. */
@@ -821,21 +825,21 @@ static WC_INLINE int wc_lmots_q_expand(byte* q, word8 n, word8 w, word8 ls,
821825
/* Get coefficient. */
822826
*qe = (byte)(a >> (8 - w));
823827
/* Subtract coefficient from sum. */
824-
sum -= *qe;
828+
sum = (word16)(sum - *qe);
825829
/* Move to next coefficient. */
826830
qe++;
827831
/* Remove width bits. */
828832
a = (byte)(a << w);
829833
}
830834
}
831835
/* Shift sum up as required to pack it on the end of hash. */
832-
sum <<= ls;
836+
sum = (word16)(sum << ls);
833837
/* For each width bit of checksum. */
834838
for (j = 16 - w; j >= ls; j--) {
835839
/* Get coefficient. */
836840
*(qe++) = (byte)(sum >> (16 - w));
837841
/* Remove width bits. */
838-
sum <<= w;
842+
sum = (word16)(sum << w);
839843
}
840844
}
841845
#endif /* !WOLFSSL_WC_LMS_SMALL */
@@ -2242,7 +2246,8 @@ static int wc_lms_treehash_init(LmsState* state, LmsPrivState* privState,
22422246
/* Copy out top root nodes. */
22432247
if ((h > params->height - params->rootLevels) &&
22442248
((i >> (h-1)) != ((i + 1) >> (h - 1)))) {
2245-
word32 off = (1U << (params->height - h)) + (i >> h) - 1U;
2249+
word32 off = ((word32)1U << (params->height - h)) +
2250+
(i >> h) - 1U;
22462251
XMEMCPY(root + off * params->hash_len, temp, params->hash_len);
22472252
}
22482253

@@ -2393,7 +2398,8 @@ static int wc_lms_treehash_update(LmsState* state, LmsPrivState* privState,
23932398
if ((ret == 0) && (q == 0) && (!useRoot) &&
23942399
(h > params->height - params->rootLevels) &&
23952400
((i >> (h-1)) != ((i + 1) >> (h - 1)))) {
2396-
word32 off = (1U << (params->height - h)) + (i >> h) - 1U;
2401+
word32 off = ((word32)1U << (params->height - h)) +
2402+
(i >> h) - 1U;
23972403
XMEMCPY(privState->root + off * params->hash_len, temp,
23982404
params->hash_len);
23992405
}
@@ -2506,8 +2512,8 @@ static void wc_lms_sig_copy(const LmsParams* params, const byte* y,
25062512
c32toa(params->lmOtsType & LMS_H_W_MASK, sig);
25072513
sig += LMS_TYPE_LEN;
25082514
/* S = u32str(q) || ots_signature || ... */
2509-
XMEMCPY(sig, y, LMS_PRIV_Y_TREE_LEN(params->p, params->hash_len));
2510-
sig += LMS_PRIV_Y_TREE_LEN(params->p, params->hash_len);
2515+
XMEMCPY(sig, y, LMOTS_Y_LEN(params->p, params->hash_len));
2516+
sig += LMOTS_Y_LEN(params->p, params->hash_len);
25112517
/* S = u32str(q) || ots_signature || u32str(type) || ... */
25122518
c32toa(params->lmsType & LMS_H_W_MASK, sig);
25132519
}
@@ -3001,7 +3007,7 @@ static int wc_hss_derive_seed_i(LmsState* state, const byte* id,
30013007
/* Get q, index, of leaf at the specified level. */
30023008
#define LMS_Q_AT_LEVEL(q, ls, l, h) \
30033009
(w64GetLow32(w64ShiftRight((q), (((ls) - 1 - (l)) * (h)))) & \
3004-
((1U << (h)) - 1U))
3010+
(((word32)1U << (h)) - 1U))
30053011

30063012
/* Expand the seed and I for further levels and set q for each level.
30073013
*
@@ -3069,7 +3075,7 @@ static int wc_hss_expand_private_key(LmsState* state, byte* priv,
30693075

30703076
/* Get q for level from 64-bit composite. */
30713077
q32 = w64GetLow32(w64ShiftRight(q, (int)(params->levels - 1U - i) *
3072-
params->height)) & ((1U << params->height) - 1U);
3078+
params->height)) & (((word32)1U << params->height) - 1U);
30733079
/* Set q of tree. */
30743080
c32toa(q32, priv);
30753081

@@ -3362,7 +3368,8 @@ static int wc_hss_update_auth_path(LmsState* state, HssPrivKey* priv_key,
33623368
word32 qm1a = LMS_AUTH_PATH_IDX(q - 1, h);
33633369
/* If different then copy in cached hash. */
33643370
if ((qa != qm1a) && (qa > maxq)) {
3365-
word32 off = (1U << (params->height - h)) + (qa >> h) - 1U;
3371+
word32 off = ((word32)1U << (params->height - h)) +
3372+
(qa >> h) - 1U;
33663373
XMEMCPY(privState->auth_path + h * params->hash_len,
33673374
privState->root + off * params->hash_len,
33683375
params->hash_len);
@@ -3605,7 +3612,7 @@ int wc_hss_make_key(LmsState* state, WC_RNG* rng, byte* priv_raw,
36053612
{
36063613
const LmsParams* params = state->params;
36073614
int ret = 0;
3608-
int i;
3615+
word32 i;
36093616
byte* p = priv_raw;
36103617
byte* pub_root = pub + LMS_L_LEN + LMS_TYPE_LEN + LMS_TYPE_LEN + LMS_I_LEN;
36113618

@@ -3619,7 +3626,7 @@ int wc_hss_make_key(LmsState* state, WC_RNG* rng, byte* priv_raw,
36193626
(params->lmOtsType & LMS_H_W_MASK));
36203627
}
36213628
/* Set rest of levels to an invalid value. */
3622-
for (; i < (int)HSS_MAX_LEVELS; i++) {
3629+
for (; i < HSS_MAX_LEVELS; i++) {
36233630
p[i] = 0xff;
36243631
}
36253632
p += HSS_PRIV_KEY_PARAM_SET_LEN;
@@ -3744,7 +3751,7 @@ int wc_hss_sign(LmsState* state, byte* priv_raw, HssPrivKey* priv_key,
37443751
ret = wc_lms_sign(state, p, msg, msgSz, sig);
37453752
if (ret == 0) {
37463753
byte* s = sig + LMS_Q_LEN + LMS_TYPE_LEN +
3747-
LMS_PRIV_Y_TREE_LEN(params->p, params->hash_len) +
3754+
LMOTS_Y_LEN(params->p, params->hash_len) +
37483755
LMS_TYPE_LEN;
37493756
byte* priv_q = p;
37503757
byte* priv_seed = priv_q + LMS_Q_LEN;
@@ -3879,8 +3886,7 @@ static int wc_hss_sign_build_sig(LmsState* state, byte* priv_raw,
38793886
LMS_PRIV_Y_TREE_LEN(params->p, params->hash_len));
38803887
}
38813888
#endif /* !WOLFSSL_LMS_NO_SIG_CACHE */
3882-
s += LMS_PRIV_Y_TREE_LEN(params->p, params->hash_len) +
3883-
LMS_TYPE_LEN;
3889+
s += LMOTS_Y_LEN(params->p, params->hash_len) + LMS_TYPE_LEN;
38843890

38853891
/* Copy the authentication path out of the private key. */
38863892
XMEMCPY(s, priv_key->state[i].auth_path,

‎wolfssl/wolfcrypt/wc_lms.h‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -479,12 +479,16 @@ enum wc_LmsState {
479479
#define LMS_PRIV_SMOOTH_LEN(l, h, rl, cb, hLen) 0U
480480
#endif
481481

482-
/* Length of one LM-OTS y[]: the C randomizer plus p hashes. Always defined
483-
* (independent of WOLFSSL_LMS_NO_SIG_CACHE) because it is also used to walk
484-
* the in-memory signature layout, not just the y cache. */
485-
#define LMS_PRIV_Y_TREE_LEN(p, hLen) \
482+
/* Length of one LM-OTS y[]: the C randomizer plus p hashes. */
483+
#define LMOTS_Y_LEN(p, hLen) \
486484
((word32)(hLen) + (word32)(p) * (word32)(hLen))
487485

486+
/* Length of one LM-OTS y[] when stored in the per-level y cache. Same value
487+
* as LMOTS_Y_LEN; kept as a separate name for the cache call sites. Always
488+
* defined (independent of WOLFSSL_LMS_NO_SIG_CACHE) because it is also used
489+
* to walk the in-memory signature layout, not just the y cache. */
490+
#define LMS_PRIV_Y_TREE_LEN(p, hLen) LMOTS_Y_LEN(p, hLen)
491+
488492
#ifndef WOLFSSL_LMS_NO_SIG_CACHE
489493
/* Length of the y data cached in private key data. */
490494
#define LMS_PRIV_Y_LEN(l, p, hLen) \

0 commit comments

Comments
 (0)