Env sanitization in Graphene pre-main wrapper

[mkow]

Hi,

I took a look at your Graphene pre-main code and I noticed that you allow LD_LIBRARY_PATH to be passed from untrusted (if I understand correctly) source. This sounds very risky to me - if user manifest contains some allowed directories (kind-of "passthrough" mounts in Graphene's confusing terminology) it will allow for arbitrary code execution from attacker.

See:

marblerun/cmd/premain-graphene/main.go

Lines 21 to 25 in fb51ebf

	if !strings.HasPrefix(env, "EDG_") && !strings.HasPrefix(env, "LD_LIBRARY_PATH=") {
	if err := os.Unsetenv(strings.SplitN(env, "=", 2)[0]); err != nil {
	panic(err)
	}
	}

[m1ghtym0]

Thanks, @mkow for reviewing our pre-main code and reaching out!
We are unhappy with the "hack" of using loader.insecure__use_host_env=1 in the first place.
Our pre-main needs some dynamic configuration that we pass in via env variables and that we can't "hardcode" in the Graphene manifest.
At the same time, we need the LD_LIBRARY_PATH of the Graphene Manifest.
We were under the impression that the LD_LIBRARY_PATH is set by the Graphene Manifest and overwrites the host's value.
Nevertheless, we're now looking into getting rid of loader.insecure__use_host_env=1 entirely.

[dimakuv]

We were under the impression that the LD_LIBRARY_PATH is set by the Graphene Manifest and overwrites the host's value.

That's true. But if it is not set and at the same time you have loader.insecure__use_host_env=1, then the untrusted envvar value is propagated inside the Graphene enclave.

[mkow]

Our pre-main needs some dynamic configuration that we pass in via env variables and that we can't "hardcode" in the Graphene manifest.

Why can't this be a part of the enclave? The problem is that a lot of popularly used envs are really dangerous and should really get into the measurement and be verified. If your "dynamic configuration" allows changing the code which will execute inside the enclave, maybe it shouldn't actually be dynamic? :)

[Nirusu]

As said in the other ticket in Graphene, we don't care about LD_LIBRARY_PATH ourselves, it just that we needed to pass this through for correct execution of the main application. Apart from this we don't really want to touch it.

[mkow]

Seems the discussion got split between those two issues, so just for bookkeeping I'll copy my response here: Until we get this feature [gramineproject/graphene#2356], please ensure that LD_LIBRARY_PATH is actually defined in the user manifest, otherwise this will be completely insecure (and we can't expect users to know this quirk).

[Nirusu]

I am wondering, is there actually any sane way to check the current manifest from inside Graphene's environment? Because I am not sure how we could reliably check against the set values in the manifest without making certain assumptions, e.g. manifest lies in the current working directory and has a certain name pattern.

What would you suggest would be a reliable way to check this? Does not need 100% perfect as we plan to switch to the feature in gramineproject/graphene#2356 in the future, but also should not be a terrible hack ;)

[dimakuv]

@Nirusu I don't think there is any sane way to peek into the current manifest.

Technically, we have an internal VMA that holds the manifest content (in raw UTF-8 text): https://github.com/oscarlab/graphene/blob/5230827b3206049add4b2f5331e057bc1fd7b797/LibOS/shim/src/bookkeep/shim_vma.c#L539. The g_pal_control->preloaded_ranges array is filled in the PAL layer here: https://github.com/oscarlab/graphene/blob/5230827b3206049add4b2f5331e057bc1fd7b797/Pal/src/host/Linux-SGX/db_main.c#L693

We don't expose internal VMAs when the in-enclave app does /proc/self/maps, so one can't deduce the manifest base address in this way...

We could provide a new pseudo-file like /dev/manifest or /proc/self/manifest, which would be read-only and return the raw UTF-8 manifest contents. @mkow What do you think about this? Seems like a generally useful feature.

[mkow]

Why do this at the Graphene / pre-main wrapper level? The coordinator knows the manifests, right? It could do sanity check on them and refuse to run such insecure configs.

[Nirusu]

Uh, I don't quite get what you mean. We should define LD_LIBRARY_PATH in the Marblerun manifest?

The premain establishes contact to the Coordinator, and the Coordinator does not know the Graphene manifest. It knows its own manifest, containing MRENCLAVE / MRSIGNER (+ related) and the environment variables / files which should be provisioned from the Coordinator (not from Graphene directly). Except for the quote generation code (and the standalone premain), the Coordinator has no libOS specific code.

During the launch of the premain, the Coordinator is not involved so far. The premain launches just as any other Graphene application. The application authenticates itself to the Coordinator during the premain based on the SGX SIGSTRUCT values, but not before.

[mkow]

Uh, I don't quite get what you mean. We should define LD_LIBRARY_PATH in the Marblerun manifest?

Yup, otherwise you'll pass unstrusted LD_LIBRARY_PATH directly to the app, right?
If I understand correctly, you use loader.insecure__use_host_env in it? It may be ok-ish because Go binaries are statically linked, so they probably don't use ld.so, but it's still a bit risky, e.g. because Go may have some magic envs which influence its execution.

The premain establishes contact to the Coordinator, and the Coordinator does not know the Graphene manifest. It knows its own manifest, containing MRENCLAVE / MRSIGNER (+ related) and the environment variables / files which should be provisioned from the Coordinator (not from Graphene directly). Except for the quote generation code (and the standalone premain), the Coordinator has no libOS specific code.

Ok, I see.

During the launch of the premain, the Coordinator is not involved so far. The premain launches just as any other Graphene application. The application authenticates itself to the Coordinator during the premain based on the SGX SIGSTRUCT values, but not before.

Yeah, I know, maybe I wasn't specific enough - what I meant is forbiding such insecure configurations during attestation of premain or somewhere in the toolchain used for enclave creation.

[Nirusu]

After thinking quite a bit about a potential workaround, I am not sure if it makes sense to invent a temporary solution for the time being until the selective environment pass-through feature is implemented in Graphene.

Here are my thoughts about this issue:

• The graphene-premain process is actually dynamically linked, hence it needs LD_LIBRARY_PATH to be set.

• To get the premain process running, an user would either need to specify LD_LIBRARY_PATH inside the Graphene manifest, specify it manually on the host side when launching pal-loader / graphene-sgx, or having it set in the background already. Now, the first two cases seem like non-issues to me, the latter one an issue.

However, there does not seem to be an easy workaround here:

• The premain could only be launched with LD_LIBRARY_PATH set, so this var already needs to be defined in some way.

• We cannot verify this against the Graphene manifest in a reliable way, as @dimakuv mentions there is no good sane way to peek inside the effective manifest of the enclave currently. We could read this from disk, but this is an issue by itself (File name? TOCTOU?).

• Setting LD_LIBRARY_PATH again in Marblerun's manifest could work out, however by the time the new environment variables are provisioned from the Coordinator, a malicious user could have already injected code via LD_LIBRARY_PATH if they wanted to.

I think the only real effective workaround for this issue seems either to build our premain process statically, let the user define LD_LIBRARY_PATH in Marblerun's manifest.json, and then provision LD_LIBRARY_PATH from there, or shift the Coordinator "discovery config" to a file, pass it as an allowed file to the premain and kick out the host-env pass-through.

However, the latter one makes configuration through K8s more difficult, and both workaround would become redundant for the future. anyway. This poses the question if all this is really worth the hassle when we are dealing with non production-ready software so far, given that you wanted to implement a more proper fix into Graphene soon. Reworking the premains, rewriting the docs and making the "Getting Started" process more difficult yet still carrying the "insecure" flag due to host env pass-through seem like quite the effort to go for a temporary solution.

I guess for the time being, we would choose to warn the users in our documentation / samples that they should really set LD_LIBRARY_PATH in the Graphene manifest, and then later on fix this properly when you introduce the feature of passing through certain environment variables.

Does this work out for you for the current time being? Of course we would keep the issue open here and not run away from it, however so far the workaround do not seem good, and we think it is not too critical as this is not production-grade yet (and screams "insecure" on launch anyway).

[dimakuv]

I have no objections. Thanks for the detailed analysis!

The premain could only be launched with LD_LIBRARY_PATH set, so this var already needs to be defined in some way.

Why is this? The default library paths are not enough for you premain?

[Nirusu]

Which default library paths? LD_LIBRARY_PATH is unset by default on my host system.

Otherweise, there is nothing so special about the needed libraries. But it does not launch LD_LIBRARY_PATH unset in Graphene, but fine when running on my host directly.

$ ldd premain-graphene
linux-vdso.so.1 =>  (0x00007fffda3db000)
libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007fe34c320000)
libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fe34bf50000)
/lib64/ld-linux-x86-64.so.2 (0x00007fe34d400000)

[dimakuv]

LD_LIBRARY_PATH doesn't need to be set for dynamically linked programs to run. There is a set of default library paths that is hard-coded in the linker (I think? or in one of those /etc/ files). And the envvar LD_LIBRARY_PATH simply "prepends" more library paths (non-standard ones), so that your program may find its non-standard libraries in non-standard places.

See also https://tldp.org/HOWTO/Program-Library-HOWTO/shared-libraries.html, Section 3.3.1.

Looking at the output of your ldd premain-graphene, there are no special library paths needed to run it. Why doesn't it run without LD_LIBRARY_PATH? What do you need to specify in this variable, and what kind of error do you get?

[thomasten]

I'm confused. Isn't the default way that a Graphene app mounts its libs at /lib and sets LD_LIBRARY_PATH accordingly?

[dimakuv]

I'm confused. Isn't the default way that a Graphene app mounts its libs at /lib and sets LD_LIBRARY_PATH accordingly?

Yes, this is correct. Sorry, I was thinking about premain-graphene and its dependencies. Now I understand what @Nirusu meant.

But I got lost somewhere here. If you have something like this in the Graphene manifest file:

loader.env.LD_LIBRARY_PATH = "/lib:/lib/x86_64-linux-gnu:/usr/lib/x86_64-linux-gnu"
loader.insecure__use_host_env = 1

Then LD_LIBRARY_PATH will be definitely overwritten to this trusted value hard-coded in the manifest. This is also what @Nirusu meant when saying "The premain could only be launched with LD_LIBRARY_PATH set, so this var already needs to be defined in some way" (and this way is -- define in the Graphene manifest).

I have a feeling that @mkow confused us all :) Michal, what was your problem with all this exactly?

[mkow]

The premain could only be launched with LD_LIBRARY_PATH set, so this var already needs to be defined in some way.

This is a very brittle assumption to rely on. If you want to go this way, then I think you should explicitly check if this path is set in the manifest and disallow running without it, there's no guarantee from Graphene side that "not customizing LD_LIBRARY_PATH has to lead to a non-working application".

Anyways, this is just a tip of the iceberg, loader.insecure__use_host_env is just insecure, as its name suggests. What if the malicious host provided LD_PRELOAD? You'll get pwned even before your filtering loop in premain manages to get executed :) You may say "but we can also put this into the manifest!". Then the attacker will provide LD_AUDIT. Then you'll block the whole list from ld.so's manpages, but still leave GODEBUG. I think at this point you can see where this is going. It's just the whole design which is insecure.

[Nirusu]

The point is understood, though this makes it sound worse than it is because all these LD shenanigans still only works when the malicious libraries are trusted or pass-through mounted. If not, well that seems to be an issue with Graphene in my opinion when a malicious host can just pass any unblessed executable code into an enclave.

Anyway, I think we all agree that insecure__use_host_env has to go away, but for our Marblerun case all alternatives I can think of for a current workaround are clumsy. I tried replacing the host env vars with a JSON config file, however this kind of makes our "switch Marble type dynamically" approach difficult to work with, used e.g. by our Redis sample. We need some kind of dynamical way to pass configuration through to the enclave. And while I thought that files would be a workaround for the time being... Well, how do I tell the enclave which config file to pick when envp and argv are both either static or potentially malicious?

Until we get gramineproject/graphene#2356, what do you suggest we could do here to mitigate this without compromising the dynamical configuration, if we do not want to pass envp/argv to the host? Because I am running out of ideas of how to do it in a sane way :)

[mkow]

The point is understood, though this makes it sound worse than it is because all these LD shenanigans still only works when the malicious libraries are trusted or pass-through mounted.

Only a single pass-through mount is required for this. Also, a "malicious" library may not even be needed, its the usage which is malicious, not necessarily the library itself. E.g. /bin/sh mounted inside the enclave can be both used as part of the workload and to gain code execution by the attacker. I think with dynamic libraries it's the same, it's just a bit harder, because you need to abuse some dynamic library, not an executable. The problem is not with the in-enclave filesystem containing some code, but with untrusted world controlling the execution.

If not, well that seems to be an issue with Graphene in my opinion when a malicious host can just pass any unblessed executable code into an enclave.

Same as above. Placing untrusted executables inside the enclave FS is perfectly fine, as long as it's inside the paths which the user allowed in the manifest. If you want to block this, then I think you're addressing the problem at a wrong point (as explained above).

Well, how do I tell the enclave which config file to pick when envp and argv are both either static or potentially malicious?

Depends if you want the config file to be integrity-checked or not. If you treat it as untrusted, then just pass it's name through argv. Untrusted argv is much less dangerous if you control the entry executable (but in your case, please carefully check if Go doesn't have any magic argvs, like it does with env). Or maybe even skip the config file, and pass everything through argv?

Until we get gramineproject/graphene#2356, what do you suggest we could do here to mitigate this without compromising the dynamical configuration, if we do not want to pass envp/argv to the host? Because I am running out of ideas of how to do it in a sane way :)

As above, argv is much less dangerous if the entry executable is your own binary :) (contrary to e.g. using Python interpreter as entrypoint). I mean, it's possible to do this securely without too much effort.

[Nirusu]

Nope, not trying to adjust it the problem by saying "your fault for putting this inside the enclave".

But these kinds of wrong approaches is also why I am not happy about replacing envp with argv for the time being. While properly way better than envp (there should be no magic Go flags AFAIK), it's still treated as an insecure flag and additionally requires quite some changes to our K8s integration. That's quite a bit of effort for a temporary workaround that's still not usable as a real solution for us.

[thomasten]

Thanks for all the input @mkow, much appreciated! We now understand that untrusted/insecure env is a no-go and there is no reliable way to secure it. argv poses problems on our side, and is also flagged insecure in Graphene, so we don't want to go with that. For now, we will hardcode the env vars in the simpler samples. Regarding the samples that need dynamic env vars, we will document that this is not ready for production yet.

We would be grateful if you can add a feature like gramineproject/graphene#2356 in the near future.

[m1ghtym0]

@daniel-weisse If I didn't miss anything the feature is in the official Gramine release and already used in our Gramine integration, right? If so, feel free to close the issue.