Last Comment Bug 426413 - Audit messages need distinct types
: Audit messages need distinct types
Status: RESOLVED FIXED
FIPS
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.12
: All Linux
: P2 normal (vote)
: 3.12.3
Assigned To: Robert Relyea
:
Mentors:
Depends on:
Blocks: FIPS2008
  Show dependency treegraph
 
Reported: 2008-04-01 13:59 PDT by Kai Engert (:kaie)
Modified: 2009-03-09 15:09 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Audit changes. (13.66 KB, patch)
2009-01-26 10:36 PST, Robert Relyea
wtc: review+
Details | Diff | Review
Audit changes -- as checked in based on wtc review comments. (13.86 KB, patch)
2009-01-27 15:10 PST, Robert Relyea
no flags Details | Diff | Review

Description Kai Engert (:kaie) 2008-04-01 13:59:12 PDT
Copied from https://bugzilla.redhat.com/show_bug.cgi?id=428274

Description of problem:
All the audit messages related to FIPS are using audit message type AUDIT_USER
which has a value of 1005. From /usr/include/linux/audit.h, the range 2400 -
2499 is reserved for user space crypto events.

We need to define the types of events that NSS is reporting and change the code
to use the correct audit message types preferably during F9 development.
Comment 1 Robert Relyea 2009-01-26 10:36:34 PST
Created attachment 358869 [details] [diff] [review]
Audit changes.
Comment 2 Wan-Teh Chang 2009-01-27 14:06:45 PST
Comment on attachment 358869 [details] [diff] [review]
Audit changes.

r=wtc, with some questions and suggested changes.

1. lib/softoken/fipsaudt.c

> void sftk_AuditCryptInit(const char *opName, CK_SESSION_HANDLE hSession,
>@@ -202,7 +202,7 @@ void sftk_AuditCryptInit(const char *opN
> 	"hKey=0x%08lX)=0x%08lX",
> 	opName, (PRUint32)hSession, mech,
> 	(PRUint32)hKey, (PRUint32)rv);
>-    sftk_LogAuditMessage(severity, msg);
>+    sftk_LogAuditMessage(severity, NSS_AUDIT_ENCRYPT, msg);
> }

I remember sftk_AuditCryptInit is used for all crypto operations,
not just C_Encrypt, so NSS_AUDIT_ENCRYPT seems a little too
narrow.  How about renaming it NSS_AUDIT_CRYPTO?

2. lib/softoken/fipstokn.c

>+#define AUDIT_CRYPTO_FAILURE_USER       2405 /* Fail decrypt,encrypt,randomiz 
>+*/

"randomiz" is missing an 'e' at the end, and the closing
"*/" should be moved to the previous line.

In the sftk_mapLinuxAuditType function:

>+    case NSS_AUDIT_TEST: 
>+    case NSS_AUDIT_RNG:
>+	return AUDIT_CRYPTO_TEST_USER;

Why is NSS_AUDIT_RNG also mapped to AUDIT_CRYPTO_TEST_USER?

>+    /* default */
>+    return AUDIT_CRYPTO_PARAM_CHANGE_USER;

You may want to move this to a "default" case in the switch
statement.  We should add a "not reached" assertion here.

>@@ -1416,7 +1464,7 @@ CK_RV FC_GetSlotInfo(CK_SLOT_ID slotID, 
> 			"self-test: continuous RNG test failed",
> 			(PRUint32)hSession,pRandomData,
> 			(PRUint32)ulRandomLen,(PRUint32)crv);
>-	    sftk_LogAuditMessage(NSS_AUDIT_ERROR, msg);
>+	    sftk_LogAuditMessage(NSS_AUDIT_ERROR, NSS_AUDIT_RNG, msg);

It seems that you could use NSS_AUDIT_TEST instead here.
Then sftk_mapLinuxAuditType doesn't need to map NSS_AUDIT_RNG
to AUDIT_CRYPTO_TEST_USER

3.   lib/softoken/softoknt.h

>+typedef enum {
>+	NSS_AUDIT_ACCESS_KEY = 0,
>+	NSS_AUDIT_CHANGE_KEY,
>+	NSS_AUDIT_COPY_KEY,
>+	NSS_AUDIT_DERIVE_KEY,
>+	NSS_AUDIT_DESTROY_KEY,
>+	NSS_AUDIT_DIGEST_KEY,
>+	NSS_AUDIT_ENCRYPT,
>+	NSS_AUDIT_FIPS_STATE,
>+	NSS_AUDIT_GENERATE_KEY,
>+	NSS_AUDIT_INIT_PIN,
>+	NSS_AUDIT_INIT_TOKEN,
>+	NSS_AUDIT_LOAD_KEY,
>+	NSS_AUDIT_LOGIN,
>+	NSS_AUDIT_LOGOUT,
>+	NSS_AUDIT_RNG,
>+	NSS_AUDIT_SET_PIN,
>+	NSS_AUDIT_TEST,
>+	NSS_AUDIT_UNWRAP_KEY,
>+	NSS_AUDIT_WRAP_KEY,
>+} NSSAuditType;

Nit: indent the inners of the enum typedef by 4.

Rename NSS_AUDIT_TEST => NSS_AUDIT_SELF_TEST?
Comment 3 Robert Relyea 2009-01-27 14:36:19 PST
> 1. lib/softoken/fipsaudt.c
> remember sftk_AuditCryptInit is used for all crypto operations

I'll change the name to NSS_AUDIT_CRYPT.

> "randomiz" is missing an 'e' at the end,

I'll fix this as well.

> Why is NSS_AUDIT_RNG also mapped to AUDIT_CRYPTO_TEST_USER?

Because it was a failure of the rng test. (though maybe  AUDIT_CRYPTO_FAILURE_USER should be used instead).

> It seems that you could use NSS_AUDIT_TEST instead here.
> Then sftk_mapLinuxAuditType doesn't need to map NSS_AUDIT_RNG
> to AUDIT_CRYPTO_TEST_USER

I probably should either map NSS_AUDIT_RNG to AUDIT_CRYPTO_FAILURE_USER, or remove NSS_AUDIT_RNG and just pass NSS_AUDIT_TEST as you suggest. Which do you think is expected (I think the former).

> Nit: indent the inners of the enum typedef by 4.

I'll still fix that.

> Rename NSS_AUDIT_TEST => NSS_AUDIT_SELF_TEST?

Your suggested name is better, I'll use that.

------------------------------------

> >+    /* default */
> >+    return AUDIT_CRYPTO_PARAM_CHANGE_USER;
> 
> You may want to move this to a "default" case in the switch
> statement.  We should add a "not reached" assertion here.

yah, I purposefully structured the code this way for 2 reasons:  1) I didn't want to try to silence the compilier warning of not returning a value [or add an annoying fake value], and 2) since we are switching on an enum, many compiliers will issue a warning if there is a missing enum case. A default statement would silence that warning.

bob
Comment 4 Wan-Teh Chang 2009-01-27 14:50:35 PST
I prefer removing NSS_AUDIT_RNG and just pass NSS_AUDIT_TEST
for the failure of the continuous RNG test, because you are
already passing NSS_AUDIT_TEST for the failure of all the other
self tests (power up self tests and pairwise consistency test).

It may be good to add a concise version of your explanation
of not using a default case as a comment, so that the next
person won't try to add it.  (Your reasons are good.)
Comment 5 Robert Relyea 2009-01-27 15:10:29 PST
Created attachment 359143 [details] [diff] [review]
Audit changes -- as checked in based on wtc review comments.
Comment 6 Robert Relyea 2009-01-27 15:13:54 PST
Checking in lib/softoken/fipsaudt.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipsaudt.c,v  <--  fipsaudt.c
new revision: 1.3; previous revision: 1.2
done
Checking in lib/softoken/fipstokn.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipstokn.c,v  <--  fipstokn.c
new revision: 1.27; previous revision: 1.26
done
Checking in lib/softoken/pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.159; previous revision: 1.158
done
Checking in lib/softoken/pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.107; previous revision: 1.106
done
Checking in lib/softoken/softoken.h;
/cvsroot/mozilla/security/nss/lib/softoken/softoken.h,v  <--  softoken.h
new revision: 1.21; previous revision: 1.20
done
Checking in lib/softoken/softoknt.h;
/cvsroot/mozilla/security/nss/lib/softoken/softoknt.h,v  <--  softoknt.h
new revision: 1.5; previous revision: 1.4
done
Comment 7 Julien Pierre 2009-01-27 15:54:39 PST
This patch broke the tree . Some symbols are not defined,such as NSS_AUDIT_TEST .
Comment 8 Robert Relyea 2009-01-27 18:09:02 PST
backing out....

I should have done a make clean in my test build.
I'll submit a new patch in the morning... thanks for the heads up Julian.

bob
Comment 9 Robert Relyea 2009-03-09 15:09:45 PDT
This is now fixed in the trunk

Note You need to log in before you can comment on or make changes to this bug.