1653641 - Audit DTLS async certificate verification

Status: RESOLVED FIXED

(NSS :: Libraries, task, P1)

Description

[Kevin Jacobs [:kjacobs]]

ekr pointed out two issues in our DTLS code:

1. The comment at [1] doesn't jive with that the code actually does. We want to re-send an ACK, not another Finished.

2. The comment at [2] indicates that async authCertificate callbacks are not properly handled in DTLS. To block it, we'd need to fail if IS_DTLS ~here. However, we have a few tests that actually do test DTLS with an async callback. We need to audit the code and add tests as necessary to be sure this actually works.

Marking as security just in case the audit uncovers any issues.

[1] https://searchfox.org/nss/source/lib/ssl/dtls13con.c#416,422
[2] https://searchfox.org/nss/source/lib/ssl/dtlscon.c#273

Comment 1

[Eric Rescorla (:ekr)]

A few points.

1. It looks like the code translates SECWouldBlock to SECSuccess here:
   https://searchfox.org/nss/source/lib/ssl/ssl3con.c#11242

2. A little bit of stepoing through these tests in the debugger suggests they behave correctly, though perhaps some more checks are required.

3. We might consider changing
   https://searchfox.org/nss/source/lib/ssl/dtlscon.c#488

to rv != SECSuccess. I changed that locally and the tests seem OK

NIing MT in case he has thoughts.

Comment 2

[Martin Thomson [:mt:]]

I couldn't see anything that would indicate that async validation didn't work. The logic we rely upon - at least as far as I could see - is not DTLS-specific.

We don't support async validation for client certificates, which is different. That would require some changes, but I suspect that they wouldn't be that intrusive even. But that's a separate bug.

I did find a few things in my investigation:

• this looks suspect. It should probably be goto loser.

• ssl3_AlwaysFail doesn't always fail. It fails just once. It should probably reset restartTarget.

• this should probably be moved to ssl3_FinishHandshake, noting that it requires the recv buf lock open.

None of that suggests anything is wrong with our code for certificate validation.

Comment 3

[Eric Rescorla (:ekr)]

I think you are right about the goto loser thing.

With that said, as far as I can tell all that happens either way is that we bubble the error up the stack and the handshake fails, so it doesn't matter if we clear the buffer (which we do for other handshake message encoding errors because we goto loser). I think what's going on here is we have harmonized error handling for the "I don't want this message yet" and the "this message is busted" cases, and one requires looping and one does not. As far as I can all all of these returns should be goto loser just for consistency.

Kevin, can you please verify.

Comment 4

[Kevin Jacobs [:kjacobs]]

I don't see any cases where the return SECFailure is problematic, but yes, it should be goto loser to be safe and consistent.

The gtest coverage for async here is actually pretty complete: I only found one test that could be easily parameterized and the important cases already cover both variants. We obviously don't have the same level of application testing, but I think we can safely say that NSS does support this.

Edit: ssl3_AlwaysFail should indeed be reset each time, but there's no problem since fatalAlertSent gets set the first time it's assigned. That is checked in ssl3_GatherCompleteHandshake

I'll attach a patch addressing the tweaks.

Comment 5

[Kevin Jacobs [:kjacobs]]

Attachment: Bug 1653641 - Cleanup inaccurate DTLS comments, code review fixes.

Comment 6

[BugBot [:suhaib / :marco]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:kjacobs, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 7

[Kevin Jacobs [:kjacobs]]

https://hg.mozilla.org/projects/nss/rev/0e1b5c711cb9d810f8958857fafb5d0349d3c56f