Closed Bug 1444148 Opened 6 years ago Closed 6 years ago

/cmd/fipstest needs KAS tests for ECC and DH

Categories

(NSS :: Libraries, enhancement, P3)

3.38
enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rrelyea, Assigned: rrelyea)

References

Details

Attachments

(2 files)

We need to add cavs tests for KAS testing to the fipstest tool.
NOTE: the fipstest tool is pretty bare bones, with no protection against crashes and overflows from incorrectly formatted CAVS inputs. I've made no attempt to either fix the existing issues, or be particularly safe in the new code (though I do kick out better error messages than the other CAVS tests.

There are 2 flavors of each test. The first flavor (functional) creates a temp ECDH/DH key, derives a resulting key and then hashes the result and returns it. The second flavor (validate) takes an externally supplied private ECDH/DH key and an externally supplied (unrelated) ECDH/DH public key. It validates the public key and then tries to derive a resulting key and hashes that resulting key. If the resulting key mathes the supplied hash, then a 'P' result is returned, otherwise an 'F' result is returned.

I did not include any verification in the scripts which drive the fips test. I didn't have an .fax files to verify against, and the 'functional' tests need the original private key for the supplied public key in order to verify (which only the LAB that created the vectors have.

bob
Assignee: nobody → rrelyea
Status: NEW → ASSIGNED
Attachment #8957239 - Flags: review?(kaie)
Out of curiosity, did you write this code from scratch, or is based on other, existing code?
Flags: needinfo?(rrelyea)
I started with the ecdsa code for ecdh, and used the ecdh code for dh. I borrowed heavily from other test cases as well (dsa, etc). Most of the parsing stuff came from elsewhere (which is why there's lots of brittleness in the parsing code and the warning about lack of protection for overflows, etc). Hopefully these will be the last CAVS tests we need to add before going to ACVP in the future.
Flags: needinfo?(rrelyea)
The 4 new functions ignore the value of the "response" parameter, is that intentional?
yes, it turns out for some kinds of key agreement protocols it matters if you are the initiator or responder. NIST tests both initiator and responder for all protocols. In this case, however, it doesn't matter, the work is exactly the same. The equivalent would be something like rsa private key operations. If you do a raw RSA sign or raw RSA decrypt, the math is the same, but if you do PKCS #1 padded RSA sign and PKCS #1 RSA decrypt, then you need to know specifically if you are signing or decrypting.
Priority: -- → P3
I've talked to Tomas Mraz, who explained me some background of this code.

My understanding:
- as of today, the fipstest program isn't executed as part of the NSS test suite
- fipstest is used to operate on input, which is provided by e.g. a FIPS validation lab
- fipstest will produce output, which will given to the lab, which then performs verification

As a result, the attached patch won't affect the regular testing of NSS.
It will only be executed manually, by people who work with a lab to get the NSS code validated.

It means, any mistakes in this code won't be detected by the NSS test suite, but only at a time a validation of NSS is attempted.

(I'm documenting these facts for reference purposes, because they weren't clear to me.)

I understand that for some classes of tests, the output is unpredictable, because it involves creating random data. A testing lab will verify that the generated data is of proper structure and the generates keys have proper expected attributes.

However, for some classes of tests, the processing performed by fipstest is predictable. In other words, for some classes of tests, given a specific input, the output will always be the same.

It seems that currently, we don't have any example test input and matching expected output easily available. I think it would be good to have that. I'll file a bug, that suggests that as a TODO.

For this particular bug, my understanding is:

Doing a sanity review of this code is all we can do. It adds an additional code path to the existing fipstest tool. The existing code pathes of the fipstest tool aren't regularly tested by the NSS test suite either.
See Also: → 1466555
I'm unable to verify this code for functional correctness. I assume you have already executed this code on input data that you were given, and you have verified that it produces the expected output, is that correct?

Would it be possible to implement bug 1466555 for this new code? If yes, please do.

I'm OK to add this code, according to the state of the fipstest code as explained in comment 6.

However, I'd strongly recommend to implement bug 1466555.

Nevertheless, even without a fix for bug 1466555, it seems better to have this code checked in into the primary NSS tree, for reference purposes. It's better than having this code live in a private location, only.
Attached patch kas-format.patchSplinter Review
This patch fixes the source code formatting of the attached add_kas_tests.patch to be according to the expectations.

Please apply this additional patch, on top of the other patch, prior to checkin in.

This files was automatically generated, it's limited to whitespace changes, and doesn't require separate review.
Comment on attachment 8957239 [details] [diff] [review]
add_kas_tests.patch

r=kaie for adding this code to the fipstest tool, which is neither part of the NSS code distribution, nor part of the NSS test suite.
Attachment #8957239 - Flags: review?(kaie) → review+
Looks like this landed. Can it be closed?
Flags: needinfo?(rrelyea)
Flags: needinfo?(rrelyea)
yes
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.39
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: