426413 - Audit messages need distinct types

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Kai Engert (:KaiE:)]

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]

Attachment: Audit changes.

Comment 2

[Wan-Teh Chang]

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]

> 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]

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]

Attachment: Audit changes -- as checked in based on wtc review comments.

Comment 6

[Robert Relyea]

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]

This patch broke the tree . Some symbols are not defined,such as NSS_AUDIT_TEST .

Comment 8

[Robert Relyea]

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]

This is now fixed in the trunk