617565 - Prevent buffer overflow in PK11_DeriveWithTemplate template handling

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Now that PK11_DeriveWithTemplate is public it needs to check its inputs more carefully to avoid stack corruption due to buffer overflow in the local keyTemplate array.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Patch to prevent buffer overflow

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Patch to prevent buffer overflow in PK11_DeriveWithTemplate (v2)

tab -> space

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I think it's a good idea to include it in the 3.12.9 release. Bob, what do you think?

Comment 4

[Robert Relyea]

Comment on attachment 496118 [details] [diff] [review]
Patch to prevent buffer overflow in PK11_DeriveWithTemplate (v2)

r- Mostly for style.

In general, I think it's a optimizing a little to much to save 4 words on the stack.

Other style issues are the backwards jump to a label as an exit. In general NSS leaps forward to the end to clean up, though repeating PORT_SetError(); return NULL; seems more reasonable.

What I would prefer is doing something simpler, however. Define MAX_ADD_ADDRS to 4, then either:

check numAttrs > (MAX_TEMPLATE_ATTRS - MAX_ADD_ADDRS) or
make keyTemplate sizeof MAX_TEMPLATE_ATTRS+MAX_ADD_ADDRS.

This also has the advantage that we can document exactly how big the template the application can give us without worrying about whether or not they have specified CKA_CLASS, CKA_KEY_TYPE, CKA_VALUE_LEN (and keysize != 0) or operation (and operation != CKA_FLAGS_ONLY),

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Prevent buffer overflow in PK11_DeriveWithTemplate (v3)

Addresses review comments.

Note that this assertion:

   PR_ASSERT(templateCount <= MAX_TEMPL_ATTRS + MAX_ADD_ATTRS);

is mostly only useful for documentation purposes because it asserting that the buffer overflow did not occur after it (would have) occurred, and the buffer overflow itself would likely cause a wrong value of templateCount to be calculated.

I had forgotten about this bug during the call earlier today but I think the fix should also land in 3.12.9.

Comment 6

[Robert Relyea]

Comment on attachment 501823 [details] [diff] [review]
Prevent buffer overflow in PK11_DeriveWithTemplate (v3)

r+ rrelyea

Comment 7

[Wan-Teh Chang]

Comment on attachment 501823 [details] [diff] [review]
Prevent buffer overflow in PK11_DeriveWithTemplate (v3)

>+#   define MAX_ADD_ATTRS 4

We don't indent C preprocessor # directives like this.
Please rewrite it as

#define MAX_ADD_ATTRS 4

>+    PR_ASSERT(templateCount <= MAX_TEMPL_ATTRS + MAX_ADD_ATTRS);

Please use
    sizeof(keyTemplate) / sizeof(keyTemplate[0])
instead of
    MAX_TEMPL_ATTRS + MAX_ADD_ATTRS

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Prevent buffer overflow in PK11_DeriveWithTemplate (v4)

Patch addressing Wan-Teh's comments above.

Comment 9

[Wan-Teh Chang]

Comment on attachment 514864 [details] [diff] [review]
Prevent buffer overflow in PK11_DeriveWithTemplate (v4)

r=wtc.

IMPORTANT: pk11_AnyUnwrapKey has the same bug:
http://mxr.mozilla.org/security/search?string=first+copy+caller+attributes+in.

Please fix pk11_AnyUnwrapKey, too.  I suggest
you create a new function for the common code
shared by the two functions.  Note that pk11_AnyUnwrapKey
requires CKA_VALUE_LEN be last in the template:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11skey.c&rev=1.122&mark=2058#2052

If you don't have time, just fix pk11_AnyUnwrapKey.

>+#define MAX_ADD_ATTRS 4
>+    CK_ATTRIBUTE    keyTemplate[MAX_TEMPL_ATTRS + MAX_ADD_ATTRS];
>+#undef MAX_ADD_ATTRS

If you want to limit MAX_ADD_ATTRS within this function,
you can also define it as a constant:
      const int MAX_ADD_ATTRS = 4;
or
      const int maxAddAttrs = 4;

It'd be nice to explain why it's 4.
(Because we may add CKA_CLASS, CKA_KEY_TYPE, CKA_VALUE_LEN,
and operation to the attribute template.)

Comment 10

[Wan-Teh Chang]

Attachment: Prevent buffer overflow in PK11_DeriveWithTemplate and pk11_AnyUnwrapKey (v5)

Replacing the macro with a const int doesn't work.

Patched checked in on the NSS trunk (NSS 3.13) and the
NSS_3_12_BRANCH (NSS 3.12.11).

Checking in pk11skey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11skey.c,v  <--  pk11skey.c
new revision: 1.123; previous revision: 1.122
done

Checking in pk11skey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11skey.c,v  <--  pk11skey.c
new revision: 1.119.2.3; previous revision: 1.119.2.2
done