robur-coop / miragevpn

An opinionated implementation of the OpenVPN protocol
BSD 2-Clause "Simplified" License
79 stars 9 forks source link

Replay window and replay packet id #148

Open reynir opened 1 year ago

reynir commented 1 year ago

The current implementation expects the replay packet id to be always be increasing by one without gap starting at 1. The openvpn-rfc says it is a monotonically increasing sequence. Another part of the rfc draft suggests keeping a sliding window of 32. For tls-crypt-v2 the sequence can start at 0x0f000001 (I believe) to signal early negotiation support (for hmac cookie only for now). The only requirement seems to be that the replay packet id is not zero. https://github.com/robur-coop/miragevpn/blob/35a8d96783695674913ee42379f8384b34d83fe9/src/engine.ml#L807-L824

https://github.com/OpenVPN/openvpn-rfc/blob/bbde60d7f28a5436e21b3f475788f7922e0d9339/openvpn-wire-protocol.xml#L365-L375 https://github.com/OpenVPN/openvpn-rfc/blob/bbde60d7f28a5436e21b3f475788f7922e0d9339/openvpn-wire-protocol.xml#L1057-L1071

hannesm commented 1 year ago

This is related to #53, with the sliding window code suggested by @cfcs in https://github.com/robur-coop/miragevpn/pull/37#issuecomment-607618553

module Sliding_window : sig
  type t
  val make : int -> t
  val add : int -> t -> (t, string) result
  val mem : t -> int -> bool
  val pp : Format.formatter -> t -> unit
end = struct
  type t = { window: int; counter: int; }
  let make counter = { window = 1; counter }
  let add n {window; counter} =
    if n <= counter then begin
      let diff = counter -n in
      if diff >= Sys.int_size
      then Error "n not in sliding window"
      else Ok {window = window lor (1 lsl diff); counter}
    end else begin (* counter > n; always succeeds *)
      let diff = n - counter in
      if diff >= Sys.int_size
      then Ok (make n) (* new window *)
      else Ok {window = (window lsl diff) lor 1; counter = n}
    end

  let pp ppf {window;counter} =
    Fmt.pf ppf "{ window = %d; counter = %d }" window counter

  let mem {window;counter} n =
    let diff = counter - n in
    if counter < n || diff > Sys.int_size
    then false
    else 0 <> window land (1 lsl diff)
end
hannesm commented 1 year ago

For more clarity: as far as I understand the OpenVPN, we should accept any replay id that is larger then the previous one.

Also, the sequence number should be increasing without gaps. Both is not correct in our implementation (but works incredible well).

reynir commented 1 year ago

I think the sliding window implementation is interesting. The default window size (see option --replay-window in openvpn) is 64, so 62 is maybe acceptable. On the other hand they write the following:

          If  you  are using a network link with a large pipeline (meaning
          that the product of bandwidth and latency is high), you may want
          to use a larger value for n. Satellite links in particular often
          require this.

This also depends on the CPU architecture; maybe a window size of 30 is undesirable(?)

hannesm commented 1 year ago

FWIW, since https://github.com/robur-coop/miragevpn/commit/a4b65957a11dda1dc55b13ebd4e7725500f30f84#diff-8df08bfd0d3762c5d7dad8c2f8a61b80c929c7e43e21ec8c228f547ff3e35e25R889 the replay ID does not need to be exactly equal anymore, but greater or equal is fine.