Hello friends, specially @dom96 which I believe is the author of the module. Nice work btw. I'm not sure if I should discuss this here before pushing a patch, or let @dom96 do it himself.
There seems to be a bug on asyncnet.nim module:
proc recvLineInto*(socket: AsyncSocket, resString: ptr string,
flags = {SocketFlag.SafeDisconn}) {.async.}The bug is, no line length checking. Still, the AsyncHttpServer relies on the code to parse http headers. This could be the subject of an easy DoS attach that sent a stream of data with no line delimiters. The fact that the AsyncHttpServer relies on that code is scary. Since recvLine*() uses recvLineInto*() internally too, it has the same problem.
Shouldn't the a max line length be respected and checked ? Or am I missing something ?
Thanks, Howe
Hello @howe:
I think I agree with you...
Disclaimer is that I'm nowhere near the expert that dom96 is, but I've been wanting an excuse to look more closely at how asyncnet / asyncdispatch is implemented for a while, so I took a look at this to see if there was either a built-in length limit, or if I could submit a pull request / bug report.
What I found is that recvLineInto calls readIntoBuf -> readInto, which uses net.BufferSize to size the internal buffer. (Assuming buffered AsyncSocket but AFAICT works similarly for non-buffered). OS-specific details follow, using either WSARecv or net.recv ultimately.
However, recvLineInto does repeatedly loop on calls to readIntoBuf (reading full message body), concatenating to its string with string.add until it receives an end of line termination. Assuming that:
it seems that this would be a memory-leak bug (not buffer overflow), or possible DoS attack as you say above. If you maliciously keep sending data without ever sending a 'r' or 'L' character, it will keep being read into the string.
It would be pretty easy to add a defaultMaxLineLength that it could check against; I'll see about doing that (I've not contributed to Nim core yet), or you can submit an issue + PR if you want to and haven't already.
That does look like a potential memory-leak/DoS attack vector. Nice catch!
Adding a default parameter to the recvLine family of procedures would be the way to go.
That said, recvLineInto is currently inherently unsafe as one of its parameters is a ptr. This procedure will go away soon and be replaced by the following: https://github.com/nim-lang/Nim/blob/def--clean-speedup-2/lib/pure/asyncnet.nim#L310. The reason this was not adopted was because it crashed with the Mark and Sweep GC when a benchmark of the asynchttpserver benchmark was performed. I have made @Araq promise me that we would restore my patch and that he would hopefully fix that GC before the next release.
Just something to keep in mind. I think an issue on Github might be better for now, since I will be editing this code anyway. A PR will cause conflicts later on.
Hello,
That is the kind of DoS I was talking about - deplete of resources, exhaustion of connections. I think we should fix it. @dom96, do you prefer to wait for the compiler fix you mentioned and then fix asyncnet along with the pointer issue, or add the length check right now? Will you fix it or want me (or @perturbation2) to do it? I think adding a stream interface (asyncnet <-> streams) to AsyncSocket would also be a nice add to the lib as well. That way, a file stream could easly be send to a remote client, or read from it. Does that sound useful? I can implement it. Do you think it's on the scope of the lib?
Thanks, Howe