-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OPA process OOM when a bundle contains a large file despite having size_limit_bytes #6514
Comments
OPA seems to be doing the right thing here ie. rejecting the large file and returning a bundle load error which in turn should kick-off an exponential back-off delay based successive download. What's happening here probably is the memory is not getting released quick enough? I guess if GC were to happen we'd see memory being released. Go 1.19 introduced a concept of soft memory limit which helps to control GC behavior and it can be set via the |
@ashutosh-narkar do we really need to load the file at all if the file size exceeds |
Looking at the implementation now, and maybe I'm missing someting, but it seems like the NextFile() function greedily reads all files on the first invocation, then serves them from cache on subsequent calls as long as there are more to return. We then call the readFile() on each of the files (which we've already read), which copies them to yet another buffer. Here we use Some alternative approaches I can think of:
2 and 3 aren't mutually exclusive, but would rather be good to have both done. To avoid keeping two copies of the file, and to not read files exceeding the limit. As for 2, perhaps there is some more elegant non-intrusive way to do it. I'm open to suggestions. |
Previously we'd check the size limit *after* the file was read, which mostly defeats the point of the limit. Now we check the size from the header in the tar archive and exit early if it exceeds the configured limit. Fixes open-policy-agent#6514 Signed-off-by: Anders Eknert <anders@styra.com>
Previously we'd check the size limit *after* the file was read, which mostly defeats the point of the limit. Now we check the size from the header in the tar archive and exit early if it exceeds the configured limit. In order to do this, I had to extend the `DirectoryLoader` interface with a method to set the max size. While I added implementations for the other (than tarball) loader types, the limit is not currently set anywhere for those. Perhaps we'll want to do that at some later point but it feels like this is mainly relevant when files are loaded via remote bundles. Fixes open-policy-agent#6514 Signed-off-by: Anders Eknert <anders@styra.com>
Thanke for investigating Anders. Do we need a CVE assigned to this? |
Anytime, Dolev 🙂 Quite pleased with the result! As for CVE, I'd lean towards no. OPA will need to run under the premise that a remote bundle server can be trusted. If that is not the case, an OOM is about the least harmful thing a malicious actor could accomplish. If they can tamper with the contents of bundles, they could e.g. change an authorization policy to allow them access, or in the case of discovery bundles, changing OPA's configuration to e.g. turn off decision logging or whatnot. Having the size limit exceeded in real-world deployments is likely going to happen by accident, like where a user accidentally includes a big file by mistake. The fact that a mistake could cause an OOM is of course not good, and I'm happy to see this fixed. But similarly, there are many mistakes one might do as a bundle server "owner" which could have quite severe consequences, so I can't say I'm more worried about this than other imaginable scenarios. Happy to discuss more if you think differently! |
Hi, I agree that if a bundle server is compromised, DoS is the least interesting abuse case. But then again, bundle signing is also a thing, so there's some assumption things can get risky even if you're supposedly trusting your bundle server, no? Totally up to the project to decide at the end of the day :) what's important is that it's fixed! |
You're right — I didn't really make the distinction between whoever creates the bundle and who hosts it, and I should have! Indeed, bundle signing is a good extra measure. Assuming that is in place, the only actor who could accomplish this would be whoever built and signed the bundle, and if that's a malicious actor... not a whole lot we can do then. Let's leave it as a (soon to be fixed!) issue for now. If others have opinions here, please make your voices heard :) Thanks Dolev! |
Previously we'd check the size limit *after* the file was read, which mostly defeats the point of the limit. Now we check the size from the header in the tar archive and exit early if it exceeds the configured limit. In order to do this, I had to extend the `DirectoryLoader` interface with a method to set the max size. While I added implementations for the other (than tarball) loader types, the limit is not currently set anywhere for those. Perhaps we'll want to do that at some later point but it feels like this is mainly relevant when files are loaded via remote bundles. Fixes open-policy-agent#6514 Signed-off-by: Anders Eknert <anders@styra.com>
Previously we'd check the size limit *after* the file was read, which mostly defeats the point of the limit. Now we check the size from the header in the tar archive and exit early if it exceeds the configured limit. In order to do this, I had to extend the `DirectoryLoader` interface with a method to set the max size. While I added implementations for the other (than tarball) loader types, the limit is not currently set anywhere for those. Perhaps we'll want to do that at some later point but it feels like this is mainly relevant when files are loaded via remote bundles. Fixes open-policy-agent#6514 Signed-off-by: Anders Eknert <anders@styra.com>
Previously we'd check the size limit *after* the file was read, which mostly defeats the point of the limit. Now we check the size from the header in the tar archive and exit early if it exceeds the configured limit. In order to do this, I had to extend the `DirectoryLoader` interface with a method to set the max size. While I added implementations for the other (than tarball) loader types, the limit is not currently set anywhere for those. Perhaps we'll want to do that at some later point but it feels like this is mainly relevant when files are loaded via remote bundles. Fixes open-policy-agent#6514 Signed-off-by: Anders Eknert <anders@styra.com>
Previously we'd check the size limit *after* the file was read, which mostly defeats the point of the limit. Now we check the size from the header in the tar archive and exit early if it exceeds the configured limit. In order to do this, I had to extend the `DirectoryLoader` interface with a method to set the max size. While I added implementations for the other (than tarball) loader types, the limit is not currently set anywhere for those. Perhaps we'll want to do that at some later point but it feels like this is mainly relevant when files are loaded via remote bundles. Fixes #6514 Signed-off-by: Anders Eknert <anders@styra.com>
OPA version: v0.60.0
Short description
When an OPA bundle server serves a tarball file containing a large arbitrary file (for the purpose of the example, a 5GB file), it seems that OPA will attempt to load into memory the bundle several times, despite having a config with a
size_limit_bytes
of 100000. This would end up crashing the server with OOM errors.When OPA loads the bundle, it will recognize that there's a file in the tarball that exceeds the size of 100000:
However, the RAM started climbing immediately (spiking to 3-5 times the size of the loaded tar ball size). I noticed that the download retry mechanism will retry loading it several times, resulting in high RAM consumption almost immediately (https://github.com/open-policy-agent/opa/blob/main/download/download.go#L210-L252 is what I could pinpoint this to, I could be wrong).
So in essence and in practice, a 5GB bundle that was loaded resulted in a server with 24GB of RAM getting OOM errors, resulting in some sort of denial of service.
Steps To Reproduce
OPA config I used:
The tarball is generated and served this way:
Expected behavior
OPA rejecting the bundle and releasing memory when a file exceeds the
size_limit_bytes
value limitThe OPA server log in debug mode:
The text was updated successfully, but these errors were encountered: