Fix overflow in Counter.add16.

[haesbaert]

I wrote in C since doing the addition with Int64.t and then an unsigned
comparison in ocaml adds a lot of clutter to the code.

As there was precedence in misc.c with caml_nc_count_16_be this seemed like the
right approach.

I'm implementing SSH and it uses the whole 128 bits as the counter in AES-CTR,
so I can expect this to overflow at some point since the initial IV can be
anything.

Maybe there is an easy-peasy-small-cool way to do this in ocaml but my initial attempt added 5-6lines and felt dirty.

[haesbaert]

Don't lose the steam, take a look at this please, I can't really use CTR without it.

[haesbaert]

I'm not sure I was vocal enough on this bug.

The initial counter is random on SSH and the counter is incremented for every packet, never being reset. Which means I can get the overflow on the very first packet.
Please consider this pull request, otherwise CTR is unusable in SSH.

[pqwy]

What is the bug you're seeing here, precisely?

[cfcs]

For other curious readers, the expected behavior on overflow is detailed here: https://tools.ietf.org/html/rfc4344#page-4 (TL;DR: overflow just resets to 0, like a normal unsigned overflow in C).

Speaking of Cipher_block.Modes2.CTR_of: can someone explain why stream is defined twice (before and after let stream_shifted)?

[cfcs]

Anyway - to try to get everyone on the same page I think the behavior @haesbaert is having problems with is this:

let ctr = Cstruct.of_hex "ffffFFFFffffFFFF ffff ffff ffff ffff";;
Cstruct.hexdump ctr;;
- ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff 
Nocrypto.Cipher_block.Counter.add16 ctr 0 1L;;
Cstruct.hexdump ctr;;
- ff ff ff ff ff ff ff ff  00 00 00 00 00 00 00 00 

Which is counter-intuitive (no pun intended), since the homegrown approach to AES-CTR (aka using it with an IV like it's CBC) developed in the RFC linked to above expects it to reset to 00000....
The fact that it doesn't flip the upper bits is also highlighted in the source code (not the .mli, so you have to go digging a bit):

(* FIXME: overflow: higher order bits. *)
let add16 cs i x = add8 cs (i + 8) x

While I cannot read @haesbaert's C code, I believe that is what he tried to address with this PR.

[pqwy]

Thanks so much for an explanation of overflows @cfcs, but my question was directed to @haesbaert.

What neither @haesbaert nor you seem to understand is that the function being fixed here is essentially a utility. Is does not generate the counter that is being encrypted.

This code path does not touch it. It's only when invoked explicitly that it enters the picture.

If not by reading the code, one of the many ways to discover this is:

# let secret = Cstruct.of_string "kickoutthejams!!";;
val secret : Cstruct.t =
  0000:  6b 69 63 6b 6f 75 74 74  68 65 6a 61 6d 73 21 21  kickoutthejams!!

# let ecb = AES.ECB.(encrypt ~key:(of_secret secret) (Cs.of_hex "ffffffffffffffff ffffffffffffffff 0000000000000000 0000000000000000"));;
val ecb : Cstruct.t =
  0000:  45 36 00 32 a9 ac c9 e4  31 d2 34 2f de 24 2a 90  E6.2....1.4/.$*.
  0010:  71 dd 23 39 27 b4 bd 89  51 22 f4 0d a5 57 32 22  q.#9'...Q"...W2"

# let ctr = AES.CTR.(encrypt ~key:(of_secret secret) ~ctr:(Cs.of_hex "ffffffffffffffff ffffffffffffffff") (Cs.of_hex "0000000000000000 0000000000000000 0000000000000000 0000000000000000"));;
val ctr : Cstruct.t =
  0000:  45 36 00 32 a9 ac c9 e4  31 d2 34 2f de 24 2a 90  E6.2....1.4/.$*.
  0010:  71 dd 23 39 27 b4 bd 89  51 22 f4 0d a5 57 32 22  q.#9'...Q"...W2"

# assert (Cstruct.equal ecb ctr) ;;
- : unit = ()

What's left is hopping the counter to chain the messages, which is easily done in a couple of lines of OCaml.

On one hand, actually confirming the issue would have saved almost a year, as

[...] otherwise CTR is unusable in SSH.

is not true. On the other hand, this is the bare minimum of effort that I expect from someone before they start slinging code.

Having said that, the CTR API is an ugly disaster. @haesbaert is not the first one to get burned by it either.

555d2fe tries to fix that core issue. You can now either cleanly step the abstracted counters by an int64, or resynchronize them with a message boundary. That CTR branch in Awa can now look a lot like the CBC branch.

[haesbaert]

The explanation of @cfcs is exactly the issue, the overflow of the lower 64 bits does not carry to the upper 64 bits.

Well while the encrypt function does not change it, I still need the utility function to actually update them as you described. Since I don't expect you to expect users to have to write their own "update counter utility", I'd say that's broken behaviour.

"Hey I don't give you a function to get the next_iv, but I give you this add16 Counter, which you can use to do it, but it's broken. And I never cared because so far every user of it starts with a zero counter for every new packet, like TLS.".

is not true. On the other hand, this is the bare minimum of effort that I expect from someone before they start slinging code.

Sorry, but I couldn't care less about the "bare minimum of effort that you expect from someone before they start slinging the code".

I find it preposterous that you want to get snappy on a pull request from February that fixes a real bug, that you even added a comment indicating it to be a real bug. Where did you get that sense of entitlement ?

Having said that, the CTR API is an ugly disaster. @haesbaert is not the first one to get burned by it either.

I didn't get burned by the API, there is a bug in a utility function which had to be fixed to properly use CTR on SSH, or any other stream protocol that moves the counter forward without ever resetting.

Therefore I stand by my previous statement:
Without the fix, CTR is unusable in SSH.

Having said that, your fix is way nicer and I can use it happily, any chance you can cut a release ?
I'm also using your hmaci instead of hmacv.

From my POV this PR can be dismissed.

[cfcs]

@pqwy No worries, always happy to offer my expertise when it comes to elusive bugs with security implications.
You're right about me failing to understand that it was a broken utility (as opposed to just being a broken - function?) -- I totally missed the piece of documentation that explains how users are supposed to increment the counter.

Anyway, I'm happy you decided to fix your flawed code.