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

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Aug 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment
link

Issue 457759: Fix existing uses of readdir_r.

Reported by bunge...@chromium.org, Feb 11 2015 Project Member

Issue description

See http://elliotth.blogspot.com/2012/10/how-not-to-use-readdirr3.html , https://android-review.googlesource.com/#/c/42664/ , and the documentation on readdir_r.

Almost all existing uses of readdir_r in Chromium [1] appear to be of the stack exploitable type. The memory pointed to by the second parameter of readdir_r must be large enough for the longest possible path name. On no system is the sizeof(dirent) the correct size. On some systems, the d_name field is of size 1, on others it is of size NAME_MAX, but misleadingly this is not always the actual maximum size of a file name (see the above aosp bug for an example of this).

The uses of readdir_r should either be replaced with readdir (where they can be, relying on the extension of glibc, bionic, and libc that guarantee that the DIR dstream will hold the state) or the size should be calculated with something like

DIR* dir = opendir(dirName));
if (NULL == dir) { return; }
int dirFd = dirfd(dir);
if (dirFd < 0) { return; }

long maxFilenameLen = fpathconf(dirFd, _PC_NAME_MAX);
if (maxFilenameLen < 1) {
    // Either there was an error or there is no limit.
    // Avoid use of NAME_MAX as it is unenforced and meaningless.
    return;
}

// POSIX (and sanity) has always required d_name to be the last member.
size_t minimumSizeofDirent = offsetof(dirent, d_name) + static_cast<size_t>(maxFilenameLen) + 1;

size_t realSizeofDirent = max(minimumSizeofDirent, sizeof(dirent));


This 'realSizeofDirent' should be used as the size of the storage passed as the second parameter of readdir_r.


[1] https://code.google.com/p/chromium/codesearch#search/&q=readdir_r&sq=package:chromium&type=cs
 

Comment 1 by rsgav...@chromium.org, Feb 11 2015

Status: Assigned

Comment 2 by bunge...@chromium.org, Feb 13 2015

Cc: bunge...@chromium.org
Labels: Security
Owner: jsc...@chromium.org
Assigning to who I think might be the right person to take a look into this. I took a quick look, but I'm not sure I can properly coordinate the required changes or evaluate the severity of this issue at this time. If someone on the security team could take a look into this, it would be most helpful.

Comment 3 by jsc...@chromium.org, Feb 13 2015

Owner: jln@chromium.org
jln@ - Could you take a look at this or delegate it to someone?

Comment 4 by jln@chromium.org, Jul 17 2015

Cc: kerrnel@chromium.org mdempsky@chromium.org rsesek@chromium.org
Sorry, this slipped through.

On Linux, I don't think we care. We know that d_name has the correct size: NAME_MAX is always 255, which is what is used in "struct dirent".

This could only matter for other POSIX systems. The only other one is OS X.

Robert, Greg, what's the situation on OSX ?

Comment 5 by mdempsky@chromium.org, Jul 17 2015

Somewhat relevant, but sadly unhelpful POSIX standard issue: http://www.austingroupbugs.net/view.php?id=696

Comment 6 by kerrnel@chromium.org, Jul 17 2015

On OS X 64 bit the d_name size is 1024 and HFS+ supports a maximum filename size of 255 UTF-16 characters (https://en.wikipedia.org/wiki/HFS_Plus). readdir_r only puts the name of the current entry into d_name, not the full path, so I don't see a problem on OS X, since a 255 character UTF-16 filename will consume 510 bytes, which is well short of 1024. Looking at ZFS, NTFS, Fat-32, ext4, these all seem to limit to much less than 1024 bytes. Am I missing any other way to overflow the buffer?

Comment 7 by bunge...@chromium.org, Jul 17 2015

@Comment #4: note that NAME_MAX being 255 *is* the bug which was causing the issues on Android (see link in initial report), because FAT32 systems may have 255 UTF-16 character names (510 bytes). This can also cause issues on desktop Linux with FAT32 flash drives and the like.

Basically, any use or reliance on NAME_MAX is suspect, as this is going to be the value in the headers on the machine which compiled the source. One should always use fpathconf(dirFd, _PC_NAME_MAX). Otherwise you're left open to someone implementing ReFS driver (2**16 length file names) and suddenly they're vulnerable.

Comment 8 by lafo...@chromium.org, Sep 23 2015

Labels: Hotlist-Recharge
This issue likely requires triage.  The current issue owner maybe inactive (i.e. hasn't fixed an issue in the last 30 days).  Thanks for helping out!

-Anthony

Comment 9 by fw...@igalia.com, Oct 12 2016

It seems that readdir_r is becoming deprecated and some compiler starts warn about that.
See https://codereview.chromium.org/2411833004/ for a commit trying to fix some warning-treated-as-errors.

Comment 10 by bugdroid1@chromium.org, Oct 31 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c2fdc2710b700158ad46111be664eac7ede0fdce

commit c2fdc2710b700158ad46111be664eac7ede0fdce
Author: fwang <fwang@igalia.com>
Date: Mon Oct 31 23:40:39 2016

Replace some deprecated usages of readdir_r with readdir

readdir_r is deprecated and using it with recent compilers cause some
warning that may be treated as build errors. readdir is recommended
instead, see http://man7.org/linux/man-pages/man3/readdir_r.3.html

This CL replaces some usages of readdir_r with readdir in order to fix
build errors with use_ozone=1.

R=jln@chromium.org,gavinp@chromium.org,dcheng@chromium.org
BUG= 457759 

Review-Url: https://codereview.chromium.org/2411833004
Cr-Commit-Position: refs/heads/master@{#428867}

[modify] https://crrev.com/c2fdc2710b700158ad46111be664eac7ede0fdce/base/files/file_enumerator_posix.cc
[modify] https://crrev.com/c2fdc2710b700158ad46111be664eac7ede0fdce/net/disk_cache/simple/simple_index_file_posix.cc
[modify] https://crrev.com/c2fdc2710b700158ad46111be664eac7ede0fdce/sandbox/linux/services/proc_util.cc

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

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/74b29880d22b51deedf8acd225fb6b17b96164ca

commit 74b29880d22b51deedf8acd225fb6b17b96164ca
Author: jbudorick <jbudorick@chromium.org>
Date: Tue Nov 01 04:20:58 2016

Revert of Replace some deprecated usages of readdir_r with readdir (patchset #5 id:80001 of https://codereview.chromium.org/2411833004/ )

Reason for revert:
Speculative: appears to be causing flaky crashes in android_webview_test_apk and adversely affecting the CQ as a result.

e.g. https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%29/builds/36829

The last few messages in the logcat include:

Device(06ec840ef0e949a6) 11-01 01:44:07.189 27745 27808 E chromium: [ERROR:simple_index_file.cc(143)] Could not get file info for /data/data/org.chromium.android_webview.shell/cache/org.chromium.android_webview/137034c0a767955d_0
Device(06ec840ef0e949a6) 11-01 01:44:07.189 27745 27808 E chromium: [ERROR:simple_index_file_posix.cc(52)] readdir /data/data/org.chromium.android_webview.shell/cache/org.chromium.android_webview: No such file or directory
Device(06ec840ef0e949a6) 11-01 01:44:07.189 27745 27808 E chromium: [ERROR:simple_index_file.cc(544)] Could not reconstruct index from disk
Device(06ec840ef0e949a6) 11-01 01:44:07.209 27745 27788 F chromium: [FATAL:simple_index.cc(404)] Check failed: load_result->did_load.

I'll try to get a fully symbolized trace tomorrow.

Original issue's description:
> Replace some deprecated usages of readdir_r with readdir
>
> readdir_r is deprecated and using it with recent compilers cause some
> warning that may be treated as build errors. readdir is recommended
> instead, see http://man7.org/linux/man-pages/man3/readdir_r.3.html
>
> This CL replaces some usages of readdir_r with readdir in order to fix
> build errors with use_ozone=1.
>
> R=jln@chromium.org,gavinp@chromium.org,dcheng@chromium.org
> BUG= 457759 
>
> Committed: https://crrev.com/c2fdc2710b700158ad46111be664eac7ede0fdce
> Cr-Commit-Position: refs/heads/master@{#428867}

TBR=dcheng@chromium.org,gavinp@chromium.org,jln@chromium.org,jorgelo@chromium.org,bbudge@google.com,tonikitoo@chromium.org,fwang@igalia.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 457759 

Review-Url: https://codereview.chromium.org/2466773003
Cr-Commit-Position: refs/heads/master@{#428934}

[modify] https://crrev.com/74b29880d22b51deedf8acd225fb6b17b96164ca/base/files/file_enumerator_posix.cc
[modify] https://crrev.com/74b29880d22b51deedf8acd225fb6b17b96164ca/net/disk_cache/simple/simple_index_file_posix.cc
[modify] https://crrev.com/74b29880d22b51deedf8acd225fb6b17b96164ca/sandbox/linux/services/proc_util.cc

Comment 12 by raphael....@intel.com, Nov 7 2016

> I'll try to get a fully symbolized trace tomorrow.

Did this ever happen? I'd like to help here since I'm being affected by the "readdir_r is deprecated" issue mentioned in comment #9.

Comment 13 by fw...@igalia.com, Nov 7 2016

See https://codereview.chromium.org/2411833004/#msg53

As I just indicated there, one workaround is to do treat_warnings_as_errors = false in your GN config.

Comment 14 by raphael....@intel.com, Nov 7 2016

Yeah, I'm aware of the workarounds but was looking for a fix in the source code instead. I see in comment #54 in the CL you've linked that you're no longer working on this, so I'll see if I have some time to poke into this later.

Comment 15 by bugdroid1@chromium.org, Aug 4 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/native_client/src/native_client.git/+/081ae0f5c1a70c28df835123ea7110b7600192bc

commit 081ae0f5c1a70c28df835123ea7110b7600192bc
Author: Tom Anderson <thomasanderson@chromium.org>
Date: Fri Aug 04 17:52:09 2017

Implement readdir()

This CL is needed because Chromium code is switching from readdir_r()
to readdir().  It is a dependency CL of:
https://chromium-review.googlesource.com/c/599120

BUG= chromium:457759 , chromium:751812 
R=bradnelson@chromium.org

Change-Id: Id09ce31b1837d0dc3d4057397d50db2781cabd4d
Reviewed-on: https://chromium-review.googlesource.com/600516
Reviewed-by: Mark Seaborn <mseaborn@chromium.org>
Commit-Queue: Mark Seaborn <mseaborn@chromium.org>

[modify] https://crrev.com/081ae0f5c1a70c28df835123ea7110b7600192bc/src/nonsfi/linux/directory.c
[modify] https://crrev.com/081ae0f5c1a70c28df835123ea7110b7600192bc/tests/nonsfi/directory_test.cc

Comment 16 by bugdroid1@chromium.org, Aug 4 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/26ca1ff809456dfa08bafe3a310c2f3da242d26d

commit 26ca1ff809456dfa08bafe3a310c2f3da242d26d
Author: nacl-deps-roller@chromium.org <nacl-deps-roller@chromium.org>
Date: Fri Aug 04 20:24:40 2017

Roll src/native_client/ 01a28a606..081ae0f5c (1 commit)

https://chromium.googlesource.com/native_client/src/native_client.git/+log/01a28a6069a9..081ae0f5c1a7

$ git log 01a28a606..081ae0f5c --date=short --no-merges --format='%ad %ae %s'
2017-08-03 thomasanderson Implement readdir()

Created with:
  roll-dep src/native_client
BUG= 457759 , 751812 


Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls


CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_dbg_32_ng,linux_nacl_sdk_build
TBR=mseaborn@chromium.org

Change-Id: Iabf19085894aa964e0c93892ba2e2656bfa3ce33
Reviewed-on: https://chromium-review.googlesource.com/602499
Reviewed-by: <nacl-deps-roller@chromium.org>
Commit-Queue: <nacl-deps-roller@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492114}
[modify] https://crrev.com/26ca1ff809456dfa08bafe3a310c2f3da242d26d/DEPS

Comment 17 by bugdroid1@chromium.org, Aug 5 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4e33415240d57999459a2b9376831fa1303eabac

commit 4e33415240d57999459a2b9376831fa1303eabac
Author: Tom Anderson <thomasanderson@chromium.org>
Date: Sat Aug 05 00:06:59 2017

Remove usage of readdir_r() in Chromium code

readdir_r() is deprecated, which causes build failures when using
glibc-2.24 or newer headers.  This CL replaces usage of readdir_r()
with readdir(), and suppresses the warning for third_party code.

BUG= 457759 , 751812 
TBR=brettw@chromium.org

Change-Id: Idfeff0d535926bc53634a7574f00605c50d532ab
Reviewed-on: https://chromium-review.googlesource.com/599120
Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
Reviewed-by: Julien Tinnes <jln@chromium.org>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492190}
[modify] https://crrev.com/4e33415240d57999459a2b9376831fa1303eabac/headless/public/util/fontconfig.cc
[modify] https://crrev.com/4e33415240d57999459a2b9376831fa1303eabac/sandbox/linux/services/proc_util.cc
[modify] https://crrev.com/4e33415240d57999459a2b9376831fa1303eabac/third_party/leveldatabase/env_chromium.cc
[modify] https://crrev.com/4e33415240d57999459a2b9376831fa1303eabac/third_party/libdrm/BUILD.gn

Comment 18 by thomasanderson@chromium.org, Aug 5 2017

Status: Fixed (was: Assigned)

Sign in to add a comment