New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 654090 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Chrome
Pri: 2
Type: Bug-Security



Sign in to add a comment

Security: libicu has buffer overflow in path traversal code

Project Member Reported by xyzzyz@chromium.org, Oct 7 2016

Issue description

VULNERABILITY DETAILS
The code in libicu looking for a file in /usr/share/zoneinfo matching given timezone data has a buffer overflow. It assumes that all paths are shorter than PATH_MAX (= 4096), which is not true at all.

Here's the relevant code:
https://cs.chromium.org/chromium/src/third_party/icu/source/common/putil.cpp?sq=package:chromium&dr=C&rcl=1475853592&l=940

Relevant snippet:

    char curpath[MAX_PATH_SIZE];
    DIR* dirp = opendir(path);
    DIR* subDirp = NULL;
    struct dirent* dirEntry = NULL;

    [...snip...]

    /* Save the current path */
    uprv_memset(curpath, 0, MAX_PATH_SIZE);
    uprv_strcpy(curpath, path);

    /* Check each entry in the directory. */
    while((dirEntry = readdir(dirp)) != NULL) {
        const char* dirName = dirEntry->d_name;
        if (uprv_strcmp(dirName, SKIP1) != 0 && uprv_strcmp(dirName, SKIP2) != 0) {
            /* Create a newpath with the new entry to test each entry in the directory. */
            char newpath[MAX_PATH_SIZE];
            uprv_strcpy(newpath, curpath);
            uprv_strcat(newpath, dirName);
    [...snip...]
    }

Clearly, if strlen(curpath) + strlen(dirname) > PATH_MAX, then uprv_strcat writes after newpath buffer.

The impact probably isn't very large, as you need elevated privileges to add stuff to /usr/share/zoneinfo anyway.

VERSION
Chrome Version: all
Operating System: Linux, probably others.

REPRODUCTION CASE
First you need to create very long path in /usr/share/zoneinfo. You'll likely need to temporarily move your regular timezone file away from /usr/share/zoneinfo so that it's not found before your crafted path. Then it works something like this:

xyzzyz@xyzzyz:~/chromium/src$ gdb -- ./out/Default/base_unittests 
GNU gdb (GDB) 7.9-gg19
Copyright (C) 2015 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-grtev4-linux-gnu".
Type "show configuration" for configuration details.

<http://go/gdb-home FAQ: http://go/gdb-faq Email: c-toolchain-team IRC: gdb>
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.
For help, type "help".
Type "apropos word" to search for commands related to "word".
---- Loading Rust pretty-printers ----
Reading symbols from ./out/Default/base_unittests...done.
Unable to determine compiler version.
Skipping loading of libstdc++ pretty-printers for now.
Non-google3 binary detected.
(gdb) break ../../third_party/icu/source/common/putil.cpp:939 if strlen(curpath) + strlen(dirName) > 4096
Breakpoint 1 at 0x11224f9: file ../../third_party/icu/source/common/putil.cpp, line 939.
(gdb) r
Starting program: /usr/local/google/home/xyzzyz/chromium/src/out/Default/base_unittests 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/grte/v4/lib64/libthread_db.so.1".
Debugger detected, switching to single process mode.
Pass --test-launcher-debug-launcher to debug the launcher itself.

Breakpoint 1, searchForTZFile (
    path=0x7ffffffd9ff0 "/usr/share/zoneinfo/America/A", 'a' <repeats 171 times>..., 
    tzInfo=0xc328ecb24a0) at ../../third_party/icu/source/common/putil.cpp:939
939                 uprv_strcpy(newpath, curpath);
(gdb) p strlen(curpath)
$1 = 4060
(gdb) p strlen(dirName)
$2 = 251
(gdb) n
940                 uprv_strcat(newpath, dirName);
(gdb) n
942                 if ((subDirp = opendir(newpath)) != NULL) {
(gdb) p strlen(newpath)
$3 = 4311

 

Comment 1 by tsepez@chromium.org, Oct 10 2016

Labels: M-55 Security_Severity-Low Security_Impact-Stable
Owner: js...@chromium.org
Status: ExternalDependency (was: Unconfirmed)
Severity low because elevated privs are needed to create such a path.

Comment 2 by xyzzyz@chromium.org, Oct 10 2016

Should I create a bug in a public ICU tracker then?

Comment 3 by xyzzyz@chromium.org, Oct 10 2016

Same issue is present in isFileModTimeLater() 

https://cs.chromium.org/chromium/src/third_party/icu/source/tools/toolutil/filetools.cpp?q=isFileModTimeLater&sq=package:chromium&dr=CSs&l=62

though it also seems to haven no real impact, as it's only called from some packaging code.

Comment 4 by js...@chromium.org, Oct 10 2016

Labels: OS-Chrome
OS=Chrome, too even though it's even more difficult to change /usr/share/zoneinfo on Google Chrome OS. 

Just filed a bug against the upstream at
 http://bugs.icu-project.org/trac/ticket/12798

(not viewable because it's flagged as sensitive). 

Comment 5 by js...@chromium.org, Oct 21 2016

Upstream fixed this issue a couple of days ago. I'll cherry-pick the change for M-55. 

Comment 6 by js...@chromium.org, Nov 8 2016

This issue was fixed in ICU 58.1 and Chrome ToT has it. 

Let me port a fix back to ICU 56.1 in M55 branch. 

Comment 7 by js...@chromium.org, Nov 11 2016

Labels: Merge-Request-55
Requesting approval for merge to M55.  

https://codereview.chromium.org/2496433004/

The M56/trunk has ICU 58.1 with the above patch. M55 has ICU 56.1 that needs cherry-picking a patch from ICU 58.1. 


Project Member

Comment 8 by sheriffbot@chromium.org, Nov 12 2016

Status: Fixed (was: ExternalDependency)
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 9 by dimu@chromium.org, Nov 12 2016

Labels: -Merge-Request-55 Merge-Approved-55 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M55 (branch: 2883)
Project Member

Comment 10 by sheriffbot@chromium.org, Nov 13 2016

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 11 by bugdroid1@chromium.org, Nov 14 2016

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/tools/buildspec/+/578dcaea09e2d010a7644668b2e2f8d3eec3435f

commit 578dcaea09e2d010a7644668b2e2f8d3eec3435f
Author: Jungshik Shin <jungshik@google.com>
Date: Mon Nov 14 22:06:04 2016

Seems like this is already merged to M55 at #11. If there is nothing is pending for M55, please remove "Merge-Approved-55" label and apply "merge-merged-2883" label. Thank you.

Comment 13 by js...@chromium.org, Nov 15 2016

Labels: -Merge-Approved-55 merge-merged-2883
Label updated per comment 12
Labels: -Hotlist-Merge-Approved
Labels: Release-0-M55
Project Member

Comment 16 by sheriffbot@chromium.org, Feb 19 2017

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment