Skip to content

Commit ce18c33

Browse files
fix(rsa,hmac): validate PSS salt and _GENERAL MAC lengths; SHA-384 block size (#166)
* refactor(attributes): deprecate Cka* in favour of the binding's CKA_* constants crypto11's Cka* list was copied by hand before the move to pkcs11-go and covers 111 of the 160 attributes in the PKCS#11 v3.2 header. The binding's cryptoki package is generated from that header and defines all of them, CKA_UNIQUE_ID, CKA_SEED and CKA_PUBLIC_CRC64_VALUE included. Each Cka* constant is now defined from the binding's constant of the same name, keeping its AttributeType type, and the group is marked deprecated so staticcheck and gopls point callers at pkcs11.CKA_*. Existing code compiles and behaves as before. crypto11 itself uses the binding's constants throughout; error messages and the names AttributeSet.String prints are unchanged. The unexported ckfArrayAttribute constant is gone, along with the attributeTypeString case for that bare flag, which is not an attribute. AttributeSet.String keeps its own name table until a pkcs11-go release provides cryptoki.AttributeName. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Nicolas-Peiffer <102670102+Nicolas-Peiffer@users.noreply.github.com> * fix(rsa,hmac): validate PSS salt and _GENERAL MAC lengths; SHA-384 block size RSA-PSS: - A negative PSSOptions.SaltLength other than the Auto and EqualsHash sentinels reached pkcs11-go's NewPSSParams, which panics on a negative salt, so Sign panicked instead of returning an error. It is now an error wrapping errUnsupportedRSAOptions. - An explicit salt longer than the modulus can carry is rsa.ErrMessageTooLong, as in crypto/rsa. - A digest whose length is not the hash's size is an error, as on the PKCS#1 v1.5 path, instead of being left to the token. The resolution lives in pssSaltLength, tested without a token, and each check has an integration subtest asserting an error and no panic. HMAC: - hmacInfos listed SHA-384 with a 64-byte block; it is 128. - NewHMAC accepted any _GENERAL output length. Zero made every Sum panic on the empty MAC, and a negative length reached the token as a huge CK_ULONG. Lengths outside 1..full size are now rejected with errInvalidMACLength before a session is taken. TestHmacInfoSizesMatchDigests now checks every entry's size, block size and _GENERAL flag against Go's own hash implementations. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Nicolas-Peiffer <102670102+Nicolas-Peiffer@users.noreply.github.com> --------- Signed-off-by: Nicolas-Peiffer <102670102+Nicolas-Peiffer@users.noreply.github.com>
1 parent 71e3ac5 commit ce18c33

5 files changed

Lines changed: 161 additions & 20 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,17 @@ reporting or the Eclipse Foundation security team — and which versions receive
177177

178178
### Fixed
179179

180+
- **RSA-PSS options are checked before reaching the token.** A negative `PSSOptions.SaltLength`
181+
other than the two sentinels made `Sign` panic, since pkcs11-go's `NewPSSParams` panics on a
182+
negative salt. It is now an error wrapping `errUnsupportedRSAOptions`, as crypto/rsa makes it.
183+
An explicit salt longer than the modulus can carry is `rsa.ErrMessageTooLong`, again as in
184+
crypto/rsa, and a digest whose length is not the hash's size is an error rather than something
185+
left to the token, matching the PKCS#1 v1.5 path.
186+
- **HMAC.** `NewHMAC` reported a `BlockSize` of 64 for `CKM_SHA384_HMAC` and
187+
`CKM_SHA384_HMAC_GENERAL`; SHA-384's block size is 128. For a `_GENERAL` mechanism it accepted
188+
any output length: 0 made every `Sum` panic on the empty MAC, and a negative length reached the
189+
token as a huge `CK_ULONG`. Lengths outside 1 to the hash's full size are now rejected with
190+
`errInvalidMACLength`. The size table is tested against Go's own hash implementations.
180191
- Key enumeration no longer aborts on a key type the package cannot represent
181192
([#68](https://github.com/eclipse-keypont/crypto11/issues/68), reported by
182193
[@Knacktus](https://github.com/Knacktus) and independently run into by

‎hmac.go‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -102,8 +102,8 @@ var hmacInfos = map[int]*hmacInfo{
102102
pkcs11.CKM_SHA224_HMAC_GENERAL: {28, 64, true},
103103
pkcs11.CKM_SHA256_HMAC: {32, 64, false},
104104
pkcs11.CKM_SHA256_HMAC_GENERAL: {32, 64, true},
105-
pkcs11.CKM_SHA384_HMAC: {48, 64, false},
106-
pkcs11.CKM_SHA384_HMAC_GENERAL: {48, 64, true},
105+
pkcs11.CKM_SHA384_HMAC: {48, 128, false},
106+
pkcs11.CKM_SHA384_HMAC_GENERAL: {48, 128, true},
107107
pkcs11.CKM_SHA512_HMAC: {64, 128, false},
108108
pkcs11.CKM_SHA512_HMAC_GENERAL: {64, 128, true},
109109
pkcs11.CKM_SHA512_224_HMAC: {28, 128, false},
@@ -117,9 +117,14 @@ var hmacInfos = map[int]*hmacInfo{
117117
// errHmacClosed is called if an HMAC is updated after it has finished.
118118
var errHmacClosed = errors.New("already called Sum()")
119119

120+
// errInvalidMACLength is returned by NewHMAC for an output length the
121+
// mechanism cannot produce.
122+
var errInvalidMACLength = errors.New("invalid MAC length")
123+
120124
// NewHMAC returns a new HMAC hash using the given PKCS#11 mechanism
121125
// and key.
122-
// length specifies the output size, for _GENERAL mechanisms.
126+
// length specifies the output size, for _GENERAL mechanisms: between 1 and the
127+
// full MAC size of the underlying hash.
123128
//
124129
// If the mechanism is not in the built-in list of known mechanisms then the
125130
// Size() function will return whatever length was, even if it is wrong.
@@ -144,12 +149,20 @@ func (key *SecretKey) NewHMAC(mech int, length int) (hash.Hash, error) {
144149
if info, ok := hmacInfos[mech]; ok {
145150
hi.blockSize = info.blockSize
146151
if info.general {
152+
// A zero length would make every Sum panic on the empty MAC, and a
153+
// negative one would reach the token as a huge CK_ULONG.
154+
if length < 1 || length > info.size {
155+
return nil, fmt.Errorf("%w: %d bytes; mechanism %#x produces 1 to %d", errInvalidMACLength, length, mech, info.size)
156+
}
147157
hi.size = length
148158
params = pkcs11.ULongToBytes(uint(length))
149159
} else {
150160
hi.size = info.size
151161
}
152162
} else {
163+
if length < 0 {
164+
return nil, fmt.Errorf("%w: %d bytes", errInvalidMACLength, length)
165+
}
153166
hi.size = length
154167
}
155168
hi.mechDescription = pkcs11.NewMechanism(uint(mech), params)

‎hmac_test.go‎

Lines changed: 50 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,12 @@ package crypto11
66

77
import (
88
"bytes"
9+
"crypto/md5"
10+
"crypto/sha1"
11+
"crypto/sha256"
12+
"crypto/sha512"
913
"errors"
14+
"hash"
1015
"sync"
1116
"testing"
1217
"time"
@@ -282,14 +287,51 @@ func TestCheckMACLength(t *testing.T) {
282287
}
283288

284289
func TestHmacInfoSizesMatchDigests(t *testing.T) {
285-
// The table is what Sum validates token output against, so an entry that
286-
// is wrong (MD5 was listed at 20 bytes) would reject every correct MAC.
287-
assert.Equal(t, 16, hmacInfos[pkcs11.CKM_MD5_HMAC].size)
288-
assert.Equal(t, 20, hmacInfos[pkcs11.CKM_SHA_1_HMAC].size)
289-
assert.Equal(t, 28, hmacInfos[pkcs11.CKM_SHA224_HMAC].size)
290-
assert.Equal(t, 32, hmacInfos[pkcs11.CKM_SHA256_HMAC].size)
291-
assert.Equal(t, 48, hmacInfos[pkcs11.CKM_SHA384_HMAC].size)
292-
assert.Equal(t, 64, hmacInfos[pkcs11.CKM_SHA512_HMAC].size)
290+
// The table is what Sum validates token output against, so a wrong size
291+
// (MD5 was listed at 20 bytes) rejects every correct MAC; and its block size
292+
// is what BlockSize reports (SHA-384 was listed at 64 bytes, not 128). Both
293+
// are checked against Go's own implementation of each hash.
294+
hashes := map[int]func() hash.Hash{
295+
pkcs11.CKM_MD5_HMAC: md5.New,
296+
pkcs11.CKM_SHA_1_HMAC: sha1.New,
297+
pkcs11.CKM_SHA224_HMAC: sha256.New224,
298+
pkcs11.CKM_SHA256_HMAC: sha256.New,
299+
pkcs11.CKM_SHA384_HMAC: sha512.New384,
300+
pkcs11.CKM_SHA512_HMAC: sha512.New,
301+
pkcs11.CKM_SHA512_224_HMAC: sha512.New512_224,
302+
pkcs11.CKM_SHA512_256_HMAC: sha512.New512_256,
303+
}
304+
// Each _GENERAL mechanism is its plain one plus one.
305+
for mech, newHash := range hashes {
306+
h := newHash()
307+
for _, m := range []int{mech, mech + 1} {
308+
info, ok := hmacInfos[m]
309+
require.True(t, ok, "mechanism %#x missing from hmacInfos", m)
310+
assert.Equal(t, h.Size(), info.size, "size of %#x", m)
311+
assert.Equal(t, h.BlockSize(), info.blockSize, "block size of %#x", m)
312+
assert.Equal(t, m == mech+1, info.general, "general flag of %#x", m)
313+
}
314+
}
315+
// RIPEMD-160 has no implementation in the standard library.
316+
assert.Equal(t, 20, hmacInfos[pkcs11.CKM_RIPEMD160_HMAC].size)
317+
assert.Equal(t, 64, hmacInfos[pkcs11.CKM_RIPEMD160_HMAC].blockSize)
318+
}
319+
320+
func TestNewHMACRejectsInvalidLength(t *testing.T) {
321+
// The length is checked before a session is taken, so no token is needed.
322+
key := &SecretKey{}
323+
for _, c := range []struct {
324+
mech, length int
325+
}{
326+
{pkcs11.CKM_SHA256_HMAC_GENERAL, 0},
327+
{pkcs11.CKM_SHA256_HMAC_GENERAL, -1},
328+
{pkcs11.CKM_SHA256_HMAC_GENERAL, 33},
329+
{pkcs11.CKM_SHA_1_HMAC_GENERAL, 21},
330+
{pkcs11.CKM_VENDOR_DEFINED, -1}, // not in the table
331+
} {
332+
_, err := key.NewHMAC(c.mech, c.length)
333+
require.ErrorIs(t, err, errInvalidMACLength, "mechanism %#x, length %d", c.mech, c.length)
334+
}
293335
}
294336

295337
func TestHmacResetAfterDeadOperationDoesNotPanic(t *testing.T) {

‎rsa.go‎

Lines changed: 29 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -573,21 +573,41 @@ func maxPSSSaltLength(pubKey crypto.PublicKey, hLen uint) (uint, error) {
573573
return uint(sLen), nil
574574
}
575575

576+
// pssSaltLength resolves opts.SaltLength to the salt length handed to the
577+
// token, rejecting what crypto/rsa rejects: a negative length other than the
578+
// two sentinels, and a salt longer than the modulus can carry. The second check
579+
// needs the public key; without one (a key found by FindRSAPrivateKey, say) an
580+
// explicit length is left to the token to judge.
581+
func pssSaltLength(pubKey crypto.PublicKey, saltLength int, hLen uint) (uint, error) {
582+
switch saltLength {
583+
case rsa.PSSSaltLengthAuto:
584+
return maxPSSSaltLength(pubKey, hLen)
585+
case rsa.PSSSaltLengthEqualsHash:
586+
return hLen, nil
587+
}
588+
if saltLength < 0 {
589+
return 0, fmt.Errorf("%w: PSS salt length %d", errUnsupportedRSAOptions, saltLength)
590+
}
591+
sLen := uint(saltLength)
592+
if limit, err := maxPSSSaltLength(pubKey, hLen); err == nil && sLen > limit {
593+
return 0, rsa.ErrMessageTooLong
594+
}
595+
return sLen, nil
596+
}
597+
576598
func signPSS(session *pkcs11Session, key *pkcs11PrivateKeyRSA, digest []byte, opts *rsa.PSSOptions) ([]byte, error) {
577599
var hMech, mgf, hLen, sLen uint
578600
var err error
579601
if hMech, mgf, hLen, err = hashToPKCS11(opts.Hash); err != nil {
580602
return nil, err
581603
}
582-
switch opts.SaltLength {
583-
case rsa.PSSSaltLengthAuto:
584-
if sLen, err = maxPSSSaltLength(key.pubKey, hLen); err != nil {
585-
return nil, err
586-
}
587-
case rsa.PSSSaltLengthEqualsHash:
588-
sLen = hLen
589-
default:
590-
sLen = uint(opts.SaltLength)
604+
// CKM_RSA_PKCS_PSS signs a digest it is told the hash of; one of another
605+
// length is a caller error, reported as signPKCS1v15 reports it.
606+
if len(digest) != int(hLen) {
607+
return nil, fmt.Errorf("digest is %d bytes; %v produces %d", len(digest), opts.Hash, hLen)
608+
}
609+
if sLen, err = pssSaltLength(key.pubKey, opts.SaltLength, hLen); err != nil {
610+
return nil, err
591611
}
592612
// The binding marshals CK_RSA_PKCS_PSS_PARAMS itself, so we no longer have
593613
// to hand-pack three CK_ULONGs and hope the layout matches the token's.

‎rsa_test.go‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,61 @@ func testRsaSigningPSS(t *testing.T, key crypto.Signer, hashFunction crypto.Hash
145145
require.NoError(t, err)
146146
})
147147
}
148+
149+
// Bad options are errors, as crypto/rsa makes them, not panics: the
150+
// binding's NewPSSParams panics on a negative salt length.
151+
badOptions := map[string]struct {
152+
digest []byte
153+
opts *rsa.PSSOptions
154+
}{
155+
"NegativeSaltLength": {plaintextHash, &rsa.PSSOptions{SaltLength: -2, Hash: hashFunction}},
156+
"OversizedSalt": {plaintextHash, &rsa.PSSOptions{SaltLength: rsaPubkey.Size(), Hash: hashFunction}},
157+
"WrongDigestLength": {plaintextHash[1:], &rsa.PSSOptions{SaltLength: rsa.PSSSaltLengthEqualsHash, Hash: hashFunction}},
158+
}
159+
for name, bad := range badOptions {
160+
t.Run(name, func(t *testing.T) {
161+
var err error
162+
require.NotPanics(t, func() { _, err = key.Sign(rand.Reader, bad.digest, bad.opts) })
163+
require.Error(t, err)
164+
})
165+
}
166+
}
167+
168+
// TestPSSSaltLength checks how PSSOptions.SaltLength is resolved before it
169+
// reaches the token, without needing one.
170+
func TestPSSSaltLength(t *testing.T) {
171+
key, err := rsa.GenerateKey(rand.Reader, 2048)
172+
require.NoError(t, err)
173+
const hLen = 32
174+
limit := uint(256 - 2 - hLen)
175+
176+
for _, c := range []struct {
177+
name string
178+
pub crypto.PublicKey
179+
saltLength int
180+
want uint
181+
wantErr error
182+
}{
183+
{"Auto", &key.PublicKey, rsa.PSSSaltLengthAuto, limit, nil},
184+
{"EqualsHash", &key.PublicKey, rsa.PSSSaltLengthEqualsHash, hLen, nil},
185+
{"Explicit", &key.PublicKey, 20, 20, nil},
186+
{"ExplicitMaximum", &key.PublicKey, int(limit), limit, nil},
187+
{"ExplicitTooLong", &key.PublicKey, int(limit) + 1, 0, rsa.ErrMessageTooLong},
188+
{"Negative", &key.PublicKey, -2, 0, errUnsupportedRSAOptions},
189+
// Without a public key only the token can judge an explicit length.
190+
{"ExplicitNoPublicKey", nil, 20, 20, nil},
191+
{"AutoNoPublicKey", nil, rsa.PSSSaltLengthAuto, 0, errUnsupportedRSAOptions},
192+
} {
193+
t.Run(c.name, func(t *testing.T) {
194+
got, err := pssSaltLength(c.pub, c.saltLength, hLen)
195+
if c.wantErr != nil {
196+
require.ErrorIs(t, err, c.wantErr)
197+
return
198+
}
199+
require.NoError(t, err)
200+
require.Equal(t, c.want, got)
201+
})
202+
}
148203
}
149204

150205
// TestMaxPSSSaltLength checks that rsa.PSSSaltLengthAuto resolves to the same

0 commit comments

Comments
 (0)
⌂ 换网址