spoof english leaks via numberingSystem: numbers (non-latn) or decimal separator (latn)

added All Platforms Fingerprinting FF115-esr labels

~~just to be clear, when not spoofing, the numbers everywhere are changed: here's arabic: see the NumberRangeOver + Underflow does not use latn 2~~

ok, i obviously need a break and should have looked at the start of left of the string - it does use 2

numbers - not all validation messages with numbers were affected, so this is definitely fixable

Yes, sometimes the English dom.properties is used, check GetMaybeSpoofedPropertiesFile (called by nsContentUtils::GetMaybeLocalizedString) in dom/base/nsContentUtils.cpp.

If we're lucky enough, GetLocalizedString is called instead of GetMaybeLocalizedString where these strings are returned, and we can just replace the function...

Err, moar !!

The strings are okay, the numbers aren't.

The problem is the ConvertNumberToString.

my initial comment was slightly misleading. It's really two issues

some numbers aren't localized
those that are leak numberingSystem when non-latn + spoof_english

The leaky error uses ConvertNumberToString, whereas the non-leaky errors use Decimal::toString.

So, what's the correct one? Localized everywhere, but fixed so that it doesn't leak, or Arabic numbers for everyone, including Arabic languages (lol)?

i can't make sense of that. It should (I assume) always reflect app language (but when we spoof english we should use latn)

Yes, it seems the correct thing also to me.

However, I've found that ConvertNumberToString eventually calls Decimal::toString... I need to dig it more (even though I can try to make the underflow/overflow call it, to see if it's magically converted to the localized number as well).

but, I can also make the argument that it should always reflect web-content language/locale (in which case we wouldn't have any issues - e.g. localization of the date picker etc) - I mean the user wants to use web pages in that requested language

we also have numbers here: https://searchfox.org/mozilla-central/source/dom/locales/en-US/chrome/layout/MediaDocument.properties

edit: checking ar it seemed to come up okay with (PNG Image, 800 × 1 pixels) — Scaled (12%) but IDK if that's because it's not localized or because we're lucky

The leaky error uses ConvertNumberToString, whereas the non-leaky errors use Decimal::toString.

Okay, this patch makes the behavior consistent, and now I get the localized numbers everywhere:

The patch

diff --git a/dom/html/input/NumericInputTypes.cpp b/dom/html/input/NumericInputTypes.cpp
index 54811ddb9d85..882604be0556 100644
--- a/dom/html/input/NumericInputTypes.cpp
+++ b/dom/html/input/NumericInputTypes.cpp
@@ -52,11 +52,7 @@ nsresult NumericInputTypeBase::GetRangeOverflowMessage(nsAString& aMessage) {
   MOZ_ASSERT(!maximum.isNaN());
 
   nsAutoString maxStr;
-  char buf[32];
-  DebugOnly<bool> ok = maximum.toString(buf, ArrayLength(buf));
-  maxStr.AssignASCII(buf);
-  MOZ_ASSERT(ok, "buf not big enough");
-
+  ConvertNumberToString(maximum, maxStr);
   return nsContentUtils::FormatMaybeLocalizedString(
       aMessage, nsContentUtils::eDOM_PROPERTIES,
       "FormValidationNumberRangeOverflow", mInputElement->OwnerDoc(), maxStr);
@@ -67,11 +63,7 @@ nsresult NumericInputTypeBase::GetRangeUnderflowMessage(nsAString& aMessage) {
   MOZ_ASSERT(!minimum.isNaN());
 
   nsAutoString minStr;
-  char buf[32];
-  DebugOnly<bool> ok = minimum.toString(buf, ArrayLength(buf));
-  minStr.AssignASCII(buf);
-  MOZ_ASSERT(ok, "buf not big enough");
-
+  ConvertNumberToString(minimum, minStr);
   return nsContentUtils::FormatMaybeLocalizedString(
       aMessage, nsContentUtils::eDOM_PROPERTIES,
       "FormValidationNumberRangeUnderflow", mInputElement->OwnerDoc(), minStr);

The result

{
  "BadInputNumber": "Please enter a number.",
  "CheckboxMissing": "Please check this box if you want to proceed.",
  "DateTimeRangeOverflow": "Please select a value that is no later than 2023-12-31.",
  "DateTimeRangeUnderflow": "Please select a value that is no earlier than 2023-12-31.",
  "FileMissing": "Please select a file.",
  "InvalidEmail": "Please enter an email address.",
  "InvalidURL": "Please enter a URL.",
  "NumberRangeOverflow": "Please select a value that is no more than ٢.",
  "NumberRangeUnderflow": "Please select a value that is no less than ٢.",
  "PatternMismatch": "Please match the requested format.",
  "RadioMissing": "Please select one of these options.",
  "SelectMissing": "Please select an item in the list.",
  "StepMismatch": "Please select a valid value. The two nearest valid values are ٢ and ٤.",
  "ValueMissing": "Please fill out this field."
}

Now, let's understand how Decimal::toString uses formatted numbers.

However, I've found that ConvertNumberToString eventually calls Decimal::toString...

That was the base class, the actual class uses ICU.

Incoming patch (almost)

diff --git a/dom/html/input/NumericInputTypes.cpp b/dom/html/input/NumericInputTypes.cpp
index 54811ddb9d85..6965b496be59 100644
--- a/dom/html/input/NumericInputTypes.cpp
+++ b/dom/html/input/NumericInputTypes.cpp
@@ -52,11 +52,7 @@ nsresult NumericInputTypeBase::GetRangeOverflowMessage(nsAString& aMessage) {
   MOZ_ASSERT(!maximum.isNaN());
 
   nsAutoString maxStr;
-  char buf[32];
-  DebugOnly<bool> ok = maximum.toString(buf, ArrayLength(buf));
-  maxStr.AssignASCII(buf);
-  MOZ_ASSERT(ok, "buf not big enough");
-
+  ConvertNumberToString(maximum, maxStr);
   return nsContentUtils::FormatMaybeLocalizedString(
       aMessage, nsContentUtils::eDOM_PROPERTIES,
       "FormValidationNumberRangeOverflow", mInputElement->OwnerDoc(), maxStr);
@@ -67,11 +63,7 @@ nsresult NumericInputTypeBase::GetRangeUnderflowMessage(nsAString& aMessage) {
   MOZ_ASSERT(!minimum.isNaN());
 
   nsAutoString minStr;
-  char buf[32];
-  DebugOnly<bool> ok = minimum.toString(buf, ArrayLength(buf));
-  minStr.AssignASCII(buf);
-  MOZ_ASSERT(ok, "buf not big enough");
-
+  ConvertNumberToString(minimum, minStr);
   return nsContentUtils::FormatMaybeLocalizedString(
       aMessage, nsContentUtils::eDOM_PROPERTIES,
       "FormValidationNumberRangeUnderflow", mInputElement->OwnerDoc(), minStr);
@@ -132,6 +124,15 @@ bool NumberInputType::ConvertNumberToString(Decimal aValue,
                                             nsAString& aResultString) const {
   MOZ_ASSERT(aValue.isFinite(), "aValue must be a valid non-Infinite number.");
 
+  const Document* document = mInputElement->OwnerDoc();
+  bool spoofLocale = nsContentUtils::SpoofLocaleEnglish() &&
+                     (!document || !document->AllowsL10n());
+  if (spoofLocale) {
+    std::string valueStr = aValue.toString();
+    return aResultString.AssignASCII(valueStr.c_str(), valueStr.length(),
+                                     fallible);
+  }
+
   aResultString.Truncate();
   ICUUtils::LanguageTagIterForContent langTagIter(mInputElement);
   ICUUtils::LocalizeNumber(aValue.toDouble(), langTagIter, aResultString);

Now the real problem is: is mInputElement->OwnerDoc() always not nullptr?

Also, let me check real quick the media thingie, so that we can send only one patch upstream.

BTW, I don't understand why the conversion that already exists is so longer than mine.

Internally, Blink's toString goes through std::string, so we don't gain anything by preallocating a buffer, we can know the exact size later...

Also, now that I'm double checking the patch... I think we can do better.

LanguageTagIterForContent: it means that somehow it's using the application's language (or whatever) as a fallback.

Check its description.

we also have numbers here: https://searchfox.org/mozilla-central/source/dom/locales/en-US/chrome/layout/MediaDocument.properties

edit: checking ar it seemed to come up okay with (PNG Image, 800 × 1 pixels) — Scaled (12%) but IDK if that's because it's not localized or because we're lucky

It doesn't format numbers, but calls directly AppendInt, same for the scale.

So, we're okay here.

we should write a test

edit: or when I add this to TZP I guess it's in our health checks

I'd totally love to have tests...

One day I'll surprise you by running TZP headlessly

mentioned in merge request !891 (merged)

marked this issue as related to tor-browser-build#41051 (closed)

marked this issue as related to tor-browser-build#41052 (closed)

We also have $S numbers here - https://searchfox.org/mozilla-central/source/dom/locales/en-US/chrome/layout/HtmlForm.properties

edit: but not sure if I can get those without user action. I'm still working my way thru a heap of upstream properties files

edit: I know this is covered, well should be - https://searchfox.org/mozilla-central/source/dom/locales/en-US/chrome/layout/xmlparser.properties - but this also has $S numbers

I think we should look for LocalizeNumber instead, but:

now it's patched
it was used only for this functionality

As for LanguageTagIterForContent, it's used also in ConvertStringToNumber, but nothing else. I wonder if we've now broken inputs for users with other numerals.

how do we test broken inputs?

I think you should try to paste ٢ as a number, and see if it complains (before and after the patch).

However, I think that this would be fingerprintable by doing

input.value = "٢";
input.checkValidity();

In that case, if we want to avoid fingerprintability, we will have to break inputs.

ok, will look at it tomorrow I hope

AFAICT we can set any value (even a string like banana [1]!!) for the number input: non numeric need to be strings, but we can also set them as strings i.e 4 vs "4". I have included both. I have also included v and x (see test code comments)

And this doesn't seem to leak anything whatsoever - it's the same regardless of (web content) locale, so I doubt app lang being different could even affect it

everyone everywhere all at once should be 2123ce85

[1] "banana": {... "value": "", "validity": true} - WTF, it's validity is true!!! yay! wot?

test

<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<title>inputtest</title>
<style>
body {background-color: white; color: black;}
.mono {font-family: monospace, "Courier New"; font-size: 11px;}
.spaces {white-space: pre-wrap;}
</style>
</head>
<body>

<table width="600">
	<col width="40%"><col width="60%">
	<tr><td>StepMismatch (min 6)</td><td><input id="inputtest" type="number" min="6" step="2" /></td></tr>
	<tr><td colspan="2"><span id="inputresult" class="mono spaces"></span></td></tr>
</table>


<script>
'use strict';
function mini(str) {
	const json = `${JSON.stringify(str)}`
	let i, len, hash = 0x811c9dc5
	for (i = 0, len = json.length; i < len; i++) {
		hash = Math.imul(31, hash) + json.charCodeAt(i) | 0
	}
	return ('0000000' + (hash >>> 0).toString(16)).slice(-8)
}

let target = document.getElementById("inputtest")
let oTests = {
	// 1st value expected valid then v
	// 3rd value expected invalid then x
	// v + x so we can check we're not falling back to the previous state
	ar: ['٤','v','٦','x'],
	en: ['4',4,'v','6',6,'x'],
	fa: ['۴','v','۶','x'],
	my: ['၄','v','၆','x'],
}
let oData = {}, aDisplay = []
for (const k of Object.keys(oTests)) {
	oData[k] = {}
	aDisplay.push("  \""+ k +"\": {")
	oTests[k].forEach(function(n) {
		target.value = n
		let value = target.value
		let charName, charValue, j
		if ("string" === typeof n) {
			charName = "charCode"; charValue = n.charCodeAt(0); j = n
		} else {
			charName = "number"; charValue = n; j = n +"_number"
		}
		let validity = target.checkValidity()
		oData[k][j] = {}
		oData[k][j][charName] = charValue
		oData[k][j]["value"] = value
		oData[k][j]["validity"] = validity
		if ("string" === typeof value) {value = "\"" + value +"\""}
		aDisplay.push("    \""+ j + "\": {\"" + charName +"\": " + charValue +", \"value\": " + value +", \"validity\": " + validity +"}")
	})
	aDisplay.push("  }")
}
document.getElementById("inputresult").innerHTML = mini(oData) +":<br>{"+ aDisplay.join("<br>") +"<br>}"

</script>
</body>
</html>

i dumped it at https://arkenfox.github.io/TZP/tests/inputtest.html to save you time (not that it's a permanent test, or maybe it should be?)

ahh ok, validity is true because the value is blank (when we try to set it with nonsense) - seems legit

ok, so tested nightly and verified fixed (both consistency across messages and not leaking localized numbers). Input doesn't seem affected at all.

Could you please check commit hash of the build you checked in about:buildconfig? I think the nightly you checked didn't have the changes yet, the one that has them will arrive in 2-4 hours (it will be 2024-01-19).

well, it must have had the patch because NumberRangeOverflow was localized

about:buildconfig = Built from https://gitlab.torproject.org/tpo/applications/tor-browser/-/commit/a7932fac62c8a955bdc3f08a9b81b7f2562a4eff

link = a7932fac

also, the issue was verified fixed - so cogito ergo sum

Oh, I see what happened. The Windows binaries are already available for download/update, even though the nightly task hasn't finished yet (we get an email when all builds - TBB + MB - are done).

decimal separators

ca
- "Seleccioneu un valor vàlid. Els dos valors vàlids més propers són 2,1726 i 1000,3209."
ca + spoof english
- "Please select a valid value. The two nearest valid values are 2,1726 and 1000,3209."
en-US
- "Please select a valid value. The two nearest valid values are 2.1726 and 1000.3209."

Oh noes.

Maybe we should add this to the upstream Bug.

marked this issue as related to #42327

assigned to @pierov

added Backport Next labels

marked this issue as related to #40789

added Uplift label

changed title from spoof english with non-latn numberingSystem languages, leaks numberingSystem to spoof english leaks via numberingSystem: numbers (non-latn) or decimal separator (latn)