Bug (CVE-2023-32359) – AX: Don't include password input value in aria-labelledby description

• Component: Accessibility
• Status: RESOLVED FIXED

[Tommy McHugh]

If an element has an aria-labelledby attribute to a password input we should not make the description of that element the value of the password.

[Radar WebKit Bug Importer]

<rdar://problem/102938762>

[Tommy McHugh]

Created attachment 463871 [details]
Patch

[Tommy McHugh]

Created attachment 463872 [details]
Patch

[Tyler Wilcock]

Comment on attachment 463872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=463872&action=review

> LayoutTests/accessibility/aria-labelledby-on-password-input.html:8
> +<div id="content">

I think you can either remove this `id`, or use it to hide the test text before `finishJSTest()` like so:

document.getElementById("content").style.visibility = "hidden";

> LayoutTests/accessibility/aria-labelledby-on-password-input.html:29
> +            await waitFor(() => password1.stringValue.length === "AXValue: hello".length);

Does this work instead of checking the length?

await waitFor(() => password1.stringValue === "AXValue: hello");

> LayoutTests/accessibility/aria-labelledby-on-password-input.html:34
> +            document.getElementById("button-2").setAttribute("aria-labelledBy", "text-1");

I think aria-labelledBy is typically all lowercased:

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-labelledby

> LayoutTests/accessibility/aria-labelledby-on-password-input.html:42
> +            output += expect("button2.description", "'AXDescription: This is before my password field This is after my password field'");

Could maybe make the test smaller while still providing the same diagnostics with:

"This is before my password field" -> "Before password"
"This is after my password field" -> "After password"

[Tommy McHugh]

Created attachment 463873 [details]
Patch

[Tommy McHugh]

(In reply to Tyler Wilcock from comment #4)
> Does this work instead of checking the length?
> 
> await waitFor(() => password1.stringValue === "AXValue: hello");
Unfortunately we can't do this here because we don't return the actual password value here only the text representation being rendered (sometimes ****). Length check seems to be what the other password tests use.

[Tommy McHugh]

Created attachment 463874 [details]
Patch

[Andres Gonzalez]

(In reply to Tommy McHugh from comment #7)
> Created attachment 463874 [details]
> Patch

--- a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
+++ b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp

+    if (auto* input = dynamicDowncast<HTMLInputElement>(element)) {
+        if (input->isPasswordField())
+            return String();

This should return a string with the same length as input->value() consisting of a repeated mask char, e.g., if input->value() is "blah", the return value here should "****".

--- /dev/null
+++ b/LayoutTests/accessibility/aria-labelledby-on-password-input.html

+        setTimeout(async function() {
+            await waitFor(() => password1.stringValue.length === "AXValue: hello".length);
+            output += expect("button1.description", "'AXDescription: '");

+        setTimeout(async function() {
+            await waitFor(() => password1.stringValue.length === "AXValue: hello".length);
+            output += expect("button1.description", "'AXDescription: '");

Should be:

+            output += expect("button1.description", "'AXDescription: *****'");

The same applies to the other instances. In general the value of the password field should not be exposed as an empty string but instead as a mask string, so that the VO user can arrow through it, know how many chars are in the filed, etc., i.e., to have the same experience as the sighted user.

[Tommy McHugh]

Created attachment 463911 [details]
Patch

[Tommy McHugh]

(In reply to Andres Gonzalez from comment #8)
> The same applies to the other instances. In general the value of the
> password field should not be exposed as an empty string but instead as a
> mask string, so that the VO user can arrow through it, know how many chars
> are in the filed, etc., i.e., to have the same experience as the sighted
> user.
Sounds good! Made this the bullet character mask to match what iOS wrapper uses. AXValue already returns masked characters, it's a VoiceOver specific issue why we don't announce those.

[Andres Gonzalez]

(In reply to Tommy McHugh from comment #9)
> Created attachment 463911 [details]
> Patch

--- a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
+++ b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp

Can we do a char replacement in place, something like:

+    if (auto* input = dynamicDowncast<HTMLInputElement>(element)) {
+        String inputValue = input->value();
+        if (input->isPasswordField()) {
+            for (size_t i = 0; i < inputValue.length(); i++)
+                inputValue[i] = '•';
+        }
+        return inputValue;
+    }

[Tommy McHugh]

(In reply to Andres Gonzalez from comment #11)
> (In reply to Tommy McHugh from comment #9)
> > Created attachment 463911 [details]
> > Patch
> 
> --- a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
> +++ b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
> 
> Can we do a char replacement in place, something like:
This doesn't work, WTFString can't assign here for characters. Took a look at header for WTFString and didn't find another method either that would do the equivalent.

[Andres Gonzalez]

(In reply to Tommy McHugh from comment #12)
> (In reply to Andres Gonzalez from comment #11)
> > (In reply to Tommy McHugh from comment #9)
> > > Created attachment 463911 [details]
> > > Patch
> > 
> > --- a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
> > +++ b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp
> > 
> > Can we do a char replacement in place, something like:
> This doesn't work, WTFString can't assign here for characters. Took a look
> at header for WTFString and didn't find another method either that would do
> the equivalent.

this may be more efficient than multiple reallocations and concatenations:

+    if (auto* input = dynamicDowncast<HTMLInputElement>(element)) {
+        String inputValue = input->value();
+        if (input->isPasswordField()) {
+            StringBuilder passwordValue;
+            passwordValue.reserveCapacity(inputValue.length());
+            for (size_t i = 0; i < inputValue.length(); i++)
+                passwordValue.append(''•');
+            return passwordValue.toString();
+        }
+        return inputValue;
+    }

[Tommy McHugh]

Created attachment 465685 [details]
Patch

[Tyler Wilcock]

rdar://102815043

[EWS]

Committed 262433@main (3b6d017ba868): <https://commits.webkit.org/262433@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 465685 [details].