Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Identity Server Security Bug: Unsafe expansion of self-closing HTML tag #17

Closed
11 tasks done
alexhiggins732 opened this issue Feb 16, 2024 · 1 comment · Fixed by #34
Closed
11 tasks done

Identity Server Security Bug: Unsafe expansion of self-closing HTML tag #17

alexhiggins732 opened this issue Feb 16, 2024 · 1 comment · Fixed by #34
Assignees
Labels
bug Something isn't working

Comments

@alexhiggins732
Copy link
Owner

alexhiggins732 commented Feb 16, 2024

Unsafe expansion of self-closing HTML tag

The original Identity Server 4 code base has several medium impact security bugs detected by CodeQL scanning.

Description:

Sanitizing untrusted input for HTML meta-characters is a common technique for preventing cross-site scripting attacks. But even a sanitized input can be dangerous to use if it is modified further before a browser treats it as HTML. A seemingly innocent transformation that expands a self-closing HTML tag from <div attr="{sanitized}"/> to <div attr="{sanitized}"></div> may in fact cause cross-site scripting vulnerabilities.

// Deserialize a standard representation
tag = ( rtagName.exec( elem ) || [ "", "" ] )[ 1 ].toLowerCase();
wrap = wrapMap[ tag ] || wrapMap._default;
tmp.innerHTML = wrap[ 1 ] + elem.replace( rxhtmlTag, "<$1></$2>" ) + wrap[ 2 ];

This self-closing HTML tag expansion invalidates prior sanitization as this regular expression may match part of an attribute value.

CodeQL

// Descend through wrappers to the right content
j = wrap[ 0 ];

Sanitizing untrusted input for HTML meta-characters is a common technique for preventing cross-site scripting attacks. But even a sanitized input can be dangerous to use if it is modified further before a browser treats it as HTML. A seemingly innocent transformation that expands a self-closing HTML tag from <div attr="{sanitized}"/> to <div attr="{sanitized}"></div> may in fact cause cross-site scripting vulnerabilities.

Examples

Tool Rule ID Query Source
CodeQL js/unsafe-html-expansion View query .../additional-methods.js:932
CodeQL js/unsafe-html-expansion View query .../jquery.validate.js#L1196-L1196

Issues

Issues

Recommendation

Use a well-tested sanitization library if at all possible, and avoid modifying sanitized values further before treating them as HTML.

An even safer alternative is to design the application so that sanitization is not needed, for instance by using HTML templates that are explicit about the values they treat as HTML.

Example

The following function transforms a self-closing HTML tag to a pair of open/close tags. It does so for all non-img and non-area tags, by using a regular expression with two capture groups. The first capture group corresponds to the name of the tag, and the second capture group to the content of the tag.

function expandSelfClosingTags(html) {
	var rxhtmlTag = /<(?!img|area)(([a-z][^\w\/>]*)[^>]*)\/>/gi;
	return html.replace(rxhtmlTag, "<$1></$2>"); // BAD
}

While it is generally known regular expressions are ill-suited for parsing HTML, variants of this particular transformation pattern have long been considered safe.

However, the function is not safe. As an example, consider the following string:

<div alt="
<x" title="/>
<img src=url404 onerror=alert(1)>"/>

When the above function transforms the string, it becomes a string that results in an alert when a browser treats it as HTML.

<div alt="
<x" title="></x" >
<img src=url404 onerror=alert(1)>"/>

References

@alexhiggins732
Copy link
Owner Author

Updates released with #34

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant