alexhiggins732 / IdentityServer8

DotNet 8, Identity, OpenID Connect and OAuth 2.0 Framework for ASP.NET Core Identity Server 8
Apache License 2.0
42 stars 18 forks source link

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

Closed alexhiggins732 closed 7 months ago

alexhiggins732 commented 7 months ago

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 commented 7 months ago

Updates released with #34