Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

ZCS-16214 #6

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

ZCS-16214 #6

wants to merge 1 commit into from

Conversation

ashishkataria86
Copy link
Member

No description provided.

@silentsakky
Copy link
Member

Please add technical information about the fix in commit and PR description so we know what and how we are fixing here

@@ -97,7 +97,28 @@ public AntiSamyDOMScanner(Policy policy) {
public AntiSamyDOMScanner() throws PolicyException {
super();
}

// Method to decode the Unicode escape sequences
private String decodeUnicodeEscapes(String input) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess import-related regex was introduced in #4, and digging deeper found that it was introduced to circumvent an issue in antisamy library nahsra#24, and as per antisamy developer it's issue with CSS parser used in antisamy which seems fixed in nahsra#108
so if we upgrade the antisamy library then it's very well possible that we could remove our custom handling and also get rid of security issue

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have further investigated this, and here are my findings:
The fix mentioned in nahsra#108 refers to enabling embedStyleSheets
<directive name="embedStyleSheets" value="true"/>

According to the documentation https://github.com/nahsra/antisamy/wiki/AntiSamy-Directives,
the embedStyleSheets directive allows external stylesheets referenced through @import to be fetched and embedded into the sanitized output. Allowing CSS imports from external URLs is a dangerous practice. It exposes the application to security risks by allowing the inclusion of potentially malicious external CSS, which goes against AntiSamy’s purpose of ensuring secure input sanitization. Support for this feature in AntiSamy is deprecated and will be removed in a future release.

As described in #2, there remains an issue where media queries are stripped during sanitization. This behavior is attributed to the underlying third-party library (org.apache.xml.serialize.HTMLSerializer) used for document serialization within AntiSamy. The fix in nahsra#108 does not explicitly address this media query stripping issue.
Therefore, it is uncertain whether the upgrade resolves the media query serialization issue.

Suggested Next Steps:
Conduct a detailed review of the latest AntiSamy release to verify if the media query stripping issue (linked to HTMLSerializer) has been resolved.
Determine whether the upgrade allows us to eliminate our custom handling without compromising security.
Ensure the embedStyleSheets directive remains disabled to mitigate risks associated with remote CSS imports.

Matcher matcher = pattern.matcher(input);

// Find all matches and replace them with the decoded character
while (matcher.find()) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please fix indentation issues

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed to match the required style

@ashishkataria86
Copy link
Member Author

ashishkataria86 commented Nov 26, 2024

Please add technical information about the fix in commit and PR description so we know what and how we are fixing here

The fix addresses the issue by decoding any Unicode-encoded characters in the string before checking for the @import rule. This ensures that encoded variants like @\\0069mport, @i\\006Dport, etc., are properly detected.

Key Changes:
Unicode Decoding: A Java function has been implemented to decode Unicode escape sequences into their corresponding characters (i for \\0069, m for \\006D, etc.). This decoding process allows us to detect obfuscated @import statements, which could otherwise bypass detection.
The .replaceAll("\\", "") method ensures that any extraneous backslashes (not part of a valid Unicode escape) are removed.

This approach improves detection of obfuscated Unicode CSS payloads by normalizing input prior to applying further regex-based filtering.

… statements and bumped the version of AntiSamy.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants