322861 - Lazy evaluation of pubk->u.ec.size in SECKEY_PublicKeyStrength and SECKEY_PublicKeyStrengthInBits is not thread-safe

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Wan-Teh Chang]

The functions SECKEY_PublicKeyStrength and
SECKEY_PublicKeyStrengthInBits contain the following
lazy evaluation code:

("SECKEYPublicKey *pubk" is the input parameter)

    switch (pubk->keyType) {
    ...
    case ecKey:
        /* Get the key size in bits and adjust */
        if (pubk->u.ec.size == 0) {
            pubk->u.ec.size = 
                SECKEY_ECParamsToKeySize(&pubk->u.ec.DEREncodedParams);
        }

This lazy evaluation of pubk->u.ec.size is not
thread-safe.

Is SECKEY_ECParamsToKeySize an expensive call to
warrant the lazy evaluation?

Comment 1

[Wan-Teh Chang]

Since the type of pubk->u.ec.size is int, it can
be read and written atomically on all the processor
architectures I know of, but it is best to avoid
such an assumption if possible.  In general,
variables used this way should have the type
sigatomic_t.

Comment 2

[Douglas Stebila]

It's not overly expensive, so getting rid of the lazy evaluation should be fine.

Comment 3

[Vipul Gupta]

(In reply to comment #2)

I agree that getting rid of it should be fine.
From what I recall, the original motivation
for doing it this way wasn't the computation
cost. Rather, at that time, the code needed
to map the encoded params to size was
in a different layer and I was trying to avoid
duplicating it. It's been a long time when
this was done so I'm very hazy on the details
(it is also possible that I made a wrong decision
back then because I did not fully understand
how the various NSS layers interact).

I just want to assure you that performance
was never the concern so it should be fine
to call this method early on when setting
the size for the first time.

vipul

Comment 4

[Robert Relyea]

This can be viewed 2 ways, lazy evaluation, or caching. I would prefer to say it's really a caching problem and always do the calculation in SECKEY_PublicKeyStrength().


bob

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Surely this must be a FIPS issue, no?

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

No, this is not fips.  The functions are in libNSS3

Comment 7

[:Cykesiopka]

This was fixed as part of Bug 320583:
https://hg.mozilla.org/projects/nss/diff/6a5aef244b43/security/nss/lib/cryptohi/seckey.c#l1.102