Use PAGE_NOACCESS for guard pages in Windows. (issue 23458022)

[bme...@chromium.org]

A small code review. Reviewers: Michael Starzinger,

Message:
Hey Michi,
Here's the change we discussed offline to use PAGE_NOACCESS on Windows,
PTAL.
-- Benedikt

Description:
Use PAGE_NOACCESS for guard pages in Windows.

Up until now we used PAGE_GUARD for guard pages in Windows, which
will raise a STATUS_GUARD_PAGE_VIOLATION exception on first access
and grant regular access afterwards. This behavior is required to
implement automatic stack checking, or more generally to implement
applications that monitor the growth of large dynamic data structures.

However, this is not what we want for our guard pages, which are
used as a security mechanism. What we really want is PAGE_NOACCESS
here, which is the Windows-equivalent of PROT_NONE that we use on
all other platforms.

Please review this at https://codereview.chromium.org/23458022/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files +4, -4:
M src/platform-cygwin.cc
M src/platform-posix.cc
M src/platform-win32.cc


Index: src/platform-cygwin.cc
diff --git a/src/platform-cygwin.cc b/src/platform-cygwin.cc
index
10525d934584608f8987a298cc9e6cfc5b82e290..8873150fb974b1c0a4897406642497e7ad6002a0
100644
--- a/src/platform-cygwin.cc
+++ b/src/platform-cygwin.cc
@@ -375,7 +375,7 @@ bool VirtualMemory::Guard(void* address) {
if (NULL == VirtualAlloc(address,
OS::CommitPageSize(),
MEM_COMMIT,
- PAGE_READONLY | PAGE_GUARD)) {
+ PAGE_NOACCESS)) {
return false;
}
return true;
Index: src/platform-posix.cc
diff --git a/src/platform-posix.cc b/src/platform-posix.cc
index
504d140138c178fe50ff19480e20dcefd3db3a99..b111213f6d61783e217293061fa9d309a8206181
100644
--- a/src/platform-posix.cc
+++ b/src/platform-posix.cc
@@ -152,7 +152,7 @@ void OS::ProtectCode(void* address, const size_t size) {
void OS::Guard(void* address, const size_t size) {
#if defined(__CYGWIN__)
DWORD oldprotect;
- VirtualProtect(address, size, PAGE_READONLY | PAGE_GUARD, &oldprotect);
+ VirtualProtect(address, size, PAGE_NOACCESS, &oldprotect);
#else
mprotect(address, size, PROT_NONE);
#endif
Index: src/platform-win32.cc
diff --git a/src/platform-win32.cc b/src/platform-win32.cc
index
87387e7d12246aa2065c8bf35108bcee3dea8633..4b2004a382cfcaae7ca45aec4b5f0b58f5de149f
100644
--- a/src/platform-win32.cc
+++ b/src/platform-win32.cc
@@ -897,7 +897,7 @@ void OS::ProtectCode(void* address, const size_t size) {

void OS::Guard(void* address, const size_t size) {
DWORD oldprotect;
- VirtualProtect(address, size, PAGE_READONLY | PAGE_GUARD, &oldprotect);
+ VirtualProtect(address, size, PAGE_NOACCESS, &oldprotect);
}


@@ -1473,7 +1473,7 @@ bool VirtualMemory::Guard(void* address) {
if (NULL == VirtualAlloc(address,
OS::CommitPageSize(),
MEM_COMMIT,
- PAGE_READONLY | PAGE_GUARD)) {
+ PAGE_NOACCESS)) {
return false;
}
return true;

[veg...@google.com]

+cdn

Original OS::Guard was submitted by Cris Neckar. I always assumed that
choice of
PROT_GUARD was intentional.

https://codereview.chromium.org/23458022/

[mstar...@chromium.org]

Looking OK from my end. I'll leave the final decision up to Cris.

https://codereview.chromium.org/23458022/

[c...@chromium.org]

On 2013/09/05 09:22:12, Michael Starzinger wrote:
> Looking OK from my end. I'll leave the final decision up to Cris.

Yes, the reason we used PAGE_GUARD is so that we would get an actionable
exception in crash reports. This allows us to filter crashes for
STATUS_GUARD_PAGE_VIOLATION and know with some degree of certainty that a
given
crash will have security relevance. Changing this to PAGE_NOACCESS would
mean we
will simply get a standard ACCESS_VIOLATION and will not have the additional
context that we are writing off the end (or before the beginning of a
guarded
segment).

That being said are their cases that you found where we are handling this
exception and it is not causing a crash? If so then I agree this is bad and
we
need to fix it. Otherwise I would ask that we keep these pages with the
current
protection so that we retain the additional context in crash reports.

Make sense?

https://codereview.chromium.org/23458022/

[bme...@chromium.org]

No, from what I know we don't handle this exception. My point is that we
should
not misuse the PAGE_GUARD mechanism here. If someone embedder (or even
Chrome)
uses PAGE_GUARD in the way it was designed to, he'll have a hard time
figuring
out what's going on when the exception handler is entered due to an
(accidential
exploit/bug) in V8. I discovered this issue during general code review, but
if
we run into such an issue in 1-2 years noone will think of this and we'll
have
to trace that down for several days (it's totally non-obvious). Especially
since
this issue is Windows-only then, because all other platforms use PROT_NONE
here.

So I vote to change this to PAGE_NOACCESS or even better, just uncommit the
guard pages. They are easy to spot already.

https://codereview.chromium.org/23458022/

[bme...@chromium.org]

CC'ing Sven for additional feedback

https://codereview.chromium.org/23458022/

[sven...@chromium.org]

Hmmm, actually I would prefer PAGE_NOACCESS, too. MSDN explicitly describes
PAGE_GUARD as a mechanism for growable data structures, e.g.
http://msdn.microsoft.com/en-us/library/windows/desktop/aa366549(v=vs.85).aspx.
Therefore I think it is a terrible idea to use this for security, this is
exactly what PAGE_NOACCESS is for. It is very weird that we depend on the
embedder's handling of PAGE_GUARD, consider e.g. using v8 from another
language
implementation (Haskell, OCaml, etc.) where stacks grown on demand via
PAGE_GUARD. Furthermore I would really like to know how much information we
actually gather from the crash dumps because of PAGE_GUARD: I am not aware
of
any bug report/issue mentioning this in the last years. I doubt that the
trouble
of a weird use of page flags is paying off, it can only detect a tiny
fraction
of bugs/security issues, and I am not convinced why these kind of issues
should
be more important than others.

https://codereview.chromium.org/23458022/

[c...@chromium.org]

On 2013/09/09 07:40:39, Sven Panne wrote:
> Hmmm, actually I would prefer PAGE_NOACCESS, too. MSDN explicitly
> describes
> PAGE_GUARD as a mechanism for growable data structures, e.g.

http://msdn.microsoft.com/en-us/library/windows/desktop/aa366549%28v=vs.85%29.aspx.

> Therefore I think it is a terrible idea to use this for security, this is
> exactly what PAGE_NOACCESS is for. It is very weird that we depend on the
> embedder's handling of PAGE_GUARD, consider e.g. using v8 from another
language
> implementation (Haskell, OCaml, etc.) where stacks grown on demand via
> PAGE_GUARD. Furthermore I would really like to know how much information
> we
> actually gather from the crash dumps because of PAGE_GUARD: I am not
> aware of
> any bug report/issue mentioning this in the last years. I doubt that the
trouble
> of a weird use of page flags is paying off, it can only detect a tiny
> fraction
> of bugs/security issues, and I am not convinced why these kind of issues
should
> be more important than others.

SOunds good. I am fine with changing this to PAGE_NOACCESS. Thanks guys

https://codereview.chromium.org/23458022/

[c...@chromium.org]

lgtm

https://codereview.chromium.org/23458022/

[bme...@chromium.org]

Committed patchset #1 manually as r16604 (presubmit successful).

https://codereview.chromium.org/23458022/