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

RESOLVED FIXED in 3.11.1

Status

P3
trivial
RESOLVED FIXED
13 years ago
4 years ago

People

(Reporter: wtc, Assigned: rrelyea)

Tracking

3.11
3.11.1

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

13 years ago
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?
(Reporter)

Comment 1

13 years ago
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.
Severity: normal → trivial

Comment 2

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

Comment 3

13 years ago
(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
(Assignee)

Comment 4

13 years ago
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
Surely this must be a FIPS issue, no?
Priority: -- → P3
Whiteboard: FIPS
QA Contact: jason.m.reid → libraries

Updated

10 years ago
Blocks: 459298
No, this is not fips.  The functions are in libNSS3
Whiteboard: FIPS

Updated

10 years ago
No longer blocks: 459298

Comment 7

4 years ago
This was fixed as part of Bug 320583:
https://hg.mozilla.org/projects/nss/diff/6a5aef244b43/security/nss/lib/cryptohi/seckey.c#l1.102
Assignee: wtc → rrelyea
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.11.1
You need to log in before you can comment on or make changes to this bug.