4 ms·

None of this is caused because of them using a dynamic language. It's caused because the developers used a function literally called "dangerouslySetInnerHTML" t

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.

vjeux 8y ago
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.
- deleted 8y ago
  [deleted]
- archgoon 8y ago
  This doesn't seem to be true, here's the v1.10.0 code: https://github.com/signalapp/Signal-Desktop/blob/f6eb745632ce87eabce93d3c12f66f0bc6146d2d/ts/components/conversation/Quote.tsx#L114 https://github.com/signalapp/Signal-Desktop/blob/f6eb745632c... 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): https://github.com/signalapp/Signal-Desktop/blob/0d00fbfb7a23a9b05ea9e0c74910d31a27e710de/ts/components/conversation/Quote.tsx https://github.com/signalapp/Signal-Desktop/blob/0d00fbfb7a2... They do seem to be using jQuery elsewhere in the code base, but I'm not familiar enough to determine how it all fits together.
  - ddtaylor 8y ago
    They are using many frameworks mixed together like jQuery, React, and underscore.
  - vjeux 8y ago
    Oh sorry you are right! I was reading the comment ( https://news.ycombinator.com/item?id=17097501 https://news.ycombinator.com/item?id=17097501 ) which said that the issue was caused with $.html
lnrdgmz 8y ago
Naive question: Are there functions analogous to `dangerouslySetInnerHTML` in non-JS GUI libraries (e.g. Qt) that will allow a similar attack?
- jrockway 8y ago
  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.
  - jhasse 8y ago
    Care to give an example?
    - camgunz 8y ago
      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.
      - jhasse 8y ago
        strcat is C and not C++ though.
        camgunz 8y ago
        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.
    - moefh 8y ago
mtgx 8y ago
I may be wrong, but I think the Signal desktop app was written by like interns or new developers working for Signal. The Android app was written by Moxie, and I think it's the one about which Matthew Green said: After reading the code, I literally discovered a line of drool running down my face. It’s really nice. https://signal.org/ https://signal.org/
- fjsolwmv 8y ago
  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.
- kbwt 8y ago
  The native code (RedPhone remnants?) did not inspire confidence last I looked at it.

vjeux 8y ago

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.

deleted 8y ago
[deleted]
archgoon 8y ago
This doesn't seem to be true, here's the v1.10.0 code: https://github.com/signalapp/Signal-Desktop/blob/f6eb745632ce87eabce93d3c12f66f0bc6146d2d/ts/components/conversation/Quote.tsx#L114 https://github.com/signalapp/Signal-Desktop/blob/f6eb745632c... 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): https://github.com/signalapp/Signal-Desktop/blob/0d00fbfb7a23a9b05ea9e0c74910d31a27e710de/ts/components/conversation/Quote.tsx https://github.com/signalapp/Signal-Desktop/blob/0d00fbfb7a2... They do seem to be using jQuery elsewhere in the code base, but I'm not familiar enough to determine how it all fits together.
- ddtaylor 8y ago
  They are using many frameworks mixed together like jQuery, React, and underscore.
- vjeux 8y ago
  Oh sorry you are right! I was reading the comment ( https://news.ycombinator.com/item?id=17097501 https://news.ycombinator.com/item?id=17097501 ) which said that the issue was caused with $.html

lnrdgmz 8y ago

Naive question: Are there functions analogous to `dangerouslySetInnerHTML` in non-JS GUI libraries (e.g. Qt) that will allow a similar attack?

jrockway 8y ago
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.
- jhasse 8y ago
  Care to give an example?
  - camgunz 8y ago
    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.
    - jhasse 8y ago
      strcat is C and not C++ though.
      - camgunz 8y ago
        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.
  - moefh 8y ago

mtgx 8y ago

I may be wrong, but I think the Signal desktop app was written by like interns or new developers working for Signal. The Android app was written by Moxie, and I think it's the one about which Matthew Green said: After reading the code, I literally discovered a line of drool running down my face. It’s really nice. https://signal.org/ https://signal.org/

fjsolwmv 8y ago
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.
kbwt 8y ago
The native code (RedPhone remnants?) did not inspire confidence last I looked at it.