The default bug view has changed. See this FAQ.

Audit messages need distinct types

RESOLVED FIXED in 3.12.3

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: kaie, Assigned: Robert Relyea)

Tracking

3.12
3.12.3
All
Linux

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: FIPS)

Attachments

(2 attachments)

(Reporter)

Description

9 years ago
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.
(Reporter)

Updated

9 years ago
Version: trunk → 3.12

Updated

9 years ago
Blocks: 459298
(Assignee)

Updated

9 years ago
Priority: -- → P2
(Assignee)

Updated

9 years ago
OS: All → Linux
(Assignee)

Updated

9 years ago
Whiteboard: FIPS
(Assignee)

Comment 1

8 years ago
Created attachment 358869 [details] [diff] [review]
Audit changes.
Attachment #358869 - Flags: review?(wtc)

Comment 2

8 years ago
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?
Attachment #358869 - Flags: review?(wtc) → review+
(Assignee)

Comment 3

8 years ago
> 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

8 years ago
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.)
(Assignee)

Comment 5

8 years ago
Created attachment 359143 [details] [diff] [review]
Audit changes -- as checked in based on wtc review comments.
(Assignee)

Comment 6

8 years ago
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
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Target Milestone: --- → 3.12.3

Comment 7

8 years ago
This patch broke the tree . Some symbols are not defined,such as NSS_AUDIT_TEST .
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 8

8 years ago
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
(Assignee)

Comment 9

8 years ago
This is now fixed in the trunk
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.