Need SSL tests using FIPS140-2 softoken

RESOLVED FIXED in 3.11.8

Status

NSS
Test
P2
enhancement
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Julien Pierre, Assigned: Slavomir Katuscak)

Tracking

3.11.1
3.11.8

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

12 years ago
Currently, ssl.sh only tests softoken in non-FIPS mode.
We need to make another pass in FIPS mode to make sure the higher-level code is OK.

SSL2 tests need to be disabled in this case, but all other tests are expected to work.
(Reporter)

Updated

12 years ago
Severity: normal → enhancement
Priority: -- → P2
Target Milestone: --- → 3.11.2
assigning to Christophe, who I believe is already working on this.
Christophe, please attach your patch (or new script) to this bug 
so that others can start to test with it.
Assignee: nobody → christophe.ravel.bugs
(Reporter)

Comment 2

12 years ago
Created attachment 222265 [details] [diff] [review]
Christophe's initial patch

This is what Christophe put together friday. It works fine against the tip. It does FIPS140-2 only, not non-FIPS and bypass modes.

This patch still tries to run the SSL2 tests, so you have to ignore these results.
(Reporter)

Comment 3

12 years ago
Retargetting all P2s to 3.11.3 .
Target Milestone: 3.11.2 → 3.11.3
(Reporter)

Comment 4

12 years ago
Comment on attachment 222265 [details] [diff] [review]
Christophe's initial patch

This patch needs to be changed to do the following before it can be checked in :
- skip all SSL2 cipher suites when the softoken is in FIPS140-2 mode
- skip all export cipher suites when the softoken is in FIPS140-2 mode
Attachment #222265 - Flags: review-
(In reply to comment #4)
> This patch needs to be changed to do the following before it can be checked in
> - skip all SSL2 cipher suites when the softoken is in FIPS140-2 mode
> - skip all export cipher suites when the softoken is in FIPS140-2 mode

Should these perhaps remain as negative tests?  
(That's not a rhetorical question.  I'm really not sure.)

(Reporter)

Comment 6

12 years ago
Nelson,

No, they can't be negative tests because not all export cipher suites fail in FIPS mode, only a few of them, as reported in bug 338211 . So the best we can do is skip them.
OS: SunOS → All
Version: unspecified → 3.11.1
(Reporter)

Comment 7

12 years ago
*** Bug 338211 has been marked as a duplicate of this bug. ***

Updated

11 years ago
Assignee: christophe.ravel.bugs → glen.beasley
Target Milestone: 3.11.3 → 3.11.8
Assignee: glen.beasley → slavomir.katuscak
(Assignee)

Comment 8

11 years ago
Created attachment 271648 [details] [diff] [review]
Patch.

New patch based on Christophe's patch.

Some changes:
-other tests are not disabled by default
-tests in FIPS mode can be disabled (using NSS_TEST_DISABLE_FIPS variable, similar to NSS_TEST_DISABLE_BYPASS)
-SSL2 tests and export tests are skipped in FIPS mode
Attachment #222265 - Attachment is obsolete: true
Attachment #271648 - Flags: review?(julien.pierre.boogz)
(Assignee)

Comment 9

11 years ago
Created attachment 271650 [details] [diff] [review]
Patch v2.

Small fix to previous patch (fixed message when skipping FIPS-mode tests).
Attachment #271648 - Attachment is obsolete: true
Attachment #271650 - Flags: review?(julien.pierre.boogz)
Attachment #271648 - Flags: review?(julien.pierre.boogz)
(Reporter)

Comment 10

11 years ago
Comment on attachment 271650 [details] [diff] [review]
Patch v2.

Why do we need to make the FIPS tests optional ?

Assuming we do, this patch is lacking error checking on the new modutil commands.
Attachment #271650 - Flags: review?(julien.pierre.boogz) → review-
(Assignee)

Comment 11

11 years ago
Created attachment 271823 [details] [diff] [review]
Patch v3.

Testing in FIPS mode takes some time. There was a request to create a shorter version of all.sh (bug 335752), and we decided to have some test blocks optional. As bypass mode is optional, I think with FIPS mode it should be the same.

Modutil error checking added in this patch.
Attachment #271650 - Attachment is obsolete: true
Attachment #271823 - Flags: review?(julien.pierre.boogz)
(Reporter)

Comment 12

11 years ago
Comment on attachment 271823 [details] [diff] [review]
Patch v3.

You need to check the return code for each modutil. You call modutil twice in a row, but only check the result from the 2nd one.
Attachment #271823 - Flags: review?(julien.pierre.boogz) → review-
(Assignee)

Comment 13

11 years ago
Created attachment 272142 [details] [diff] [review]
Patch v4.

Added checking for return values.

In fact I was not checking return value of modutil before, but return value of grep piped to it (to check if fips module is really in db). Now there are 3 tests - return value of first modutil (setting mode), return value of second modutil (db list) and return value of grep (checking if fips module is in db).
Attachment #271823 - Attachment is obsolete: true
Attachment #272142 - Flags: review?(julien.pierre.boogz)
(Assignee)

Comment 14

11 years ago
Created attachment 272143 [details] [diff] [review]
Patch v5.

Fixed messages.
Attachment #272142 - Attachment is obsolete: true
Attachment #272143 - Flags: review?(julien.pierre.boogz)
Attachment #272142 - Flags: review?(julien.pierre.boogz)
(Reporter)

Comment 15

11 years ago
Slavo, there is no difference between patchv4 and patchv5 . What did you intend to change ?
(Reporter)

Comment 16

11 years ago
Comment on attachment 272143 [details] [diff] [review]
Patch v5.

OK, I see it. I think you meant to change the words "on" to "off" in the second block (turning FIPS "off" rather than "on"). Please attach the correct patch.
Attachment #272143 - Flags: review?(julien.pierre.boogz) → review-
(Assignee)

Comment 17

11 years ago
Comment on attachment 272143 [details] [diff] [review]
Patch v5.

New patch was patch v4, but after I sent it to bugzilla I fixed on to off and sent is as patch v5. So patch v5 should be the correct patch and changes between v3 and v5 are described in comment #13.
Attachment #272143 - Flags: review- → review?(julien.pierre.boogz)
(Reporter)

Comment 18

11 years ago
Comment on attachment 272143 [details] [diff] [review]
Patch v5.

Slavo,

You mistakenly attached the same patch for patchv4 and patchv5 . The diff diff feature in bugzilla shows no differences, as well as manual inspection. Please attach the patch with the correct strings.
Attachment #272143 - Attachment is obsolete: true
Attachment #272143 - Flags: review?(julien.pierre.boogz)
(Assignee)

Comment 19

11 years ago
Created attachment 272942 [details] [diff] [review]
Patch v6.

Sorry, I haven't noticed this. Sending new patch now.
Attachment #272942 - Flags: review?(julien.pierre.boogz)
(Reporter)

Updated

11 years ago
Attachment #272942 - Flags: review?(julien.pierre.boogz) → review+
(Assignee)

Comment 20

11 years ago
Trunk:
Checking in ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.81; previous revision: 1.80
done
(Assignee)

Updated

11 years ago
Attachment #272942 - Flags: superreview?(nelson)
Comment on attachment 272942 [details] [diff] [review]
Patch v6.

r=nelson.  Note that

>+    if [ "${FIPSMODE}" = "true" ] ; then
>+        RET_EXP=0
>+    else
>+        RET_EXP=1
>+    fi

is equivalent to this:

      [ "${FIPSMODE}" = "true" ] ; RET_EXP=$?
Attachment #272942 - Flags: superreview?(nelson) → superreview+
(Assignee)

Comment 22

11 years ago
Checked in last patch + Nelson's suggested change from previous comment.

Trunk:
Checking in ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.82; previous revision: 1.81
done

Branch:
Checking in ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.61.2.13; previous revision: 1.61.2.12
done
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.