Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!

Bug 203777

Summary: dev-libs/libcdio < 0.78.2-r4 Buffer overflow via long filename in Joliet (CVE-2007-6613)
Product: Gentoo Security Reporter: Devon Miller <devon.miller>
Component: VulnerabilitiesAssignee: Gentoo Security <security>
Status: RESOLVED FIXED    
Severity: normal CC: devon.miller, flameeyes, lars, media-video
Priority: High    
Version: unspecified   
Hardware: All   
OS: Linux   
URL: http://savannah.gnu.org/bugs/?21910
Whiteboard: B2 [glsa]
Package list:
Runtime testing required: ---
Attachments:
Description Flags
libcdio-info-buffer.patch
none
Buffer patch with correction for +1 in the wrong place
none
Upstream patch for misplaced +1 none

Description Devon Miller 2007-12-30 14:49:01 UTC
dev-libs/libcdio-0.78.2 provides iso-info and cd-info both exhibit this vulnerability.

A properly formatted iso image can produce a coredump and may allow for malicious code injection.

Reproducible: Always

Steps to Reproduce:
1. mkdir -p tmp/dir1
2. echo file_with_really_really_long_silly_name_to_test_iso_info_buffer
3. mkisofs -J -R -volid My_Image -o test.iso tmp
4. iso-info -l test.iso

Actual Results:  
iso-info lists files and directories up to the ver long file name at which point it segfaults.

Expected Results:  
a listing of the directories

in src/iso-info.c, in function "print_iso9660_recurse" there is a block of code that begins:

 _CDIO_LIST_FOREACH (entnode, entlist)
    {
      iso9660_stat_t *p_statbuf = _cdio_list_node_data (entnode);
      char *psz_iso_name = p_statbuf->filename;
      char _fullname[4096] = { 0, };
      char translated_name[MAX_ISONAME+1];


      if (yep != p_statbuf->rr.b3_rock || 1 == opts.no_rock_ridge) {
        iso9660_name_translate_ext(psz_iso_name, translated_name,
                                   i_joliet_level);
        snprintf (_fullname, sizeof (_fullname), "%s%s", psz_path,
                  translated_name);
      } else {
        snprintf (_fullname, sizeof (_fullname), "%s%s", psz_path,
                  psz_iso_name);
     

I believe either the test "yep != p_statbuf->rr.b3_rock" is incorrect, or possibly the code in lib/iso9660 that sets "b3_rock". The field "b3_rock" is an enumeration with the values "nope", "yep", "dunno". The test would be valid if "b3_rock" was a boolean, but the additional "dunno" state causes a problem.

When the iso is created with the options given above, the value of "b3_rock" is "dunno" (integer 2). This causes the code to enter the "iso9660_name_translate_ext" which writes beyond the end of "translated_name" and corrupts the stack.

I have also submitted this upstream as bug #21910
Comment 1 Robert Buchholz (RETIRED) gentoo-dev 2007-12-30 15:30:31 UTC
The upstream bug report is marked private, so I restricted this bug for now.
Please be aware that this vulnerability cannot be considered confidential anymore, as it was public in this bugtracker before.

Diego, please advise.
Comment 2 Devon Miller 2007-12-30 16:36:02 UTC
Sorry about that, I didn't see an obvious way to enter a private bug.

It's currently fixed in upstream CVS (iso-info.c:1.36, cd-info.c:1.150).

I'm attaching a patch that fixes the current ebuild
Comment 3 Robert Buchholz (RETIRED) gentoo-dev 2007-12-30 17:07:05 UTC
To enter restricted bugs, just click the "Only users in all of the selected groups can view this bug:  Gentoo Security" checkbox in the expert bug filing page.
Comment 4 Diego Elio Pettenò (RETIRED) gentoo-dev 2007-12-30 18:21:50 UTC
I don't really know what to advise on such short notice as I didn't even know of this before. I can tell there are quite some changes in libcdio CVS, good ones mostly but I wouldn't fast track to stable a new release of it right now (and I say this because I did make some of the changes ;)), so I'd suppose extracting the commit patch would probably be the best thing.
Comment 5 Robert Buchholz (RETIRED) gentoo-dev 2007-12-30 18:43:09 UTC
Created attachment 139644 [details, diff]
libcdio-info-buffer.patch

Upstream committed patch
Comment 6 Robert Buchholz (RETIRED) gentoo-dev 2007-12-30 18:48:15 UTC
Diego, sounds good -- find the patch attached. As it is already in the upstream CVS, please prepare an ebuild. 
Comment 7 Diego Elio Pettenò (RETIRED) gentoo-dev 2007-12-30 19:20:51 UTC
libcdio-0.78.2-r2 added to portage.. although I'm not the libcdio ebuild maintainer since almost an year ;)

Please when you add arches add also mips so that they can keyword a recent version, thanks.
Comment 8 Robert Buchholz (RETIRED) gentoo-dev 2007-12-30 19:35:58 UTC
Arch Security Liaisons, please test and mark stable dev-libs/libcdio-0.78.2-r2.
Target keywords : "alpha amd64 arm hppa ia64 ppc ppc64 sh sparc x86"

CC'ing current Liaisons:
  alpha : ferdy
  amd64 : welp
   hppa : jer
    ppc : dertobi123
  ppc64 : corsair
  sparc : fmccor
    x86 : opfer
Comment 9 Christian Faulhammer (RETIRED) gentoo-dev 2007-12-30 21:53:04 UTC
(In reply to comment #8)
> Arch Security Liaisons, please test and mark stable dev-libs/libcdio-0.78.2-r2.
> Target keywords : "alpha amd64 arm hppa ia64 ppc ppc64 sh sparc x86"

 Works on x86, but I am not able to commit to other directories than profiles at the moment...crappy lappy.  Anyway, anyone is free to fix it for x86.
Comment 10 Jeroen Roovers (RETIRED) gentoo-dev 2007-12-31 06:16:57 UTC
Stable for HPPA and x86 (comment #9).
Comment 11 Markus Rothe (RETIRED) gentoo-dev 2007-12-31 14:30:42 UTC
adding ranger because my machine is down.

ranger: comment #8
Comment 12 Brent Baude (RETIRED) gentoo-dev 2007-12-31 16:05:47 UTC
ppc64 done
Comment 13 Peter Weller (RETIRED) gentoo-dev 2007-12-31 17:40:27 UTC
amd64 done.
Comment 14 Diego Elio Pettenò (RETIRED) gentoo-dev 2007-12-31 20:52:26 UTC
Rocky mailed this on libcdio-dev so it's now public. I'll mail the patch for this in a few minutes and then work toward a new release, although I still want 0.78.2-r2 as target stable ;)
Comment 15 Robert Buchholz (RETIRED) gentoo-dev 2008-01-01 15:46:15 UTC
public via http://lists.gnu.org/archive/html/libcdio-devel/2007-12/msg00009.html

Arches, please test and mark stable dev-libs/libcdio-0.78.2-r2.
Target keywords : "alpha amd64 arm hppa ia64 ppc ppc64 sh sparc x86"
Already stabled : "amd64 hppa ppc x86"
Missing keywords: "alpha arm ia64 ppc64 sh sparc"
Comment 16 Raúl Porcel (RETIRED) gentoo-dev 2008-01-01 15:57:43 UTC
alpha/ia64/sparc stable
Comment 17 Brent Baude (RETIRED) gentoo-dev 2008-01-01 18:00:01 UTC
ppc64 done
Comment 18 Robert Buchholz (RETIRED) gentoo-dev 2008-01-01 22:06:02 UTC
request filed
Comment 19 bannedit 2008-01-03 03:34:49 UTC
(In reply to comment #0)
> dev-libs/libcdio-0.78.2 provides iso-info and cd-info both exhibit this
> vulnerability.
> 
> A properly formatted iso image can produce a coredump and may allow for
> malicious code injection.
> 
> Reproducible: Always
> 
> Steps to Reproduce:
> 1. mkdir -p tmp/dir1
> 2. echo file_with_really_really_long_silly_name_to_test_iso_info_buffer
> 3. mkisofs -J -R -volid My_Image -o test.iso tmp
> 4. iso-info -l test.iso
> 
> Actual Results:  
> iso-info lists files and directories up to the ver long file name at which
> point it segfaults.
> 
> Expected Results:  
> a listing of the directories
> 
> in src/iso-info.c, in function "print_iso9660_recurse" there is a block of code
> that begins:
> 
>  _CDIO_LIST_FOREACH (entnode, entlist)
>     {
>       iso9660_stat_t *p_statbuf = _cdio_list_node_data (entnode);
>       char *psz_iso_name = p_statbuf->filename;
>       char _fullname[4096] = { 0, };
>       char translated_name[MAX_ISONAME+1];
> 
> 
>       if (yep != p_statbuf->rr.b3_rock || 1 == opts.no_rock_ridge) {
>         iso9660_name_translate_ext(psz_iso_name, translated_name,
>                                    i_joliet_level);
>         snprintf (_fullname, sizeof (_fullname), "%s%s", psz_path,
>                   translated_name);
>       } else {
>         snprintf (_fullname, sizeof (_fullname), "%s%s", psz_path,
>                   psz_iso_name);
> 
> 
> I believe either the test "yep != p_statbuf->rr.b3_rock" is incorrect, or
> possibly the code in lib/iso9660 that sets "b3_rock". The field "b3_rock" is an
> enumeration with the values "nope", "yep", "dunno". The test would be valid if
> "b3_rock" was a boolean, but the additional "dunno" state causes a problem.
> 
> When the iso is created with the options given above, the value of "b3_rock" is
> "dunno" (integer 2). This causes the code to enter the
> "iso9660_name_translate_ext" which writes beyond the end of "translated_name"
> and corrupts the stack.
> 
> I have also submitted this upstream as bug #21910
> 

(In reply to comment #5)
> Created an attachment (id=139644) [edit]
> libcdio-info-buffer.patch
> 
> Upstream committed patch
> 

Shouldn't the +1 go outside the strlen bracets? Likely won't cause a problem now that things are on the heap but... worth mentioning I think

Comment 20 Devon Miller 2008-01-03 13:15:28 UTC
(In reply to comment #19)
> >       char translated_name[MAX_ISONAME+1];
> > 
> 
> (In reply to comment #5)
> > Created an attachment (id=139644) [edit]
> > libcdio-info-buffer.patch
> > 
> > Upstream committed patch
> > 
> 
> Shouldn't the +1 go outside the strlen bracets? Likely won't cause a problem
> now that things are on the heap but... worth mentioning I think
> 

Yes, good catch, As it is, the alloca'd string may be 2 bytes too small.I have updated the upstream bug report with this information,
Comment 21 Devon Miller 2008-01-03 13:24:39 UTC
Created attachment 139967 [details, diff]
Buffer patch with correction for +1 in the wrong place

Moved the +1 outside the strlen call so the size is the length of the string plus 1, not the length of the string minus 1.
Comment 22 Diego Elio Pettenò (RETIRED) gentoo-dev 2008-01-03 13:45:09 UTC
I think it the original version was by design: the translation strings remove ;1 suffix from filenames, so strlen(foo+1) is equivalent to writing strlen(foo)-1, just making strlen faster (afair, strlen() is O(n)).
Comment 23 Devon Miller 2008-01-04 03:12:36 UTC
Created attachment 140011 [details, diff]
Upstream patch for misplaced +1

Patch from upstream cvs repo.
Comment 24 Devon Miller 2008-01-04 03:14:49 UTC
(In reply to comment #22)
> I think it the original version was by design: the translation strings remove
> ;1 suffix from filenames, so strlen(foo+1) is equivalent to writing
> strlen(foo)-1, just making strlen faster (afair, strlen() is O(n)).

Negative. Upstream has confirmed and patched. See attachment #140011 [details, diff]
Comment 25 Robert Buchholz (RETIRED) gentoo-dev 2008-01-04 03:35:57 UTC
media-video, your ball.
Comment 26 Pierre-Yves Rofes (RETIRED) gentoo-dev 2008-01-04 22:35:05 UTC
*** Bug 204333 has been marked as a duplicate of this bug. ***
Comment 27 Diego Elio Pettenò (RETIRED) gentoo-dev 2008-01-04 23:21:53 UTC
Committed 0.78.2-r3 straight-to-stable for those who already stabled -r2.
Comment 28 Robert Buchholz (RETIRED) gentoo-dev 2008-01-04 23:38:02 UTC
Arches, please test and mark stable dev-libs/libcdio-0.78.2-r3.
Target keywords : "alpha amd64 arm hppa ia64 ppc ppc64 sh sparc x86"
Already stabled : "alpha amd64 hppa ia64 ppc ppc64 sparc x86"
Missing keywords: "arm sh"
Comment 29 Robert Buchholz (RETIRED) gentoo-dev 2008-01-08 22:56:27 UTC
Stanislav Brabec from SUSE pointed out that allocating a larger string on the stack might easier lead to stack exhaustion, which can be shown by using some ten thousand files with long file names.
Not sure on whether upstream will apply his patches, but would you consider this a regression (media-video/Diego)?
Comment 30 Robert Buchholz (RETIRED) gentoo-dev 2008-01-09 21:11:06 UTC
Diego, do you consider this a regression? If not, we'd glsa this now, otherwise we'll go stabling a new ebuild.

The patch has been committed to the upstream CVS.
Comment 31 bannedit 2008-01-10 22:57:55 UTC
(In reply to comment #29)
> Stanislav Brabec from SUSE pointed out that allocating a larger string on the
> stack might easier lead to stack exhaustion, which can be shown by using some
> ten thousand files with long file names.
> Not sure on whether upstream will apply his patches, but would you consider
> this a regression (media-video/Diego)?
> 
I was looking at this scenario just recently. From the alloca() man page:

RETURN VALUE
       The  alloca()  function returns a pointer to the beginning of the allo-
       cated space.  If the allocation causes stack overflow, program behavior
       is undefined.

In some cases I think this could cause some major problems even further security issues. In the simplest case this will only cause stability issues. My suggestion would be to use the heap rather than alloca().
Comment 32 Sune Kloppenborg Jeppesen gentoo-dev 2008-01-13 14:08:14 UTC
Seems like we should go back to ebuild to get this fixed?
Comment 33 Diego Elio Pettenò (RETIRED) gentoo-dev 2008-01-13 14:37:35 UTC
Sorry I was out of order because of the cold/fever, I'll see to handle this today.
Comment 34 Diego Elio Pettenò (RETIRED) gentoo-dev 2008-01-13 16:17:59 UTC
0.78.2-r4 and 0.79-r1 are in tree. I didn't commit straight to stable this time, just for safety.
Comment 35 Sune Kloppenborg Jeppesen gentoo-dev 2008-01-13 16:39:33 UTC
Thx Diego. Arches please test and mark stable. Target keywords are:

libcdio-0.78.2-r4.ebuild:KEYWORDS="alpha amd64 arm hppa ia64 ppc ppc64 sh sparc ~sparc-fbsd x86 ~x86-fbsd"
Comment 36 Brent Baude (RETIRED) gentoo-dev 2008-01-13 17:56:54 UTC
ppc64 done
Comment 37 Tobias Scherbaum (RETIRED) gentoo-dev 2008-01-13 20:19:39 UTC
ppc stable
Comment 38 Markus Meier gentoo-dev 2008-01-13 20:55:15 UTC
x86 stable
Comment 39 Ferris McCormick (RETIRED) gentoo-dev 2008-01-14 14:16:09 UTC
Sparc stable.
Comment 40 Raúl Porcel (RETIRED) gentoo-dev 2008-01-14 20:12:44 UTC
alpha/ia64 stable
Comment 41 Jeroen Roovers (RETIRED) gentoo-dev 2008-01-14 20:38:05 UTC
Stable for HPPA.
Comment 42 Robert Buchholz (RETIRED) gentoo-dev 2008-01-15 17:42:58 UTC
amd64 stable, [glsa] again.
Comment 43 Robert Buchholz (RETIRED) gentoo-dev 2008-01-20 00:44:34 UTC
GLSA 200801-08, thanks.