[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sun, 14 Dec 2014 08:38:15 +0100
From: Felix Janda <felix.janda@...teo.de>
To: musl@...ts.openwall.com
Subject: Re: Merging ns_parse from Alpine
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;
> }
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.
