Change compiler to safely write unsafe smis when they are spilled from...

Change compiler to safely write unsafe smis when they are spilled from
the frame, not when they are pushed onto frame.  Also fixes unrelated
flaw in virtual-frame.cc.


Committed: http://code.google.com/p/v8/source/detail?r=1294

[Kevin Millikin (Chromium), 2009-02-16 14:03:34]

There doesn't seem to be any security problem with constant folding of arbitrary
smis, or specializing for one argument being an arbitrary smi.  The only place I
would check and choose an alternate instruction sequence is right at the point
of emitting an immediate in an instruction.

http://codereview.chromium.org/21392/diff/1/6
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/21392/diff/1/6#newcode1256
Line 1256: // Consider large constant smis to be non-smis, since using them
It seems like is should be safe to do compile-time comparison on any smi; and
the one-operand-is-smi case should be safe as long as it doesn't emit an
Immediate explicitly (in other words, push the test way down to just before you
emit the cmp instruction for right_side in the one-operand-is-smi case).

http://codereview.chromium.org/21392/diff/1/6#newcode1888
Line 1888: // should not be a security problem. min_index and range are from
source.
I don't understand this comment.  Naively, it seems that if min_index and range
are from the source then they *are* potentially unsafe smis.

http://codereview.chromium.org/21392/diff/1/6#newcode3041
Line 3041: // To prevent long attacker-controlled byte sequences in code, larger
Just get rid of this comment completely.  It's a bad idea to document something
that doesn't happen here, here.

[William Hesse, 2009-02-16 16:01:01]

http://codereview.chromium.org/21392/diff/1/6
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/21392/diff/1/6#newcode1256
Line 1256: // Consider large constant smis to be non-smis, since using them
On 2009/02/16 14:03:34, Kevin Millikin wrote:
> It seems like is should be safe to do compile-time comparison on any smi; and
> the one-operand-is-smi case should be safe as long as it doesn't emit an
> Immediate explicitly (in other words, push the test way down to just before
you
> emit the cmp instruction for right_side in the one-operand-is-smi case).

Done.

http://codereview.chromium.org/21392/diff/1/6#newcode1888
Line 1888: // should not be a security problem. min_index and range are from
source.
On 2009/02/16 14:03:34, Kevin Millikin wrote:
> I don't understand this comment.  Naively, it seems that if min_index and
range
> are from the source then they *are* potentially unsafe smis.

Done. min_index is handled if it is more than 16 bits, and the fast switch
statement is not generated if range is more than 16 bits.

http://codereview.chromium.org/21392/diff/1/6#newcode3041
Line 3041: // To prevent long attacker-controlled byte sequences in code, larger
On 2009/02/16 14:03:34, Kevin Millikin wrote:
> Just get rid of this comment completely.  It's a bad idea to document
something
> that doesn't happen here, here.

Done.

[Kevin Millikin (Chromium), 2009-02-16 16:09:06]

LGTM if it passes all the tests.

[iposva, 2009-02-17 06:36:37]

Please at least separate the unsafe smi spilling, loading from virtual frame
from the "additions to security" features in this change. Generally I think we
were safe before and I do think that at least the additions for the jump table
are broken.

http://codereview.chromium.org/21392/diff/2001/3005
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/21392/diff/2001/3005#newcode1309
Line 1309: if (IsUnsafeSmi(right_side.handle())) {
I think it is overkill to try to worry about a single user-controlled 4-byte
sequence in generated code such as this one here.

http://codereview.chromium.org/21392/diff/2001/3005#newcode1890
Line 1890: // should not be a security problem. min_index and range are from
source.
This comment makes no sense. Since min_index and range are controlled from
source these are user controlled values in the code, which you equal to security
problem. You are contradicting yourself.

http://codereview.chromium.org/21392/diff/2001/3005#newcode1895
Line 1895: Immediate((min_index << kSmiTagSize) & 0xFFFF0000));
What are you trying to achieve with this bit magic here? I would guess that it
goes pretty wrong when min_index is a negative value. Also since you are
shifting with kSmiTagSize before applying the mask you are losing the bits you
are trying to preserve (if my guess about your intention is right). If my guess
is wrong, then there should have been a comment...

Did you test this code?

http://codereview.chromium.org/21392/diff/2001/3005#newcode1898
Line 1898: }
Overall I think it is overkill to try to worry about a single user-controlled
4-byte sequence in generated code such as this one here.

http://codereview.chromium.org/21392/diff/2001/3005#newcode1903
Line 1903: // Range has already been tested against kMaxSmiInlinedBits, and is
OK.
In that case an ASSERT ensuring this statement into the future is necessary.

http://codereview.chromium.org/21392/diff/2001/3006
File src/codegen.cc (right):

http://codereview.chromium.org/21392/diff/2001/3006#newcode517
Line 517: // Range is more than 16 bits, unreasonable, insecure.
It is definitely not unreasonable to generate a jump table for a large range.
One of the reasons we have jump tables is to reduce the amount of generated
code. A 4-byte jump entry is a lot smaller than a compare&branch sequence.

Also I don't think being able to control a single 4-byte sequence in generated
code is insecure, but we should evaluate this.

[William Hesse, 2009-02-17 15:54:27]

I have separated out all the unrelated changes, and all this change list now
does is to move the safe spilling of smi constants to the point where they are
used or spilled.

Tested with IsUnsafeSmi always true, always false, and true for smis with more
than 16 bits, in debug and release mode, on  all tests.

http://codereview.chromium.org/21392/diff/2001/3005
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/21392/diff/2001/3005#newcode1309
Line 1309: if (IsUnsafeSmi(right_side.handle())) {
On 2009/02/17 06:36:37, iposva wrote:
> I think it is overkill to try to worry about a single user-controlled 4-byte
> sequence in generated code such as this one here.

You are right.  I will not add code for these two user-supplied ints in the fast
switch statement code.

http://codereview.chromium.org/21392/diff/2001/3005#newcode1898
Line 1898: }
On 2009/02/17 06:36:37, iposva wrote:
> Overall I think it is overkill to try to worry about a single user-controlled
> 4-byte sequence in generated code such as this one here.

All of this has been removed.

http://codereview.chromium.org/21392/diff/2001/3006
File src/codegen.cc (right):

http://codereview.chromium.org/21392/diff/2001/3006#newcode517
Line 517: // Range is more than 16 bits, unreasonable, insecure.
On 2009/02/17 06:36:37, iposva wrote:
> It is definitely not unreasonable to generate a jump table for a large range.
> One of the reasons we have jump tables is to reduce the amount of generated
> code. A 4-byte jump entry is a lot smaller than a compare&branch sequence.
> 
> Also I don't think being able to control a single 4-byte sequence in generated
> code is insecure, but we should evaluate this.

Removed

[iposva, 2009-02-17 17:27:06]

Otherwise LGTM

-Ivan

http://codereview.chromium.org/21392/diff/1008/1012
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/21392/diff/1008/1012#newcode1326
Line 1326: }
This apparently survived your purge of unrelated single 4-byte user controlled
sequences.

[William Hesse, 2009-02-18 09:04:56]

http://codereview.chromium.org/21392/diff/1008/1012
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/21392/diff/1008/1012#newcode1326
Line 1326: }
On 2009/02/17 17:27:06, iposva wrote:
> This apparently survived your purge of unrelated single 4-byte user controlled
> sequences.

This is not an unrelated change.  It is one of the few places where a constant
result is used as an immediate operand.  All these places should be guarded with
an unsafe smi check, and load unsafe smis with ToRegister or LoadUnsafeSmi. 
When other operations implement constant folding by treating constant results as
a special case, they will also need to use this guard.  Right now, binary
operations (and some comparisons) catch constants earlier, by spotting the
literal AST node type.  That code is being changed to recognize constant results
instead, like this does.