Code flaw? Looks like Dilithium polyz_unpack masks wrong index.

[paulforbugs]

In the many implementation files of poly.c in src/sig/dilithium/* in polyz_unpack

    r->coeffs[2*i+0] |= (uint32_t)a[5*i+1] << 8;
    r->coeffs[2*i+0] |= (uint32_t)a[5*i+2] << 16;
    r->coeffs[2*i+0] &= 0xFFFFF;

    r->coeffs[2*i+1]  = a[5*i+2] >> 4;
    r->coeffs[2*i+1] |= (uint32_t)a[5*i+3] << 4;
    r->coeffs[2*i+1] |= (uint32_t)a[5*i+4] << 12;
    r->coeffs[2*i+0] &= 0xFFFFF;

    r->coeffs[2*i+0] = GAMMA1 - r->coeffs[2*i+0];
    r->coeffs[2*i+1] = GAMMA1 - r->coeffs[2*i+1];

it may be the case that oqs code is erroneously masking the wrong bit in the second chunk, where

r->coeffs[2*i+0] &= 0xFFFFF;

might need to be

r->coeffs[2*i+1] &= 0xFFFFF;

which may affect the value obtained in the subsequent subtraction from GAMMA1.

I do see in ml-dsa implementations that it is commented out like this

/* r->coeffs[2*i+1] &= 0xFFFFF; */ /* No effect, since we're anyway at 20 bits */

Is this something to worry about?

[paulforbugs]

Now that I look at this again, maybe it's not a functional issue because we're only going up to 20 bits anyway?

[dstebila]

Tagging @gregorseiler who I believe wrote much of the Dilithium reference implementation to see if he can offer some insight.

[baentsch]

As a) no answer is forth coming since a few months, b) Dilithium has been removed and c) the reference implementation underlying ML-DSA also goes away soon I think this issue can be closed. Anyone objecting, please speak up in the next few days. Thanks.