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

add defined IV size for GCM. clarified text #894

Closed
wants to merge 1 commit into from
Closed

Conversation

@anurodhp
Copy link

anurodhp commented Feb 15, 2020

OMEMO media sharing specifies the size of the iv to 12 but this XEP does not. In the absence of a recommended size, the only way for someone to implement OMEMO is to reach out to an exiting developer of a client and ask them what size they use. At the moment it is not possible to implement OMEMO based on this XEP alone.

@licaon-kter

This comment has been minimized.

Copy link
Contributor

licaon-kter commented Feb 16, 2020

Typo initialisation in both places.

@anurodhp

This comment has been minimized.

Copy link
Author

anurodhp commented Feb 17, 2020

Typo initialisation in both places.

oops, thanks fixed.

@jubalh

This comment has been minimized.

Copy link
Contributor

jubalh commented Feb 17, 2020

@horazont

This comment has been minimized.

Copy link
Contributor

horazont commented Feb 17, 2020

@strb Please review.

Ri0n added a commit to psi-im/plugins that referenced this pull request Feb 17, 2020
see xsf/xeps#894 for details
@goffi-contrib

This comment has been minimized.

Copy link
Contributor

goffi-contrib commented Feb 18, 2020

Hi, byte is missing a s in several places (12 bytes, 16 bytes).

@horazont

This comment has been minimized.

Copy link
Contributor

horazont commented Feb 20, 2020

@Flowdalic There is no reason to close this.

@Flowdalic

This comment has been minimized.

Copy link
Contributor

Flowdalic commented Feb 20, 2020

I can't remember explicitly closing it. I guess it happened because I've merged a Smack PR which states that it fixes this very issue.

@iNPUTmice

This comment has been minimized.

Copy link
Contributor

iNPUTmice commented Feb 21, 2020

Should be a fairly uncontroversial change.
@anurodhp can you bump the version and add a version change log?
@goffi-contrib I think "12 byte IV" is correct ("12-byte IV" might be slightly more correct?)

@goffi-contrib

This comment has been minimized.

Copy link
Contributor

goffi-contrib commented Feb 21, 2020

@iNPUTmice ah maybe, sorry for the noise then :)

tehnick added a commit to tehnick/haikuports that referenced this pull request Feb 24, 2020
Important changes in upstream:
Changed OMEMO Initialization Vector length to 12
See: xsf/xeps#894
diversys pushed a commit to haikuports/haikuports that referenced this pull request Feb 24, 2020
Important changes in upstream:
Changed OMEMO Initialization Vector length to 12
See: xsf/xeps#894
@mar-v-in

This comment has been minimized.

Copy link
Contributor

mar-v-in commented Feb 28, 2020

In the absence of a recommended size, the only way for someone to implement OMEMO is to reach out to an exiting developer of a client and ask them what size they use. At the moment it is not possible to implement OMEMO based on this XEP alone.

Just wanted to point out that the conclusion here just makes no sense. The fact that the XEP does not specify an IV size doesn't imply it is not possible to implement. For AES GCM, every IV size is valid and the protocol also allows to transfer IVs of arbitrary length (the only restriction is that the base64 encoded IV must be small enough such that the enclosing stanza can actually be transferred). Without any restrictions, a correct implementation of that part of the XEP needs to support every possible IV size incoming and can send any IV size of their choice. While arguably, the XEP misses some parts that are crucial to implement it, the IV size is none of them.

Additionally, I'd like to note that the pull request message complains about the absence of a recommended size, whereas the actual commit adds a required size, without there being a need to require it. The only advantage of 12 byte IVs is a slight performance improvement. In the case of OMEMO the actual length/entropy of the IV doesn't matter at all (from a crypto perspective) given that every encryption uses a fresh, randomly generated key.

By not adding a recommendation but a requirement, this change is not backwards compatible to the previous XEP that allowed sending any IV size. In the past we typically used namespace bumps when doing breaking changes in experimental XEPs, even though there is no requirement to do so according to XEP-0001.

I therefor advocate for a) changing the 12 byte IV to a SHOULD instead of MUST or b) bumping the namespace.

@anurodhp

This comment has been minimized.

Copy link
Author

anurodhp commented Mar 5, 2020

the primary problem with using the unspecified (or in this case 16 byte ) IV is that not all implementations implement the hashing function (notably on iOS but there are probably others) this ambiguity has introduced fragmentation and incompatibility with OMEMO between platforms. At the moment there is no guarantee that any two omemo clients can talk to each other despite both being fully implemented according to the spec. It also requires the implementor to verify their library supports the hash and also increases the scope of testing for a developer.

@anurodhp anurodhp force-pushed the anurodhp:master branch from 0daf623 to 583528a Mar 5, 2020
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Mar 5, 2020

CLA assistant check
All committers have signed the CLA.

@anurodhp

This comment has been minimized.

Copy link
Author

anurodhp commented Mar 5, 2020

@goffi-contrib english is a werid language
@iNPUTmice updated with revision info

@mar-v-in

This comment has been minimized.

Copy link
Contributor

mar-v-in commented Mar 5, 2020

notably on iOS but there are probably others

notably your implementation on iOS does not support 16 byte IV, while others on iOS do and that's why you are pushing for this change to happen, causing fragmentation in the XMPP protocol. You are putting the 12 byte IV mostly so that you can claim it's everyone elses fault to not do 12 byte when in fact it was your fault to not be compatible with the defacto standard of accepting any and sending 16 byte IV.

The change to send 12 byte IV was planned to happen with the next iteration of omemo anyway, which will include a namespace bump solving the issue I mentioned above. If you'd just have waited for another few months, there would be no big issues whatsoever. Now we have a bunch of clients in the wild incompatible with each other but unable to detect due to the devices not being able to announce which IV sizes they support.

@Ppjet6

This comment has been minimized.

Copy link
Member

Ppjet6 commented Mar 5, 2020

Sorry to be the party crasher, but standard discussions should be in the standards list, please, @mar-v-in, @anurodhp.

@mar-v-in

This comment has been minimized.

Copy link
Contributor

mar-v-in commented Mar 5, 2020

Sorry @Ppjet6, but there is no thread regarding this PR on the standards mailing list, so no way to answer there. also I guess: all is said, harm is done, no way back from here.

@strb

This comment has been minimized.

Copy link
Contributor

strb commented Mar 8, 2020

At the OMEMO-focused sprint this past weekend, we've made very good progress on the new version of OMEMO that @mar-v-in mentioned. It will bump the namespace and resolve this issue for good.

As it stands, I don't see a necessity to enforce 12 bytes for the IV at this point. While it's generally desirable to have a mandated size for the IV, this way of simply mandating one, which is different from what's deployed in many implementations, without any migration path whatsoever, is not a great solution. It suddenly pushes clients out of compliance, in a breaking change, through no fault of their own. If anything, we should be mandating support for both variants, and only making a recommendation for 12, for a certain amount of time.

But since the new version of OMEMO is forthcoming, most likely within the next few days, it's not worth going to the trouble IMO.

@Ppjet6

This comment has been minimized.

Copy link
Member

Ppjet6 commented Mar 10, 2020

Closing this as it's been nacked by the author. #903 seems to be an answer to this.

@Ppjet6 Ppjet6 closed this Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.