NetBSD Problem Report #45285

From www@NetBSD.org  Tue Aug 23 16:14:54 2011
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by www.NetBSD.org (Postfix) with ESMTP id E93D063BEE5
	for <gnats-bugs@gnats.NetBSD.org>; Tue, 23 Aug 2011 16:14:53 +0000 (UTC)
Message-Id: <20110823161453.3016E63BAC3@www.NetBSD.org>
Date: Tue, 23 Aug 2011 16:14:53 +0000 (UTC)
From: mm@FreeBSD.org
Reply-To: mm@FreeBSD.org
To: gnats-bugs@NetBSD.org
Subject: makefs does not properly convert ISO level 1 and 2 filenames (buffer overflow)
X-Send-Pr-Version: www-1.0

>Number:         45285
>Category:       bin
>Synopsis:       makefs does not properly convert ISO level 1 and 2 filenames (buffer overflow)
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    bin-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Aug 23 16:15:00 +0000 2011
>Closed-Date:    Mon Apr 30 23:06:28 +0000 2012
>Last-Modified:  Mon Apr 30 23:06:28 +0000 2012
>Originator:     Martin Matuska
>Release:        any
>Organization:
FreeBSD
>Environment:
>Description:
makefs does not properly verify the maximum filename length in the special "." case for both ISO level 1 and ISO level 2 filename conversion.
This creates broken images or causes a buffer overflow (ISO level 2).

ISO level 1:
If a filename contains only dots or up to 8 characters followed by dots the 8+3 limit check doesn't work.

ISO level 2:
If a filename contains a dot in the first 30 characters and a dot on the 30th character, the length limit check doesn't work and the buffer is overflowed.
>How-To-Repeat:
mkdir level1
touch level1/12345............
makefs -t cd9660 -o isolevel=1 test.iso level1

mkdir level2
touch level2/1234567890.2345678901234567.....34567890123456789012345
makefs -t cd9660 -o isolevel=2 test.iso level2
>Fix:
Index: src/usr.sbin/makefs/cd9660.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/makefs/cd9660.c,v
retrieving revision 1.31
diff -u -p -r1.31 cd9660.c
--- src/usr.sbin/makefs/cd9660.c	6 Aug 2011 23:25:19 -0000	1.31
+++ src/usr.sbin/makefs/cd9660.c	23 Aug 2011 16:02:27 -0000
@@ -1637,7 +1637,7 @@ cd9660_level1_convert_filename(const cha

 	while (*oldname != '\0') {
 		/* Handle period first, as it is special */
-		if (*oldname == '.') {
+		if (*oldname == '.' && extlen < 3) {
 			if (found_ext) {
 				*newname++ = '_';
 				extlen ++;
@@ -1652,8 +1652,7 @@ cd9660_level1_convert_filename(const cha
 			    *oldname == ',' && strlen(oldname) == 4)
 				break;
 			/* Enforce 12.3 / 8 */
-			if (((namelen == 8) && !found_ext) ||
-			    (found_ext && extlen == 3)) {
+			if (namelen == 8 && !found_ext)
 				break;
 			}

@@ -1698,7 +1697,7 @@ cd9660_level2_convert_filename(const cha
 	int extlen = 0;
 	int found_ext = 0;

-	while (*oldname != '\0') {
+	while (*oldname != '\0' && namelen + extlen < 30) {
 		/* Handle period first, as it is special */
 		if (*oldname == '.') {
 			if (found_ext) {
@@ -1718,8 +1717,6 @@ cd9660_level2_convert_filename(const cha
 			if (diskStructure.archimedes_enabled &&
 			    *oldname == ',' && strlen(oldname) == 4)
 				break;
-			if ((namelen + extlen) == 30)
-				break;

 			 if (islower((unsigned char)*oldname))
 				*newname++ = toupper((unsigned char)*oldname);

>Release-Note:

>Audit-Trail:

State-Changed-From-To: open->feedback
State-Changed-By: christos@NetBSD.org
State-Changed-When: Tue, 23 Aug 2011 13:09:46 -0400
State-Changed-Why:
fixed, thanlks


From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/45285 CVS commit: src/usr.sbin/makefs
Date: Tue, 23 Aug 2011 13:09:11 -0400

 Module Name:	src
 Committed By:	christos
 Date:		Tue Aug 23 17:09:11 UTC 2011

 Modified Files:
 	src/usr.sbin/makefs: cd9660.c

 Log Message:
 PR/45285: Martin Matuska: makefs does not properly convert ISO level 1 and 2
 filenames (buffer overflow)

 makefs does not properly verify the maximum filename length in the
 special "." case for both ISO level 1 and ISO level 2 filename
 conversion.  This creates broken images or causes a buffer overflow
 (ISO level 2).

 ISO level 1:
 If a filename contains only dots or up to 8 characters followed by
 dots the 8+3 limit check doesn't work.

 ISO level 2:
 If a filename contains a dot in the first 30 characters and a dot
 on the 30th character, the length limit check doesn't work and the
 buffer is overflowed.

 $ mkdir level1
 $ touch level1/12345............
 $ makefs -t cd9660 -o isolevel=1 test.iso level1

 $ mkdir level2
 $ touch level2/1234567890.2345678901234567.....34567890123456789012345
 $ makefs -t cd9660 -o isolevel=2 test.iso level2


 To generate a diff of this commit:
 cvs rdiff -u -r1.31 -r1.32 src/usr.sbin/makefs/cd9660.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Martin Matuska <mm@FreeBSD.org>
To: gnats-bugs@NetBSD.org
Cc: Christos Zoulas <christos@netbsd.org>, gnats-admin@netbsd.org, 
 netbsd-bugs@netbsd.org
Subject: Re: PR/45285 CVS commit: src/usr.sbin/makefs
Date: Tue, 23 Aug 2011 20:10:57 +0200

 My patch for cd9660.c contained one typo:
 @@ -1652,8 +1652,7 @@ cd9660_level1_convert_filename(const cha
                             *oldname == ',' && strlen(oldname) == 4)
                                 break;
                         /* Enforce 12.3 / 8 */
 -                       if (((namelen == 8) && !found_ext) ||
 -                           (found_ext && extlen == 3)) {
 +                       if (namelen == 8 && !found_ext)
                                 break;
                         }


 The "}" at the end of this snipplet should be removed, too.
 Thanks!

 On 23. 8. 2011 19:10, Christos Zoulas wrote:
 > The following reply was made to PR bin/45285; it has been noted by GNATS.
 >
 > From: "Christos Zoulas" <christos@netbsd.org>
 > To: gnats-bugs@gnats.NetBSD.org
 > Cc: 
 > Subject: PR/45285 CVS commit: src/usr.sbin/makefs
 > Date: Tue, 23 Aug 2011 13:09:11 -0400
 >
 >  Module Name:	src
 >  Committed By:	christos
 >  Date:		Tue Aug 23 17:09:11 UTC 2011
 >  
 >  Modified Files:
 >  	src/usr.sbin/makefs: cd9660.c
 >  
 >  Log Message:
 >  PR/45285: Martin Matuska: makefs does not properly convert ISO level 1 and 2
 >  filenames (buffer overflow)
 >  
 >  makefs does not properly verify the maximum filename length in the
 >  special "." case for both ISO level 1 and ISO level 2 filename
 >  conversion.  This creates broken images or causes a buffer overflow
 >  (ISO level 2).
 >  
 >  ISO level 1:
 >  If a filename contains only dots or up to 8 characters followed by
 >  dots the 8+3 limit check doesn't work.
 >  
 >  ISO level 2:
 >  If a filename contains a dot in the first 30 characters and a dot
 >  on the 30th character, the length limit check doesn't work and the
 >  buffer is overflowed.
 >  
 >  $ mkdir level1
 >  $ touch level1/12345............
 >  $ makefs -t cd9660 -o isolevel=1 test.iso level1
 >  
 >  $ mkdir level2
 >  $ touch level2/1234567890.2345678901234567.....34567890123456789012345
 >  $ makefs -t cd9660 -o isolevel=2 test.iso level2
 >  
 >  
 >  To generate a diff of this commit:
 >  cvs rdiff -u -r1.31 -r1.32 src/usr.sbin/makefs/cd9660.c
 >  
 >  Please note that diffs are not public domain; they are subject to the
 >  copyright notices on the relevant files.
 >  


 -- 
 Martin Matuska
 FreeBSD committer
 http://blog.vx.sk

From: christos@zoulas.com (Christos Zoulas)
To: Martin Matuska <mm@FreeBSD.org>, gnats-bugs@NetBSD.org
Cc: gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: PR/45285 CVS commit: src/usr.sbin/makefs
Date: Tue, 23 Aug 2011 15:17:57 -0400

 On Aug 23,  8:10pm, mm@FreeBSD.org (Martin Matuska) wrote:
 -- Subject: Re: PR/45285 CVS commit: src/usr.sbin/makefs

 | My patch for cd9660.c contained one typo:
 | @@ -1652,8 +1652,7 @@ cd9660_level1_convert_filename(const cha
 |                             *oldname == ',' && strlen(oldname) == 4)
 |                                 break;
 |                         /* Enforce 12.3 / 8 */
 | -                       if (((namelen == 8) && !found_ext) ||
 | -                           (found_ext && extlen == 3)) {
 | +                       if (namelen == 8 && !found_ext)
 |                                 break;
 |                         }
 | 
 | 
 | The "}" at the end of this snipplet should be removed, too.
 | Thanks!

 Fixed already, thanks!

 christos

From: Martin Matuska <mm@FreeBSD.org>
To: gnats-bugs@NetBSD.org
Cc: Christos Zoulas <christos@zoulas.com>, gnats-admin@netbsd.org, 
 netbsd-bugs@netbsd.org
Subject: Re: PR/45285 CVS commit: src/usr.sbin/makefs
Date: Tue, 23 Aug 2011 21:51:28 +0200

 This is a multi-part message in MIME format.
 --------------050608050508030407040804
 Content-Type: text/plain; charset=ISO-8859-1
 Content-Transfer-Encoding: 7bit

 I am really sorry but I don't know how that happened there was another
 displacement in the patch, too.
 Here is the bugfix:

 Index: usr.sbin/makefs/cd9660.c
 ===================================================================
 RCS file: /cvsroot/src/usr.sbin/makefs/cd9660.c,v
 retrieving revision 1.33
 diff -u -p -r1.33 cd9660.c
 --- usr.sbin/makefs/cd9660.c    23 Aug 2011 19:17:07 -0000    1.33
 +++ usr.sbin/makefs/cd9660.c    23 Aug 2011 19:50:20 -0000
 @@ -1635,9 +1635,9 @@ cd9660_level1_convert_filename(const cha
      int extlen = 0;
      int found_ext = 0;

 -    while (*oldname != '\0') {
 +    while (*oldname != '\0' && extlen < 3) {
          /* Handle period first, as it is special */
 -        if (*oldname == '.' && extlen < 3) {
 +        if (*oldname == '.') {
              if (found_ext) {
                  *newname++ = '_';
                  extlen ++;

 Thank you very much for understanding.
 Martin.

 On 23. 8. 2011 21:20, Christos Zoulas wrote:
 > The following reply was made to PR bin/45285; it has been noted by GNATS.
 >
 > From: christos@zoulas.com (Christos Zoulas)
 > To: Martin Matuska <mm@FreeBSD.org>, gnats-bugs@NetBSD.org
 > Cc: gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
 > Subject: Re: PR/45285 CVS commit: src/usr.sbin/makefs
 > Date: Tue, 23 Aug 2011 15:17:57 -0400
 >
 >  On Aug 23,  8:10pm, mm@FreeBSD.org (Martin Matuska) wrote:
 >  -- Subject: Re: PR/45285 CVS commit: src/usr.sbin/makefs
 >  
 >  | My patch for cd9660.c contained one typo:
 >  | @@ -1652,8 +1652,7 @@ cd9660_level1_convert_filename(const cha
 >  |                             *oldname == ',' && strlen(oldname) == 4)
 >  |                                 break;
 >  |                         /* Enforce 12.3 / 8 */
 >  | -                       if (((namelen == 8) && !found_ext) ||
 >  | -                           (found_ext && extlen == 3)) {
 >  | +                       if (namelen == 8 && !found_ext)
 >  |                                 break;
 >  |                         }
 >  | 
 >  | 
 >  | The "}" at the end of this snipplet should be removed, too.
 >  | Thanks!
 >  
 >  Fixed already, thanks!
 >  
 >  christos
 >  


 -- 
 Martin Matuska
 FreeBSD committer
 http://blog.vx.sk


 --------------050608050508030407040804
 Content-Type: text/plain;
  name="cd9660_fix.patch"
 Content-Transfer-Encoding: 7bit
 Content-Disposition: attachment;
  filename="cd9660_fix.patch"

 Index: usr.sbin/makefs/cd9660.c
 ===================================================================
 RCS file: /cvsroot/src/usr.sbin/makefs/cd9660.c,v
 retrieving revision 1.33
 diff -u -p -r1.33 cd9660.c
 --- usr.sbin/makefs/cd9660.c	23 Aug 2011 19:17:07 -0000	1.33
 +++ usr.sbin/makefs/cd9660.c	23 Aug 2011 19:50:20 -0000
 @@ -1635,9 +1635,9 @@ cd9660_level1_convert_filename(const cha
  	int extlen = 0;
  	int found_ext = 0;

 -	while (*oldname != '\0') {
 +	while (*oldname != '\0' && extlen < 3) {
  		/* Handle period first, as it is special */
 -		if (*oldname == '.' && extlen < 3) {
 +		if (*oldname == '.') {
  			if (found_ext) {
  				*newname++ = '_';
  				extlen ++;

 --------------050608050508030407040804--

State-Changed-From-To: feedback->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Mon, 30 Apr 2012 23:06:28 +0000
State-Changed-Why:
Committed back in August.


>Unformatted:

Home	PR Database Search