Dear Daniel, comments in-line.
Best regards Michael On Jul 10, 2009, at 1:19 PM, Daniel Mentz via RT wrote:
Dear Michael, I've got some concerns regarding your patch: Michael Tuexen via RT wrote:I have looked at the patch provided by Daniel. All suggested changes areOK, but there are two additional things which should be fixed: 1. In ssl3_read_n() the argument max is overwritten before used.I don't understand all the details of the code but I'm wondering if there was a reason for overwriting max. I did not write the code but I can imagine that it's not a bug and that max is ignored on purpose. Although I do not know the details. Shouldn't you at least bounds check the parameter max then? If I'mcorrect then (max >= n) and (max <= rb->len - rb->offset) must be true.
I think ignoring a parameter when the description at the beginning describes is usage is a bug. I added code for input validation for the max argument. Thanks for pointing that out.
From looking at the code I can see that ssl3_read_n() gets called withmax=s->s3->rbuf.len which is equal to rb->len. Plus rb->offset is alwaysgreater or equal than the variable "align" which is !=0 on some platforms. So in this case max (which is equal to rb->len) is greaterthan (rb->len - rb->offset) and therefore max is too large in this case.2. If additional data is behind a valid DTLS record in the UDP packet,it is read as an additional record instead of being discarded.RFC 4347 says in 4.1.1.: "Multiple DTLS records may be placed in a single datagram." Did you take this into account? To me it seems like you only read the first DTLS record in a datagram and ignore all remaining records.
You are right, I forgot about that (we do not allow this in the SCTP case, since SCTP does the bundling). So I removed the code. Updated patch attached. Thanks for the comments.
Regards, Daniel ______________________________________________________________________ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
dtls.patch
Description: Binary data
