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 1 user

Issue metadata

Status: Fixed
Closed: Nov 2011
EstimatedDays: ----
NextAction: ----
OS: Linux , All
Pri: 1
Type: Bug-Security

  • Only users with EditIssue permission may comment.

Sign in to add a comment

Stack-buffer-overflow in base::files::(anonymous namespace)::InotifyReaderTask::Run

Project Member Reported by, Nov 22 2011 Back to list

Issue description

Detailed report:

Fuzzer: Marty_html_twiddler

Crash Type: Stack-buffer-overflow READ 8
Crash Address: 0x7f764f5ae8a0
Crash State:
  - crash stack -
  base::files::(anonymous namespace)::InotifyReaderTask::Run

Unminimized Testcase:
Labels: OS-Linux
This can be SecCritical, but i suspect the attack vector depends on the command line params ?

mnissler@, dmaclach@, can you please take a look.
Kostya: ah, it actually makes perfect sense now
these FD_ folks are macros
and they access the local variable as an array using the first parameter as an index (sort of)
Abhishek: DCHECK(i + event_size <= static_cast<size_t>(bytes_read));
i dont like DCHECKs
Kostya: hardly related
the bug is on 'rfds' local var
As Kostya triaged this, the bug seems to be probably on one of the FD_ lines.
FD_ISSET(shutdown_fd_, &rfds) is read from local
whereas FD_SET does read-then-write

An fd_set is a fixed size buffer. Executing FD_CLR() or FD_SET() with a value of fd that is negative or is equal to or larger than
FD_SETSIZE will result in undefined behavior. Moreover, POSIX requires fd to be a valid file descriptor.
(from man FD_SET)

We should add a CHECK(inotify_fd_ < FD_SETSIZE)
also check for >= 0. Also do it for shutdown_fd_

Better to crash on a CHECK that a stack buffer overflow.
Note that inotify_fd_ is checked for >= 0 before constructing InotifyReaderTask and shutdown_fd_ comes directly from a pipe() call, so the only fd_set related stack overflow I can see would be due to large file descriptor numbers. FD_SETSIZE is 1024 according to my system headers, so this seems quite unlikely...

Regarding the DCHECK(i + event_size <= static_cast<size_t>(bytes_read)), I don't think there's a possible stack overflow here, assuming that std::vector always allocates its buffer on the heap.

Put up a CL that adds the file descriptor CHECKS:
Status: Assigned (was: NULL)
Labels: -SecImpacts-None SecImpacts-Stable Merge-Approved SecImpacts-Beta Mstone-15
Status: FixUnreleased (was: NULL)
Labels: -Merge-Approved Merge-Merged
Merged to 15 in r111505.
Merged to 16 in r111506.
Labels: -Mstone-15 Mstone-16
Thanks a lot Mattias for the fix and the merge.
Labels: -SecSeverity-High SecSeverity-Medium
Seems like a medium severity. Couldn't get a reliable repro and we don't know to what extent this can be intentionally triggered.
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 11 by, May 15 2012

Status: Fixed (was: NULL)
Marking old security bugs Fixed..
Project Member

Comment 12 by, Oct 13 2012

Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.
Project Member

Comment 13 by, Mar 10 2013

Labels: -Area-Internals -Type-Security -SecSeverity-Medium -SecImpacts-Stable -Stability-AddressSanitizer -SecImpacts-Beta -Mstone-16 Security-Impact-Beta Security-Severity-Medium Cr-Internals Performance-Memory-AddressSanitizer Security-Impact-Stable M-16 Type-Bug-Security
Project Member

Comment 14 by, Mar 13 2013

Labels: Restrict-View-EditIssue
Project Member

Comment 15 by, Mar 13 2013

Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue
Labels: -Restrict-View-SecurityNotify -Restrict-View-EditIssue
Project Member

Comment 17 by, Mar 21 2013

Labels: -Security-Impact-Stable Security_Impact-Stable
Project Member

Comment 18 by, Mar 21 2013

Labels: -Security-Severity-Medium Security_Severity-Medium
Project Member

Comment 19 by, Apr 1 2013

Labels: -Performance-Memory-AddressSanitizer Stability-Memory-AddressSanitizer
Project Member

Comment 20 by, Jun 14 2016

Labels: -security_impact-beta
Project Member

Comment 21 by, Oct 1 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit - Your friendly Sheriffbot
Project Member

Comment 22 by, Oct 2 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit - Your friendly Sheriffbot
Labels: allpublic

Sign in to add a comment