Closed Bug 1887898 (CVE-2024-12224) Opened 1 year ago Closed 10 months ago

Improper URL parsing in servo/rust-url

Categories

(Core :: Networking, defect, P2)

Product:
Core
Component:
Networking
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
130 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- wontfix
firefox-esr128 --- wontfix
firefox130 --- fixed

People

(Reporter: kageshiron, Assigned: hsivonen)

References

Details

(Keywords: reporter-external, sec-moderate, Whiteboard: [necko-triaged] [necko-monitor])

Attachments

(1 file)

Deno sample
59.80 KB, image/png
Details
Attached image Deno sample —

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:124.0) Gecko/20100101 Firefox/124.0

Steps to reproduce:

Version 2.5.0 https://github.com/servo/rust-url
(This issue does not occur in Firefox, regardless of the setting of network.url.useDefaultURI. I have contacted info@servo.org and have been informed that I may post it here.)

servo/rust-url does not properly parse some URLs (hostnames).

sample 1

The URL https://xn--test-.example.com is a URL that can be used by Google Chrome, for example. However, rust-url parses this as https://test.example.com.

(code 1)

use url::Url;
fn main() {
    let url = Url::parse("https://xn--example-wz6d.com");
    match url {
        Ok(v) => println!("ok {}", v),
        Err(e) => println!("err {}", e),
    };
    let url = Url::parse("https://xn--example-.com");
    match url {
        Ok(v) => println!("ok {}", v),
        Err(e) => println!("err {}", e),
    };
}

sample 2 (deno)

deno (https://deno.com/ ) and WinterJS (https://github.com/wasmerio/winterjs ) are well-known JavaScript runtimes. It internally parses URLs using rust-url and returns improper parsed results.

(code 2)

console.log(new URL("http://xn--test-.example.com").origin);

sample 3 other software and services

HTTP Request Library
https://crates.io/crates/reqwest
HTTP Request Tool
https://github.com/ducaale/xh
DNS Tools
https://github.com/hickory-dns/hickory-dns

If you are a Discord ( https://discord.com/ ) user, try sending the following URL in a chat
https://xn--developer-.mozilla.org/
You will see the OGP for https://developer.mopzilla.org.

https://crates.io/crates/reqwesthttps://github.com/hickory-dns/hickory-dns

As an actual vulnerability, I have confirmed that it is possible to bypass CSRF Protection in Web Framework Hono.
Hono performs URL parsing when comparing URL and Origin headers.

origin === new URL(c.req.url).origin

https://github.com/honojs/hono/blob/d3403942c12c16847b0808cee1dcb0a2d6c81d1b/src/middleware/csrf/index.ts#L17

Thus, the following request bypasses the CSRF filter when running on deno.
test.example.com --> xn--test-.example.com

There may be other libraries or sites with similar or more serious problems.

This URL is not appropriate for the specification, but clients such as Chrome allow this domain name, and if more products use rust or deno, the problem may become apparent.

Possible examples: (not a real problem at this time)

  • Domain name dependent validation such as ACME challenges may be bypassed and inappropriate certificates may be issued
  • DKIM, CSRF protection, etc. may be bypassed

Actual results:

(code 1 actual)
ok https://test.example.com/
ok https://test.example.com/

(code 2 actual)
https://test.example.com

Expected results:

(code 1 expected)
ok https://test.example.com/
ok https://xn--test-.example.com/
( or "err invalid international domain name")

(code 2 expected)
https://xn--test-.example.com (or parse error)

Group: core-security → network-core-security

We use rust-url in a bunch of places: it's not great if it gets a different answer than nsStandardURL and could lead to security issues.

Flags: needinfo?(kershaw)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Severity: -- → S3
Flags: needinfo?(kershaw)
Priority: -- → P2
Whiteboard: [necko-triaged] [necko-priority-queue]

Calling this "sec-moderate" for now, but depending on what this is used for it's not too hard to hypothesize a "sec-high" impact if a feature uses it in just the wrong way.

Keywords: sec-moderate

We only use rust-url to parse non-special schemes (and that's currently preffed off) - so http/https/ws/wss are not affected.

I think this is a case the URL standard doesn't specify.
See the reference URL parser

Firefox and Safari throw an exception, Chrome parses it as https://xn--test-.example.com, while the reference parser does the same as rust-url.

Flags: needinfo?(annevk)

When I look at https://www.rfc-editor.org/rfc/rfc3492.txt section 6.2 it has this step:

{if n is a basic code point then fail}

It appears this is not implemented in the reference URL parser?

Flags: needinfo?(annevk)

I have already fixed this on my branch at https://github.com/hsivonen/rust-url/tree/icu4x as a side effect of reimplementing the internals of the idna crate and noticing that this (literally per spec, AFAICT!) behavior made no sense and didn't match Firefox and Safari, and I have already reported this to the Unicode Consortium as a spec bug.

Pulling the code into Gecko is bug 1889536. Prerequisites include reviewing and landing the above-mentioned branch, and also getting https://github.com/unicode-org/icu4x/pull/4712 and https://github.com/unicode-org/icu/pull/2945 reviewed and landed (unless we want to take a lot of code into Gecko that's not upstreamed, yet).

(In reply to Anne (:annevk) from comment #4)

When I look at https://www.rfc-editor.org/rfc/rfc3492.txt section 6.2 it has this step:

{if n is a basic code point then fail}

How is this step relevant to the problem reported here? AFAICT, the problem here is that the loop condition "while the input is not exhausted do begin" simply ends up not looping at all and never looping not being an error.

I think the best way to fix this is to check for the label ending with a hyphen when it has been established that the label starts with xn--. If the label is just "xn--", it ends with a hyphen and should be treated as an error. Likewise, "xn--test-" ends with a hyphen: that is, if there are no Punycode digits, the label ends with a hyphen, which needs to be treated as an error. (I asked for an ends-with-hyphen check to be added to UTS 46.)

Yeah, I missed some steps. It seems it assumes this scenario cannot occur because you are not allowed to encode, so this broke when we had to allow hyphens in arbitrary places, but didn't do that carefully enough:

Using hyphen-minus as the delimiter implies that the encoded string can end with a hyphen-minus only if the Unicode string consists entirely of basic code points, but IDNA forbids such strings from being encoded.

Whiteboard: [necko-triaged] [necko-priority-queue] → [necko-triaged] [necko-monitor]

Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons

Keywords: reporter-external

The current status is:

I think I should make the Google folks in that discussion, Manish and Shane, aware of this issue as context why it would be appropriate for the url 2.x stream to get the idna 1.0 fixes. (Fixing this in the idna 0.5 version stream does not seem like a particularly good use of time.)

Thanks!
I also think that RFC952 does not allow hyphenated hostnames and should be a parse error precisely because it does not allow hyphenated hostnames. However, many real-world implementations allow this.

As a result of my research on URLs, I would like to offer a suggestion that would sort out the URL specification. However, there are multiple URL specifications and do you know where the most appropriate suggestion would be.

(In reply to kageshiron from comment #9)

I also think that RFC952 does not allow hyphenated hostnames and should be a parse error precisely because it does not allow hyphenated hostnames. However, many real-world implementations allow this.

What do you mean by hyphenated hostnames?

Browsers implement the WHATWG URL Standard, which deliberately sets the flags that UTS 46 offers the way it does. This includes neither enforcing the STD3 prohibition on label-initial or label-trailing hyphens nor the later reservation of two hyphens in the third or fourth scalar value positions in a label.

I don't expect us to treat the hyphen treatment of the WHATWG URL Standard as a security bug.

As for progress on making the url crate depend on idna crate version 1.0.x, which fixes the original bug reported here, I opened https://github.com/servo/rust-url/pull/965 , which is designed remove the objections that resulted in url reverting back to depending on idna 0.5.

With the publication of version 2.5.3 of the url crate, this is now finally (re-)fixed in the scope of the url crate by making url depend on idna 1.x instead of 0.5 again.

Meanwhile, the spec fix has become official: Step 3 under https://www.unicode.org/reports/tr46/#ProcessingStepPunycode .

dveditz, how should we go about disclosing this? The root cause for why idna 0.5 had this security issue is that the spec had this security issue, so it's plausible that there are other implementations that also followed the spec to the letter and ended up with the same problem. Also, comment 0 says the problem can be demoed end-to-end with Hono. Though this issue should probably get a RustSec entry about idna 0.5 eventually, what's the appropriate level of coordination with other projects prior to issuing a RustSec entry?

Status: NEW → RESOLVED
Closed: 10 months ago
Flags: needinfo?(dveditz)
Resolution: --- → FIXED

FWIW, in addition to https://xn--example-.com/ parsing as https://example.com/, there's also https://example.com.xn--/ parsing as https://example.com./ , so also the latter needs checking if checking what else is vulnerable.

Assignee: nobody → hsivonen
Group: network-core-security → core-security-release
status-firefox130: --- → fixed
status-firefox-esr115: --- → wontfix
status-firefox-esr128: --- → wontfix
Target Milestone: --- → 130 Branch

I'm trying to figure out what a RustSec entry should say, and my current draft is this:

[advisory]
id = "RUSTSEC-0000-0000"

package = "idna"

# Disclosure date of the advisory as an RFC 3339 date (mandatory)
date = "yyyy-MM-dd"

categories = ["privilege-escalation"]

keywords = ["idna", "punycode", "same-origin", "domain-name"]

# Is there going to be a CVE id from the Mozilla pool?
aliases = ["CVE-2024-XXXX"]

[versions]
patched = [">= 1.0.0"]

idna accepts Punycode labels that do not produce any non-ASCII when decoded

idna 0.5.0 and earlier accepts Punycode labels that do not produce any non-ASCII output, which means that either ASCII labels or the empty root label can be masked such that they appear unequal without IDNA processing or when processed with a different implementation and equal when processed with idna 0.5.0 or earlier.

Concretely, example.org and xn--example-.org become equal after processing by idna 0.5.0 or earlier. Also, example.org.xn-- and example.org. become equal after processing by idna 0.5.0 or earlier.

This can lead to privilege escalation when combined with a client that resolves domains with such labels instead of treating them as errors that preclude DNS resolution / URL fetching and with the attacker managing to introduce a DNS entry (and TLS certificate) for an xn---masked name that turns into the name of the target when processed by idna 0.5.0 or earlier.

Additional information

This issue resulted from idna 0.5.0 and earlier implementing the UTS 46 specification literally on this point and the specification having this bug. The specification bug has been fixed in revision 33 of UTS 46.

Reporter, regarding the paragraph starting with "This can lead to privilege escalation", is it correct that to exploit Hono running on Deno with idna 0.5.0 or earlier, the following also need to hold:

  1. The user needs to be using a Chromium-based browser (not Gecko or WebKit-based browser)
  2. The attacker needs to be able to introduce a DNS entry with the xn---masked name, i.e. the attack requires the TLD operator of the target not to reject registrations that have the Punycode prefix but produce no non-ASCII when decoded.
  3. The attacker needs to be able to obtain a TLS certificate for the attack site, which requires a CA that does not reject certificate requests for names that have the Punycode prefix but produce no non-ASCII when decoded.

Am I missing something? (I can imagine that there might be other name comparison situations that could lead to privilege escalation, but the Hono on Deno case seems like the only concrete one described so far, so I think it would be appropriate to mention the mitigating factors so as to not exaggerate the impact.)

Flags: needinfo?(kageshiron)

(I have verified that the issue was present already in the first non-placeholder version of idna. Also, the RustSec entry should probably have a "Remedy" section remarking that even though this issue was fixed in 1.0.0, due to another bug, using a version older than 1.0.2 is not advised, and for the cases that depend on idna through url, url should be updated to 2.5.3 or later.)

Aside: It seems to me that it's a bad idea for Hono to parse the value of the Origin header as URL instead of doing a direct string comparison with the header value, since it adds exposure to a bug like this without, AFAICT, any benefit, so at some point, reporting that to Hono would seem appropriate.

dveditz, how should we go about disclosing this?

This is kind of a mess, isn't it?

It doesn't look like the Unicode folks considered it a "security issue", they just updated the spec as a bugfix. In the abstract, parsing a URL and getting "A" instead of "B" or an error is just a difference; it's only a security bug in applications that make security decisions based on URLs. But that's a lot of apps! Lots of the other historical changes to UTS#46 over the years resulted in different output. You could probably find specific edge cases where those led to security vulnerabilities in some product or other, but there haven't been security advisories for UTS#46 before.

So you've fixed idna, and that impacts rust-url and who knows how many other crates built on them. It does seem important to get the word out. I'm not sure it's a "vulnerability" for idna because if an app always processes URLs—even ASCII-only ones—they will always come out the same way. The problem is if an application assumes an ASCII-only domain (such as an Origin: header) doesn't need to be run through the process. There's no guarantee that any such domain received from outside the application was processed using the same version of UTS#46 using the same flags. A strong PRO for giving this a CVE is that it will feed into dependency-checking security tools and make many more people aware of the issue than just issuing an advisory/warning without a CVE.

And then we see the same problem in the "Live URL Viewer" built on a completely different library. That uses the "tr46" package written by the jsdom folks themselves. That package is also used by 20 million on GitHub and has 31.6 million weekly downloads on npmjs.com. How many potentially vulnerable apps might be in that massive pile?

The tr46 package seems to align with UTS#46 version 15.1.0 because it supports the IgnoreInvalidPunycode flag that was introduced in that version.

Added a flag IgnoreInvalidPunycode, which makes labels such as xn--a.com valid. This allows for an all-ASCII fast-path following common industry practice.

The tr46 package library defaults that flag to false (behaves the same as pre-15.1.0) and that's what the Live URL Viewer uses. The WHATWG URL spec specifically references version 15.1.0 of UTS#46, but the processing steps never mention IgnoreInvalidPunycode or what value to use. Without an agreed spec change to use new and incompatible processing (formerly invalid URLs become valid), the Live URL Viewer is correct to use a flag value of false. Chromium, on the other hand, seems to set IgnoreInvalidPunycode set to true (based on behavior, haven't checked their code). That is, chromium leaves xn--a.com and xn--test-.com untouched rather than an invalid url and "test.com" respectively as you'd expect from UTS#46 processing prior to the change Henri just got added in version 16.0

Whatever is done for idna should be done for tr46. If they are considered vulnerabilities and given a CVE they should have different CVEs because they are different code fixes, even though both stem from the same spec problem. Downstream packages that are fixed merely by upgrading their idna or tr46 libraries should reference the library CVE if they need to issue an advisory rather than get a new one. They would only get a CVE of their own if they required additional fixes on top of the library upgrade to fix their vulnerability (and they should still include a reference to the library CVE as well).

This is the kind of thing that might be best coordinated by CERT, if we think the consequences of this bug are severe enough to warrant that. Are they? CERT coordination takes FOREVER, though; I'm not sure it's worth it for this case. Registries shouldn't be creating domain records that are malformed like this, but of course people could misconfigure their own domain's nameserver to support malformed subdomains. Browsers following the URL spec won't be loading any sites with malformed IDN names so that will protect users. Except for chromium browsers that support "IgnoreInvalidPunycode == true". That change means chromium users can now load lots of potential domain names that other browsers would consider an invalid URL and not load, and that because of this spec issue some servers will handle incorrectly.

Flags: needinfo?(dveditz)

(In reply to kageshiron from comment #9)

I also think that RFC952 does not allow hyphenated hostnames and should be a parse error precisely because it does not allow hyphenated hostnames. However, many real-world implementations allow this.

RFC 952 explicitly allows the "minus sign"; names can be "hyphenated", but they can't start or end with one. That was in 1985. In the explosive venture-capital fueled growth of the World Wide Web a decade later a lot of people new to the field built things that worked for them without a whole lot of knowledge of or regard for standards. That resulted in a huge compatibility mess between what standards said, what sites actually did, and how browsers interpreted the sites. That eventually led Mozilla, Apple, and Opera to create WHATWG to try to make sense of the mess and to create a path forward that recognized the reality of the Web as it existed.

As a result of my research on URLs, I would like to offer a suggestion that would sort out the URL specification. However, there are multiple URL specifications and do you know where the most appropriate suggestion would be.

All the major browser vendors participate in WHATWG and aim to follow the URL spec. That spec defers to the Unicode specs for certain aspects of processing, which in turn references various RFCs.

If you want to influence browsers you should start with the WHATWG spec. If a particular browser isn't following those specs quite right then report that to the specific vendor. Adding Web Platform Tests (https://wpt.fyi) for edge cases will help make sure people don't miss that point in the future. In this case the biggest issue appeared to be in the Unicode UTS#46 spec.

Henri already filed an issue on the URL spec about setting a value for IgnoreInvalidPunycode: https://github.com/whatwg/url/issues/821

See Also: → https://github.com/whatwg/url/issues/821

(In reply to Daniel Veditz [:dveditz] from comment #16)

It doesn't look like the Unicode folks considered it a "security issue"

To be fair, the reports to Unicode (including the one I filed before seeing this Bugzilla entry) weren't framed as security reports.

Lots of the other historical changes to UTS#46 over the years resulted in different output.

I believe this case is unusual in its direction compared to previous changes (that either have caused previously intentionally matching output to intentionally become distinct or that have turned previously erroneous input into non-erroneous input).

The problem is if an application assumes an ASCII-only domain (such as an Origin: header) doesn't need to be run through the process.

On the contrary, Hono has this issue, because it runs the Origin header through UTS 46 processing.

A strong PRO for giving this a CVE is that it will feed into dependency-checking security tools and make many more people aware of the issue than just issuing an advisory/warning without a CVE.

Yeah. I think this should have a RustSec entry, since those feed into tooling.

And then we see the same problem in the "Live URL Viewer" built on a completely different library.

Is it OK to add Domenic Denicola, who has done the most recent work on the tr46 npm module, to the CC list of this bug?

Also, would it be OK for me to communicate to Deno developers that a RustSec entry about idna can be expected, and the most convincing avenue to exploit so far depends on Deno currently pinning an out-of-date version of url, so it would be good if they'd update the deps before a RustSec entry shows up?

Chromium, on the other hand, seems to set IgnoreInvalidPunycode set to true (based on behavior, haven't checked their code).

AFAICT, Chromium's behavior isn't explainable by the flags. Instead, Chromium has a preprocessing layer before passing input to the ICU4C UTS 46 implementation. I believe that Safari and Firefox are evidence that such a layer isn't need for Web compat. The layer in Chromium has the effect of adding non-conformance, and I would prefer Chromium to get rid of that layer in order to achieve interop and spec compliance.

This is the kind of thing that might be best coordinated by CERT, if we think the consequences of this bug are severe enough to warrant that. Are they?

I think from what has been described so far it depends on whether TLDs and CAs have checks against names that do not conform with STD3 when viewed on the pre-IDNA ASCII level. (The names involved here would fail pre-IDNA STD3 validation due to the trailing hyphen.)

I don't have the time to find out if there exist TLDs that would allow registering xn--example- or CAs that would issue a certificate to such a name.

I don't want to exaggerate this issue without end-to-end proof of seriousness, but it does not seem particularly improbable that there's exist a TLD that didn't validate against a trailing hyphen and a CA that issued for names that exist in DNS. (We've previously seen GitHub, Glitch, and YouTube mint subdomain DNS entries that are prohibited by the IETF's hyphen rules but that are covered by wildcard certificates.)

If a particular browser isn't following those specs quite right then report that to the specific vendor. Adding Web Platform Tests (https://wpt.fyi) for edge cases will help make sure people don't miss that point in the future. In this case the biggest issue appeared to be in the Unicode UTS#46 spec.

WPT imports the UTS 46 test suite from the Unicode Consortium, so in this case WPT hasn't been effective at getting the Chromium devs to remove the code that adds non-conforming behaviors on top of ICU4C: https://wpt.fyi/results/url/IdnaTestV2.window.html?label=master&label=experimental&aligned&q=%2Furl%2FIdnaTestV2.window.html

Flags: needinfo?(dveditz)

It appears that libidn2 does not have this problem even though its data is at Unicode 15.0.0.

(In reply to Henri Sivonen (:hsivonen) from comment #19)

And then we see the same problem in the "Live URL Viewer" built on a completely different library.

Is it OK to add Domenic Denicola, who has done the most recent work on the tr46 npm module, to the CC list of this bug?

I was going to suggest that. Added.

Also, would it be OK for me to communicate to Deno developers that a RustSec entry about idna can be expected,

Yes, not a problem.

I don't have the time to find out if there exist TLDs that would allow registering xn--example- or CAs that would issue a certificate to such a name.

I don't want to exaggerate this issue without end-to-end proof of seriousness, but it does not seem particularly improbable that there's exist a TLD that didn't validate against a trailing hyphen and a CA that issued for names that exist in DNS. (We've previously seen GitHub, Glitch, and YouTube mint subdomain DNS entries that are prohibited by the IETF's hyphen rules but that are covered by wildcard certificates.)

I'm reasonably confident that registries would not allow domains like that to be registered on TLDs, or at least mainline TLDs (it's possible some goldrush clownshoes organizations running some of the vanity gTLDs don't know what they are doing). I'm equally confident that there ARE such labels as subdomains, whether added as an experiment, out of cluelessness, or maliciously. In fact, you mentioned above that you encountered some on well-known sites.

WPT imports the UTS 46 test suite from the Unicode Consortium, so in this case WPT hasn't been effective at getting the Chromium devs to remove the code that adds non-conforming behaviors on top of ICU4C: https://wpt.fyi/results/url/IdnaTestV2.window.html?label=master&label=experimental&aligned&q=%2Furl%2FIdnaTestV2.window.html

I did say "help" 🤷🏽‍♀️ ? There are thousands of WPT failures and teams have different prioties. WPTs make sure they are at least aware of the issue.

Flags: needinfo?(dveditz)

Domenic, please see comment 16 and onwards. What do you think about the tr46 npm package shipping step 3. under https://www.unicode.org/reports/tr46/#ProcessingStepPunycode before issuing a RustSec advisory about the idna crate (ahead of upgrading to Unicode 16.0 data perhaps)?

Flags: needinfo?(d)

I'm not too concerned about the security of jsdom/whatwg-url or live URL viewer, as I don't believe they're used in issuing SSL certificates. So my default stance would be to wait until we have a fully-updated URL Standard, and WPT suite, for Unicode 16.0.0, and then upgrade tr46 + whatwg-url for the Unicode 16.0 changes to UTS 46. This is what we have done for previous changes that have impacted URL parsing. I have no concerns about issuing advisories for idna head of any such changes.

From what I understand there's some unwillingness from WebKit (and maybe others?) to update the WPTs before February, per https://github.com/web-platform-tests/wpt/pull/48301#issuecomment-2505755969 . So that would probably be the earliest I get around to revising tr46.

Contrary to comment #19, I don't think this is the first change of this type to URL parsing, where multiple inputs that used to be the same became distinct? Looking through the last few years, there have been some changes to what characters are unescaped, or how file: URLs are normalized, which seem like they'd belong in the same category.

Flags: needinfo?(d)

(In reply to Domenic Denicola from comment #23)

I'm not too concerned about the security of jsdom/whatwg-url or live URL viewer, as I don't believe they're used in issuing SSL certificates.

I think the concern is about potential other uses (directly or transitively) of the the tr46 npm package.

So my default stance would be to wait until we have a fully-updated URL Standard, and WPT suite, for Unicode 16.0.0, and then upgrade tr46 + whatwg-url for the Unicode 16.0 changes to UTS 46. This is what we have done for previous changes that have impacted URL parsing.

I believe between your comment and now, the URL Standard got changes (mainly explicitly setting IgnoreInvalidPunycode to false) to make it up-to-date given UTS 46 changes. That leaves the WPT part.

I have no concerns about issuing advisories for idna head of any such changes.

Thanks.

From what I understand there's some unwillingness from WebKit (and maybe others?) to update the WPTs before February, per https://github.com/web-platform-tests/wpt/pull/48301#issuecomment-2505755969 . So that would probably be the earliest I get around to revising tr46.

dveditz, given the above, what's your opinion on appropriate RustSec advisory timing?

Contrary to comment #19, I don't think this is the first change of this type to URL parsing, where multiple inputs that used to be the same became distinct? Looking through the last few years, there have been some changes to what characters are unescaped, or how file: URLs are normalized, which seem like they'd belong in the same category.

I meant the UTS 46 processing for the domain only, not the URL parsing around it.

Flags: needinfo?(dveditz)

dveditz, given the above, what's your opinion on appropriate RustSec advisory timing?

It doesn't have any bearing. They don't want to change the WPT yet because browsers agreed to Interop2024 goals based on the existing tests; moving the goalposts wouldn't be fair. It's not based on security reasons.

Flags: needinfo?(dveditz)

(In reply to Daniel Veditz [:dveditz] from comment #25)

dveditz, given the above, what's your opinion on appropriate RustSec advisory timing?

It doesn't have any bearing. They don't want to change the WPT yet because browsers agreed to Interop2024 goals based on the existing tests; moving the goalposts wouldn't be fair. It's not based on security reasons.

I meant the part of the tr46 npm module probably staying as-is all the way to February and comment 16 saying "Whatever is done for idna should be done for tr46. If they are considered vulnerabilities and given a CVE they should have different CVEs because they are different code fixes, even though both stem from the same spec problem.".

dveditz, Do we assign a CVE number to this? Shall I push out a RustSec advisory ahead of a tr46 change? Does the draft in comment 13 look OK?

lucacasonato, I see that the url dependency of Deno has been updated. Is there a Deno need to delay a RustSec advisory still?

Reporter, could you, please, confirm the mitigating factors from comment 13? Do you wish to be acknowledged as kageshiron in the RustSec advisory?

Flags: needinfo?(hello)
Flags: needinfo?(dveditz)

dveditz, Do we assign a CVE number to this? Shall I push out a RustSec advisory ahead of a tr46 change? Does the draft in comment 13 look OK?

Domenic was not worried about this advisory coming out ahead of the tr46 one.

The draft in comment 13 looks fine. The category bothers me, but it's the closest fit from the choices given at https://rustsec.org/categories/.

The reason the category bothers me is the same reason I have trouble answering your question about assigning a CVE number. This is not in any way a "privilege escalation" for idna itself, or really a proper vulnerability. It was just an incorrect result. On the other hand, this type of incorrectness could certainly lead to security vulnerabilities in users of the library, and those could be of the "privilege escalation" type. So, yes, it would be beneficial to issue an advisory as a warning, and if we do that we should assign a CVE from the Mozilla pool.

Flags: needinfo?(dveditz)

Assigning CVE-2024-12224

Alias: CVE-2024-12224

(In reply to Daniel Veditz [:dveditz] from comment #27)

dveditz, Do we assign a CVE number to this? Shall I push out a RustSec advisory ahead of a tr46 change? Does the draft in comment 13 look OK?

Domenic was not worried about this advisory coming out ahead of the tr46 one.

The draft in comment 13 looks fine. The category bothers me, but it's the closest fit from the choices given at https://rustsec.org/categories/.

The reason the category bothers me is the same reason I have trouble answering your question about assigning a CVE number. This is not in any way a "privilege escalation" for idna itself, or really a proper vulnerability. It was just an incorrect result. On the other hand, this type of incorrectness could certainly lead to security vulnerabilities in users of the library, and those could be of the "privilege escalation" type. So, yes, it would be beneficial to issue an advisory as a warning, and if we do that we should assign a CVE from the Mozilla pool.

Thanks. I've tweaked the draft wording to clarify that it's not a privilege escalation in idna itself. Also added a) a link to this bug (I'm assuming it's better to have the link in the advisory early even if this bug is opened up only later on), b) an acknowledgements section, and c) links that are relevant to read when upgrading idna/url.

I'd like to get the advisory out early next week so that to the extent folks get alerts about RustSec advisories affecting their dependencies, such alerts don't trigger closer to a major holiday.

New draft:

[advisory]
id = "RUSTSEC-0000-0000"

package = "idna"

# Disclosure date of the advisory as an RFC 3339 date (mandatory)
date = "yyyy-MM-dd"

categories = ["privilege-escalation"]

keywords = ["idna", "punycode", "same-origin", "domain-name"]

aliases = ["CVE-2024-12224"]

url = "https://bugzilla.mozilla.org/show_bug.cgi?id=1887898"

[versions]
patched = [">= 1.0.0"]

idna accepts Punycode labels that do not produce any non-ASCII when decoded

idna 0.5.0 and earlier accepts Punycode labels that do not produce any non-ASCII output, which means that either ASCII labels or the empty root label can be masked such that they appear unequal without IDNA processing or when processed with a different implementation and equal when processed with idna 0.5.0 or earlier.

Concretely, example.org and xn--example-.org become equal after processing by idna 0.5.0 or earlier. Also, example.org.xn-- and example.org. become equal after processing by idna 0.5.0 or earlier.

In applications using idna (but not in idna itself) this may be able to lead to privilege escalation when combined with a client that resolves domains with such labels instead of treating them as errors that preclude DNS resolution / URL fetching and with the attacker managing to introduce a DNS entry (and TLS certificate) for an xn---masked name that turns into the name of the target when processed by idna 0.5.0 or earlier.

Remedy

Upgrade to idna 1.0.3 or later, if depending on idna directly, or to url 2.5.4 or later, if depending on idna via url. (This issue was fixed in idna 1.0.0, but versions earlier than 1.0.3 are not recommended for other reasons.)

When upgrading, please take a moment to read about alternative Unicode back ends for idna.

If you are using Rust earlier than 1.81 in combination with SQLx 0.8.2 or earlier, please also read an issue about combining them with url 2.5.4 and idna 1.0.3.

Additional information

This issue resulted from idna 0.5.0 and earlier implementing the UTS 46 specification literally on this point and the specification having this bug. The specification bug has been fixed in revision 33 of UTS 46.

Acknowledgements

Thanks to kageshiron for recognizing the security implications of this behavior.

There's now a Deno release with updated dependencies.

Flags: needinfo?(hello)

Thank you! I can also verify that there is no problem with the latest version of Deno.

In the course of additional verification, I found an example of a new URL() in the Origin header inside the Next.js anti-CSRF process.
Since this process in Next.js requires a custom HTTP header, a CSRF attack is not possible and therefore not vulnerable. However, it may be worth recognizing this as an example of how some processing can be bypassed by this issue.

https://github.com/vercel/next.js/blob/a6da830de821d584c08a67dbd753ed00bc09c8b7/packages/next/src/server/app-render/action-handler.ts#L488

I have also found similar problemsin other programming language libraries and have reported them. The conditions affected in that library are limited and are not considered high priority.

Flags: needinfo?(kageshiron)

Given the discussion, not sure if it makes sense to put the RustSec advisory in the privilege-escalation category? (Not sure if RustSec requires at least one category.)

(In reply to Dirkjan Ochtman (:djc) from comment #32)

Given the discussion, not sure if it makes sense to put the RustSec advisory in the privilege-escalation category?

I think it's the closest match of the available categories.

(Not sure if RustSec requires at least one category.)

The category appears to be optional. However, I'm still inclined to tag this privilege escalation, since potential privilege escalation in an app that (transitively) depends on idna is the reason to consider the behavior in question to have security implications and to issue an advisory.

Or, I guess I can leave the category out to avoid suggesting privilege-escalation in idna itself while still leaving those words in the description.

I re-read the inclusion criteria and it's unclear to me how it interacts with the optionality comment in the advisory template, so I left the category in and added a comment about it on the PR:
https://github.com/rustsec/advisory-db/pull/2163

(In reply to kageshiron from comment #31)

Thank you! I can also verify that there is no problem with the latest version of Deno.

Thank you for verifying. And thank you for reporting in the first place.

The RustSec advisory has been published: https://rustsec.org/advisories/RUSTSEC-2024-0421.html

Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: