gh-libtom/libtomcrypt

Watch

Fork 0

OCB3 is not according to RFC7253 #256

Merged

sjaeckel merged 8 commits from fix/256 into release/1.18.0

2017-08-07 09:03:39 -05:00

sjaeckel commented

2017-08-02 12:11:09 -05:00

(Migrated from github.com)

While documenting OCB3 I realized that the implementation is not according to draft-irtf-cfrg-ocb-03 (which later on became rfc7253) as stated in dfb4ec3b95 but to draft-irtf-cfrg-ocb-02.

Changing the implementation would also require to change the API, therefore I'm unsure if we should patch it:

'at all' - because not much people seem to use it as nobody complained yet
'in the next version' - so we get 1.18.0 out (TBH I'd be fine to even remove it from the release)
'now' - because I don't like to publish draft versions of algorithms

While documenting OCB3 I realized that the implementation is not according to [draft-irtf-cfrg-ocb-03](https://tools.ietf.org/html/draft-irtf-cfrg-ocb-03) (which later on became rfc7253) as stated in dfb4ec3b95c1518f093491c6a1847627deffc282 but to [draft-irtf-cfrg-ocb-02](https://tools.ietf.org/html/draft-irtf-cfrg-ocb-02). Changing the implementation would also require to change the API, therefore I'm unsure if we should patch it: 1. _'at all'_ - because not much people seem to use it as nobody complained yet 1. _'in the next version'_ - so we get 1.18.0 out (TBH I'd be fine to even remove it from the release) 1. _'now'_ - because I don't like to publish draft versions of algorithms

sjaeckel commented

2017-08-02 09:18:03 -05:00

(Migrated from github.com)

My vote goes for 2. + we remove it from v1.18.0 as I'm also not happy with the quality of the code.

My vote goes for _2._ + we remove it from v1.18.0 as I'm also not happy with the quality of the code.

karel-m commented

2017-08-02 09:38:16 -05:00

(Migrated from github.com)

Yes, remove it from 1.18 (or at least disable by default like we do it with
katja).

Considering the fact that there were 7 (seven) drafts before rfc7253, it is
not so important whether our (well, my) code follows draft 2 or 3, we need
to comply with the final rfc.

I can hack on this issue, but I would appretiate if you can post how you
think the API should look like (I have not look into the rfc yet).

I am also open to remove ocb3 completely (for ever) as I am not using it
anymore. But maybe let's first check how much work it will need to fix.

Other question is: do we want to keep both OCB and OCB3 in long term?

Yes, remove it from 1.18 (or at least disable by default like we do it with katja). Considering the fact that there were 7 (seven) drafts before rfc7253, it is not so important whether our (well, my) code follows draft 2 or 3, we need to comply with the final rfc. I can hack on this issue, but I would appretiate if you can post how you think the API should look like (I have not look into the rfc yet). I am also open to remove ocb3 completely (for ever) as I am not using it anymore. But maybe let's first check how much work it will need to fix. Other question is: do we want to keep both OCB and OCB3 in long term?

sjaeckel commented

2017-08-02 10:48:06 -05:00

(Migrated from github.com)

Yes, remove it from 1.18 (or at least disable by default like we do it with
katja).

Okay. I'm still not sure whether we should disable it by default or remove it (c.f. the rest of the reply).

Considering the fact that there were 7 (seven) drafts before rfc7253, it is
not so important whether our (well, my) code follows draft 2 or 3, we need
to comply with the final rfc.

According to http://web.cs.ucdavis.edu/~rogaway/ocb/news/ there was the last algorithmic change from draft-2 to draft-3.

I can hack on this issue, but I would appretiate if you can post how you
think the API should look like (I have not look into the rfc yet).

I am also open to remove ocb3 completely (for ever) as I am not using it
anymore. But maybe let's first check how much work it will need to fix.

The change is minimal (the taglen has to be known on init), c.f. fix/256

Other question is: do we want to keep both OCB and OCB3 in long term?

There's also still this licensing thing with all versions of OCB... http://web.cs.ucdavis.edu/~rogaway/ocb/license.htm

I'd say as ltc is an academics-inspired project we should keep it. (This changed from the first point after I implemented the patch :-) )

> Yes, remove it from 1.18 (or at least disable by default like we do it with > katja). Okay. I'm still not sure whether we should disable it by default or remove it (c.f. the rest of the reply). > Considering the fact that there were 7 (seven) drafts before rfc7253, it is > not so important whether our (well, my) code follows draft 2 or 3, we need > to comply with the final rfc. According to http://web.cs.ucdavis.edu/~rogaway/ocb/news/ there was the last algorithmic change from draft-2 to draft-3. > I can hack on this issue, but I would appretiate if you can post how you > think the API should look like (I have not look into the rfc yet). > > I am also open to remove ocb3 completely (for ever) as I am not using it > anymore. But maybe let's first check how much work it will need to fix. The change is minimal (the taglen has to be known on init), c.f. [fix/256](https://github.com/libtom/libtomcrypt/tree/fix/256) > Other question is: do we want to keep both OCB and OCB3 in long term? There's also still this licensing thing with all versions of OCB... http://web.cs.ucdavis.edu/~rogaway/ocb/license.htm I'd say as ltc is an academics-inspired project we should keep it. (This changed from the first point after I implemented the patch :-) )

fperrad (Migrated from github.com) reviewed

2017-08-02 12:11:09 -05:00

karel-m commented

2017-08-02 15:40:52 -05:00

(Migrated from github.com)

Do we support empty aad (aad==NULL, aadlen==0) and empty plaintext
(pt==NULL, ptlen==0)?

To be honest, I still have not read the RFC so I am not sure if empty
plaintext is legal here.

I'll be able to do the review no sooner than during the coming weekend.

Do we support empty aad (aad==NULL, aadlen==0) and empty plaintext (pt==NULL, ptlen==0)? To be honest, I still have not read the RFC so I am not sure if empty plaintext is legal here. I'll be able to do the review no sooner than during the coming weekend.

karel-m commented

2017-08-02 16:01:39 -05:00

(Migrated from github.com)

We should perhaps add all test vectors from RFC 7253 - Appendix A, some of
them test empty pt/aad that I've mentioned above.

We should perhaps add all test vectors from RFC 7253 - Appendix A, some of them test empty pt/aad that I've mentioned above.

sjaeckel commented

2017-08-03 06:23:10 -05:00

(Migrated from github.com)

Do we support empty aad (aad==NULL, aadlen==0)

done in b2448c593a

and empty plaintext (pt==NULL, ptlen==0)?

done in 868c5a82c3

We should perhaps add all test vectors from RFC 7253 - Appendix A, some of them test empty pt/aad that I've mentioned above.

ours do that as well, I added the test of the RFC with the 96bit tag
I don't think it's worth the effort adding all the tests from the RFC...

> Do we support empty aad (aad==NULL, aadlen==0) done in b2448c593a93524c26925d384def7edc6715c623 > and empty plaintext (pt==NULL, ptlen==0)? done in 868c5a82c31c816dc39a1ea7abed7f979d7e0bd6 > We should perhaps add all test vectors from RFC 7253 - Appendix A, some of them test empty pt/aad that I've mentioned above. ours do that as well, I added the test of the RFC with the 96bit tag I don't think it's worth the effort adding all the tests from the RFC...

karel-m (Migrated from github.com) approved these changes

2017-08-06 16:08:05 -05:00

karel-m (Migrated from github.com) left a comment

👍 please merge

karel-m commented

2017-08-07 09:14:17 -05:00

(Migrated from github.com)

I have one more ocb3_done related issue which I've found out when switching my perl bindings to the new OCB3 API.

I'll send it as a separate PR today evening (just want you to know if you plan to release the next RC).

I have one more `ocb3_done` related issue which I've found out when switching my perl bindings to the new OCB3 API. I'll send it as a separate PR today evening (just want you to know if you plan to release the next RC).