4 ms·

Can you provide a link to the commit that fixes it? Shouldn't there have been an announcement to inform users what has been leaked and under which circumstance

by TekMol 5y ago

Can you provide a link to the commit that fixes it? Shouldn't there have been an announcement to inform users what has been leaked and under which circumstances? How can user A send an image to user B that neither of them took? Isn't everything end-2-end encrypted? Then how can unencrypted data from user C end up on the device of user B?

agilob 5y ago
No PR with name that would suggest the fix in the client https://github.com/signalapp/Signal-Android/pulls?q=is%3Apr+author%3Agreyson-signal+is%3Aclosed https://github.com/signalapp/Signal-Android/pulls?q=is%3Apr+... and no PR from OP in the opensource part of the server https://github.com/signalapp/Signal-Server/pulls?q=is%3Apr+is%3Aclosed+author%3Agreyson-signal https://github.com/signalapp/Signal-Server/pulls?q=is%3Apr+i...
- deleted 5y ago
  [deleted]
- seg_lol 5y ago
  *edit, I think this issue was specific to the Android client, the desktop client has a totally different sqlite schema. The child comment to your comment is deleted, but I think autoincrement IDs shouldn't be used under an ambient authority context. It would make more sense to have IDs based on an LSF or Feistel sequence, perhaps split into a master ID and a conversation sequence. Autoincrement on this field makes it easy for off by one errors. Even just moving to guids and maintaining a proper parent child relationship would have prevented this. Or maybe there should be a database per conversation (set of all parties). https://github.com/signalapp/Signal-Android/commit/83086a5a2b8cae3ac40dea75d8c9533457a31858 https://github.com/signalapp/Signal-Android/commit/83086a5a2... https://github.com/signalapp/Signal-Android/commit/b9657208fea1c7bcfb90b7400037a7858ba56516 https://github.com/signalapp/Signal-Android/commit/b9657208f... Row IDs shouldn't have so much power.
  - WesolyKubeczek 5y ago
    The good thing about autoincrementing unsigned 64-bit integers is that 1) it's insanely fast, 2) SQLite is doing it automatically, 3) seriously, why don't they have it on all tables yet? SQLite guarantees no collisions within a table. Doing homegrown ID generation is how such bugs are being introduced in the first place. Your application level trigger got bypassed, oops. Your check was experimentally disabled and left like that for a year until people started noticing, oops. If you make an SQLite-backed application and it has a bug like this, I can safely bet $500 it's not going to be SQLite that has the bug. Just don't expose the numeric IDs in links, and generate your UUIDs in tables where you need to point to rows in an outside-accessible link. This is database 101, for crying out loud.
tomudding 5y ago
> Can you provide a link to the commit that fixes it? If I understand the issue [0] correctly, these two commits should be the fix: https://github.com/signalapp/Signal-Android/commit/e90fa05d608d49759e4cbac8cf233a797e0ee395 https://github.com/signalapp/Signal-Android/commit/e90fa05d6... https://github.com/signalapp/Signal-Android/commit/b9657208fea1c7bcfb90b7400037a7858ba56516 https://github.com/signalapp/Signal-Android/commit/b9657208f... The former updates how recipients (or really threads, I suppose) are merged (the issue occurred when trimming threads) and the latter changes the way how thread ids are generated (now automatically incremented). Together they should prevent unrelated recipients (threads) from being merged. [0]: https://github.com/signalapp/Signal-Android/issues/10247#issuecomment-886239978 https://github.com/signalapp/Signal-Android/issues/10247#iss...
- deleted 5y ago
  [deleted]
- winrid 5y ago
  I love the terrible commit name. "Updating recipient merging."
- hackinthebochs 5y ago
  Using incrementing ids as your source of ownership is just asking for trouble. This just means a programming error can have a high probability of ids lining up and leaking resources. Guids make this practically impossible.
  - rattray 5y ago
    Interesting, I hadn't thought of that advantage of uuids/guids before.
  - fulafel 5y ago
    I wonder if there's any widely implemented programming pattern that would catch this better, eg consisting of concatenated type code and id. Using GUIDs here would still hide the bug, not flag an error.
Popegaf 5y ago
From the explanation [0], this looks like the commit [1]. I do have to say though, the commit messages are very barren and barely any reference an issue directly. My guess is that they develop on private branches with an internal issue tracker and decide not to reference issues as that would be confusing. It would help a bunch if they referenced the public issues and made it clear which issues they're working on. They don't seem to use Github Projects [2]. All in all, the communication / transparency of the project is lacking - even though I'm glad they're providing something usable. Hopefully Matrix will be able to provide something as easily usable as well. [0]: https://github.com/signalapp/Signal-Android/issues/10247#issuecomment-886239978 https://github.com/signalapp/Signal-Android/issues/10247#iss... [1]: https://github.com/signalapp/Signal-Android/commit/83086a5a2b8cae3ac40dea75d8c9533457a31858 https://github.com/signalapp/Signal-Android/commit/83086a5a2... [2]: https://github.com/signalapp/Signal-Android/projects https://github.com/signalapp/Signal-Android/projects
konschubert 5y ago
If it’s a client bug that just switches out recipients, then the messages will just get encrypted for the wrong recipient.

by TekMol 5y ago

agilob 5y ago

No PR with name that would suggest the fix in the client https://github.com/signalapp/Signal-Android/pulls?q=is%3Apr+author%3Agreyson-signal+is%3Aclosed https://github.com/signalapp/Signal-Android/pulls?q=is%3Apr+... and no PR from OP in the opensource part of the server https://github.com/signalapp/Signal-Server/pulls?q=is%3Apr+is%3Aclosed+author%3Agreyson-signal https://github.com/signalapp/Signal-Server/pulls?q=is%3Apr+i...

deleted 5y ago
[deleted]
seg_lol 5y ago
*edit, I think this issue was specific to the Android client, the desktop client has a totally different sqlite schema. The child comment to your comment is deleted, but I think autoincrement IDs shouldn't be used under an ambient authority context. It would make more sense to have IDs based on an LSF or Feistel sequence, perhaps split into a master ID and a conversation sequence. Autoincrement on this field makes it easy for off by one errors. Even just moving to guids and maintaining a proper parent child relationship would have prevented this. Or maybe there should be a database per conversation (set of all parties). https://github.com/signalapp/Signal-Android/commit/83086a5a2b8cae3ac40dea75d8c9533457a31858 https://github.com/signalapp/Signal-Android/commit/83086a5a2... https://github.com/signalapp/Signal-Android/commit/b9657208fea1c7bcfb90b7400037a7858ba56516 https://github.com/signalapp/Signal-Android/commit/b9657208f... Row IDs shouldn't have so much power.
- WesolyKubeczek 5y ago
  The good thing about autoincrementing unsigned 64-bit integers is that 1) it's insanely fast, 2) SQLite is doing it automatically, 3) seriously, why don't they have it on all tables yet? SQLite guarantees no collisions within a table. Doing homegrown ID generation is how such bugs are being introduced in the first place. Your application level trigger got bypassed, oops. Your check was experimentally disabled and left like that for a year until people started noticing, oops. If you make an SQLite-backed application and it has a bug like this, I can safely bet $500 it's not going to be SQLite that has the bug. Just don't expose the numeric IDs in links, and generate your UUIDs in tables where you need to point to rows in an outside-accessible link. This is database 101, for crying out loud.

tomudding 5y ago

> Can you provide a link to the commit that fixes it? If I understand the issue [0] correctly, these two commits should be the fix: https://github.com/signalapp/Signal-Android/commit/e90fa05d608d49759e4cbac8cf233a797e0ee395 https://github.com/signalapp/Signal-Android/commit/e90fa05d6... https://github.com/signalapp/Signal-Android/commit/b9657208fea1c7bcfb90b7400037a7858ba56516 https://github.com/signalapp/Signal-Android/commit/b9657208f... The former updates how recipients (or really threads, I suppose) are merged (the issue occurred when trimming threads) and the latter changes the way how thread ids are generated (now automatically incremented). Together they should prevent unrelated recipients (threads) from being merged. [0]: https://github.com/signalapp/Signal-Android/issues/10247#issuecomment-886239978 https://github.com/signalapp/Signal-Android/issues/10247#iss...

deleted 5y ago
[deleted]
winrid 5y ago
I love the terrible commit name. "Updating recipient merging."
hackinthebochs 5y ago
Using incrementing ids as your source of ownership is just asking for trouble. This just means a programming error can have a high probability of ids lining up and leaking resources. Guids make this practically impossible.
- rattray 5y ago
  Interesting, I hadn't thought of that advantage of uuids/guids before.
- fulafel 5y ago
  I wonder if there's any widely implemented programming pattern that would catch this better, eg consisting of concatenated type code and id. Using GUIDs here would still hide the bug, not flag an error.

Popegaf 5y ago

From the explanation [0], this looks like the commit [1]. I do have to say though, the commit messages are very barren and barely any reference an issue directly. My guess is that they develop on private branches with an internal issue tracker and decide not to reference issues as that would be confusing. It would help a bunch if they referenced the public issues and made it clear which issues they're working on. They don't seem to use Github Projects [2]. All in all, the communication / transparency of the project is lacking - even though I'm glad they're providing something usable. Hopefully Matrix will be able to provide something as easily usable as well. [0]: https://github.com/signalapp/Signal-Android/issues/10247#issuecomment-886239978 https://github.com/signalapp/Signal-Android/issues/10247#iss... [1]: https://github.com/signalapp/Signal-Android/commit/83086a5a2b8cae3ac40dea75d8c9533457a31858 https://github.com/signalapp/Signal-Android/commit/83086a5a2... [2]: https://github.com/signalapp/Signal-Android/projects https://github.com/signalapp/Signal-Android/projects

konschubert 5y ago

If it’s a client bug that just switches out recipients, then the messages will just get encrypted for the wrong recipient.