charrua icon indicating copy to clipboard operation
charrua copied to clipboard

calling buf_of_pkt with very long lists of options can crash

Open yomimono opened this issue 8 years ago • 3 comments

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

yomimono avatar Jun 01 '17 00:06 yomimono

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.

haesbaert avatar Jun 12 '17 21:06 haesbaert

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

yomimono avatar Jun 16 '17 21:06 yomimono

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.

haesbaert avatar Jul 05 '17 07:07 haesbaert