354900 - Audit modifications, accesses, deletions, and additions of cryptographic keys

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Wan-Teh Chang]

FIPS 140-2 Derived Test Requirement AS06.17 requires the module
to record modifications, accesses, deletions, and additions of
cryptographic keys.

We need to audit the following PKCS #11 functions of the softoken.

* Object management functions, when the object is a cryptographic key
   (object class CKO_PUBLIC_KEY, CKO_PRIVATE_KEY, and CKO_SECRET_KEY)
  o FC_CreateObject: addition of cryptographic keys
  o FC_CopyObject: access and addition of cryptographic keys
  o FC_DestroyObject: deletion of cryptographic keys
  o FC_GetObjectSize: access of cryptographic keys
  o FC_GetAttributeValue: access of cryptographic keys
  o FC_SetAttributeValue: modification of cryptographic keys
* Key management functions
  o FC_GenerateKey: addition of cryptographic keys
  o FC_GenerateKeyPair: addition of cryptographic keys
  o FC_WrapKey: access of cryptographic keys
  o FC_UnwrapKey: access and addition of cryptographic keys
  o FC_DeriveKey: access and addition of cryptographic keys 
* Cipher "Init" functions
  o FC_EncryptInit: access of cryptographic keys
  o FC_DecryptInit: access of cryptographic keys
  o FC_SignInit: access of cryptographic keys
  o FC_SignRecoverInit: access of cryptographic keys
  o FC_VerifyInit: access of cryptographic keys
  o FC_VerifyRecoverInit: access of cryptographic keys
* Miscellaneous
  o FC_DigestKey: access of cryptographic keys

The audit messages generated by these functions are specified at
http://wiki.mozilla.org/FIPS_Operational_Environment#AS06.17

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch for stage 1

I'm going to implement the audit logging in two stages.  In stage 1,
I will do everything except logging the key objects' attributes.

Summary of the patch.

1. New file fipsaudt.c

To minimize the changes to existing files, I created the new
file fipsaudt.c for the new audit functions.  There is one audit
function for each of the PKCS #11 functions.  In general, the
audit function for FC_Foo is sftk_AuditFoo.  The exception is
the FC_<CryptoOperation>Init functions, which share the same
audit function sftk_AuditCryptInit.

The audit functions take all the input arguments of the
corresponding PKCS #11 functions plus "CK_RV rv".

If you look at these audit functions, you'll see that they are
all similar, so it's possible for them to share more code.  But
I don't have time to do that now.

2. softoken.h

The new audit functions are declared in softoken.h.

3. fipstokn.c and pkcs11c.c

Change the existing audit logging code to print the session
handles hSession using the format 0x08%lX (as a hexadecimal
number) instead of %lu (as a decimal number).

4. fipstokn.c

Rename the names of some input arguments usXXX to ulXXX
to match their names in the PKCS #11 standard v2.20.

5. fipstokn.c

Change the functions to call the audit functions before
they return.  This requires changing the functions to
have essentially only one return statement so that we
can call the audit function before the return statement.
Some of these functions may have some "unimportant"
early return statements, which won't be logged.

Unfortunately I have to change the function
fips_login_if_key_object.  Its name is not accurate
when the new audit logging code is added, because
we need to audit not only secret and private keys
but also public keys.

So I coin the term "secure key" to mean a secret or
private key.

I also need to have the object class of the object.
So I have to make the fips_login_if_key_object function
return the object class.  In the end, I decided to change
the code:

     rv = fips_login_if_key_object(hSession, hObject);

to

     CK_OBJECT_CLASS objClass = CKO_NOT_A_KEY;
     rv = fips_get_object_class(hSession, hObject, &objClass);
     if ((rv == CKR_OK) && SFTK_IS_SECURE_KEY_OBJECT(objClass)) {
 	rv = sftk_fipsCheck();
     }

You will see that the if statement is equivalent to
the following code in the fips_login_if_key_object function:

     if (rv == CKR_OK) {
 	if ((objClass == CKO_PRIVATE_KEY) || (objClass == CKO_SECRET_KEY)) {
 	    rv = sftk_fipsCheck();
 	}
     }

Please let me know if you have questions.  Glen, I can
go over the patch with you in person on Monday.

Comment 2

[Robert Relyea]

Comment on attachment 240688 [details] [diff] [review]
Proposed patch for stage 1

r+ with the following fixed:

1) Make sure the copyright block is correct (Initial developer == Netscape?).

2) Use your new macros (SFTK_IS_SECURE_KEY and SFTK_IS_KEY) in FC_CreateObject.

You are correct about duplication. At some point the following should be fixed:

1) The whole rv=fips_get_object_class(...)
block thing should be replaces with a single function or macro.

2) we should add a function that takes an object handle and template and prints out the object (when you add logging template information, this would be a good time to add this).

bob

Comment 3

[Wan-Teh Chang]

Attachment: Proposed patch for stage 1 (as checked in)

I made the two changes that Bob suggested.
1. In the new file fipsaudt.c, the license header says the Original Code
is "Network Security Services (NSS)" instead of "the Netscape security
libraries", and the Initial Developer of the Original Code is
"Red Hat, Inc." instead of "Netscape Corporation".
2. I use the new macros SFTK_IS_SECURE_KEY_OBJECT and SFTK_IS_KEY_OBJECT
in FC_CreateObject.

I checked in the patch on the NSS trunk (NSS 3.12) and the NSS_3_11_BRANCH
(NSS 3.11.4).  I will attach another patch that cleans up the code soon.

Checking in fipsaudt.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipsaudt.c,v  <--  fipsaudt.c
initial revision: 1.1
done
Checking in fipstokn.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipstokn.c,v  <--  fipstokn.c
new revision: 1.22; previous revision: 1.21
done
Checking in manifest.mn;
/cvsroot/mozilla/security/nss/lib/softoken/manifest.mn,v  <--  manifest.mn
new revision: 1.26; previous revision: 1.25
done
Checking in pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.91; previous revision: 1.90
done
Checking in softoken.h;
/cvsroot/mozilla/security/nss/lib/softoken/softoken.h,v  <--  softoken.h
new revision: 1.14; previous revision: 1.13
done

Checking in fipsaudt.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipsaudt.c,v  <--  fipsaudt.c
new revision: 1.1.2.1; previous revision: 1.1
done
Checking in fipstest.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipstest.c,v  <--  fipstest.c
new revision: 1.13.2.9; previous revision: 1.13.2.8
done
Checking in fipstokn.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipstokn.c,v  <--  fipstokn.c
new revision: 1.11.2.11; previous revision: 1.11.2.10
done
Checking in manifest.mn;
/cvsroot/mozilla/security/nss/lib/softoken/manifest.mn,v  <--  manifest.mn
new revision: 1.24.2.2; previous revision: 1.24.2.1
done
Checking in pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.68.2.21; previous revision: 1.68.2.20
done
Checking in softoken.h;
/cvsroot/mozilla/security/nss/lib/softoken/softoken.h,v  <--  softoken.h
new revision: 1.7.30.5; previous revision: 1.7.30.4
done

Comment 4

[Wan-Teh Chang]

Attachment: Proposed patch for stage 1 (as checked in, corrected)

I checked in a change to mozilla/security/nss/lib/softoken/fipstest.c
on the NSS_3_11_BRANCH (rev. 1.13.2.9) by mistake.  I've backed it out.
Here is the corrected patch.

Comment 5

[Wan-Teh Chang]

Attachment: Code cleanup patch

This patch adds consolidates some duplicate code, checks for
NULL pointers, and renames a macro.

1. fipsaudt.c

Consolidate the code that prints the returned object handle
value in the new function sftk_PrintReturnedObjectHandle.
We check for a NULL "pointer to object handle" argument in
this function.

2. fipsaudt.c

Add a new function sftk_PrintMechanism to print pMechanism
and the CK_MECHANISM structure it points to.  We check for
a NULL pMechanism argument in this function.

3. fipstokn.c

The SFTK_IS_SECURE_KEY_OBJECT is renamed SFTK_IS_NONPUBLIC_KEY_OBJECT.
Please let me know if you think "nonpublic key" is clearer than
"secure key".

Comment 6

[Wan-Teh Chang]

Attachment: Code cleanup patch v2

This patch also implements the first "future work" change that
Bob suggested in comment 2.

The reason I didn't do this before was that without a good function
name, the code would actually be harder to understand.  The function
obtains the object class, and calls sftk_fipsCheck() if the object is
a secret key or private key.  It's not easy to come up with a function
name that describes what it does succinctly.  In this patch, I'm using
sftk_get_object_class_and_fipsCheck().  I hope you can suggest a better
name.

Again, please let me know if you think SFTK_IS_NONPUBLIC_KEY_OBJECT
is a better name than SFTK_IS_SECURE_KEY_OBJECT.

Bob, I'm also wondering if softoken.h is the right header for these
audit-related function declarations.  lib/softoken has several private
headers, and I don't know what their purposes are.  Is there a more
appropriate header for declaring these audit functions, or should I
add a new private header fipstokn.h?

Comment 7

[Wan-Teh Chang]

Comment on attachment 241086 [details] [diff] [review]
Code cleanup patch v2

By the way, the original name of that function is
fips_login_if_key_object.  That name is not suitable
for two reasons:
1. the function now also returns the object class to
the caller.  The name fips_login_if_key_object doesn't
convey that.
2. "key object" is not accurate because we don't
call sftk_fipsCheck() if the object is a public key
object.

Comment 8

[glen beasley]

Comment on attachment 241086 [details] [diff] [review]
Code cleanup patch v2

looks good.

Comment 9

[Robert Relyea]

Comment on attachment 241086 [details] [diff] [review]
Code cleanup patch v2

r+=relyea

Comment 10

[Wan-Teh Chang]

Comment on attachment 241086 [details] [diff] [review]
Code cleanup patch v2

I checked in the "code cleanup patch v2" on the NSS trunk (3.12)
and NSS_3_11_BRANCH (3.11.4).

Checking in fipsaudt.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipsaudt.c,v  <--  fipsaudt.c
new revision: 1.2; previous revision: 1.1
done
Checking in fipstokn.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipstokn.c,v  <--  fipstokn.c
new revision: 1.23; previous revision: 1.22
done

Checking in fipsaudt.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipsaudt.c,v  <--  fipsaudt.c
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in fipstokn.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipstokn.c,v  <--  fipstokn.c
new revision: 1.11.2.12; previous revision: 1.11.2.11
done

Comment 11

[Neil Williams]

Looks good to me too.