1088950 - Fix inappropriate_fallback alert handling

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Martin Thomson [:mt:]]

Attachment: 0001-Fix-handling-of-inappropriate_fallback-alert.patch

As it happens, discussion on the downgrade SCSV clarified the handling of the alert.  The fix for bug 1072382 causes the alert to mark the next highest version as good.  That's not quite right, since the alert could indicate a downgrade of more than one version.  If that happens, then the downgrade causes not just one hard failure, but multiple.

We should just reset what we know about the origin with respect to fallback and leave it at that.  This seems rare, but the fix is trivial, so here we are.

Comment 1

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8511333 [details] [diff] [review]
0001-Fix-handling-of-inappropriate_fallback-alert.patch

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

LGTM with comment addressed.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +874,5 @@
> +
> +  MutexAutoLock lock(mutex);
> +
> +  IntoleranceEntry entry;
> +  if (mTLSIntoleranceInfo.Get(key, &entry)) {

add a 'entry.AssertInvariant();' just after this

Comment 2

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-1088950-Fix-handling-of-inappropriate_fallback-a.patch

I noticed a fairly straightforward refactor; might be worth having a quick look.

Comment 3

[Martin Thomson [:mt:]]

Attachment: 0002-Bug-1088950-Adding-some-testing.patch

I figure that I shouldn't be so sloppy when it comes to testing either.

Comment 4

[Martin Thomson [:mt:]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6050bfe38e90

Comment 5

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8512345 [details] [diff] [review]
0001-Bug-1088950-Fix-handling-of-inappropriate_fallback-a.patch

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

LGTM.

Comment 6

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8512346 [details] [diff] [review]
0002-Bug-1088950-Adding-some-testing.patch

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

Looks good.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e41f03a6b325
https://hg.mozilla.org/integration/mozilla-inbound/rev/484e85ba2134

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e41f03a6b325
https://hg.mozilla.org/mozilla-central/rev/484e85ba2134