java-html-sanitizer CssSchema: Allow negative token values for margin property

This fixes the https://github.com/OWASP/java-html-sanitizer/issues/183

Sep 05 '19 07:09 janis-github

@mikesamuel,

How could my fix possibly break the build system? And reduce coverage in completely different/unmodified file?

Thanks, Janis.

Sep 05 '19 10:09 janis-github

How could my fix possibly break the build system? And reduce coverage in completely different/unmodified file?

@janis-github The baseline run in Travis isn't super old, but sometimes this happens when Travis has changed something like the set of supported JDKs, or a new version of the maven test coverage plugin does things subtly differently. If we resolve other issues, I'm happy to pull and merge manually.

Sep 05 '19 16:09 mikesamuel

Thanks for the patch.

There used to be a problem with negative margins allowing HTML like the below to escape visual containment mechanisms like CSS overflow.

<!doctype html>
<html>
  <meta charset="utf-8" />
<body>

<!--  trusted content -->
<form style="float: right; width: 10em" id="sidebar">
  <label for="account">Email:</label> <input type="text" name="account">
  <br><label for="pwd">Password:</label> <input type="password" name="pwd"></p>
</form>
<div style="width: 30em">
  <p>Lorem ipsum sic dolor amet
  <p>Lorem ipsum sic dolor amet
  <p>Lorem ipsum sic dolor amet
  <p>Lorem ipsum sic dolor amet

  <h2>Comments</h2>
  <div style="overflow: auto">
<!-- /trusted content -->


<!--  sanitized, third-party content -->
    <p>Comment content</p>

    <p style="margin: -8em 0 8em 25em">Contact user support +1-800-PHISHER</p>
<!-- /sanitized, third-party content -->

  </div>
</html>

It used to be the case, that certain browsers (sorry I forget which) used to render that HTML thus:

Note that the "Contact user support +1-800-PHISHER" text appears below the password input where it seems to be part of the high-reputation login form, instead of being constrained to the rectangle for low-reputation content.

Before allowing negative margins, I'd want to double check that browsers no longer have that bug.

Would replacing negative values with 0 instead of dropping them when non-negative values are allowed provide a short-term improvement?

Sep 05 '19 16:09 mikesamuel

I see your point. I was thinking about that and similar behaviour can be achieved with large positive margins going out of the box, too. (Do not have sample for that, though) There is no solution based only on margin values. So, normalising negatives to 0 (or any other number for that matter) does not solve the problem. In my specific case which triggered this issue setting left margin to -1.6em did actually make the output prettier. To be 100% sure sanitiser would need to render the html and "see" if there are any suspicious overlaps, etc... But that would be well beyond the the scope of this lib, wouldn't it? Maybe normalise to some sane value like up to -5em/-50px is ok, but anything less than that is bad? And make those configurable (separate for vertical/horizontal, top/margin-top, left/right, specific for each element, etc...). I do not know if that's worth it, really.

Sep 06 '19 08:09 janis-github

@janis-github, Sorry for the delay. The problem was that visual cropping was bypassed when margins were negative. Not that you can move things around. I'm not aware of any numeric overflow problems that bypassed visual cropping with large margins.

Sep 18 '19 18:09 mikesamuel

Ok, resetting negative margins to 0 probably is the best solution then. Sorry, I'll not have a PR as it involves going significantly deeper in your code as I'd like, currently. I.e. resetting token values to zero instead of just ignoring.

Sep 20 '19 12:09 janis-github