search

gkdr / libomemo

Implements OMEMO in C.
MIT License
55 stars 16 forks source link

12 byte IV for outgoing messages #24

Closed Neustradamus closed 3 years ago

Neustradamus commented 4 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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

Neustradamus commented 3 years ago

@gkdr: A little list:

@fortysixandtwo: About the XEP-0384, it is to be compatible with all XMPP client, it is v0.3.0.

All people must to update the code to new OMEMO... It must be prepared, but not pushed...

Yuubi-san commented 3 years ago

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 commented 3 years ago

@Yuubi-san 96 bits = 12 bytes :slightly_smiling_face: 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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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 :slightly_smiling_face:

Neustradamus commented 3 years ago

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

It is solved in Alpine and Debian ;)

fortysixandtwo commented 3 years ago

Hey @gkdr,

I would be happier with a patch supporting both 12 and 16 byte IVs. See also: https://github.com/xsf/xeps/pull/894#issuecomment-596255509 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 commented 3 years ago

@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 commented 3 years ago

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

Neustradamus commented 3 years ago

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. -> https://github.com/gkdr/libomemo/issues/26

Thanks in advance.

Neustradamus commented 3 years ago

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

languitar commented 3 years ago

@languitar: Can you add this patch in Arch?

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

Neustradamus commented 3 years ago

@languitar: Thanks :)

Neustradamus commented 3 years ago

@leahneukirchen: Thanks :)

Now it is done in VoidLinux too:

rmader commented 3 years ago

Maintainer of the fedora copr for pidgin-lurch here. Just wanted to drop that I personally find it rather awkward to downstream-patch a crypto library. Even if the change might be good and non-critical, IMO it's simply not maintainers business to adopt changes they might not fully understand, especially if upstream is aware of the issue and actively participating in the conversation. It's bad style - IMO.

Thus I personally will wait for @gkdr to release an updated version when seen fit.

Neustradamus commented 3 years ago

@rmader: Thanks for your reply :)

All people wait @gkdr since 16th February 2020. But to have a perfect compatibility with all clients, it is needed to add this patch before the merging.

@mar-v-in has done a comment here: https://github.com/gkdr/libomemo/issues/24#issuecomment-734437892 after that Dino 0.2.0 has been released with the fix.

You can see that the problem is not new:

hartwork commented 3 years ago

@gkdr I don't understand all of it but https://github.com/dino/dino/issues/265#issuecomment-617768725 may be of interest with regard to your question whether things are going to break

Neustradamus commented 3 years ago

@hartwork: Thanks for your comment, and after the cited @mar-v-in comment, the change has been officially integrated in Dino 0.2.0 (2020-11-12):

mar-v-in commented 3 years ago

The transition from 16-byte to 12-byte IV is in 3 phases:

  1. Make sure all clients can receive both 12-byte and 16-byte IV, continue to send 16-byte IV
  2. Start sending 12-byte IV, continue to receive 16-byte IV
  3. Optional: Drop support for 16-byte IV.

Legacy clients are compatible with phase 1 clients, phase 1 clients are compatible with phase 2 clients and phase 2 clients are compatible with phase 3 clients. Thus compatibility is ensured as long as all clients stick with each phase for a certain time.

Dino moved to phase 2 in version 0.1.1 after all clients and client versions with significant use were at least phase 1. Some clients moved to phase 2 earlier and some (like libomemo/lurch) did not transition to phase 2 yet. The common agreement is that implementations should be in phase 2 right now, which is fully compatible with phase 1, so that no compatibility issues arise.

The issue is that Monal is already phase 3, against the common agreement, and did so when most clients were still in phase 1. This caused incompatibilities. @Neustradamus likes to make it look like Monal is the "modern" implementation and everyone else lacks behind and thus is incompatible with the de-facto standard. This is not the case. The de-facto standard is phase 2 and Monal is not implementing that, thereby causing incompatibilities when there was a solid plan to do the migration without such incompatibilities.

I guess most clients will never implement the optional phase 3. It is not specified anywhere that you need to do it (strictly speaking, it's not even XEP-0384 0.3.0 compatible) and it's not relevant for security reasons. Depending on your implementation it might make things easier to drop support for 16-byte IV, but for most enforcing 12-byte would actually require more work. At some point we'll have to transition to omemo:1 as specified in XEP-0384 0.4+, making further changes to the "siacs omemo" implementations obsolete.

FWIW, transition to omemo:1 will be possible without compatibility issues for as long as all clients follow a similar transition scheme. @Neustradamus please don't nag people into implementing it before the transition scheme is properly documented and implemented in first test implementations by the authors of XEP-0384. Because your actions are not helpful for ensuring compatibility.

Neustradamus commented 3 years ago

Thanks @mar-v-in: Thanks for your comment but I think that you have created fear to people.

Yes, libomemo has not done the different phases, badly... But it is time to change it to have compatibility with all OMEMO 0.3.0 clients. Better in the master branch of libomemo but before the merging, it is better to have the solution in other place, we wait since several months (16th February 2020).

It is really important to exchange secure encrypted data instead of unencrypted unsecure data. Currently we need to disable OMEMO.

I precise that the good "12 byte random IV" has been specified in a proposal (2018-05-31) by the same original author:

Of course XEP-0384 0.4.0+ has changed ("urn:xmpp:omemo:1"), there is a ticket to do an update in a new branch (example: "omemo1") or a new repository (example: "libomemo-v2") to keep OMEMO 0.3.0 compatibility...

Note: It would have been much easier to release a 0.3.1 version for clarification this point before a 0.4.0 version, I always said it.

hartwork commented 3 years ago

@Neustradamus who is "we" in "[c]urrently we need to disable OMEMO"?

hartwork commented 3 years ago

@mar-v-in thanks for your detailed reply, very nice! :+1:

[..] The common agreement is that implementations should be in phase 2 right now, [..]

I have no doubts about that, yet if you do have some link for a source on that, I think it would helpful be sure to do the right thing.

The issue is that Monal is already phase 3, against the common agreement, and did so when most clients were still in phase 1. This caused incompatibilities.

Are you suggesting that the best option would be to patch Monal back to phase 2? Would that be unrealistic, hard, or break other things?

With regard to PR #27, macro OMEMO_AES_GCM_IV_LENGTH seems only be used for sending but not for receiving. So my impression is — but please verify! — that #27 has the effect of moving libomemo into phase 2 (not phase 3), in terms of the terminology used by @mar-v-in above. @gkdr can you confirm?

mar-v-in commented 3 years ago

@hartwork I'm not sure we have the current transition state written down anywhere. Phase 2 is what pretty much everyone implements. AFAIK, libomemo/lurch is the last to have its latest release version at phase 1, but obviously outdated versions at phase 1 are still actively being used.

The reason for Monal to not implement 16 byte IV is that Apple's crypto library for iOS has no support for it and apps that ship their own implementation of crypto need to register with French authorities to be published in France (stupid lawmakers doing stupid laws). Monal developers decided they don't want to go through this extra work and thus skipped 16-byte IV support, knowing it would cause compatibility issues. It also sped up moving from phase 1 to phase 2, so I don't want to blame anyone for still being phase 1.

It is also my understanding that #27 moves libomemo/lurch from phase 1 to phase 2, which should not cause any compatibility issues as pre-phase-1 clients basically no longer exist and everything else is compatible in phase 2.

hartwork commented 3 years ago

@mar-v-in thanks once more, very informative. Let's see what @gkdr thinks when he finds time.

gkdr commented 3 years ago

@Neustradamus please stop saying that something is broken without concrete examples. this library doesn't do anything wrong in regard to the spec. why do you have to deactivate OMEMO? is it monal you're trying to communicate with? adding the supported version is a good idea.

@fortysixandtwo the change in #27 only concerns the outgoing size. i guess the constant's name is not clear enough, my bad. there has always been read support for any size (that is backed by the used crypto lib - gcrypt in the default case), as i simply get the length as a result from g_base64_decode(). i don't think that letting the end user choose the IV size is really useful, and setting the constant at compile time would not be any more difficult than patching it, assuming it's properly documented.

@mar-v-in thanks a lot for the explanations. i'll definitely go with your suggestion then. i just didn't have time to keep up with which client currently implements what.

@languitar now that i now what's going on, a new release should be very simple. :slightly_smiling_face:

@rmader first of all, thanks for your help :slightly_smiling_face: and yes, i'm a bit confused why there are so many downstream patches without trying to ask the maintainer. to be fair, i didn't really respond to the issue until now, and i hope the participants of this discussion see why a mass-issue-opening about a PR in the XEP repo which was not accepted did not catch my attention. the downstream patches did, though, so thanks for commenting here.

@hartwork thanks for the link. i caught about half of that on some other channels, so i didn't feel confident in making a decision to change anything.

thanks everyone for your help figuring this out.

Neustradamus commented 3 years ago

@ all: Thanks for your comments, this ticket is now in progress after several months.

@gkdr: All clients send with 12-byte IV except all linked to libomemo. When we (Pidgin/libpurple/lurch/libomemo users) speak with a person who uses Monal, we need to disable OMEMO in my client because it is not received... I am not alone to have this problem...

The problem is not new and after a long time I have published this ticket the 16th February 2020 and I have published the PR the 23rd April 2020.

@anurodhp has done an update of the XEP-0384 which has not accepted. Personally, I would have validated a 0.3.1 version (not a 0.4.0) to be clear. But it was not done.

Neustradamus commented 3 years ago

Now the patch is in:

gkdr commented 3 years ago

@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.

hartwork commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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

gkdr commented 3 years ago

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 :slightly_smiling_face:

gkdr commented 3 years ago

merged the pr, so i'm closing this.

Neustradamus commented 3 years ago

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

@ all: There is a 0.7.0 release:

powerman commented 3 years ago

@ 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 commented 3 years ago

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

hartwork commented 3 years ago

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 commented 3 years ago

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

hartwork commented 3 years ago

@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 commented 3 years ago

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

hartwork commented 3 years ago

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

gkdr commented 3 years ago

@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 :grimacing: )

fortysixandtwo commented 3 years ago

Don't worry about it ;)