Closed Bug 1301547 Opened 3 years ago Closed Last year

remove use of DER_Lengths from PSM

Categories

(Core :: Security: PSM, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox51 --- wontfix
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

(Blocks 2 open bugs)

Details

(Keywords: sec-low, sec-want, Whiteboard: [psm-assigned][post-critsmash-triage][adv-main63+])

Attachments

(1 file, 1 obsolete file)

See also bug 1298798. DER_Lengths has a number of issues including reading out-of-bounds memory (so certainly vulnerable to DOS). As far as I can tell, the only reason PSM calls DER_Lengths is to implement an ancient (circa 2000) workaround for "2.0 enterprise server" (see https://hg.mozilla.org/projects/nss/annotate/4ca6e9545364/security/nss/lib/ssl/cmpcert.c#l74 ). We can probably remove this outright, but to be safe it might be best to fix this in multiple phases:

1. Reimplement the workaround without DER_Lengths
2. Add telemetry to see if this code is ever actually run
3. Remove it when the telemetry tells us this is never actually run
So far telemetry indicates this is incredibly rare: https://mzl.la/2Lbi1Lz (rare enough to be 0/~200k events with filtering - without filtering, there are a total of 18 sessions where this was apparently encountered).
Assignee: nobody → dkeeler
Priority: P2 → P1
Whiteboard: [psm-backlog] → [psm-assigned]
Attached patch patchSplinter Review
Attachment #8992489 - Flags: review?(franziskuskiefer)
Comment on attachment 8992489 [details] [diff] [review]
patch

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

yay. Thanks.
I filed bug 1476200 to clean this up in NSS as well.

FYI you can use phabricator for security bugs as well (visibility is according to bugzilla CC and security groups).
Attachment #8992489 - Flags: review?(franziskuskiefer) → review+
Telemetry showed that DER_Lengths is hardly used and keeler removed it from PSM.
We can remove it from NSS as well.
Deprecating util functions is a little tricky so I didn't do that yet (they're always used in the wrapper, which breaks the build).
Attachment #8992562 - Attachment is obsolete: true
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #3)

Thanks!

> Comment on attachment 8992489 [details] [diff] [review]
> patch
> 
> Review of attachment 8992489 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> yay. Thanks.
> I filed bug 1476200 to clean this up in NSS as well.
> 
> FYI you can use phabricator for security bugs as well (visibility is
> according to bugzilla CC and security groups).

Oh, right - I forgot that.
From the discussion in the original bug (bug 1298798), this seems like a denial-of-service issue (an attacker can probably read out-of-bounds and make Firefox crash, but it doesn't seem like much more beyond that).
Keywords: sec-low
https://hg.mozilla.org/mozilla-central/rev/bbe392227b7d
Group: crypto-core-security → core-security-release
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Seems like this can ride the trains, but feel free to nominate if you feel strongly otherwise.
See Also: → 1298798
Flags: qe-verify-
Whiteboard: [psm-assigned] → [psm-assigned][post-critsmash-triage]
Whiteboard: [psm-assigned][post-critsmash-triage] → [psm-assigned][post-critsmash-triage][adv-main63+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.