5 ms·

While I agree that Electon offers a massive amount of footguns, neither Javascript nor Electon was the issue in this case. The issue was using innerHTML (or rat

While I agree that Electon offers a massive amount of footguns, neither Javascript nor Electon was the issue in this case. The issue was using innerHTML (or rather $.html()) with strings concatenated together from user input. Something you should never do. Could as well just call eval() directly on it, or pass the input to gcc, compile it and run the resulting binary. The Signal devs thought $.html() does some kind of escaping: https://github.com/signalapp/Signal-Desktop/commit/9d41b8616296f1b328aa864e0114b99d7f11ca06 https://github.com/signalapp/Signal-Desktop/commit/9d41b8616... (this commit made something that was easy to exploit into something that was even easier to exploit). To be honest, I'd lay more blame on the authors of the DOM spec making innerHTML a setter than on Electron, and jQuery exposing this misfeature even more with $.html(), teaching an army of web developers to do the wrong thing. We've all seen numerous (XSS) vulnerabilities in all kinds of websites, browser extensions, Electron apps, etc resulting from this API, tho in Electron apps it gets particularly devastating as often you'd get code execution not just in a sandboxed website but full code execution under the current user credentials in the system.

pfg 8y ago
So to be clear, a lot of the blame definitely belongs in the "all that comes with it" bucket here, which is one of the reasons why you should think twice about developing desktop apps using a platform that forces you to deal with not only the usual desktop app security concerns, but also all the things that make web apps vulnerable. Still, when you ship an app with a relatively strict Content Security Policy as Signal did (including using script-src 'self'), you don't really expect a simple XSS vulnerability to lead to RCE, but it turns out that policy doesn't really do much in an Electron app.
caf 8y ago
Surely the collective noun for footguns is a cache :)
seba_dos1 8y ago
>The Signal devs thought $.html() does some kind of escaping: Uhm... that's a really rookie mistake to make. Like, one of the very basics of jQuery usage. I'm not exactly sure what to think about it after seeing this commit you linked...
- twr 8y ago
  I don't know if this is correct, but, I once got the impression that Signal Desktop was under the sole purview of a new hire at OWS. In other words, Moxie doesn't review the commits. I hope I'm wrong, but even if I'm not, I suppose it makes no difference, as he's arguably responsible either way.
  - debacle 8y ago
    Isn't this supposed to be an app for secure communication? Theo would have a conniption.
    - fjsolwmv 8y ago
      Has Theo de Raadt ever endorsed the security of anything except the default install of OpenBSD?
- ddtaylor 8y ago
  The worst part is that someone assumed something then removed the code that did the escaping without even doing the most basic of tests, like even in the browser just doing a quick foo.html('<script>alert("oh snap this is bad")</script>')
  - deleted 8y ago
    [deleted]
  - deleted 8y ago
    [deleted]
  - icebraining 8y ago
    Maybe he did and it didn't work for some reason and he thought it was good. Shit happens. I think the most worrying part is not having been caught in code review. Signal does code review, right?
  - Cakez0r 8y ago
    Even worse than that, it looks like there _were_ unit tests to check that input was correctly sanitized, but the patch that introduced the bug also explicitly changed the unit tests to ensure that input was _not_ being escaped! A mistake that seems like it could've been caught in a code review!
thaumasiotes 8y ago
> The issue was using innerHTML (or rather $.html()) with strings concatenated together from user input. > The Signal devs thought $.html() does some kind of escaping I mean, it does do a kind of escaping. If you assign javascript to innerHTML directly, it won't execute. jQuery specifically checks whether you're adding a script tag, and if so, it takes the extra step to execute it for you.
- derefr 8y ago
  You mean, the innerHTML of a <script> element. Which isn’t really a thing, because the inside of a <script> tag is a document boundary—assigning raw Javascript to innerText or innerHTML directly would make no sense in either case. You need to wrap your Javascript in a CDATA node ;)
  - thaumasiotes 8y ago
    No, analogously to adding a script tag with jQuery, I meant adding a script tag, with javascript inside, to the innerHTML of some other element.
grrowl 8y ago
> The Signal devs thought $.html() does some kind of escaping: https://github.com/signalapp/Signal-Desktop/commit/9d41b8616.. https://github.com/signalapp/Signal-Desktop/commit/9d41b8616.... (this commit made something that was easy to exploit into something that was even easier to exploit). This is an absolutely egregious rookie error. I wouldn't touch the Signal desktop app with a 10 foot pole after seeing that commit.
- cyphar 8y ago
  What's incredible is that the author actually had to modify an XSS test so that it read: > const expected: string = "Hello<br><script>alert('evil');</script>World!"; (Meaning they actually changed a line that had "<script>alert('evil')</script>" and didn't notice.) I have seen this before though, with some folks removing path sanitisation code I added several years prior to fix a CVE. So it's not uncommon (it also got merged, so when I found out and fixed it I added a very large and scary comment to stop people from doing it again).
- mercer 8y ago
  Why is this a 404 now?
  - Stephen304 8y ago
    Looks like grrowl copied and repasted the truncated display text of the link. The full link is 2 comments up: https://github.com/signalapp/Signal-Desktop/commit/9d41b8616296f1b328aa864e0114b99d7f11ca06 https://github.com/signalapp/Signal-Desktop/commit/9d41b8616...
komuW 8y ago
Is there a linter that can catch this kind of mistakes?
charleslmunger 8y ago
> The issue was using innerHTML (or rather $.html()) with strings concatenated together from user input. Something you should never do. Could as well just call eval() directly on it, or pass the input to gcc, compile it and run the resulting binary. Yes, but most engineers would look at that last element and say "what on earth is going on here", where $.html() being dangerous is something that engineers who don't usually work on web might not know about. You're right about blaming the DOM spec, but there's no actual reason for signal desktop to interact with that poorly designed spec except that they chose electron as a framework.

pfg 8y ago

So to be clear, a lot of the blame definitely belongs in the "all that comes with it" bucket here, which is one of the reasons why you should think twice about developing desktop apps using a platform that forces you to deal with not only the usual desktop app security concerns, but also all the things that make web apps vulnerable. Still, when you ship an app with a relatively strict Content Security Policy as Signal did (including using script-src 'self'), you don't really expect a simple XSS vulnerability to lead to RCE, but it turns out that policy doesn't really do much in an Electron app.

caf 8y ago

Surely the collective noun for footguns is a cache :)

seba_dos1 8y ago

>The Signal devs thought $.html() does some kind of escaping: Uhm... that's a really rookie mistake to make. Like, one of the very basics of jQuery usage. I'm not exactly sure what to think about it after seeing this commit you linked...

twr 8y ago
I don't know if this is correct, but, I once got the impression that Signal Desktop was under the sole purview of a new hire at OWS. In other words, Moxie doesn't review the commits. I hope I'm wrong, but even if I'm not, I suppose it makes no difference, as he's arguably responsible either way.
- debacle 8y ago
  Isn't this supposed to be an app for secure communication? Theo would have a conniption.
  - fjsolwmv 8y ago
    Has Theo de Raadt ever endorsed the security of anything except the default install of OpenBSD?
ddtaylor 8y ago
The worst part is that someone assumed something then removed the code that did the escaping without even doing the most basic of tests, like even in the browser just doing a quick foo.html('<script>alert("oh snap this is bad")</script>')
- deleted 8y ago
  [deleted]
- deleted 8y ago
  [deleted]
- icebraining 8y ago
  Maybe he did and it didn't work for some reason and he thought it was good. Shit happens. I think the most worrying part is not having been caught in code review. Signal does code review, right?
- Cakez0r 8y ago
  Even worse than that, it looks like there _were_ unit tests to check that input was correctly sanitized, but the patch that introduced the bug also explicitly changed the unit tests to ensure that input was _not_ being escaped! A mistake that seems like it could've been caught in a code review!

thaumasiotes 8y ago

> The issue was using innerHTML (or rather $.html()) with strings concatenated together from user input. > The Signal devs thought $.html() does some kind of escaping I mean, it does do a kind of escaping. If you assign javascript to innerHTML directly, it won't execute. jQuery specifically checks whether you're adding a script tag, and if so, it takes the extra step to execute it for you.

derefr 8y ago
You mean, the innerHTML of a <script> element. Which isn’t really a thing, because the inside of a <script> tag is a document boundary—assigning raw Javascript to innerText or innerHTML directly would make no sense in either case. You need to wrap your Javascript in a CDATA node ;)
- thaumasiotes 8y ago
  No, analogously to adding a script tag with jQuery, I meant adding a script tag, with javascript inside, to the innerHTML of some other element.

grrowl 8y ago

> The Signal devs thought $.html() does some kind of escaping: https://github.com/signalapp/Signal-Desktop/commit/9d41b8616.. https://github.com/signalapp/Signal-Desktop/commit/9d41b8616.... (this commit made something that was easy to exploit into something that was even easier to exploit). This is an absolutely egregious rookie error. I wouldn't touch the Signal desktop app with a 10 foot pole after seeing that commit.

cyphar 8y ago
What's incredible is that the author actually had to modify an XSS test so that it read: > const expected: string = "Hello<br><script>alert('evil');</script>World!"; (Meaning they actually changed a line that had "<script>alert('evil')</script>" and didn't notice.) I have seen this before though, with some folks removing path sanitisation code I added several years prior to fix a CVE. So it's not uncommon (it also got merged, so when I found out and fixed it I added a very large and scary comment to stop people from doing it again).
mercer 8y ago
Why is this a 404 now?
- Stephen304 8y ago
  Looks like grrowl copied and repasted the truncated display text of the link. The full link is 2 comments up: https://github.com/signalapp/Signal-Desktop/commit/9d41b8616296f1b328aa864e0114b99d7f11ca06 https://github.com/signalapp/Signal-Desktop/commit/9d41b8616...

komuW 8y ago

Is there a linter that can catch this kind of mistakes?

charleslmunger 8y ago

> The issue was using innerHTML (or rather $.html()) with strings concatenated together from user input. Something you should never do. Could as well just call eval() directly on it, or pass the input to gcc, compile it and run the resulting binary. Yes, but most engineers would look at that last element and say "what on earth is going on here", where $.html() being dangerous is something that engineers who don't usually work on web might not know about. You're right about blaming the DOM spec, but there's no actual reason for signal desktop to interact with that poorly designed spec except that they chose electron as a framework.