> It looks to me that most uses of innerHTML in Royale are assigning text > to various labels (like Button).
I’m not sure which case you’re referring to. Ignoring examples, ASDoc and RoyaleSite, here is every use of innerHTML in the framework with comments: HTMLText -- A component created specifically for applying innerHTML. It would generally be used in mxml where the content would be provided by the develper ImageAndTextButton -- Uses innerHTML to combine text and and img tag. I guess it could be safer if it would use Image and TextNode elements. Label -- has an html getter/setter which is clearly for markup so innerHTML is necessary. LoadIndicator -- uses innerHTML, but the markup is generated internally and not exposed, so not a risk. TextButton -- has an html getter/setter which is clearly for markup so innerHTML is necessary. It has a separate text getter/setter which *does not* use innerHTML UnselectableElementBead -- uses self-generated innerHTML for setting a style. Not a risk. addDynamicSelector -- uses innerHTML for setting a style but only the first time it's used. Should not be a risk. InspireTreeIconBead -- similar to UnselectableElementBead Flat DropDownList -- uses innerHTML in six places. I don't know why. Graphics -- uses innerHTML for self generated markup. Not a risk. TextNodeContainerBase has an innerHTML getter/setter because it's emulating the corresponding HTML elements. InnerHTML -- is (as its name suggests) a component for setting innerHTML. Jewel Button -- has an html getter/setter which is clearly for markup so innerHTML is necessary. It has a separate text getter/setter which *does not* use innerHTML Jewel Label -- has an html getter/setter which is clearly for markup so innerHTML is necessary. It has a separate text getter/setter which *does not* use innerHTML Jewel SnackbarView uses innerHTML for the "message". I don't know why. MX Button -- I found it used innerHTML which should have been textContent. Fixed. MX Label -- has htmlText getter/setter which is clearly for markup so innerHTML is necessary. MX TinyEditor -- has htmlText getter/setter which is clearly for markup so innerHTML is necessary. MX UITextField -- has htmlText getter/setter which is clearly for markup so innerHTML is necessary. MX UITextFormat -- uses innerHTML for measuring. Should be safe. Spark ButtonBase -- uses innerHTML. I don't know why. Spark DropDownListButton -- uses innerHTML to draw the skin. The label is part of that. It's possible that should be sanitized. TextLine -- uses innerHTML in two places where textContent could likely be used, but the string it's using came from textContent, so it should not be a risk. Summary: Ones which could use looking into: ImageAndTextButton Flat DropDownList Jewel SnackbarView Spark ButtonBase Spark DropDownListButton I’m not personally very familiar with either Jewel or the Spark components, so someone else should comment on those. The other risky area in HTML is setting “src” for Image tags and the like. We’re not sanitizing that, but again, I’m not sure what the attack would be. > On Dec 10, 2021, at 7:41 AM, Edward Stangler <estang...@bradmark.com> wrote: > > > Closure's sanitizer was originally based on Google Caja sanitizer. It > looks like Angular DomSanitizer was also based on Caja's or Closure's. > All of them are whitelist-style sanitizers. > > It looks to me that most uses of innerHTML in Royale are assigning text > to various labels (like Button). Is it intentional that labels can > contain arbitrary HTML, or even basic visual markup? Or should most/all > of them use textContent instead? (innerText would be insufficient to > secure this, but could be used as an intermediate solution, I guess, to > retain basic visual markup.) > > > > On 12/9/2021 10:14 AM, Harbs wrote: >> I just went poking around and I found that Google Closure has a pretty >> extensive library for sanitizing HTML: >> https://github.com/google/closure-library/tree/master/closure/goog/html/sanitizer >> >> Considering we’re already using the goog libs for other things, it should be >> fairly straight-forward to wrap the functionality in Royale classes. Feel >> free to work on that… ;-) >> >> I do think that the sanitizing should be opt-in. >> >> Harbs >> >>> On Dec 9, 2021, at 5:03 PM, Kessler CTR Mark J wrote: >>> >>> I am on the opposite spectrum of this opinion. We had to write our own >>> library on-top of the basic Royale for our applications that was more >>> security minded. All of our defaults are for innerText as it will not >>> interpret the contents or use new variants that already have security built >>> it such as a textarea's "value" has security considerations by default now. >>> This is important as cybersecurity teams or software tests can easily show >>> basic XSS in fields either reflected or stored. Remember the end users are >>> the ones that are directly affected by vulnerabilities built into a web >>> application and a developer that does not follow good sanitization >>> practices will surely allow easily preventable vulnerabilities in. >>> >>> We should always have secure defaults, but allow developers to violate >>> good security practices on their own as a conscious decision. >>> >>> >>> -Mark K > >
