mirage/charrua

calling buf_of_pkt with very long lists of options can crash

yomimono opened this issue · 3 comments

yomimono commented

It's possible to call buf_of_pkt on a Dhcp_wire.pkt with an options field containing so many options that the buffer obtained for write is overrun in buf_of_options:

    (Invalid_argument
  "Cstruct.blit_from_string src=[64] dst=[2039,9](2048) dst-off=0 len=64")
    Raised at file "pervasives.ml", line 33, characters 20-45
    Called from file "lib/dhcp_wire.ml", line 841, characters 4-34
    Called from file "list.ml", line 88, characters 24-34
    Called from file "lib/dhcp_wire.ml", line 1036, characters 15-56
    Called from file "lib/dhcp_wire.ml", line 1124, characters 20-60
haesbaert commented

This is known. I think it should not be addressed, the size should be sufficient for any real option list, since this is something that doesn't come from the network (it comes from us), I believe it's ok.

yomimono commented

Let's at least throw a less inscrutable exception if our attempt to write triggers Invalid_argument?

haesbaert commented

I agree, I think we should change the interface and return a result.
Also we could consider using 2k, and if it fails we double, until we reach 10k which would be a jumbo frame.
It's a bit overkill though, I really doubt anyone would ever go >1500 on an dhcp packet.
I've been wanting to change pkt_of_buf, to stop taking the len, that's pretty stupid, we could then hitchike this change together, since it's a big API breakage.

  • 👍1