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 are >> OK, 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'm > correct 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 with > max=s->s3->rbuf.len which is equal to rb->len. Plus rb->offset is > always > greater or equal than the variable "align" which is !=0 on some > platforms. So in this case max (which is equal to rb->len) is greater > than (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 [email protected] > Automated List Manager [email protected] >
dtls.patch
Description: Binary data
