From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/6719 Path: news.gmane.org!not-for-mail From: Felix Janda Newsgroups: gmane.linux.lib.musl.general Subject: Re: Merging ns_parse from Alpine Date: Sun, 14 Dec 2014 08:38:15 +0100 Message-ID: <20141214073650.GA1330@euler> References: <20141214004320.GA17102@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1418542761 21233 80.91.229.3 (14 Dec 2014 07:39:21 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sun, 14 Dec 2014 07:39:21 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-6732-gllmg-musl=m.gmane.org@lists.openwall.com Sun Dec 14 08:39:12 2014 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1Y03lo-0006SD-8a for gllmg-musl@m.gmane.org; Sun, 14 Dec 2014 08:39:12 +0100 Original-Received: (qmail 15416 invoked by uid 550); 14 Dec 2014 07:39:09 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 15402 invoked from network); 14 Dec 2014 07:39:09 -0000 Mail-Followup-To: musl@lists.openwall.com Content-Disposition: inline In-Reply-To: <20141214004320.GA17102@brightrain.aerifal.cx> User-Agent: Mutt/1.5.22 (2013-10-16) Xref: news.gmane.org gmane.linux.lib.musl.general:6719 Archived-At: Rich Felker wrote: > I'm working on merging Timo's patch for ns_parse: > > http://git.alpinelinux.org/cgit/aports/tree/main/musl/1001-add-basic-dns-record-parsing-functions.patch?id=81d50064c335467fdfd80368bac6707d70db1af7 > > The first issue that came up in the process is that arpa/nameser.h, > which was previously not used by musl itself and really should never > have been accepted in its current form, is full of junk like > statement-expressions. Including it in a file that will be compiled > with musl adds build dependency on these nonstandard features. I > cleaned that up with no problem (just un-inlining the macros since > we're adding function versions anyway), but there are a few more > issues. The NS_GET* macros still seem to be used a lot in the code. > The main issue is that the parser functions have pointer arithmetic > overflows (UB) checking against the end-of-message pointer. I've tried > to fix that and I'm attaching a patch for review, along with my > version of the fixed file. I'd appreciate comments on whether I missed > anything. > > Other changes were mostly cosmetic or at least mechanical. > > Rich I didn't notice any missed checks but I think that some checks can be simplified: [..] > int ns_initparse(const unsigned char *msg, int msglen, ns_msg *handle) > { > int i, r; > > handle->_msg = msg; > handle->_eom = msg + msglen; > if (msglen < (2 + ns_s_max) * NS_INT16SZ) goto bad; > NS_GET16(handle->_id, msg); > NS_GET16(handle->_flags, msg); > for (i = 0; i < ns_s_max; i++) { > if (NS_INT16SZ > handle->_eom - msg) goto bad; Isn't this uneccessary given the above check? > NS_GET16(handle->_counts[i], msg); > } > for (i = 0; i < ns_s_max; i++) { > if (handle->_counts[i]) { > handle->_sections[i] = msg; > r = ns_skiprr(msg, handle->_eom, i, handle->_counts[i]); > if (r < 0) return -1; > msg += r; > } else { > handle->_sections[i] = NULL; > } > } > if (msg != handle->_eom) goto bad; > handle->_sect = ns_s_max; > handle->_rrnum = -1; > handle->_msg_ptr = NULL; > return 0; > bad: > errno = EMSGSIZE; > return -1; > } > > int ns_skiprr(const unsigned char *ptr, const unsigned char *eom, ns_sect section, int count) > { > const unsigned char *p = ptr; > int r; > > while (count--) { > r = dn_skipname(p, eom); > if (r < 0) goto bad; > if (r + 2 * NS_INT16SZ > eom - p) goto bad; > p += r + 2 * NS_INT16SZ; > if (section != ns_s_qd) { > if (NS_INT32SZ + NS_INT16SZ > eom - p) goto bad; > p += NS_INT32SZ; > NS_GET16(r, p); > if (r > eom - p) goto bad; Couldn't the two checks be combined into one? > p += r; > } > } > return ptr - p; > bad: > errno = EMSGSIZE; > return -1; > } > > int ns_parserr(ns_msg *handle, ns_sect section, int rrnum, ns_rr *rr) > { > int r; > > if (section < 0 || section >= ns_s_max) goto bad; > if (section != handle->_sect) { > handle->_sect = section; > handle->_rrnum = 0; > handle->_msg_ptr = handle->_sections[section]; > } > if (rrnum == -1) rrnum = handle->_rrnum; > if (rrnum < 0 || rrnum >= handle->_counts[section]) goto bad; > if (rrnum < handle->_rrnum) { > handle->_rrnum = 0; > handle->_msg_ptr = handle->_sections[section]; > } > if (rrnum > handle->_rrnum) { > r = ns_skiprr(handle->_msg_ptr, handle->_eom, section, rrnum - handle->_rrnum); > if (r < 0) return -1; > handle->_msg_ptr += r; > handle->_rrnum = rrnum; > } > r = dn_expand(handle->_msg, handle->_eom, handle->_msg_ptr, rr->name, NS_MAXDNAME); > if (r < 0) return -1; dn_expand doesn't set errno. > handle->_msg_ptr += r; > if (2 * NS_INT16SZ > handle->_eom - handle->_msg_ptr) goto size; > NS_GET16(rr->type, handle->_msg_ptr); > NS_GET16(rr->rr_class, handle->_msg_ptr); > if (section != ns_s_qd) { > if (NS_INT32SZ + NS_INT16SZ > handle->_eom - handle->_msg_ptr) goto size; > NS_GET32(rr->ttl, handle->_msg_ptr); > NS_GET16(rr->rdlength, handle->_msg_ptr); > if (rr->rdlength > handle->_eom - handle->_msg_ptr) goto size; > rr->rdata = handle->_msg_ptr; > handle->_msg_ptr += rr->rdlength; > } else { > rr->ttl = 0; > rr->rdlength = 0; > rr->rdata = NULL; > } > handle->_rrnum++; > if (handle->_rrnum > handle->_counts[section]) { > handle->_sect = section + 1; > if (handle->_sect == ns_s_max) { > handle->_rrnum = -1; > handle->_msg_ptr = NULL; > } else { > handle->_rrnum = 0; > } > } > return 0; > bad: > errno = ENODEV; > return -1; > size: > errno = EMSGSIZE; > return -1; > } > > int ns_name_uncompress(const unsigned char *msg, const unsigned char *eom, > const unsigned char *src, char *dst, size_t dstsiz) > { > int r; > r = dn_expand(msg, eom, src, dst, dstsiz); > if (r < 0) errno = EMSGSIZE; > return r; > }