Date: Thu, 4 Sep 2014 18:41:20 -0400 From: Rich Felker <dalias@...c.org> To: musl@...ts.openwall.com Subject: Re: [PATCH] fix handling of zero length domain names in dn_expand On Fri, Sep 05, 2014 at 12:15:52AM +0200, Szabolcs Nagy wrote: > * Rich Felker <dalias@...c.org> [2014-09-04 16:20:53 -0400]: > > > char *dend = dest + (space > 254 ? 254 : space); > > > - int len = -1, i, j; > > > - if (p==end || !*p) return -1; > > > + int len = -1, i, j, first = 1; > > > + if (p==end || dest==dend) return -1; > > > > Note that this does nothing to handle negative space, whereas ncopa's > > i assumed it to be ub Yes, technically it probably should be considered as such, but I was thinking of crazy things like large buffer lengths silently getting converted from size_t to int due to the poor choice of types for the signature. I definitely wouldn't want to treat negative values as large unsigned values -- this could overflow a small buffer, and real negatives could happen if "remaining buffer length" was computed poorly and not checked. One could argue that all of these considerations are beyond the scope of what the function needs to do, but checing is easy, and I'm so sick of bugs and potential vulns from this function already... > > such. Semantically it seems to be a zero-length component at the end > > (corresponding to "example.com.." rather than "example.com.", in the > > notation where final dots are not elided). Understanding whether it's > > legal or not probably requires some language-lawyering with RFC 1035, > > the rfc is not very clear but i think this case should work > > a name is a sequence of labels terminated by a 0 length label > > a compressed name is a leading sequence of labels terminated > by a pointer that can be expanded to a name > > if 'sequence of labels' can be empty and the conversion to > dotted string notation is done after concatenating the > leading and trailing sequence then there is no '..' issue OK, based on this explanation, I think both forms are valid. I always get stuck in the trap of reasoning in the mechanism of conversion rather than the "sequence of labels" abstraction. > > - char *dend = dest + (space > 254 ? 254 : space); > > + char *dend; > > int len = -1, i, j; > > - if (p==end || !*p) return -1; > > + if (p==end || size <= 0) return -1; > > + dend = dest + (space > 254 ? 254 : space); > > ok this part is better than mine Yes, whatever else we do, I think this is a nice addition. Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.