1940804 - (CVE-2025-1014) Using u16::max instead of u16::MAX potentially allows very long certificates

Status: RESOLVED FIXED

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

Description

[Theemathas]

Steps to reproduce:

Sorry if this is the incorrect place to file this report. I found this code which is definitely incorrect: https://github.com/mozilla/gecko-dev/blob/48caa4719ed2c75a2937bcfea9e7326e87846984/security/manager/ssl/cert_storage/src/lib.rs#L1037

In rust, u16::max is a function (it takes two numbers and returns the larger one). On the other hand, u16::MAX is a value equal to 2^16-1. The code in the link is u16::max when the intention was u16::MAX. The code then converts the function pointer into an integer, which results in a large integer. Which means that this validation logic could potentially accept very long certificates as valid.

Actual results:

N/A

Expected results:

N/A

Comment 1

[Benjamin Beurdouche [:beurdouche]]

Setting security flag, just in case, we can always declassify.

Comment 2

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

Good catch! In practice, I don't think this is an issue, because all of these certificates are coming from Mozilla.

Comment 3

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

Attachment: Bug 1940804 - avoid some "as" casts in cert_storage r?jschanck

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc3462ef5136
avoid some "as" casts in cert_storage r=jschanck

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/fc3462ef5136

Comment 6

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

Attachment: Bug 1940804 - avoid some "as" casts in cert_storage r?jschanck

Original Revision: https://phabricator.services.mozilla.com/D233775

Comment 7

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: this probably won't affect users, because the data comes from a trustworthy source, but still, it's definitely wrong
• Code covered by automated testing: yes
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: low
• Explanation of risk level: this patch changes some casts into statements that can be typechecked by the compiler, so it's definitely safer
• String changes made/needed: none
• Is Android affected?: yes

Comment 8

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

Attachment: Bug 1940804 - avoid some "as" casts in cert_storage a=RyanVM

Original Revision: https://phabricator.services.mozilla.com/D233775

Comment 9

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: this shouldn't affect users, since the data comes from a trusted source, but it's still wrong
• Code covered by automated testing: yes
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: low
• Explanation of risk level: this patch changes some casts into statements that can be typechecked by the compiler, so it's definitely safer
• String changes made/needed: none
• Is Android affected?: yes

Comment 10

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/6ecc1a33e29a

Comment 11

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/94b283c7793f

Comment 12

[amarc]

Backed out changeset 94b283c7793f for causing build bustages: https://hg.mozilla.org/releases/mozilla-esr128/rev/9d507a026111de465ea2198b61e8e610cd64a6ae
Failure log

Comment 13

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

I updated the esr128 patch - should be good to go now.

Comment 14

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/ce91ea8a663d

Comment 15

[Tyson Smith [:tsmith]]

Attachment: advisory.txt

Comment 16

[Frederik Braun [:freddy]]

I was asked to make this public and this was fixed more than 3 months ago. There is also little new information from what is already public in https://github.com/rust-lang/rust-clippy/issues/13973#issuecomment-2833386390.