Bug 911929 - cut might do an unbounded strdup (coreutils-i18n.patch)
Summary: cut might do an unbounded strdup (coreutils-i18n.patch)
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: coreutils
Version: 18
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Ondrej Vasik
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2013-02-16 20:09 UTC by Mark Wielaard
Modified: 2013-02-18 09:34 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-02-18 09:34:27 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)
fix-coreutils-i18n.patch-to-terminate-mbdelim-string.patch (2.00 KB, patch)
2013-02-16 20:19 UTC, Mark Wielaard
no flags Details | Diff

Description Mark Wielaard 2013-02-16 20:09:21 UTC
Description of problem:

cut might do an unbounded strdup of in i18n mode of the delimiter string.

Version-Release number of selected component (if applicable):

coreutils-8.17-8.fc18.x86_64

How reproducible:

Always

Steps to Reproduce:

$ echo "a: bc def" | LANG=en_US.utf8 valgrind --track-origins=yes -q cut -d " " -f 2-
==23741== Conditional jump or move depends on uninitialised value(s)
==23741==    at 0x4A091B8: strlen (mc_replace_strmem.c:403)
==23741==    by 0x407228: xstrdup (xmalloc.c:123)
==23741==    by 0x4025E4: main (cut.c:1255)
==23741==  Uninitialised value was created by a stack allocation
==23741==    at 0x401B30: main (cut.c:1094)
==23741== 
bc def

Additional info:

The issue is this part of the patch:

+#if HAVE_MBRTOWC
+              if(MB_CUR_MAX > 1)
+                {
+                  mbstate_t state;
+
+                  memset (&state, '\0', sizeof(mbstate_t));
+                  delimlen = mbrtowc (&wcdelim, optarg, strnlen(optarg, MB_LEN_MAX), &state);
+
+                  if (delimlen == (size_t)-1 || delimlen == (size_t)-2)
+                    ++force_singlebyte_mode;
+                  else
+                    {
+                      delimlen = (delimlen < 1) ? 1 : delimlen;
+                      if (wcdelim != L'\0' && *(optarg + delimlen) != '\0')
+                        FATAL_ERROR (_("the delimiter must be a single character"));
+                      memcpy (mbdelim, optarg, delimlen);
+                    }
+                }

Note how the memcpy only copies the characters and doesn't add a zero terminator to mbdelim. So when we come here:

+#ifdef HAVE_MBRTOWC
+      if (MB_CUR_MAX > 1 && !force_singlebyte_mode)
+        {
+          output_delimiter_string = xstrdup(mbdelim);
+          output_delimiter_length = delimlen;
+        }

The xstrdup(mbdelim) might copy lots of (uninitialized) bytes from mbdelim because it isn't NULL terminated.

In practice there will eventually be zero byte somewhere in memory which will stop the xstrdup. But it will waste space. It looks like the rest of the code will use output_delimiter_length so it won't lead to bad results (unless the xstrdup runs out of memory or into invalid memory).

Comment 1 Mark Wielaard 2013-02-16 20:19:06 UTC
Created attachment 698276 [details]
fix-coreutils-i18n.patch-to-terminate-mbdelim-string.patch

Comment 2 Ondrej Vasik 2013-02-18 09:34:27 UTC
Thanks for noticing that and patch, I commited your fix to the Rawhide, closing RAWHIDE. Will include it eventually to the next future update of coreutils in F18/F17.


Note You need to log in before you can comment on or make changes to this bug.