From owner-freebsd-arch@FreeBSD.ORG Fri Nov 5 18:12:04 2004 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id C740516A4CE for ; Fri, 5 Nov 2004 18:12:04 +0000 (GMT) Received: from comp.chem.msu.su (comp.chem.msu.su [158.250.32.97]) by mx1.FreeBSD.org (Postfix) with ESMTP id BE02B43D1F for ; Fri, 5 Nov 2004 18:12:03 +0000 (GMT) (envelope-from yar@comp.chem.msu.su) Received: from comp.chem.msu.su (localhost [127.0.0.1]) by comp.chem.msu.su (8.12.9p2/8.12.9) with ESMTP id iA5IC2Up090032 for ; Fri, 5 Nov 2004 21:12:02 +0300 (MSK) (envelope-from yar@comp.chem.msu.su) Received: (from yar@localhost) by comp.chem.msu.su (8.12.9p2/8.12.9/Submit) id iA5IC16M090030 for arch@freebsd.org; Fri, 5 Nov 2004 21:12:01 +0300 (MSK) (envelope-from yar) Date: Fri, 5 Nov 2004 21:12:01 +0300 From: Yar Tikhiy To: arch@freebsd.org Message-ID: <20041105181201.GA88657@comp.chem.msu.su> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.6i Subject: Fixes for tar(5) format handling in pax(1) X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 05 Nov 2004 18:12:04 -0000 Hi folks, I was pointed out by one of us at PR bin/40466, which described a bug in pax(1) related to handling pathnames of the maximum length specified by tar(5). A quick look at src/bin/pax/tar.c revealed more off-by-one errors and potential buffer overruns in this area. Many of them come from the fact that ustar allows pathnames (name, prefix, linkname) to be unterminated if they fill the entire field while old tar doesn't. Finally I made a patch that should address all of the issues found. Since we have pax in /bin, I'd appreciate if anybody reviewed my patch. Thanks. -- Yar Index: tar.c =================================================================== RCS file: /home/ncvs/src/bin/pax/tar.c,v retrieving revision 1.23 diff -u -p -r1.23 tar.c --- tar.c 6 Apr 2004 20:06:48 -0000 1.23 +++ tar.c 5 Nov 2004 17:44:08 -0000 @@ -387,7 +387,13 @@ tar_rd(ARCHD *arcn, char *buf) * copy out the name and values in the stat buffer */ hd = (HD_TAR *)buf; - arcn->nlen = l_strncpy(arcn->name, hd->name, sizeof(arcn->name) - 1); + /* + * old tar format specifies the name always be null-terminated, + * but let's be robust to broken archives. + * the same applies to handling links below. + */ + arcn->nlen = l_strncpy(arcn->name, hd->name, + MIN(sizeof(hd->name), sizeof(arcn->name)) - 1); arcn->name[arcn->nlen] = '\0'; arcn->sb.st_mode = (mode_t)(asc_ul(hd->mode,sizeof(hd->mode),OCT) & 0xfff); @@ -417,7 +423,7 @@ tar_rd(ARCHD *arcn, char *buf) */ arcn->type = PAX_SLK; arcn->ln_nlen = l_strncpy(arcn->ln_name, hd->linkname, - sizeof(arcn->ln_name) - 1); + MIN(sizeof(hd->linkname), sizeof(arcn->ln_name)) - 1); arcn->ln_name[arcn->ln_nlen] = '\0'; arcn->sb.st_mode |= S_IFLNK; break; @@ -429,7 +435,7 @@ tar_rd(ARCHD *arcn, char *buf) arcn->type = PAX_HLK; arcn->sb.st_nlink = 2; arcn->ln_nlen = l_strncpy(arcn->ln_name, hd->linkname, - sizeof(arcn->ln_name) - 1); + MIN(sizeof(hd->linkname), sizeof(arcn->ln_name)) - 1); arcn->ln_name[arcn->ln_nlen] = '\0'; /* @@ -533,7 +539,7 @@ tar_wr(ARCHD *arcn) case PAX_SLK: case PAX_HLK: case PAX_HRG: - if (arcn->ln_nlen > (int)sizeof(hd->linkname)) { + if (arcn->ln_nlen >= (int)sizeof(hd->linkname)) { paxwarn(1,"Link name too long for tar %s", arcn->ln_name); return(1); } @@ -749,12 +755,19 @@ ustar_rd(ARCHD *arcn, char *buf) */ dest = arcn->name; if (*(hd->prefix) != '\0') { - cnt = l_strncpy(dest, hd->prefix, sizeof(arcn->name) - 2); + cnt = l_strncpy(dest, hd->prefix, + MIN(sizeof(hd->prefix), sizeof(arcn->name) - 2)); dest += cnt; *dest++ = '/'; cnt++; } - arcn->nlen = cnt + l_strncpy(dest, hd->name, sizeof(arcn->name) - cnt); + /* + * ustar format specifies the name may be unterminated + * if it fills the entire field. this also applies to + * the prefix and the linkname. + */ + arcn->nlen = cnt + l_strncpy(dest, hd->name, + MIN(sizeof(hd->name), sizeof(arcn->name) - cnt - 1)); arcn->name[arcn->nlen] = '\0'; /* @@ -848,7 +861,7 @@ ustar_rd(ARCHD *arcn, char *buf) * copy the link name */ arcn->ln_nlen = l_strncpy(arcn->ln_name, hd->linkname, - sizeof(arcn->ln_name) - 1); + MIN(sizeof(hd->linkname), sizeof(arcn->ln_name) - 1)); arcn->ln_name[arcn->ln_nlen] = '\0'; break; case CONTTYPE: @@ -900,7 +913,7 @@ ustar_wr(ARCHD *arcn) */ if (((arcn->type == PAX_SLK) || (arcn->type == PAX_HLK) || (arcn->type == PAX_HRG)) && - (arcn->ln_nlen >= (int)sizeof(hd->linkname))) { + (arcn->ln_nlen > (int)sizeof(hd->linkname))) { paxwarn(1, "Link name too long for ustar %s", arcn->ln_name); return(1); } @@ -925,17 +938,16 @@ ustar_wr(ARCHD *arcn) * occur, we remove the / and copy the first part to the prefix */ *pt = '\0'; - l_strncpy(hd->prefix, arcn->name, sizeof(hd->prefix) - 1); + l_strncpy(hd->prefix, arcn->name, sizeof(hd->prefix)); *pt++ = '/'; } else memset(hd->prefix, 0, sizeof(hd->prefix)); /* * copy the name part. this may be the whole path or the part after - * the prefix + * the prefix. both the name and prefix may fill the entire field. */ - l_strncpy(hd->name, pt, sizeof(hd->name) - 1); - hd->name[sizeof(hd->name) - 1] = '\0'; + l_strncpy(hd->name, pt, sizeof(hd->name)); /* * set the fields in the header that are type dependent @@ -978,8 +990,8 @@ ustar_wr(ARCHD *arcn) hd->typeflag = SYMTYPE; else hd->typeflag = LNKTYPE; - l_strncpy(hd->linkname,arcn->ln_name, sizeof(hd->linkname) - 1); - hd->linkname[sizeof(hd->linkname) - 1] = '\0'; + /* the link name may occupy the entire field in ustar */ + l_strncpy(hd->linkname,arcn->ln_name, sizeof(hd->linkname)); memset(hd->devmajor, 0, sizeof(hd->devmajor)); memset(hd->devminor, 0, sizeof(hd->devminor)); if (ul_oct((u_long)0L, hd->size, sizeof(hd->size), 3)) @@ -1072,9 +1084,9 @@ name_split(char *name, int len) * check to see if the file name is small enough to fit in the name * field. if so just return a pointer to the name. */ - if (len < TNMSZ) + if (len <= TNMSZ) return(name); - if (len > (TPFSZ + TNMSZ)) + if (len > (TPFSZ + TNMSZ + 1)) return(NULL); /* @@ -1083,7 +1095,7 @@ name_split(char *name, int len) * to find the biggest piece to fit in the name field (or the smallest * prefix we can find) */ - start = name + len - TNMSZ; + start = name + len - TNMSZ - 1; while ((*start != '\0') && (*start != '/')) ++start; @@ -1101,7 +1113,7 @@ name_split(char *name, int len) * the file would then expand on extract to //str. The len == 0 below * makes this special case follow the spec to the letter. */ - if ((len >= TPFSZ) || (len == 0)) + if ((len > TPFSZ) || (len == 0)) return(NULL); /*