Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

calling buf_of_pkt with very long lists of options can crash #56

Open
yomimono opened this issue Jun 1, 2017 · 3 comments
Open

calling buf_of_pkt with very long lists of options can crash #56

yomimono opened this issue Jun 1, 2017 · 3 comments

Comments

@yomimono
Copy link
Contributor

yomimono commented Jun 1, 2017

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
Copy link
Member

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
Copy link
Contributor Author

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

@haesbaert
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants