287418 - Softoken has unnecessary memory allocations when doing DSA.

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Robert Relyea]

The DSA Sign stub unnecessarily allocates a temparary buffer to hold the result
from DSA_Sign. (softoken/pkcs11c.c):

static SECStatus
nsc_DSA_Sign_Stub(void *ctx, void *sigBuf,
                  unsigned int *sigLen, unsigned int maxSigLen,
                  void *dataBuf, unsigned int dataLen)
{
    SECItem signature = { 0 }, digest;
    SECStatus rv;
    NSSLOWKEYPrivateKey *key = (NSSLOWKEYPrivateKey *)ctx;

    (void)SECITEM_AllocItem(NULL, &signature, maxSigLen);
    digest.data = (unsigned char *)dataBuf;
    digest.len = dataLen;
    rv = DSA_SignDigest(&(key->u.dsa), &signature, &digest);
    *sigLen = signature.len;
    PORT_Memcpy(sigBuf, signature.data, signature.len);
    SECITEM_FreeItem(&signature, PR_FALSE);
    return rv;
}

The function should look like:

static SECStatus
nsc_DSA_Sign_Stub(void *ctx, void *sigBuf,
                  unsigned int *sigLen, unsigned int maxSigLen,
                  void *dataBuf, unsigned int dataLen)
{
    SECItem signature, digest;
    SECStatus rv;
    NSSLOWKEYPrivateKey *key = (NSSLOWKEYPrivateKey *)ctx;

    signature.data = (unsigned char *)sigBuf;
    signature.len = maxSigLen;
    digest.data = (unsigned char *)dataBuf;
    digest.len = dataLen;
    rv = DSA_SignDigest(&(key->u.dsa), &signature, &digest);
    *sigLen = signature.len;
    return rv;
}

A similiar fix should happen for ECDSA stub.

Comment 1

[Wan-Teh Chang]

Bob, could you go ahead and check in the fix?  If
you want, you can attach a patch.  I think the
change you proposed is correct.

Comment 2

[Robert Relyea]

Attachment: Remove redundant allocate on DSA.

Comment 3

[Robert Relyea]

Attachment: A -u version of the same patch for easier review.

Comment 4

[Wan-Teh Chang]

Comment on attachment 188488 [details] [diff] [review]
A -u version of the same patch for easier review.

r=wtc.

Comment 5

[Julien Pierre]

Comment on attachment 188488 [details] [diff] [review]
A -u version of the same patch for easier review.

Looks fine. It would be even better if we had our const-ness right so we could
declare DSA_SignDigest and ECDSA_SignDigest to take pointers to const keys, and
signature as a const SECItem . But const done right involves painful
propagation :(.

Comment 6

[Wan-Teh Chang]

The "data" member of a const SECItem structure
is still a non-const unsigned char * pointer.
You need a new SECItemConst type like:

  typedef struct SECItemConstStr SECItemConst;

  struct SECItemConstStr {
      SECItemType type;
      const unsigned char *data;
      unsigned int len;
  };

Comment 7

[Robert Relyea]

Checking in pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.61; previous revision: 1.60
done