Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

12 byte IV for outgoing messages #24

Closed
Neustradamus opened this issue Feb 16, 2020 · 55 comments
Closed

12 byte IV for outgoing messages #24

Neustradamus opened this issue Feb 16, 2020 · 55 comments

Comments

@Neustradamus
Copy link
Contributor

Neustradamus commented Feb 16, 2020

The XEP-0384 0.3.0 does not specify the "IV" value and the 0.3.1 version has been missing before the 0.4.0 version.
All OMEMO 0.3.0 clients must send with 12-byte instead 16-byte IV.

This issues is solved in #27.

More informations:

Already good:

Linked to:

Neustradamus added a commit to Neustradamus/libomemo that referenced this issue Apr 22, 2020
@Neustradamus
Copy link
Contributor Author

@gkdr: After Dino 0.2.0 released the 12th November 2020, it is time to see for libomemo, no?

@craftyguy: What do you think?

Linked to:

@craftyguy
Copy link

It seems to work, I haven't done a ton of testing, but I've been able to communicate with folks who are on Conversations/Android.

@gkdr
Copy link
Owner

gkdr commented Nov 14, 2020

one of the last comments in the thread you linked says that the IV size will not be enforced for the omemo version this implements. what is the exact problem? which clients are incompatible leaving it as it is? which clients are incompatible after the change?

@Neustradamus
Copy link
Contributor Author

Neustradamus commented Nov 14, 2020

@gkdr: It is neccessary to have this in all OMEMO clients based on XEP-0384 v0.3.0 before the major changes in v0.4.0+.

It is been solved in Dino 0.2.0, few days ago.

The last historical XMPP client which does not support the good value is Pidgin with this libomemo...

@gkdr
Copy link
Owner

gkdr commented Nov 16, 2020

@Neustradamus can you show me where the IV length is specified? the thread you linked seems says the opposite - no IV length should be enforced. do you know about chatsecure specifically?

@fortysixandtwo
Copy link
Contributor

After reading through the xeps PR I'm a bit confused.
Especially after reading this comment.
xsf/xeps#894 (comment)
@Neustradamus FYI: I have prepared a new Debian package,
but now I'm not entirely sure whether to proceed with the upload.

@Yuubi-san
Copy link

Yuubi-san commented Nov 17, 2020

Not sure how XEP versioning works, but I doubt there will be 0.3.1 clarifying the IV wording, and 0.4 (omemo:1) switched to a different encryption mode and key size altogether.
Since 0.3 implemenation is still very relevant, I think the sane thing to do is to follow what major clients are already doing: accept both IV sizes common in the wild, and send out only 16-byte ones.
To be pedantically on-spec, it seems to me everyone should've been accepting any IV size right from the start. That's not realistic, though. Actually, I hear some crypto libraries support only 12-byte IVs.
I'm not sure where the 16-byte IV usage could have even come from, as the recommended GCM IV size seems to be 96. I think https://github.com/iNPUTmice/Conversations/issues/2578 explains it pretty well.

@gkdr
Copy link
Owner

gkdr commented Nov 17, 2020

@Yuubi-san 96 bits = 12 bytes 🙂 the 16 bytes were set by Conversations in the beginning and everyone just tried to be compatible as the spec basically did not exist. it's probably some weird android default thing. aside from that, i completely agree - staying 'on-spec' is no use if it's incompatible with major clients, especially if this is a thing that's not even defined to begin with.

what i want to know, and am trying desperately to get out of @Neustradamus who is pushing this change - if I change it to 12 byte IVs, which clients break? so far i didn't get a straight answer is all. or is the list supposed to mean that all of them are fine with it? changing the constant is not a big deal, i just want to make sure nothing breaks.

ps: iirc i just get the length of the IV i was given and set that as the size to give to the crypto lib, so this lib is compatible with any received length. (this assumes the lib will tell me if the length is invalid.) the sent out IVs are the only issue.

@Yuubi-san
Copy link

Yuubi-san commented Nov 17, 2020

@gkdr

96 bits = 12 bytes 🙂

Of course, that's what I meant, but failed to specify the units. :)

it's probably some weird android default thing.

Could be the API weirdness, yeah. Despite the implementation special-casing 12 as the simple code path: https://android.googlesource.com/platform/external/bouncycastle/+/d3dea7fcf0e206859d8df1af01523f40d8008195/bcprov/src/main/java/org/bouncycastle/crypto/modes/GCMBlockCipher.java#150

is the list supposed to mean that all of them are fine with it?

I think so, but see below.

iirc i just get the length of the IV i was given and set that as the size to give to the crypto lib, so this lib is compatible with any received length. (this assumes the lib will tell me if the length is invalid.) the sent out IVs are the only issue.

Sooo... then it seems you're already doing what others do. I dont think anyone is sending 12-byte IVs as of now. I could be wrong, though, but then I'd say it's their fault for rushing the transition (if it'll happen at all at some point).

BTW, are you also handling (the fragment part of) the aesgcm URI scheme in this lib?

@Neustradamus
Copy link
Contributor Author

Hi all, sorry for the delay, I was blocked by GitHub.

Now the patch is in:

What's next?

@languitar: Can you add this patch in Arch?

@gkdr: Not possible to merge it directly, better than a patch, no?
In the same time, please look other OS patches.

Note: Debian 11 freeze is soon, it will be nice to have a best version before Debian 12 in 2023.

@gkdr
Copy link
Owner

gkdr commented Nov 25, 2020

@Yuubi-san monal did only 12 byte IV because that's the only thing the ios standard lib supports afaik. that triggered it all. so this is not compatible with monal, but i don't know what other compatibilities this lib will lose.

@Neustradamus i'm not saying it's impossible to merge it, i just don't have the time to hunt down omemo implementation specifics. will clients break if i do this?

@craftyguy and @fortysixandtwo: what made you patch that?

i just want to know why to make this change is all, i'm not against it 🙂

@Neustradamus
Copy link
Contributor Author

@gkdr: You do not understand, currently, libomemo is broken without this patch.

It is solved in Alpine and Debian ;)

@fortysixandtwo
Copy link
Contributor

fortysixandtwo commented Nov 26, 2020

Hey @gkdr,

I would be happier with a patch supporting both 12 and 16 byte IVs. See also:
xsf/xeps#894 (comment)
Especially:
If anything, we should be mandating support for both variants, and only making a recommendation for 12, for a certain mount of time.

Having this said what made me patch was the great number of other clients which apparently
included similar patches.

And btw the new XEP 0384 mandates 16byte IVs.
https://github.com/xsf/xeps/pull/903/files#diff-60576967ad6e70447a97c8fd764446c67a5c4730fda2b093e195144902ad0244R299
Divide the HKDF output into a 32-byte encryption key, a 32-byte authentication key and a 16 byte IV.

@mar-v-in
Copy link

mar-v-in commented Nov 26, 2020

@gkdr My suggestion would be to send 12-byte IV when aiming for compatibility with "siacs" OMEMO (XEP-0384 0.3.0). Large majority of clients support receiving 12-byte IV (all except not-latest version of some clients AFAIK) and it is required for Monal. All other major clients can receive 16-byte IV, which is why you don't see significant issues when sending 16-byte.

When receiving, you can virtually allow every size your library supports - which seems to be what you are already doing. Most clients send 12-byte IV but some send 16-byte IV. The IV is not required for any security property of OMEMO (as OMEMO never uses the same key for AES encryption), using 12- or 16-byte IV thus doesn't matter at all (except for compatibility with Monal).

@fortysixandtwo The OMEMO specified in XEP-0384 0.4 and later versions is largely incompatible with "siacs" OMEMO except that keys/trust can be transferred and, as of today and as far as I am aware, no client implements it in production. The IVs used there are not relevant for "siacs" OMEMO (also they are used as an input for AES-CBC which needs to be 16 byte whereas AES-GCM as used in "siacs" OMEMO can handle any size of IV).

@selurvedu
Copy link

@mar-v-in Huge thanks for clarifying this! Now it all starts to make sense.

@Neustradamus
Copy link
Contributor Author

Thanks @mar-v-in :)

@gkdr: Please add in the README the 0.3.0 version near XEP-0384:
Implements OMEMO (XEP-0384 0.3.0) in C.
-> #26

Thanks in advance.

@Neustradamus
Copy link
Contributor Author

Note: I have done same for libpurple-omemo-plugin:

@languitar
Copy link

@languitar: Can you add this patch in Arch?

I've just pushed that patch. Any chance to get a new release here?

@Neustradamus
Copy link
Contributor Author

@Neustradamus
Copy link
Contributor Author

@leahneukirchen: Thanks :)

Now it is done in VoidLinux too:

@gkdr
Copy link
Owner

gkdr commented Nov 29, 2020

@Neustradamus yes, and it was the Monal client's choice to be incompatible to the other clients (even though i kinda understand their reasoning). no where does it say that sending a 12 byte IV is "correct". please stop pushing downstream changes, it makes the whole situation even more confusing.

@gkdr gkdr changed the title A correct support of OMEMO: 12 byte for initilization vector (IV) 12 byte IV for outgoing messages Nov 29, 2020
@hartwork
Copy link
Contributor

hartwork commented Nov 29, 2020

For the record, I packaged lurch for Gentoo and refused to patch downstream prior to better understanding the situation myself and a decision made upstream.

@rmader
Copy link
Contributor

rmader commented Nov 29, 2020

@Neustradamus: just to be clear: I appreciate your energy and effort in general, thanks for that.

As far as I can see, the issue felt through the cracks - that can happen, especially as the initial report and the PR (#27) were - well, not very informative or obviously actionable. Thus the argument of "this has been known for month" is a bit mood - the second post is 17 days ago and 15 days ago it caught @gkdr s attention.

So, again, thanks for the initiative, but for the next time: please first try to catch upstream attention more aggressively before contacting package maintainers. If upstream is inactive and not reachable for a long time - well, in case of a crypto library that would scream for a fork or drop altogether. So really, downstream patches should only be used as a last resort for dead projects that might still be in use - IMO :)

Neustradamus added a commit to Neustradamus/libomemo that referenced this issue Nov 30, 2020
The XEP-0384 0.3.0 does not specify the "IV" value and the 0.3.1 version
 has been missing before the 0.4.0 version.
All OMEMO 0.3.0 clients must send with 12-byte instead 16-byte IV.

This PR solves this problem, linked to gkdr#24.
@Neustradamus
Copy link
Contributor Author

@gkdr: I have updated my description here and my PR like your have requested me.

@gkdr
Copy link
Owner

gkdr commented Dec 2, 2020

i didn't mean to say that i don't understand why someone would patch this downstream - if monal compatibility is important, it's certainly valid. thank you guys for maintaining this.

@Neustradamus thanks for putting effort into this as well. i didn't mean to be dismissive, please just be careful with words like "correct" or "incorrect" in the future unless you have very good arguments, or be prepared for discussions 🙂

gkdr pushed a commit that referenced this issue Dec 2, 2020
The XEP-0384 0.3.0 does not specify the "IV" value and the 0.3.1 version
 has been missing before the 0.4.0 version.
All OMEMO 0.3.0 clients must send with 12-byte instead 16-byte IV.

This PR solves this problem, linked to #24.
@gkdr
Copy link
Owner

gkdr commented Dec 2, 2020

merged the pr, so i'm closing this.

@gkdr gkdr closed this as completed Dec 2, 2020
@Neustradamus
Copy link
Contributor Author

@gkdr: Thanks for merging in "dev", in "master" and for 0.7.0 build.

@ all: There is a 0.7.0 release:

@powerman
Copy link

powerman commented Dec 3, 2020

@ all: There is a 0.7.0 release:

Thanks! I'm subscribed to "releases" here, but didn't get a notification from GitHub about 0.7.0 because it's not a "real release" from GitHub's point of view - i.e. @gkdr didn't clicked a button "make a release" from 0.7.0 tag. To me this GitHub's behaviour looks quite annoying, but there are ways to automate this (like using GitHub Actions to make releases from some tags).

@gkdr
Copy link
Owner

gkdr commented Dec 3, 2020

@powerman oh thanks, that's good to know. i just created a real release.

gentoo-bot pushed a commit to gentoo/gentoo that referenced this issue Dec 3, 2020
This fixes communication with users of iOS Monal
Related: gkdr/libomemo#24
Signed-off-by: Sebastian Pipping <sping@gentoo.org>
Package-Manager: Portage-3.0.0, Repoman-2.3.23
@hartwork
Copy link
Contributor

hartwork commented Dec 3, 2020

I have updated the copy of libomemo bundled by lurch in Gentoo now. It would be nice, if these two projects would communicate using shared libraries and be more independent. The current approach of linking against .a files is rather unusual and not ideal. Thanks in advance.

@agx
Copy link

agx commented Dec 3, 2020

@hartwork debian uses libomemo as shared lib. https://salsa.debian.org/DebianOnMobile-team/libomemo / https://salsa.debian.org/DebianOnMobile-team/purple-lurch

@hartwork
Copy link
Contributor

hartwork commented Dec 3, 2020

@hartwork debian uses libomemo as shared lib. https://salsa.debian.org/DebianOnMobile-team/libomemo / https://salsa.debian.org/DebianOnMobile-team/purple-lurch

Hi @agx I checked the two links: The only shared lib I see is lurch itself. Also, downstream patching is ideally zero so shared libs is something that is done once and for all upstream, ideally. Am I missing something?

@fortysixandtwo
Copy link
Contributor

@hartwork Relevant upstream PRs: https://github.com/gkdr/lurch/pull/151/files
gkdr/axc#17
#30

@hartwork
Copy link
Contributor

hartwork commented Dec 3, 2020

@fortysixandtwo those are probably all nice, but I'd need things on master and in releases. I upvoted the two open ones now.

@gkdr
Copy link
Owner

gkdr commented Dec 4, 2020

@hartwork i divided the project into submodules just for development and clear responsibilities, and to be honest i never thought any of this would end up in package repos (or even used at all). since git submodules are not ideal for packaging and github also ignores them for the autogenerated tarballs i also provide a tarball of all the code bundled together. however, no one seems to use those. which in turn influenced me to not release the plugin itself for every submodule change, but whatever. also, for a long time the core component libsignal-protocol-c (or libaxolotl when i started) was (iirc intentionally) not available from package repos, and for windows there is still no better way than to link it together since there is no windows build of it.

@fortysixandtwo i am terribly sorry. with the little time i can put into this right now, i just keep losing track of things. plus github deleted old notifications on me by itself or something? i am logged in almost every day though since i use this account at my workplace, so please don't hesitate to ping me if something else comes up. thanks for your contribution. (@hartwork i don't get notified from reacts. is there a way to turn that on?)

(i usually used to work on this during my commute by train, but can probably imagine that's not happening currently 😬 )

@fortysixandtwo
Copy link
Contributor

Don't worry about it ;)

@hartwork
Copy link
Contributor

hartwork commented Dec 4, 2020

@gkpr I don't think GitHub reaction notification is possible. It would probably very noisy.
I'd be happy to help with review of issues and pull request a bit if we could do a voice/video call in English or German to coordinate so that I don't waste time going in any wrong direction or dead ends, as that would be very frustrating. If that sounds fair and interesting, please drop me a mail at firstname@lastname.org with my name filled in. Now or later, as you like and if you like.

@tehnick
Copy link

tehnick commented Dec 18, 2020

@gkdr
As for builds for MS Windows, have in mind that there are special package managers for preparing of build environments. For example, here are libsignal-protocol-c package in a most popular ones:
https://github.com/mxe/mxe/blob/master/src/libsignal-protocol-c.mk
https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-libsignal-protocol-c/PKGBUILD

Also it is not hard to add package to vcpkg when needed.

BTW, in MXE even libomemo package exist. But it should be updated though.

@Neustradamus
Copy link
Contributor Author

@tehnick: About libomemo and MXE, there is a ticket:

@Neustradamus
Copy link
Contributor Author

Some comments are hidden by default, I recall to all that:

Alpine, Debian (+ PureOS), Gentoo, Alt Linux are already perfect.

Some OS have only one package instead of 3:

  • axc
  • libomemo
  • lurch

Some OS maintainers have not updated packages yet -> Compatibility problem, time to update.

In more, there is a problem with the lurch package name, never same name:

@hartwork
Copy link
Contributor

hartwork commented Jan 14, 2021

Some OS have only one package instead of 3:

  • axc
  • libomemo
  • lurch

Please note that before bothj axc and libomemo start building shared libraries (.so files) — libomemo seems done as of 0.7.0 but axc seems todo as of gkdr/axc#17 — , providing three packages is of little extra value because you'd still need to recompile lurch when one of the other two updated because of static linking. So that needs to be improved upstream before it can really improve downstream. (I'm happy to join working on that upstream if a tight feedback loop is guaranteed.)

In more, there is a problem with the lurch package name, never same name:

Distributions have different naming schemes plus at least some follow the upstream naming as far as technically possible. The upstream name is "lurch" and not "pidgin-lurch" hence I went for "lurch" in "Gentoo" initially. Name x11-plugins/pidgin-lurch would actually fit the Gentoo naming scheme better but a rename has costs to users and developers and it would still not be in line with the other names you quote above.
So let's maybe focus on renaming things that are so badly named that they impose a practical problem with regard to users of that very Linux distrubution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests