4 ms·

Someone seems to have broken the demo by typing in some JavaScript. Doesn't seem to be sanitizing input completely. EDIT: Looks like it's CSS, not JS. In case

Someone seems to have broken the demo by typing in some JavaScript. Doesn't seem to be sanitizing input completely. EDIT: Looks like it's CSS, not JS. In case it helps, here's what I'm seeing [1], and here's the code from the message: <style>* { float: left; display: block }</style> [1] http://imgur.com/BoZ6lrF http://imgur.com/BoZ6lrF EDIT 2: Yup, style tags don't seem to be escaped. Tried changing colors of the room a few times, and it worked: <style>* { color: green; }</style> EDIT 3: Issue filed here: https://github.com/HashNuke/mogo-chat/issues/2 https://github.com/HashNuke/mogo-chat/issues/2

voicereasonish 13y ago
This should be a major red flag to anyone. You don't make an app/website secure by deciding on a list of things you need to sanitise. You sanitise everything to start with. A very common rookie error.
- gildas 13y ago
  > You don't make an app/website secure by deciding on a list of things you need to sanitise. I agree > You sanitise everything to start with. So you need to list everything you need to sanitise... A better approach is to ban "innerHTML" from your code. You should always display user generated text in text nodes.
  - voicereasonish 13y ago
    Just to clarify: var t = document.createTextNode(msg); content.appendChild(t); That code sanitises all possible content in msg. I don't need to list out HTML tags, script/style tags, do special case for unicode exploits, etc. You need to list what variables are "unsafe", but you don't need to list out the ways they might be unsafe. If it's got the potential to be unsafe, assume it's completely unsafe in every conceivable way, and don't use it in any context apart from as an unsafe text string. The rookie code is something like: msg.replace("something I think is unsafe", "something safer"); content.innerHTML+=msg; And agreed. InnerHTML should be removed from browsers.
    - opendais 13y ago
      Ya, but if they built it so msg='<b>msg</b>' that would remove the bold, no? So it is a bit more complex than that if they want to enable user markup. https://code.google.com/p/pagedown/source/browse/Markdown.Sanitizer.js https://code.google.com/p/pagedown/source/browse/Markdown.Sa... https://code.google.com/p/pagedown/wiki/PageDown https://code.google.com/p/pagedown/wiki/PageDown
      - dethstar 13y ago
        I'm not even a front end guy but I'm pretty sure the field they are adding the user message to should handle the style, not the user message.
        opendais 13y ago
        If one uses common choices [e.g. Markdown] that isn't how the parsers are designed. It is [message] -> [parse] -> [sanitize], generally.
      - voicereasonish 13y ago
        If you want to enable user markup, then build a simple parser, and use that to generate the correct styling you require.
        opendais 13y ago
        My point was: A) It was not as simple as you suggested if there was markup involved in the message. B) They'd have to use a parser and I linked to a parser that sanitizes that was once used in a pretty big network of sites. I'm uncertain if you misunderstood or are simply agreeing with me in a tone of writing that makes it sound like you disagree.
        ashearer 13y ago
        I think the point was that it's inherently less safe to allow arbitrary markup and then attempt to sanitize it, than to make a full parser that's incapable of generating unsafe HTML at any stage, all other things being equal. The safety of widely-deployed Markdown + sanitizer libraries is largely thanks to testing at scale and a history of patches for XSS vulnerabilities.
- yebyen 13y ago
  I, for one, am glad to see example Elixir apps with some polish that are published freely. I've been meaning to get into Elixir and Erlang, but lack of polished example apps has been a stumbling block for me, and though I have no immediate need for a TeamChat app at all, it's one of those examples like "The Todos App" that you can even perform as a code-kata in your language of choice. It would be great if I didn't have to use any Off-the-Shelf code at all, or if I must, if I actually had the time and knowledge to review it for serious vulnerabilities. But posts like this are why I come to HN.
SingAlong 13y ago
Thanks for reporting Steve. I'll fix it. EDIT: Should be fixed now. Thanks JangoSteve for reporting this.
- _puk 13y ago
  Pierre's been tinkering.. Error on startup No route matches get to ["%3Ca%20target=%27_blank%27%20href=%27http:", "pierregoutheraud.fr"]

voicereasonish 13y ago

This should be a major red flag to anyone. You don't make an app/website secure by deciding on a list of things you need to sanitise. You sanitise everything to start with. A very common rookie error.

gildas 13y ago
> You don't make an app/website secure by deciding on a list of things you need to sanitise. I agree > You sanitise everything to start with. So you need to list everything you need to sanitise... A better approach is to ban "innerHTML" from your code. You should always display user generated text in text nodes.
- voicereasonish 13y ago
  Just to clarify: var t = document.createTextNode(msg); content.appendChild(t); That code sanitises all possible content in msg. I don't need to list out HTML tags, script/style tags, do special case for unicode exploits, etc. You need to list what variables are "unsafe", but you don't need to list out the ways they might be unsafe. If it's got the potential to be unsafe, assume it's completely unsafe in every conceivable way, and don't use it in any context apart from as an unsafe text string. The rookie code is something like: msg.replace("something I think is unsafe", "something safer"); content.innerHTML+=msg; And agreed. InnerHTML should be removed from browsers.
  - opendais 13y ago
    Ya, but if they built it so msg='<b>msg</b>' that would remove the bold, no? So it is a bit more complex than that if they want to enable user markup. https://code.google.com/p/pagedown/source/browse/Markdown.Sanitizer.js https://code.google.com/p/pagedown/source/browse/Markdown.Sa... https://code.google.com/p/pagedown/wiki/PageDown https://code.google.com/p/pagedown/wiki/PageDown
    - dethstar 13y ago
      I'm not even a front end guy but I'm pretty sure the field they are adding the user message to should handle the style, not the user message.
      - opendais 13y ago
        If one uses common choices [e.g. Markdown] that isn't how the parsers are designed. It is [message] -> [parse] -> [sanitize], generally.
    - voicereasonish 13y ago
      If you want to enable user markup, then build a simple parser, and use that to generate the correct styling you require.
      - opendais 13y ago
        My point was: A) It was not as simple as you suggested if there was markup involved in the message. B) They'd have to use a parser and I linked to a parser that sanitizes that was once used in a pretty big network of sites. I'm uncertain if you misunderstood or are simply agreeing with me in a tone of writing that makes it sound like you disagree.
        ashearer 13y ago
        I think the point was that it's inherently less safe to allow arbitrary markup and then attempt to sanitize it, than to make a full parser that's incapable of generating unsafe HTML at any stage, all other things being equal. The safety of widely-deployed Markdown + sanitizer libraries is largely thanks to testing at scale and a history of patches for XSS vulnerabilities.
yebyen 13y ago
I, for one, am glad to see example Elixir apps with some polish that are published freely. I've been meaning to get into Elixir and Erlang, but lack of polished example apps has been a stumbling block for me, and though I have no immediate need for a TeamChat app at all, it's one of those examples like "The Todos App" that you can even perform as a code-kata in your language of choice. It would be great if I didn't have to use any Off-the-Shelf code at all, or if I must, if I actually had the time and knowledge to review it for serious vulnerabilities. But posts like this are why I come to HN.

SingAlong 13y ago

Thanks for reporting Steve. I'll fix it. EDIT: Should be fixed now. Thanks JangoSteve for reporting this.

_puk 13y ago
Pierre's been tinkering.. Error on startup No route matches get to ["%3Ca%20target=%27_blank%27%20href=%27http:", "pierregoutheraud.fr"]