Problems with our extended CK_C_INITIALIZE_ARGS structure.

NEW
Assigned to

Status

NSS
Libraries
P2
minor
12 years ago
7 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Robert Relyea)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 obsolete attachments)

(Reporter)

Description

12 years ago
NSS defines its extended CK_C_INITIALIZE_ARGS structure
as follows:

typedef struct CK_C_INITIALIZE_ARGS {
  CK_CREATEMUTEX CreateMutex;
  CK_DESTROYMUTEX DestroyMutex;
  CK_LOCKMUTEX LockMutex;
  CK_UNLOCKMUTEX UnlockMutex;
  CK_FLAGS flags;
  /* The official PKCS #11 spec does not have a 'LibraryParameters' field, but
   * a reserved field. NSS needs a way to pass instance-specific information
   * to the library (like where to find its config files, etc). This
   * information is usually provided by the installer and passed uninterpreted
   * by NSS to the library, though NSS does know the specifics of the softoken
   * version of this parameter. Most compliant PKCS#11 modules expect this
   * parameter to be NULL, and will return CKR_ARGUMENTS_BAD from
   * C_Initialize if Library parameters is supplied. */
  CK_CHAR_PTR *LibraryParameters;
  /* This field is only present if the LibraryParameters is not NULL. It must
   * be NULL in all cases */
  CK_VOID_PTR pReserved;
} CK_C_INITIALIZE_ARGS;

Note that 'LibraryParameters' has the type 'CK_CHAR_PTR *'.
It is a pointer to a pointer.  We always cast it to 'char *'
before use:

    if ((init_args && init_args->LibraryParameters)) {
        sftk_parameters paramStrings;

        crv = secmod_parseParameters
                ((char *)init_args->LibraryParameters, &paramStrings, isFIPS);

Since CK_CHAR is 'unsigned char', at first glance the 'char *'
cast is to cast away the 'unsigned', but it actually also gets
rid of the extraneous '*' in 'CK_CHAR_PTR *'.

The second problem, which Nelson pointed out before, is
that we probably should remove the 'pReserved' member
because the new 'LibraryParameters' member is exactly
what the original 'pReserved' member was for.
(Reporter)

Comment 1

12 years ago
Created attachment 226874 [details] [diff] [review]
Set and check the pReserved member

Bob said the pReserved member must be set to NULL
and is used as an end-of-structure marker.  I guess
that's a reasonable interpretation of the pReserved
member.  In any case, our pk11wrap layer isn't
setting pReserved to NULL, and softoken isn't checking
pReserved is NULL.  This patch does both.
Attachment #226874 - Flags: superreview?(nelson)
Attachment #226874 - Flags: review?(rrelyea)
(Reporter)

Comment 2

12 years ago
Comment on attachment 226874 [details] [diff] [review]
Set and check the pReserved member

OK, I found that the change to pk11load.c isn't
necessary because the pReserved member is set to
secmodLockFunctions.pReserved by a structure copy,
and the static variable secmodLockFunctions.pReserved
is initialized to 0 by the compiler.
I, for one, would like to see our structure exactly match the standard 
structure in size.  I would propose to eliminate pReserved from ours,
perhaps using something like this:

#ifdef NO_NSS_EXTENSIONS
  CK_VOID_PTR pReserved;  
#else
  CK_CHAR_PTR pLibraryParameters;
#endif
Comment on attachment 226874 [details] [diff] [review]
Set and check the pReserved member

r=nelson
If we're going to keep pReserved, then this fix is good.
Attachment #226874 - Flags: superreview?(nelson) → review+

Comment 5

12 years ago
Regarding comment 1, I don't see how pReserved could be assumed to be an end-of-structure marker, since its value is undefined in the PKCS#11 spec. The spec defines the CK_C_INITIALIZE_ARGS structure, as well as its size. I would favor something like Nelson's change in comment 3 instead.
(Assignee)

Updated

12 years ago
Attachment #226874 - Flags: review?(rrelyea) → review+
(Reporter)

Comment 6

12 years ago
Created attachment 234515 [details] [diff] [review]
Check the pReserved member (as checked in, and backed out)

I checked in this patch on the NSS trunk (3.12) and
NSS_3_11_BRANCH (3.11.3).
Attachment #226874 - Attachment is obsolete: true

Comment 7

12 years ago
(In reply to comment #6)
> Created an attachment (id=234515) [edit]
> Check the pReserved member (as checked in)
> 
> I checked in this patch on the NSS trunk (3.12) and
> NSS_3_11_BRANCH (3.11.3).

The patch does

-    if ((init_args && init_args->LibraryParameters)) {
+    if ((init_args && init_args->LibraryParameters && !init_args->pReserved)) {

which seems like a bad idea.

This accesses the NSS definition of CK_C_INITIALIZE_ARGS.pReserved, which is incompatible the standard PKCS#11 definition of CK_C_INITIALIZE_ARGS and lies beyond the end of the standard structure.

For any application that calls NSS using the standard structure definition, you are accessing an uninitialized memory location, which will have an arbitrary value of might not be mapped at all.

In other words, any standard PKCS#11 application is potentially broken by this patch. Please undo it.
(Reporter)

Comment 8

12 years ago
Comment on attachment 234515 [details] [diff] [review]
Check the pReserved member (as checked in, and backed out)

Andreas, I was in fact worrying about the same thing and
wanted to ask you:

When Java uses the NSS softoken, does it use the NSS extended
definition of CK_C_INITIALIZE_ARGS and set the pReserved field
to NULL?

I'm now worried that some softoken users may just use the
standard PKCS #11 definition of CK_C_INITIALIZE_ARGS and
use its pReserved field as the LibraryParameters field for
softoken.  So I'm no longer sure if my patch (attachment
234515 [review]) is a good idea.

Since Andreas and I both thought of this issue, I've backed
out my patch.
Attachment #234515 - Attachment is obsolete: true

Comment 9

12 years ago
(In reply to comment #8)
> 
> When Java uses the NSS softoken, does it use the NSS extended
> definition of CK_C_INITIALIZE_ARGS and set the pReserved field
> to NULL?

No, we use the standard PKCS#11 definition of CK_C_INITIALIZE_ARGS and just set pReserved to the NSS parameter string. Since the standard definition does not include the extra field, it is never initialized.

In my opinion, if no part of NSS ever used the extra field defined in the NSS version of the structure, it seems sensible to remove the declaration, as Nelson suggested earlier.
Priority: -- → P4

Updated

9 years ago
Blocks: 459298
Assignee: nobody → rrelyea
Priority: P4 → P2
Whiteboard: FIPS
Target Milestone: --- → 3.12.7

Comment 10

9 years ago
If this bug is completed by Nov17 2008 it will be included in the FIPS2008 validation otherwise it will be dropped for a later release.

Updated

9 years ago
Whiteboard: FIPS

Comment 11

9 years ago
removed from FIPS2009. will consider for future release.
No longer blocks: 459298
(Reporter)

Updated

8 years ago
Attachment #234515 - Attachment description: Check the pReserved member (as checked in) → Check the pReserved member (as checked in, and backed out)
(Reporter)

Updated

8 years ago
Target Milestone: 3.12.7 → 3.13
You need to log in before you can comment on or make changes to this bug.