This was originally reported as a private security issue, but has been approved for handling in the public queue by the Drupal Security Team.
The resetForm() method provided by jquery-form is vulnerable to XSS if it is used on a label element and an attacker can control the value of the for attribute of this element.
It passes the value of this attribute to $() without making sure it does not contain HTML:
case 'label':
var forEl = $(el.attr('for'));
{{ attach_library('core/internal.jquery.form') }}
<label id="label" for="<img src=x onerror=alert()>">Label</label>
<script>
window.addEventListener('load', function() {
jQuery('#label').resetForm();
});
</script>
Drupal never uses resetForm() and the library is internal (#3293156: core/jquery.form library deprecated) so it should not be used by contrib.
The solution could be to remove resetForm() entirely since we don't use it.
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !16653changes, plain diff MR !16187
Comments
Comment #3
prudloff commentedDo we need a CR if the library is marked as internal?
Comment #4
smustgrave commentedI don't think so. As you mentioned no contrib should be using it, if they are I'd assume they know they are taking the risk.
Comment #5
catchIn this case I think we do need a change record - while the entire library is internal, it's loaded on every form so the method is available to use, and we'll want to backport this to patch releases probably.
Comment #6
smustgrave commentedFor the CR
Comment #7
prudloff commentedI drafted a CR.
Comment #8
dcam commentedI edited the CR a little. I wanted to say explicitly that there is no replacement. Also, the 2022 CR about the library being internal suggested copying the Core library in case it becomes unavailable someday. So I wanted to stress that people should not do that with this function. Otherwise it looks good.
Comment #9
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #10
prudloff commentedI fixed the conflicts.
Comment #11
dcam commentedThe rebase looks good.
Comment #12
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #14
brandonlira commentedRebased the MR against the latest main and resolved the conflict in OpenTelemetryPerformanceTest.php.
Kept the current main ScriptCount value and the reduced ScriptBytes value expected after removing resetForm().
Comment #15
smustgrave commentedRestoring status from #11
Comment #17
godotislateCommitted bbe11d6 and pushed to main. Thanks!
There's a merge conflict in the numbers for the performance test, so that'll need a separate MR for 11.x. @catch also mentioned possibly backporting to a patch release in #5, so I'll confirm that we want this for 11.4.x
Comment #19
brandonlira commentedOpened a separate MR !16653 for the 11.x port.
Ports bbe11d6d93e to 11.x and resolves the OpenTelemetryPerformanceTest.php conflict by keeping the 11.x ScriptCount and StylesheetBytes values, while reducing ScriptBytes by the same amount as the main commit.
Please let me know if anything else is needed.
Thanks!
Comment #20
dcam commentedThe only difference between the two MRs is the
ScriptBytesin the performance test. Otherwise, the 11.x MR is identical to the one that went into main.Comment #22
godotislateCommitted aa558c8 and pushed to 11.x. Thanks!
Comment #24
quietone commentedUpdated and published the change record.