Fix inappropriate_fallback alert handling

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mt, Assigned: mt)

Tracking

Trunk
mozilla36
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
Attachment #8511333 - Flags: review?(dkeeler)
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
Attachment #8511333 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 2

5 years ago
I noticed a fairly straightforward refactor; might be worth having a quick look.
Attachment #8511333 - Attachment is obsolete: true
Attachment #8512345 - Flags: review?(dkeeler)
(Assignee)

Comment 3

5 years ago
I figure that I shouldn't be so sloppy when it comes to testing either.
Attachment #8512346 - Flags: review?(dkeeler)
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.
Attachment #8512345 - Flags: review?(dkeeler) → review+
Comment on attachment 8512346 [details] [diff] [review]
0002-Bug-1088950-Adding-some-testing.patch

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

Looks good.
Attachment #8512346 - Flags: review?(dkeeler) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e41f03a6b325
https://hg.mozilla.org/mozilla-central/rev/484e85ba2134
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.