Skip to content
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

Implement injection of a forkserver into the compiler process #3

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

atrosinenko
Copy link
Collaborator

Implement a forkserver inspired by AFL fuzzer. Unlike the original
design, this one relies on library preloading and seccomp to
eliminate static or dynamic recompilation of the compiler.

For single-threaded, single-process compilers this can be
several times faster, provided that the interacting
with any input files is delayed until absolutely necessary.

For example, it is the case when the compiler process first
loads the standard library for several seconds and then
processes scratch files in several milliseconds.

@atrosinenko
Copy link
Collaborator Author

On review request: for now, SCM code base is quite sloppy: it has inconsistent throws specifications, logging to stdout/stderr, etc. But I would rather delay it more towards 1.0 release for better understand required public APIs.

Right now, I would rather discuss potential security implications of such an approach. First, I thought communicating with forkserver via UDP, but this would be quite hard to restrict to a single user, etc. Now it seems I have a single but quite significant questionable part: compiling and injecting .so into current user's process. This should probably be secured via proper handling of temporary files, so it's the main question to be reviewed.

@atrosinenko
Copy link
Collaborator Author

Hmm... Looks like standard BufferedReader works perfectly. Interesting, can it "over-buffer" while reading a single line? Otherwise, as far as I understand, reading from a pipe on Linux generally returns as soon as any data is available.

Implement a forkserver inspired by AFL fuzzer. Unlike the original
design, this one relies on library preloading and seccomp to
eliminate static or dynamic recompilation of the compiler.

For single-threaded, single-process compilers this can be
several times faster, provided that the interacting
with any input files is delayed until absolutely necessary.

For example, it is the case when the compiler process first
loads the standard library for several seconds and then
processes scratch files in several milliseconds.
Copy link
Member

@adangel adangel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review.

for now, SCM code base is quite sloppy: it has inconsistent throws specifications, logging to
stdout/stderr, etc. But I would rather delay it more towards 1.0 release for better understand required
public APIs.

That's fine. As for logging, I would consider using slf4j rather than java.util.logging since it provides more flexibility later on.

Now it seems I have a single but quite significant questionable part: compiling and injecting .so into
current user's process. This should probably be secured via proper handling of temporary files, so
it's the main question to be reviewed.

Do you have any specific attack vector in mind?
We are injecting the .so into the compiler process (or whatever thing we are supposed to be executing), aren't we? (so we are not modifying the current user's process, which is java/pmd-scm).
Are you concerned, that the .so could be manipulated before it is used (and after we have compiled it)?
For now, I would say, the file permissions are ok, so that this scenario is prevented.
The attacker could however potentially manipulate any other input file, which could in theory trigger unexpected actions - I mean input files used for executing pmd-scm. But I guess, that's almost always the possibility and could be prevented by using proper file permissions/umask settings.

/**
* The size of buffer for a single output line read by this class.
*/
private final static int MAX_LINE_LENGTH = 65536;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, this is not needed anymore, since you are using now BufferedReader (which probably uses the same value by default).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, forget to remove, thanks!

*/
private Path compilePreloadedObject() throws IOException {
try {
Path input = Files.createTempFile("pmd-scm-forkserver-", ".c");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generated temp filename has a random number in it, so the attacker would need to guess the name, if he wants to inject his own code.
The created temp file is in the standard temp directory. By default, it is created with only the user permissions, so no one else can read/modify it.

Path output = Files.createTempFile("pmd-scm-forkserver-", ".so");
deleteAtExit.add(output);
Files.copy(getClass().getResourceAsStream("forksrv-preload.c"), input, StandardCopyOption.REPLACE_EXISTING);
String compiler = System.getenv("CC");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on what access the attacker already has, he could provide a different program as "cc", that we would use then without knowing. We simply execute "cc" (or whatever env CC points to).

.start();
int exitCode = compilerProcess.waitFor();
if (exitCode != 0) {
throw new IOException("Cannot compile forkserver preloaded object: compiler exited with code " + exitCode);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically this is not a IOException - something's wrong with the compiler or the source code. But that's a detail (like the logging vs. system.err)

@atrosinenko
Copy link
Collaborator Author

Thank you for review!

We are injecting the .so into the compiler process (or whatever thing we are supposed to be executing), aren't we? (so we are not modifying the current user's process, which is java/pmd-scm).

Yes, we are injecting the .so into spawned compiler process, not into the JVM.

Are you concerned, that the .so could be manipulated before it is used (and after we have compiled it)? For now, I would say, the file permissions are ok, so that this scenario is prevented.

Yes, I am primarily concerned about an attacker (on a multi-user system) being able to manipulate either the compiled .so OR unpacked .c source. I expect an attacker being able to monitor the created file names in, say, /tmp, and so don't want to rely on timings (AFAIK it is quite common practice now, but I'm not sure), so I would want to rely on the permissions only. And yes, I hope that temporary files are created with the proper permissions by the JVM by default precisely for this purpose. But they still need to be used properly: for example, is Files.copy(...) an appropriate method to use? (In case you already know "canonical" answer, otherwise it is probably googleable by me)

The attacker could however potentially manipulate any other input file, which could in theory trigger unexpected actions - I mean input files used for executing pmd-scm. But I guess, that's almost always the possibility and could be prevented by using proper file permissions/umask settings.

Hmm... I even not considered this yet :) But I suppose, that since input/output file names are user-controlled anyway, I don't have to impose some security restrictions on them. I consider this user's environment safe as well ($CC value and an executable it points to, for example) - otherwise the user seems to have much more serious problems even without SCM :)

So, my concerns are not about creating obscure security measures in an already compromised account but just about not creating new threats (such as a possibility for other local user to inject some code to be executed by us because of improper API handling in SCM).

@adangel adangel changed the base branch from master to main September 19, 2024 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants