Closed Bug 1075991 Opened 5 years ago Closed 5 years ago

Telemetry for TLS fallback protection

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: mt, Assigned: mt)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Might be good to track occurrences of TLS version intolerance fallback causes the server to send an inappropriate_fallback alert.

The simplest solution is to increment a single counter each time the alert is received.

A more thorough solution might include separate counters for each of the reasons, so that we can track what types of errors cause inappropriate fallbacks.
:keeler, do you have any opinion on whether the more detailed information would help?  We'd have to modify the intolerance store to track the error code that triggered the version fallback.  The upside of doing the more complex option is that, tracking the original error would allow be necessary for bug 1075167 (fixing error reporting) if we went there.
Flags: needinfo?(dkeeler)
I've taken the high road here which should allow us to report the reason when TLS version intolerance is falsely detected.

This actually includes unit tests :)  The next one won't :(
Attachment #8498481 - Flags: review?(dkeeler)
Flags: needinfo?(dkeeler)
I'm a little sad about how big this diff is, but it was the only way to ensure that the telemetry buckets are consistent between the various version-fallback options.

Tested using about:telemetry.
Attachment #8498482 - Flags: review?(dkeeler)
See Also: → POODLE
Comment on attachment 8498481 [details] [diff] [review]
0001-Bug-1075991-Remember-version-intolerance-reason-code.patch

Review of attachment 8498481 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +878,5 @@
>    if (entry.intolerant != 0) {
>      // We've tried connecting at a higher range but failed, so try at the
>      // version we haven't tried yet, unless we have reached the minimum.
>      if (range.min < entry.intolerant) {
>        range.max = entry.intolerant - 1;

Do we need to do anything with the intoleranceReason in adjustForTLSIntolerance here?
Attachment #8498481 - Flags: review?(dkeeler) → review+
Comment on attachment 8498482 [details] [diff] [review]
0002-Bug-1075991-Tracking-cause-of-inappropriate-TLS-vers.patch

Review of attachment 8498482 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +1090,5 @@
> +    // same buckets as the telemetry below, except that bucket 0 will include
> +    // all cases where there wasn't an original reason.
> +    PRErrorCode originalReason = socketInfo->SharedState().IOLayerHelpers()
> +      .getIntoleranceReason(socketInfo->GetHostName(),
> +                            socketInfo->GetPort());

nit: seems like this line could go on the end of the previous one
Attachment #8498482 - Flags: review?(dkeeler) → review+
(In reply to David Keeler (:keeler) [use needinfo?] from comment #5)
> >      if (range.min < entry.intolerant) {
> >        range.max = entry.intolerant - 1;
> 
> Do we need to do anything with the intoleranceReason in
> adjustForTLSIntolerance here?

Not especially.  The things that remember things set multiple things, but when we get it out again, there are two separate ways to access the data.  This uses the version information only; the "why" isn't needed right here.
Rebased, carrying r=keeler
Attachment #8498481 - Attachment is obsolete: true
Attachment #8499238 - Flags: review+
The line just fit :)  r=keeler
Attachment #8498482 - Attachment is obsolete: true
Attachment #8499239 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b1a9b31ab26e
https://hg.mozilla.org/mozilla-central/rev/59ef591ec318
Assignee: nobody → martin.thomson
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.