Maybe secure chat clients shouldn't be written in JavaScript or other languages that have excessive dynamicness? Signal seems to be written mostly in languages that are bad for security (significantly worse than the best alternatives). Maybe I'm just a language nerd without any clue about the trade-offs, but I trust the Wire software more. Note that this just applies to mobile clients and server - Wire, like Signal, chose to build their desktop+webapp in JavaScript :(
As much as I'm not a fan of JavaScript, the problem is not so much the language but rather the choice of Electron and all that comes with it. Heck, even a web version or Chrome app would've successfully mitigated these attacks. Electron means you're one XSS away from remote code execution, and even worse, it makes it way harder to mitigate XSS through CSP (which Signal did utilize, but script-src 'self' can easily be bypassed in Electron).
FWIW, Signal's native mobile apps are written in Java and Objective-C respectively, so there's not really much of a difference compared to Wire (which is a good choice as well). Still, even a hypothetical React Native app written in JavaScript wouldn't be much worse; after all, React Native isn't just a Web View made to look like a native app, but uses actual native components.
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.
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.
(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).
>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...
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>')
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!
These are the kinds of things that make tinfoil hats wonder if any agencies interested in subverting encryption "plant" employees. It's difficult to differentiate stupidity from malice. We know they use shell companies and subvert device manufacturers and standards, so it's not unfathomable.
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?
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.
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.
> 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.
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 ;)
> 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.
Sure, you can write secure apps in Electron, just like you can do risky stuff in real-life and be fine most of the time. But why take the risk?
Had the app been written with a native language and SDK, they wouldn’t need to worry about escaping or anything. I have yet to hear about getting remote code execution for dumping text into an UILabel or similar, while XSS happens almost every day.
>Electron means you're one XSS away from remote code execution, and even worse, it makes it way harder to mitigate XSS through CSP (which Signal did utilize, but script-src 'self' can easily be bypassed in Electron).
There's a bit of an explanation of this in the article describing the other XSS that's recently been found in Signal[1]. Basically, since the Electron app itself runs under the file:// origin, 'self' can be bypassed with varying degrees of difficulty depending on the platform. On Windows, it's trivial because you can use UNC paths to a SMB share containing a malicious JavaScript file (i.e. file://1.2.3.4/payload.js). On other platforms, you'd need to find a way to place the file on a path accessible via file:// first, for example by sending the file via Signal itself and hoping the user accepts the download.
There are ways to lock down the CSP further to mitigate this, but no one really expects script-src 'self' to be unsafe, especially when it's what their documentation recommends.
None of this is caused because of them using a dynamic language. It's caused because the developers used a function literally called "dangerouslySetInnerHTML" that doesn't escape HTML. That's it. It's just lazy programming.
We called it that way in React in order to call out attention to the fact that it was actually dangerous. React also properly escapes everything else it prints.
The app isn't using React but jQuery, which doesn't have those protections.
They do seem to be using react, and using dangerouslySetInnerHTML. Now that said, I haven't confirmed that this is the code that caused the issue, but it is in the Quotes component, which is referenced in the article.
They seem to have fixed this specific issue a few days ago (v.11.0):
I hope Moxie learned that programming his people/team is as programming his software. This tarnishes Signal, regardless of how good the Android app is.
I mean, in C++ "=" could be called "dangerouslySetAribtraryMemoryLocation" and it would be just as accurate. In native code, even trivial operations like concatenating two strings or setting a variable can cause arbitrary code to execute.
strcat (or, honestly, anything in string.h). strcat assumes its first argument has enough allocated space for the contents of the 2nd argument, and that the 2nd argument is NULL terminated. If either of those assumptions is wrong, strcat will overwrite memory, corrupting either your heap or your stack, both of which can lead to arbitrary code execution. It's laughably easy to do, so easy that even typing the letters `strcat` into your program is forbidden in basically every C/C++ shop.
Nah it's both. C++ was deliberately designed to be a superset of C. It's diverged a little bit, but it's mostly still the case. Or, call it `std::strcat` if you like.
Yes the only problem is that the text markup language happens to include by default a Turing complete network-enabled live-interpreted programming language because 25 years ago someone wanted to write a funny message in the Netscape status bar.
If a presentation layer API (=HTML) provides developers with the convenience of composing UI elements by concatenating markup with remotely sourced input, and at the same time allows inline scripts to be eval'ed when merely present in particular markup attributes, and additionally sometimes hooks up un-sandboxed native APIs with full access to $HOME, it has really laid down the groundwork for a client-side can of worms that makes sql injection and php evals a run for its money.
With the prevalence of XSS and CSRF vulns on the regular sandboxed web, it's pretty brave to take that model into unsandboxed fat clients..?!
It'd probably be useful to distinguish between runtime environments that Javascript is typically encountered (Browsers + Electron) as opposed to the language itself. If it were a language issue, a developer might believe that they could simply switch to a different language, say rust, and compile to WebAssembly and be safe.
However, as you point out, the issue lies in the presentation layer unexpectedly executing code (or receiving inputs from unexpected and untrusted sources). This issue wouldn't be solved by switching to a different language. The core issue here isn't Javascript per se, but the dangerous runtime environments that are browsers and browser approximations (electron) that are designed to execute code from 3rd parties.
A database provides developers with the convenience of composing queries by concatenating strings. You still need to be really incompetent to do so in 2018.
Implying in any way that all JS programs are insecure and all other languages are secure is ridiculous and probably harmful, leading people to insecure choices in other languages. I've seen plenty of security vulnerabilities in strongly typed languages in my days as a software dev
Googled it for you. From their github repo... "To date the OpenPGP.js code base has undergone two complete security audits from Cure53. The first audit's report has been published here." https://github.com/openpgpjs/openpgpjs
I believe parent was asking for secure software written in JS. I'm curious, too. (And examples of insecure software written in C suprise no one, do they?)
JavaScript may have many problems, but I don't really think security is one of them. In a properly isolated sandbox, such as a web browser, it's much more difficult to gain arbitrary code execution privileges than a native desktop app.
