Suggestions for Improvement in Detecting Hidden Form Elements Methods

[Felix026x]

Dear KeePassXC Developer Team,

We have recently delved into the KeePassXC AutoType (or Autofill functionality) to detect the visibility of web form elements. Through meticulous debugging and analysis, we have identified potential areas for improvement in the current detection method to provide a more robust security guarantee. We believe that enhancing this detection mechanism can further bolster the security and accuracy of KeePassXC when dealing with hidden form elements.

We have inspected the isVisible function in the source code https://github.com/keepassxreboot/keepassxc-browser/blob/develop/keepassxc-browser/content/fields.js, and observed the existing implementation. It conducts its detection process through the following steps: it starts by checking the size and position of the element, then verifies the CSS visibility of the element, and finally the opacity of its parent elements.

We now summarize the results and report the following scenarios: the password field is hidden, and KeePassXC should have detected its invisibility and should not autofill the password into it. After inspecting and debugging the source code, we find several potential issues.

1. Lack of Detection for Element Overlay: The current method does not account for cases where elements might be covered by other page elements, making them invisible to users. For instance, a password field in a form could be obscured by another element or moved outside the parent element with a modification of the CSS overflow: hidden property to achieve hiding. (1-Overlay)
2. Limitations in Parent Element Opacity Detection: The practice of using the f.style method to detect opacity presents a limitation, as it only captures inline CSS property values directly defined within the element (e.g.,<input id="password" style="opacity: 0;">). This means that opacity set through nested CSS files or <style> tags in the HTML <header> tags for a parent element cannot be effectively detected. (2-Parent-Opacity)
3. Insufficiency in Element's Own Visibility Detection: The current detection mechanism fails to cover CSS properties such as clip and clip-path, which may lead to certain hiding techniques not being promptly identified. (3-Clip-Path-Self)
4. Incomplete Detection Regarding Parent Elements: The present detection lacks a necessary mechanism to check for CSS properties that may affect the visibility of child elements without alerting their size (e.g., the parent element is set CSS property with content-visibility or clip-path). This might result in elements that are capable of being filled but are invisible being missed. (4-Content-Visibility-Parent)

We have provided four examples on a demo website to demonstrate the reproduction process.

• 1-Overlay: https://felix026x.github.io/overlay.html
• 2-Parent-Opacity: https://felix026x.github.io/parent-opacity.html
• 3-Clip-Path-Self: https://felix026x.github.io/clip-path-self.html
• 4-Content-Visibility-Parent: https://felix026x.github.io/content-visibility.html

Expected Behavior

We provide four types of web forms for reproduction, where we hide the password field using one of the four above techniques in login forms and only a username and a login button are available. The expected behavior should be that the password field should not be detected and the password value should not be filled into the hidden field.

Current Behavior

The current behavior is that KeePassXC detects a password field in the web form without a visible password field and fill the password into the field.

Possible Solution

Based on the above-reported issues, we suggest the developers to utilize a more comprehensive method for detecting hidden fields. For instance, developers could refer to Bitwarden's method. We have developed an improved method for KeePassXC. The main modification is as follows. We can attach the modified file in this thread if required.

1. Add a function to check whether the target element is clipped and add this function into the if-else in the Check CSS visibility part.
2. Add other CSS properties when checking the parent element's property and modify the f.style into getComputedStyle(f)
3. Add the isElementNotHiddenButBehindAnotherElement function to check whether the target element is hidden behind another another element.

Developers could improve this method to detect hidden elements accurately and effectively. Could we request a pull request to add this code?

Steps to Reproduce (for bugs)

As we have implemented four website instances to demonstrate the invisible element detection issues, we utilize the same login form for each website, where the username field is visible and the password field is hidden using different techniques. The reproduction process is the same.

1. Install the KeePassXC Windows Desktop Application and KeePassXC Chrome Browser Extension, create a database, and connect the desktop application with the browser extension.
2. Add a credential item for https://felix026x.github.io (including username and password using fake data such as testuser and testpassword) in the KeePassXC desktop application.
3. Visit each URL of the four test ones (e.g.,https://felix026x.github.io/content-visibility.html), and try to trigger the autofill functionality by clicking the KeePassXC icon. (You may need to ensure that the credential could be filled into the website.)
4. Click the KeePassXC icon in the address bar of the browser, check whether KeePassXC reports that no login forms have been detected (i.e., a username-only form).
5. After triggering the autofill functionality, we can click F12 to call the Chrome Browser DevTools, and input document.getElementById("password").value; in the console.
6. Check whether the console outputs the stored password.
7. The expected experimental results are that we cannot see the password field, but this field is detected by KeePassXC, and we can use document.getElementById("password").value; in DevTools to get the password value.

Debug info

KeePassXC - 2.7.7
KeePassXC-Browser - 1.9.0.3
Operating system: Win64
Browser: Chrome/Chromium 124.0.0.0

[varjolintu]

Just to note: it's very slow and costly to start checking every element, it parents and the DOM tree. The current implementation is a compromise that does quick checks without slowing down the extension too much. There's a separate Improved Input Field detection that can be enabled per site, and it does some more accurate identification.

[Felix026x]

Thank you very much for your prompt and detailed response!

We understand the necessity of balancing between ensuring smooth extension operation and enhancing the accuracy of input field detection. We believe adopting a better recognition scheme could further ensure the security of autofill capabilities, and we hope you might make some improvements, if possible.

Besides, we are excited to hear about the development of the "Improved Input Field detection" feature as a solution for more precise identification. We would like to know a bit more about several aspects:

1. How exactly does this feature work? Does it improve detection accuracy by analyzing deeper levels of the DOM structure, or does it employ another mechanism?
2. How can one enable the "Improved Input Field detection" feature for specific websites that are frequently used? Is there a user guide or documentation users can refer to?
3. What kind of performance impact should users anticipate upon enabling this feature? For instance, should users expect a significant increase in page loading times or the extension's resource usage?

I greatly appreciate the efforts and innovations the KeePassXC team has made to provide a more secure and efficient user experience. I look forward to your reply and learning more about how to fully leverage this advanced feature of KeePassXC.

Best Regards,

Felix

[droidmonkey]

We have developed an improved method for KeePassXC. The main modification is as follows. We can attach the modified file in this thread

Hello, if you have proposed code changes, then you should start a pull request. See here for how to do that: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request

[varjolintu]

How exactly does this feature work? Does it improve detection accuracy by analyzing deeper levels of the DOM structure, or does it employ another mechanism?

At this point it mostly just traverses the children of elements, looking for input fields and ignoring any unwanted nodes. This is helpful for example when identifying input fields inside Shadow DOM. getComputedStyle() is not used here either.

How can one enable the "Improved Input Field detection" feature for specific websites that are frequently used? Is there a user guide or documentation users can refer to?

Extension settings has a Site Preferences tab, where a URL can be added with that option enabled.

What kind of performance impact should users anticipate upon enabling this feature? For instance, should users expect a significant increase in page loading times or the extension's resource usage?

getComputedStyle() is the function that slows things down quite a bit. The impact depends on how complex the site is, and how many parents or child elements must be traversed.

[Felix026x]

Dear KeePassXC team,

I want to extend my sincere thanks for your kind and informative response to my query. Your explanation is incredibly helpful and provides me with a clearer understanding of the matter.

Besides, I am motivated to contribute directly to the project and preparing to submit a pull request for my developed code.

I look forward to potentially collaborating with the team and am eager to contribute to the community. Thank you once again for your supportive and constructive feedback.

Best regards,
Felix

[droidmonkey]

Let's talk about the risk we are trying to solve here since that matters when balancing security and performance/usability. One reason to detect well hidden fields is to prevent an attacker scenario where a malicious script is running on an otherwise trusted site. In this scenario, I doubt enhanced visibility checks are going to save you. Visibility checks are mainly to prevent false positive fill recommendations with a side benefit of potential security.

[Felix026x]

Thanks for your response.

In my opinion, when the password field is invisible but is detected by KeePassXC as a active field, KeePassXC will fill this field using stored password. This is an unexpected result. I believe an improved visibility checks can provide users with more security guarantee. Besides, the case that an invisible field can not be filled should be expected. This does not cause too much harm to usability if the detection method does not bring too much performance overhead.

Best regards,
Felix