Open AndreyAkinshin opened 9 years ago
Here we create two nullable ints, pass it to Corrupt1 , pass it to Corrupt2 , and print full 4-bytes value of HasValue of the second parameters. The expected value: 00000001 , but you can actually get something like E0D3E201 .
Why is the expected value 1? The size of bool
is 1 byte and you're attempting to read 4 bytes from that bool's location. Is there any text in the ECMA spec that requires padding to be 0 initialized?
Well,
bool b = false; Console.WriteLine("{0:X8}", b);
Prints False.
Despite what he is doing, he is actually expected to be calling the WriteLine with a boolean argument.
Hibernating Rhinos Ltd
Oren Eini* l CEO l *Mobile: + 972-52-548-6969
Office: +972-4-622-7811 l Fax: +972-153-4-622-7811
On Wed, Jul 29, 2015 at 8:15 AM, mikedn notifications@github.com wrote:
Here we create two nullable ints, pass it to Corrupt1 , pass it to Corrupt2 , and print full 4-bytes value of HasValue of the second parameters. The expected value: 00000001 , but you can actually get something like E0D3E201 .
Why is the expected value 1? The size of bool is 1 byte and you're attempting to read 4 bytes from that bool's location. Is there any text in the ECMA spec that requires padding to be 0 initialized?
— Reply to this email directly or view it on GitHub https://github.com/dotnet/coreclr/issues/1305#issuecomment-125846552.
The HasValue
name is misleading, it's not a bool
, it's an int
. You'd expect the observable behavior of struct to be fully zero-initialized, even if in practice the optimizer may omit some of that initialization (as long as it's not observable). I suspect that the backing int
storage of an int?
isn't speced and that the exact bits of a true boolean aren't; so if you're overlapping such a value using explicit struct layout you may well have undefined behavior. However, regardless of that, you'd hope that doing so would not leak unrelated stack information since that's a potential security-relevant information disclosure. I don't think the chance of this kind of code being relied on in the wild is very large, however...
@EamonNerbonne HasValue is really a bool, not an int. If you create a Nullable<byte>
, it occupies 2 bytes. Nullable<short>
occupies 4 bytes.
The fact that the HasValue occupies four bytes in your case is really just an alignment padding.
@janvorli, my question is the following: is it legal that we have some information from the old stack frame in the padding?
Turns out that the answer is in the C# spec though not in an obvious location:
18.5.8 ... For alignment purposes, there may be unnamed padding at the beginning of a struct, within a struct, and at the end of the struct. The contents of the bits used as padding are indeterminate.
There's no way this is a JIT bug as the code is incorrect to begin with. In the best case this is a feature request to prevent people from shooting themselves in the foot when they get things wrong.
Also, the ECMA standard says:
For a nullable type, the default value is one for which HasValue returns false.
@mikedn, ok, now I agree with you, it is not a JIT bug. However, what do you think: should RyuJIT initialize int?
with zeros or the current behavior is acceptable?
@AndreyAkinshin I haven't actually tested but I see that PEVerify doesn't complain about such types even if the ECMA spec appears to imply that they are not verifiable somewhere in II.22.8:
Note that an ExplicitLayout type might result in a verifiable type, provided the layout does not create a type whose fields overlap.
If such type are indeed verifiable then there's probably a small security risk in leaking information to some sort of sandboxed code. I'm not very familiar with the .NET security stuff (full trust FTW!) so I can't comment much on this. I'm tempted to say that this is a rather obscure situation and as such it's not worth the trouble.
@mikedn, ok, the issue has been updated.
OK, I did some testing with sandboxed code and this kind of leak can't happen. Types with overlapping fields can be used in code that lacks SkipVerification permission but only if the overlapping field types have only public fields themselves or are primitives. Nullable<T>
doesn't have any public fields so attempts to overlap nullable fields result in an exception:
System.TypeLoadException: Illegal layout for type 'ClassLibrary1.FooBar' from assembly 'ClassLibrary1, Version=1.0.0.0, Culture=neutral, PublicKeyToken=5e1ccff8d2fa4fe6': valuetypes with non-public fields may not overlap with other fields.
That makes sense since otherwise the sandboxed code could easily access private fields by using such tricks.
Besides, inter appdomain calls require marshaling and that serializes valuetypes passed in as parameters. Since serialization only cares about fields the padding bits are lost during such calls.
This is an interesting case. As others have also concluded, I do not believe it is a JIT bug. It is not required to zero out the padding in a struct with holes in its layout. The JIT is zero-initializing the struct with overlapping fields, but since the int? struct it is copying from has "indeterminate" values, those are propagated to the NullableInt struct.
That said, I would say that this is a spec bug, and that probably it should either be unverifiable to initialize an overlapping struct with values that have "holes" (of course, the spec would have to be much crisper about that), OR when that happens, the JIT should be required to fill the padding with zeros. It would be unfortunate to penalize the usual case of structs without explicit layout by requiring that all padding be zero-initialized.
Although II.22.8 seems to imply that overlapping fields may not be verifiable, that text is "informative only", and the only specific cases that are called out are in II.10.7 "Controlling instance layout" (and also mentioned in the partition III) where the spec only mentions overlapping of reference types or overlapping of fields with different accessibility.
It turns out that you can actually have problems with sandboxed code if you use your own structs:
public class Sandboxed : MarshalByRefObject {
private struct Null {
public bool b;
public int i;
}
[StructLayout(LayoutKind.Explicit)]
private struct FooBar {
[FieldOffset(0)]
public Null n;
[FieldOffset(0)]
public int b;
}
public void Hello() {
Foo(new Null { b = true });
}
private static void Foo(Null x) {
Console.WriteLine("{0:X8}", new FooBar { n = x }.b);
}
}
Unlike the original code this code passes verification because it doesn't attempt to overlap valuetypes with non-public fields. It also doesn't relay on the caller to pass in any values, it creates its own value. This allows the sandboxed code to extract some bits of information that remained on the stack from a previous method call. This could be a security issue as the same stack is used by both the caller and sandboxed appdomains.
@janvorli I meant the HasValue
in the original code which is an int: [FieldOffset(0)] public int HasValue;
The bits may be indeterminate, but that just means the JIT isn't required to fill them with zero's (or some other pattern). Distinct from that spec, it's a potential security risk to leak bits from other memory locations - so while the bits may be indeterminate, that doesn't mean it's wise to leave them unassigned.
I'm joining this issue because I've been directed here by @jkotas from dotnet/coreclr#8742
As I understand it, there's a difference between RyuJIT and legacy JIT where the legacy JIT initializes padding in structs with zero, whereas RyuJIT does not.
If this is correct, I believe this change has the potential to open up security bugs in applications that will migrate to RyuJIT.
Consider (low level) networking code that reads and writes packets to an remote end. Those packets may have been defined as structs, and you can use methods such as Marshal.StructureToPtr
to serialize them from memory to the network.
There's code doing that; I've been able to identify a couple of projects using, for example, the NetworkStream Marshal.StructureToPtr site:github.com search query.
I also believe it is reasonable for the code to expect the padding to be initialized with zeros. It appears the spec states that, this is from 8.9.7 Value type definition in ECMA 335
8.9.7 Value type definition
- Unlike object types, instances of value types do not require a constructor to be called when an instance is created. Instead, the verification rules require that verifiable code initialize instances to zero (null for object fields)
(emphasis mine).
As a general remark, I don't buy into the "people shooting themselves in the foot" argument - I thought one of the main selling points of a managed runtime/language was that the runtime tries to prevent you from doing that.
Net, the conclusion is that the change in how padding is treated in RyuJIT vs regular JIT will now cause code that does networking, relies on structures to read/write messages to/from the network and has padding in these structures to leak memory values to the remote end. That doesn't sound good; it also looks like with netstandard2.0
, it'll be easy to consume libraries written for .NET/regular JIT on .NET Core with RyuJIT, further widening the scope of this problem.
Just imagine the blog headlines: "A very subtle, hard to identify security bug was introduced in my application (by a 3rd party library) when migrating from .NET to .NET Core/regular JIT to RyuJIT".
Thoughts?
There are cases where the gaps are left uninitialized with old JIT on full .NET Framework as well. You are just lucky that you are not running into them. RyuJIT and .NET Core has better optimizations and thus the cases where the gaps are left uninitialized are more frequent.
@qmfrederik It does initialize the fields to null, no? It is just the padding that isn't set, and how would you even see that, if you are accessing just the data through the struct?
@ayende If you serialize the struct to a byte array via Marshal.StructureToPtr for example, and use the result in networking code - then everyone can see it. See the Google in my previous comment, it shows up some code that may hit this behavior.
@jkotas I wad thinking avout the optimizations. Is it really more performant to zero out struct's memory one field at a time as opposed to the entire memory block at once? Take a struct with an int, a bit and 3 bits of padding. Would there be an impact to just initialize all 8 bits to zero at once?
Just imagine the blog headlines: "A very subtle, hard to identify security bug was introduced in my application (by a 3rd party library) when migrating from .NET to .NET Core/regular JIT to RyuJIT".
Do you have a specific security bug in mind apart from the potential information disclosure issue that has already been mentioned?
As a general remark, I don't buy into the "people shooting themselves in the foot" argument - I thought one of the main selling points of a managed runtime/language was that the runtime tries to prevent you from doing that.
The moment you use things like Marshal.StructureToPtr
the whole "managed" aspect is thrown out of the window.
@qmfrederik Given that strings and arrays don't typically live on the stack, I would be very surprised if you'll get any real leakage from such a scenario. And if you are passing raw structures around via the network, you are already doing several things wrong and making assumptions about the security of the communication.
I wad thinking avout the optimizations. Is it really more performant to zero out struct's memory one field at a time as opposed to the entire memory block at once? Take a struct with an int, a bit and 3 bits of padding. Would there be an impact to just initialize all 8 bits to zero at once?
The JIT already does too much zero initialization as it is. I don't see why it should do even more zero initialization just to handle a corner case. With the exception of the weird case of explicit field layout normal, verifiable code can't access the padding bytes.
If you serialize the struct to a byte array via Marshal.StructureToPtr
StructureToPtr is not a serialization tool, it is an interop marshaling tool. If you are using StructureToPtr for anything other than interop then be prepared for surprises.
@mikedn
Do you have a specific security bug in mind apart from the potential information disclosure issue that has already been mentioned?
Is potential information disclosure not important enough?
The JIT already does too much zero initialization as it is. I don't see why it should do even more zero initialization just to handle a corner case.
But doesn't 8.9.7 Value type definition in ECMA 335 state it should? I also have a hard time seeing how code which takes the size of a struct and initializes the entire memory block to zero would be slower than code which inspects a struct, loops over all fields and initializes each field to zero.
@ayende
And if you are passing raw structures around via the network, you are already doing several things wrong
Perhaps, but the point is that there's already code out there doing that, and I haven't been able to reproduce this behavior on NetFX; so all of a sudden you get a change in behavior when moving to .NET Core. At the very least, something to think about.
@CarolEidt You mentioned this might be considered a spec bug. If it is, what's the correct way raise a bug against the spec?
Is potential information disclosure not important enough?
Important or not the question was if there's another potential security issue that hasn't yet been spotted.
But doesn't 8.9.7 Value type definition in ECMA 335 state it should?
The specification rarely discusses the issue of padding and it's not clear if operations such as "initialize instances to zero" include padding or not. At the end of the day we could very well blame other parts of the runtime:
But again, verification is optional and in CoreCLR is not even enabled. And "verification rules require that verifiable code initialize instances to zero (null for object fields)" really means that verifiable IL is supposed to use .locals init
and initobj
as needed. That the JIT decides to throw away initobj
& co. and then you use StructureToPtr and send the resulting bytes over the network has little to do with verification rules.
I also have a hard time seeing how code which takes the size of a struct and initializes the entire memory block to zero would be slower than code which inspects a struct, loops over all fields and initializes each field to zero.
The code that loops over all field and initializes each field to zero doesn't actually exist. See the code that I posted in your original issue.
so all of a sudden you get a change in behavior when moving to .NET Core
This isn't .NET Core specific, .NET Framework + RyuJIT generates identical code.
You mentioned this might be considered a spec bug. If it is, what's the correct way raise a bug against the spec?
The potential spec bug is that verifiable code can access the unnitialized padding bytes, not that the padding bytes aren't zero initialized.
@mikedn Thanks for the feedback!
To your points:
Important or not the question was if there's another potential security issue that hasn't yet been spotted.
I have not; the security-related impact I'm referring to is parts of memory on the stack being unintentionally disclosed.
At the end of the day we could very well blame other parts of the runtime:
- Why does boxing (your example code does boxing) copy the padding bytes?
Interestingly, I have not observed this behavior when using the generic version of Marshal.StructureToPtr
(so when removing the boxing from the code). I'm not sure whether that's just a coincidence or not.
This isn't .NET Core specific, .NET Framework + RyuJIT generates identical code.
OK, thanks for the clarification - although with the code I posted in the other issue, the test passes on net461
but fails on netcoreapp1.0
. Perhaps it's just a coincidence?
The potential spec bug is that verifiable code can access the unnitialized padding bytes, not that the padding bytes aren't zero initialized.
I think @CarolEidt said that:
I would say that this is a spec bug, and that probably it should either be unverifiable to initialize an overlapping struct with values that have "holes" (of course, the spec would have to be much crisper about that), OR when that happens, the JIT should be required to fill the padding with zeros.
which I read differently from how you read it.
Interestingly, I have not observed this behavior when using the generic version of Marshal.StructureToPtr (so when removing the boxing from the code). I'm not sure whether that's just a coincidence or not.
The lack of zero initialization of padding bytes is the result of a compiler optimization and optimizations aren't guaranteed to be always performed. Or they are performed and combined with other optimizations and then you get different results. For example, if initialization and boxing happen in the same method it's possible that the compiler eliminates the intermediary stack variable and directly initializes the boxed struct. Since boxed structs are reference types and reference type are always fully zero initialized the issue disappears.
OK, thanks for the clarification - although with the code I posted in the other issue, the test passes on net461 but fails on netcoreapp1.0. Perhaps it's just a coincidence?
The test depends on the stack contents. If the stack location happens to already contain 0 then the test will pass. There may be other differences in code generation that affect the stack layout/contents. Or the .NET Framework version you tested with doesn't yet contain this optimization (I tested with the latest Win 10 preview which probably has some sort of .NET 4.6.3).
which I read differently from how you read it.
The " it should either be unverifiable to initialize an overlapping struct ..." part refers to the situation we have in this particular issue - structs with explicit field layout that pass as verifiable and allow one to access padding bytes. Your code doesn't have the same issue.
@mikedn Thanks for the very detailed answer.
Reading the answer, there seem to be a lot of factors in the equation, which make that in most development scenarios, you'll never see this happening. You're only likely to detect this in release mode, and even only then if you're very lucky.
Plus, consensus seems to be that the spec, at best, is not very clear about what is supposed to happen.
I did some more Googling and there's a lot of code out there that uses Marshal.StructureToPtr
to serialize a struct and then write it to a stream. The pattern doesn't seem uncommon for binary network protocols that usually define their headers as basic C structs.
Net, because
I still think it's something which should be prevent from happening :)
If it can't be the default behavior for perf reasons (although I'd like to better understand them), perhaps an attribute on the struct layout to state whether or not to initialize the padding bytes? Or special handling in Marshal.StructureToPtr
?
I did some more Googling and there's a lot of code out there that uses Marshal.StructureToPtr to serialize a struct and then write it to a stream. The pattern doesn't seem uncommon for binary network protocols that usually define their headers as basic C structs.
That's unfortunate. I don't think one needs to be familiar with inner JIT workings to realize that using something from the Marshal
class for purposes other than interop marshaling might not be a great option. But yes, it can be convenient and people tend to like that.
That said, StructureToPtr isn't the only way to get at those uninitialized padding bytes. There's unsafe code, explicit struct layout, memory mapped files (they allow structs to be written and probably suffer from the same issue as StructureToPtr). Ultimately the main problem is that many aren't even aware of the existing of padding bytes.
I don't know, maybe the JIT should just initialize structs with padding and call it a day. But after seeing the rather numerous complaints about performance I'm not sure if people will easily agree to that.
Well, in a lot of binary file layouts/network protocols you'll find entire data blocks or at least headers defined as C structs.
Take the TCP packet header. If I have a byte stream that I know contains a TCP header, I could define the TCP Header as a struct in C# and use Marshal
/unsafe/... as an easy way to convert the byte stream into an object which I can manipulate.
Is there something fundamentally wrong about representing things like the TCP header as a struct in C#? And what would be the alternative to using Marshal
/unsafe? Manually crafting code that reads the data field by field? That's a lot of code to write, and a lot of ways bugs can creep into your code.
(Happy to continue this part of the conversation through a different channel if it would be cluttering this GitHub bug report)
Is there something fundamentally wrong about representing things like the TCP header as a struct in C#?
Not really (and apart from manually writing the individual fields there aren't many alternatives - there are serialization frameworks for this but sometimes they're overkill) but you have to be careful. If you have padding you can widen the fields or add new fields to make that padding "explicit". byte
field followed by an int
? Make it int
. short
field followed by an int
? Make it int
or add another short
field to act as padding. And so on.
C/C++ have similar issues. For example some C/C++ compilers won't initialize the padding in the following example
struct Hdr {
char c;
int i;
};
void init(Hdr* p) {
Hdr h { 2, 3 };
*p = h;
}
Well, at least in C/C++ there's memset
(though you can't be 100% sure that the compiler doesn't optimize that too, that's how we ended up with things like SecureZeroMemory
).
(Happy to continue this part of the conversation through a different channel if it would be cluttering this GitHub bug report)
Seems fine to me. It adds a different perspective to the existing issue.
@qmfrederik in any such case, you'll always have very explicit control over padding.
@ayende - Not sure I follow. I would prefer to just use something like [StructLayout(Pack = 4)]
if I know that's the convention, rather than having to use the options @mikedn summarized.
But if I do that, I hit the issue described in this bug.
@mikedn - Yup, I kind of think that summarizes it - if you have structs that are externally visible and you want to make sure the padding is zero, your option right now is to make the padding "explicit" or widen the field (but that will result in another issue, as you now can assign a value of 1024 to a field that was supposed to be a byte).
My take from this - there "should" (I know it's easy for me to say) be an option to state that you want your padding initialized to zero. I'd even vote for that to be the default behavior. As we've seen on the other issue, I don't think it always incurs a performance penalty (but that's up for debate).
What that option should look like - the default behavior, an extra parameter to [StructLayout]
or just an explicit call equivalent to SecureZeroMemory
- I don't know :)
I do a lot of work with structs, and we always use Explicit
and [FieldOffset]
to control exactly where everything is going to be.
@qmfrederik Note that this call will do it:
fixed(YourStruct*p = val)
{
memset(p, 0, sizeof(YourStruct)); // won't be optimized away by the C# compiler since it doesn't know about it :-)
}
@ayende Cool, thanks!
Just to be sure: with Explicit
and [FieldOffset]
, you can still have implicit padding in your struct, like in this example:
[StructLayout(LayoutKind.Explicit, Pack = 4)]
internal struct ConstTestStruct
{
[MarshalAs(UnmanagedType.U4)]
[FieldOffset(0)]
public int Number1;
[MarshalAs(UnmanagedType.U1)]
[FieldOffset(4)]
public byte Number2;
}
So the last three bytes of this struct could still contain random values from the stack. So I'd need to either widen Number2
to an int
or add a byte
and a short
to manually pad the struct. (Not something an IL weaver couldn't do).
Regarding memset
, apart from some extra work that would be required to make it portable, seems like it would do the trick. But it's a pity you can't have parameter-less constructors for structs in C# :).
an explicit call equivalent to
SecureZeroMemory
Adding zero memory API that is guaranteed to not be optimized away sounds fine to me.
@qmfrederik If you want to have no padding, StructLayout
have a Size
property.
Note that if you want a cleared struct, you can do PtrToStrucutre from known zero buffer.
@jkotas As I understand it, the C# compiler will never optimize a pinvoke call, so I don't think it would be needed.
RyuJIT doesn't initialize new nullable structs with zeros. It is not a JIT bug (see @mikedn's comment). However, theoretically, it can be a security hole because RyuJIT takes data from the previous stack frame. I suggest to discuss such behavior.
Description
Let's consider the following code:
Here we create two nullable ints, pass it to
Corrupt1
, pass it toCorrupt2
, and print full 4-bytes value ofHasValue
of the second parameters. The expected value:00000001
, but you can actually get something likeE0D3E201
.Explanation
What does it mean? The last byte of
E0D3E201
is01
, it is actual value ofHasValue
. But what aboutE0D3E2
? It is some data on the stack: RyuJIT doesn't fill bytes betweenHasValue
andValue
by zeros. Let's look to the asm:As we can see, RyuJIT uses
byte ptr
to setHasValue
. This instruction sets only one byte and keeps 3 other bytes untouched. What do you think, is it ok? Can JIT do it? Is there any troubles with marshaling in this case?The LegacyJIT add an additional instruction which initialize all bytes of
int?
with zeros:A usage example
Let's consider the following situation: we have some secret data (
SuperSecretChar
in the example) and we use it for some secret purpose (SuperSecretLogic
,SuperSecretLogic2
). After returning from the secret logic, some secret data can remain on the stack. Next, we try to run our own logic that try to catch a portion of secret data with help ofint?
. The source:If you run the code, you should see the following result:
This specific logic creates
p4
exactly in the place with the target secret data.As I know, old JITs always initialize all bytes of new struct instance. Is now behavior is correct?
category:security theme:zero-init skill-level:intermediate cost:medium