1698503 - (CVE-2021-29946) Stop Alt-Svc connections to go to blocked ports, when they are written and parsed as exceeding 16 bit

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Frederik Braun [:freddy]]

When seeing an Alt-Svc header in nsHttpChannel::ProcessAltService(), we first copy it into a string and pass it into AltSvcMapping::ProcessHeader, which then parses the a substring after the colon character as a port into an int32_t.

Unfortunately and unlike other parts of our code (I haven't checked other uses of NS_CheckPortSafety exhaustively), this is never being range-checked to be within the valid port ranges of int16_t.

So, despite adding a test for the AltSvc case, I have taken the liberty to modify the AllowPort function. But there are multiple ways in which we could do that..

Alternatives:

• disallow values with bits beyond the typical size right in the utility function and return false (e.g., disallowed)
• unset the high bits (logical-and with 0xFFFF) and let it through according to the block list
• change the function signature to expect a uint16_t

@dragana: Which of you would you prefer here? I'll attach a patch that goes with option 1, but happy to adjust.

Anecdotally, we used to have a 16-bit type with bug 95488, but a global rewrite from NSPR numeric types to stdint in bug 579517 incorrectly removed that. lolsob

I'll rate it as sec-low, might be sec-moderate under certain circumstances.

Comment 1

[Frederik Braun [:freddy]]

Attachment: Bug 1698503 - robuster port blocking r=dragana

Comment 2

[Frederik Braun [:freddy]]

try https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f9c1a5500688e79889cb8f500ae04e134787883

Comment 3

[Frederik Braun [:freddy]]

Anecdotally, we used to have a 16-bit type with bug 95488, but a global rewrite from NSPR numeric types to stdint in bug 579517 incorrectly removed that. lolsob

I was reading that incorrectly. We regressed with bug 1556019.

Comment 4

[Anne (:annevk)]

That bug only changes a single variable, the signature above already has uint32...

Comment 5

[Frederik Braun [:freddy]]

@annevk: Always had.

But we only started calling the NS_CheckPortSafety function after bug 1552993, but that parsed it as 32-bit int, so that always allowed the (badPort+0xFFFF) loophole..

Comment 6

[Dragana Damjanovic [:dragana]]

(In reply to Frederik Braun [:freddy] from comment #0)

When seeing an Alt-Svc header in nsHttpChannel::ProcessAltService(), we first copy it into a string and pass it into AltSvcMapping::ProcessHeader, which then parses the a substring after the colon character as a port into an int32_t.

Unfortunately and unlike other parts of our code (I haven't checked other uses of NS_CheckPortSafety exhaustively), this is never being range-checked to be within the valid port ranges of int16_t.

So, despite adding a test for the AltSvc case, I have taken the liberty to modify the AllowPort function. But there are multiple ways in which we could do that..

Alternatives:

• disallow values with bits beyond the typical size right in the utility function and return false (e.g., disallowed)
• unset the high bits (logical-and with 0xFFFF) and let it through according to the block list
• change the function signature to expect a uint16_t

@dragana: Which of you would you prefer here? I'll attach a patch that goes with option 1, but happy to adjust.

Anecdotally, we used to have a 16-bit type with bug 95488, but a global rewrite from NSPR numeric types to stdint in bug 579517 incorrectly removed that. lolsob

int32_t is use on purpose. Value -1 means "default port for a scheme" (80/443).
unsetting the higher bits will only lead to confusion.
The approach you are taking is the only option.

Comment 7

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

robuster port blocking r=dragana,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/774f4e003006bd1f211b1848cccbb00d89fe64d1
https://hg.mozilla.org/mozilla-central/rev/774f4e003006

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Probably not terribly important to backport since it's only sec-low, but it does graft cleanly to ESR78. Do we want to consider backporting there?

Comment 9

[Frederik Braun [:freddy]]

Yeah, let's backport if it applies cleanly. The secondary effects of exploiting this bug can be worse in a busier network than in a typical end user scenario.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR78 approval in that case :)

Comment 11

[Frederik Braun [:freddy]]

Comment on attachment 9209142 [details]
Bug 1698503 - robuster port blocking r=dragana

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: The impact of exploiting this is likely higher for busier networks (e.g., enterprise secenarios)
• User impact if declined: Missing security fix
• Fix Landed on Version:
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): The fix does not break existing workflows. Ports shouldn't exceed 16 bits.
• String or UUID changes made by this patch: None

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9209142 [details]
Bug 1698503 - robuster port blocking r=dragana

Approved for 78.10esr.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/104461bd6bcfa793dd7d93c989cfb518586c5741

Comment 14

[Tom Ritter [:tjr]]

Attachment: advisory.txt